Ensure prometheus metrics with the same name are serialized as a group. #4386
Conversation
| 1633947011000000000L, 1633950672000000000L, KP_VP_ATTR, 5)))); | ||
| 1633947011000000000L, | ||
| 1633950672000000000L, | ||
| Attributes.of(TYPE, "mcds"), |
There was a problem hiding this comment.
Before all the metrics had the same name, meter, attributes, which is not representative of anything the SDK could actually output. So added this attribute to make a bit more realistic
| @@ -14,7 +14,9 @@ class JpmsTest { | |||
| @Test | |||
| void noLinkageErrors() { | |||
| SdkMeterProvider meterProvider = | |||
| SdkMeterProvider.builder().registerMetricReader(PrometheusHttpServer.create()).build(); | |||
| SdkMeterProvider.builder() | |||
| .registerMetricReader(PrometheusHttpServer.builder().setPort(0).build()) | |||
There was a problem hiding this comment.
This test flaked on this PR so added the fix in
Codecov Report
@@ Coverage Diff @@
## main #4386 +/- ##
=========================================
Coverage 89.95% 89.96%
- Complexity 4897 4901 +4
=========================================
Files 567 567
Lines 15167 15180 +13
Branches 1462 1462
=========================================
+ Hits 13643 13656 +13
Misses 1052 1052
Partials 472 472
Continue to review full report at Codecov.
|
| + "instrument_name_bucket{kp=\"vp\",le=\"+Inf\"} 2.0 1633950672.000 # {span_id=\"0000000000000002\",trace_id=\"00000000000000000000000000000001\"} 4.0 0.001\n" | ||
| + "# TYPE instrument_name gauge\n" | ||
| + "# HELP instrument_name description\n" | ||
| + "instrument_name_total{type=\"mcds\"} 5.0 1633950672.000\n" |
There was a problem hiding this comment.
Not related to this specific PR, but the spec requires that unit be appended to the metric name as a suffix. We don't do that anywhere today. I believe that would represent a breaking behavior change for prometheus users as their metric names would change when unit is present.
Still, would be good to do that soon to minimize the impact.
There was a problem hiding this comment.
Thanks for noticing this - filed open-telemetry/opentelemetry-specification#2497
This doesn't stop us from exporting two identical time series, filed open-telemetry/opentelemetry-specification#2493 for spec on working on that. This should still help in many cases.
Fixes #4382