-
Notifications
You must be signed in to change notification settings - Fork 906
Update prom client with support for UTF-8 #7588
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
base: main
Are you sure you want to change the base?
Conversation
1f6e6cf
to
145e722
Compare
1fc8ed8
to
894b226
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7588 +/- ##
============================================
- Coverage 89.99% 89.98% -0.01%
- Complexity 7079 7081 +2
============================================
Files 803 803
Lines 21412 21428 +16
Branches 2086 2087 +1
============================================
+ Hits 19269 19282 +13
- Misses 1479 1482 +3
Partials 664 664 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
48d1537
to
5842f02
Compare
@jkwatson please have a look |
...rs/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusMetricReader.java
Show resolved
Hide resolved
} | ||
|
||
prometheusBuilder.setUtf8SupportEnabled( | ||
config.getBoolean("otel.exporter.prometheus.utf8", false)); |
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.
Is this the official property? I thought no new properties were being added to the spec?
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.
See #7588 (comment)
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.
That doesn't answer my question... if we're adding new configuration properties, I'd like to make sure they're "official" in the spec, and we're not going off on our own, especially as there has been a moratorium set on the new configuration properties/env vars in the spec (at least, the last I heard).
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.
Got it 😄
This is the part in the Go SDK: https://github.com/open-telemetry/opentelemetry-go/blob/d0cab8666b740c975f028236610cab2663f02031/exporters/prometheus/config.go#L44-L66
@ArthurSens it seems there is no env var to set the translation strategy - is that missing?
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.
got the answer: it should be translation_strategy
instead: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/prometheus.md#configuration
// Resource attributes from the metric SDK resource translated to target_info | ||
stringKeyValue( | ||
"service_name", | ||
"service.name", |
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.
why did these keys change, when we haven't explicitly changed the exporter?
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 set UTF-8 to true in most tests - including here
no longer blocked by the spec? |
We talked in the spec meeting that it's up to the SDKs to decide if So from that point of view, we're free to keep If
We can decide that we're OK with the breaking change - and then we don't need this property. |
To avoid a breaking change
otel.exporter.prometheus.utf8
-> this is wrong__
opentelemetry-specification#4634