Migrate prometheus exporter to otlptranslator#7044
Migrate prometheus exporter to otlptranslator#7044dashpole merged 9 commits intoopen-telemetry:mainfrom
Conversation
When creating new metrics with OTel-Go-SDK, I don't expect people to add unit/type suffixes manually. So, to increase performance, we decided not to deduplicate suffixes in this code path. Did I have the wrong expectations? If people prefer Prometheus' ways of doing things, I expected they would use Prometheus SDK instead of OTel's. |
|
We can tell people who are relying on the suffix deduplication to disable suffixes and add them themselves if they want. I'll remove those test cases |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7044 +/- ##
=====================================
Coverage 82.8% 82.8%
=====================================
Files 261 261
Lines 24361 24371 +10
=====================================
+ Hits 20173 20201 +28
+ Misses 3813 3795 -18
Partials 375 375
🚀 New features to boost your workflow:
|
a467273 to
019acff
Compare
|
Thanks everyone! This is ready for review. |
|
It would be nice to test this in the collector. I expect that the instrumented metrics don't have unit/type suffixes in their names (which would cause that duplicated suffix problem), but I haven't checked yet |
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
|
@dashpole, should this PR be blocked by:
|
|
@pellared do you think it is OK to include this PR in a patch release? Otherwise, I can open a revert PR for the offending change, rather than fix it this way |
|
I would prefer including this PR in the patch release |
|
+1 for merging this and including in the patch release. If it is not included that patch release itself needs to be retracted and that is not the original plan. |
Patch release for #7039 ### Changed - Retract `v0.59.0` release of `go.opentelemetry.io/otel/exporters/prometheus` module which appends incorrect unit suffixes. (#7046) - Change `go.opentelemetry.io/otel/exporters/prometheus` to no longer deduplicate suffixes when UTF8 is enabled. It is recommended to disable unit and counter suffixes in the exporter, and manually add suffixes if you rely on the existing behavior. (#7044) ### Fixed - Fix `go.opentelemetry.io/otel/exporters/prometheus` to properly handle unit suffixes when the unit is in brackets. E.g. `{spans}`. (#7044)
Reverts #13429 so that we can bump to the to-be-released bugfix version that adds open-telemetry/opentelemetry-go/pull/7044
|
@dashpole - Is this the same process that is followed for other languages when exporting to prometheus? Since this was a behavior change, and it broke grpc-go, I'm wondering if it will impact grpc-java/c++/etc, or if they are already doing something to avoid the translation. |
|
This won't impact other languages @dfawley |
|
Sorry if it wasn't clear, but my question is whether this is the same behavior that other languages have. But also it seems like we were using the library incorrectly by not including braces around our non-standard units, and if we had done so we would not have been broken. So perhaps the question is moot. |
|
It should be the same behavior across languages. If you find that it isn't, let me know and i'll make sure it gets fixed. Sorry for any breakage this caused! |
Fixes #7039
Fixes #6704
This uses the common prometheus/otlptranslator library to handle name conversion. It was a little tricky to work around the fact that the library only lets us configure whether all suffixes are added or not. But we want to keep supporting WithoutUnit and WithoutCounterSuffixes for a while longer. Those will eventually be deprecated and replaced after open-telemetry/opentelemetry-specification#4533 is released.
We decided to go ahead with the changes despite it being a small behavioral change when UTF8 is enabled. See: prometheus/otlptranslator#44 for the rationale.
This adds a unit test to verify that it properly handles bracketed units. The test fails on main with:
cc @TylerHelmuth @ywwg @ArthurSens