Skip to content

PrometheusHttpServer drops metrics with same name and different type#5078

Merged
jack-berg merged 5 commits intoopen-telemetry:mainfrom
jack-berg:prometheus-conflicts
Jan 5, 2023
Merged

PrometheusHttpServer drops metrics with same name and different type#5078
jack-berg merged 5 commits intoopen-telemetry:mainfrom
jack-berg:prometheus-conflicts

Conversation

@jack-berg
Copy link
Copy Markdown
Member

Resolves #5014.

@jack-berg jack-berg requested a review from a team January 3, 2023 22:52
.setAttributes(Attributes.of(stringKey("ks"), "vs"))
.build(),
"instrument.name",
"monotonic.cumulative.double.sum",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the metrics in this test used the same name, which produce name collisions with the new rules. I've changed the metric names to be a dot.delimited.version of the names of the fields they're assigned to.

Map<String, List<MetricData>> metricsByName = new LinkedHashMap<>();
Set<InstrumentationScopeInfo> scopes = new LinkedHashSet<>();
// Iterate through metrics, filtering and grouping by headerName
for (MetricData metric : metrics) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A for loop is more appropriate because we now need to track the unique scopes we've seen and metric name conflicts, which we return to the caller for logging purposes.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 3, 2023

Codecov Report

Base: 91.06% // Head: 91.06% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (19d0646) compared to base (76f7a96).
Patch coverage: 93.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5078   +/-   ##
=========================================
  Coverage     91.06%   91.06%           
+ Complexity     4888     4887    -1     
=========================================
  Files           553      553           
  Lines         14453    14465   +12     
  Branches       1381     1387    +6     
=========================================
+ Hits          13161    13173   +12     
  Misses          895      895           
  Partials        397      397           
Impacted Files Coverage Δ
...etry/exporter/prometheus/PrometheusHttpServer.java 80.73% <92.30%> (+0.73%) ⬆️
.../opentelemetry/exporter/prometheus/Serializer.java 86.68% <93.93%> (-0.21%) ⬇️
...metry/sdk/metrics/export/PeriodicMetricReader.java 90.00% <0.00%> (+2.85%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

LOGGER.log(
Level.WARNING,
"Metric conflict(s) detected. Multiple metrics with same name but different type: "
+ conflictHeaderNames.stream().collect(joining(",", "[", "]")));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goal is to log each name conflict that is detected once to avoid spammy logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prometheus exporter handles instrumentation scope and prevents collisions

2 participants