-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add utf8 support to the prometheus exporter #5755
Add utf8 support to the prometheus exporter #5755
Conversation
a18a9c8
to
d2187a4
Compare
cc @ywwg |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5755 +/- ##
=======================================
- Coverage 84.5% 84.5% -0.1%
=======================================
Files 272 272
Lines 22798 22773 -25
=======================================
- Hits 19283 19257 -26
- Misses 3172 3173 +1
Partials 343 343 |
d2187a4
to
2a39499
Compare
cc @open-telemetry/wg-prometheus |
The escaping scheme is designed to be used as a hint when a UTF-8-capable system is delivering metrics to a system that does not or one that does but has UTF-8 turned off. In this case, the specified escaping scheme setting is used rather than using the default non-round-trippable underscore-replacement scheme. Therefore, this option is not intended to be set to NoEscaping when UTF8 is the default. I can see how this is confusing, I might update the documentation to make this more clear. |
(part of the confusion is because the same common library is used by metrics producers and consumers, so it's not clear which side of the conversation these settings apply to) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with this exporter, does it do content negotiation with prometheus? That handshake is the intended way to determine whether prometheus can accept UTF8 or needs escaping.
Some more detail: prometheus will put "escaping=allow-utf-8" in the Accept header if it accepts UTF-8, otherwise it may specify a different escaping scheme. If there is no "escaping" term, the producer should use its own default escaping scheme. (If it set "NoEscaping", then it would send UTF-8 and the metrics would be rejected. (That could be an intended behavior, I suppose) https://github.com/prometheus/prometheus/blob/main/config/config.go#L482 |
This is just a wrapper around the Prometheus go client library, and users will still use promhttp to expose metrics endpoints, and should get all content negotiation, etc.
For our tests, If I only set |
It sounds like GatherAndCompare is not respecting the default. Are you able to find where in the call stack the escaping is happening? We may have missed an entry point. I tried setting some breakpoints and was unable to find where the call was happening. I think it would have to be a call to model.EscapeName.
That's a big question tbh! I selected Values escaping because it is roundtrippable, so producers sending UTF-8 metrics can reliably query them back again without risk of collisions. But this may be unexpected behavior for Prometheus users, in which case we could change the built-in default escaping method in common/model. |
I think it happens from here, with That should return Then the encoder calls
That eventually is used to call |
so this could be fixed by having |
should be helpful for open-telemetry/opentelemetry-go#5755 Signed-off-by: Owen Williams <owen.williams@grafana.com>
@ywwg so the plan is to wait for client_golang to update to the latest prom/common release, then add the escaping option when comparing. We will probably need to remove the existing sanitization with a feature gate.
This sounds like something we should track separately. Should I open an issue in prom/common? |
yup, I'll poke people to push out another common release
yes that sounds like a good idea |
For open-telemetry/opentelemetry-go#5755 Signed-off-by: Owen Williams <owen.williams@grafana.com>
converting to draft for now |
For open-telemetry/opentelemetry-go#5755 Signed-off-by: Owen Williams <owen.williams@grafana.com>
For open-telemetry/opentelemetry-go#5755 Signed-off-by: Owen Williams <owen.williams@grafana.com>
client_golang updated, so you should be unblocked |
4e063a2
to
6a8be44
Compare
I am able to remove overriding But I wasn't able to remove the check for |
21f2319
to
f5299e8
Compare
Right now common.model is still defaulted to Legacy validation, and when 3.0 comes out we will switch this to UTF-8. Until then, it's necessary to set that value manually, either at the top of tests, or in a Feel free to hit me up on CNCF slack, or we can do a video conference to talk it through. |
Great. Just making sure I wasn't missing something |
f5299e8
to
d238707
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one question, otherwise lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for battle-testing the new API!
87a016d
to
712fca0
Compare
712fca0
to
ff43372
Compare
### Added - Support `OTEL_EXPORTER_OTLP_LOGS_INSECURE` and `OTEL_EXPORTER_OTLP_INSECURE` environments in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc`. (#5739) - The `WithResource` option for `NewMeterProvider` now merges the provided resources with the ones from environment variables. (#5773) - The `WithResource` option for `NewLoggerProvider` now merges the provided resources with the ones from environment variables. (#5773) - Add UTF-8 support to `go.opentelemetry.io/otel/exporters/prometheus`. (#5755) ### Fixed - Fix memory leak in the global `MeterProvider` when identical instruments are repeatedly created. (#5754) - Fix panic instruments creation when setting meter provider. (#5758) - Fix panic on instruments creation when setting meter provider. (#5758) - Fix an issue where `SetMeterProvider` in `go.opentelemetry.io/otel` might miss the delegation for instruments and registries. (#5780) ### Removed - Drop support for [Go 1.21](https://go.dev/doc/go1.21). (#5736, #5740, #5800)
Changes
Disable sanitization when the UTF-8 support is enabled in the Prometheus library.
Usage
To enable UTF-8 support for the Prometheus exporter after this change, set the following in your application:
See
exporters/prometheus/testdata/counter_utf8.txt
for an example of the text exposition format including names/labels with dots.