Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update our OTel dependencies, and move config into GMP module #709

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

dashpole
Copy link
Contributor

We need to switch from prometheus.BuildPromCompliantName to prometheus.BuildCompliantName, which takes an argument for enabling unit suffixes. We would like to allow users to disable unit suffixes, as the (upstream) feature gate will not apply to unit suffixes anymore.

I decided to simplify our API surface in the GMP module to reduce the number of breaking changes we will need to make in the future. To do this, I moved configuration related to naming, resource, and extra metrics into a config struct in this module. This will not result in any breaking changes upstream, since it copies existing configuration from the GMP exporter for target and scope info.

This also simplifies up the ExtraMetrics function, since we don't need to have it return anything.

@dashpole dashpole requested a review from a team as a code owner August 21, 2023 18:45
@dashpole dashpole requested a review from aabmass August 21, 2023 18:46
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #709 (0b29338) into main (ef9b15e) will increase coverage by 0.04%.
The diff coverage is 87.87%.

@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
+ Coverage   69.56%   69.60%   +0.04%     
==========================================
  Files          36       37       +1     
  Lines        4711     4728      +17     
==========================================
+ Hits         3277     3291      +14     
- Misses       1286     1289       +3     
  Partials      148      148              
Files Changed Coverage Δ
exporter/collector/config.go 45.04% <ø> (ø)
exporter/collector/metrics.go 73.01% <33.33%> (-0.05%) ⬇️
...porter/collector/googlemanagedprometheus/config.go 80.00% <80.00%> (ø)
...collector/googlemanagedprometheus/extra_metrics.go 79.79% <100.00%> (+1.07%) ⬆️
...ector/googlemanagedprometheus/monitoredresource.go 100.00% <100.00%> (ø)
...porter/collector/googlemanagedprometheus/naming.go 100.00% <100.00%> (ø)
...tor/integrationtest/testcases/testcases_metrics.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dashpole dashpole merged commit 8963492 into GoogleCloudPlatform:main Aug 22, 2023
25 checks passed
@dashpole dashpole deleted the update-otel branch August 22, 2023 18:19
codeboten pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Aug 29, 2023
Update googlecloud dependency from v0.42.0 to v0.43.0. See the full set
of changes here:
https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/releases/tag/v0.43.0

I refactored the API surface of the googlemanagedprometheus package to
minimize future breaking changes in
GoogleCloudPlatform/opentelemetry-operations-go#709.
This created a Config struct in that package with configuration options
for functions in the package. It is backwards-compatible with the
configuration currently in the googlemanagedprometheus exporter, but
adds a new option `add_metric_suffixes`.

This also removes the deprecated BuildPromCompliantName now that the
googlemanagedprometheus library has migrated to the new function.

**Testing:**

Expanded config unit test to cover all fields.

**Documentation:**

Updated the README with new `add_metric_suffixes` configuration. This
configuration matches the configuration used by the prometheus remote
write and prometheus exporters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants