-
Notifications
You must be signed in to change notification settings - Fork 932
Update Prometheus Exporter dependencies to use no-protobuf formats (and adds test) #7664
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7664 +/- ##
=========================================
Coverage 90.06% 90.06%
Complexity 7104 7104
=========================================
Files 805 805
Lines 21484 21484
Branches 2093 2093
=========================================
Hits 19349 19349
Misses 1472 1472
Partials 663 663 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Here's the runtime classpath back in 1.50.0: And here's the classpath as of this PR: Notably, the What's going on here? How was protobuf export supported previously without the |
|
|
Got it thanks for the context. Ok, so we have a decision to make. Do we (opentelemetry-java) want to make protobuf serialization opt-in or opt-out? When protobuf was shaded, we had no choice. I lean towards opt-out (which is what this PR does), since an opt-in strategy makes it cumbersome to use protobuf + prometheus + otel java agent. |
| implementation(project(":exporters:prometheus")) | ||
| implementation("io.prometheus:prometheus-metrics-exporter-httpserver") | ||
| implementation("io.prometheus:prometheus-metrics-exporter-httpserver") { | ||
| exclude(group = "io.prometheus", module = "prometheus-metrics-exposition-formats") |
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.
What is the goal of this exclusion?
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.
it aligns the classpath with what is used for compile - see
| implementation("io.prometheus:prometheus-metrics-exporter-httpserver") { |
| HttpMethod.GET, | ||
| "/metrics", | ||
| HttpHeaderNames.ACCEPT, | ||
| "Accept: application/vnd.google.protobuf;proto=io.prometheus.client.MetricFamily")) |
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.
Technically we have coverage for this here already: https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/prometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerTest.java#L587-L595
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 didn't see - this just didn't fail, because the dependency in question has been added the test classpath..
I agree - make it easy to use by default |
|
@jack-berg @jkwatson as agreed (in SIG), we'll create a patch release once this is merged |
Fixes #7659