Merge colliding Prometheus label values#8364
Conversation
Preserve Prometheus compatibility when normalized attribute names collide by joining the values in original-key order instead of letting later entries overwrite earlier ones. Add regression tests for metric and resource attribute collisions. Fixes open-telemetry#8236
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8364 +/- ##
============================================
+ Coverage 90.82% 90.86% +0.04%
- Complexity 7927 7960 +33
============================================
Files 895 896 +1
Lines 23872 24039 +167
Branches 2378 2390 +12
============================================
+ Hits 21681 21844 +163
- Misses 1446 1457 +11
+ Partials 745 738 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@psx95 Thanks for the thoughtful review |
|
There's significant conceptual overlap with #8346 - so I'd park this PR for now. |
Understood, thanks for the context. There's definitely conceptual overlap with #8346 in the Otel2PrometheusConverter label translation logic. I'll park this PR for now and revisit once #8346 settles, so the collision-handling fix can be cleanly applied on top of the final translation model. |
| MetricDataType.LONG_SUM, | ||
| Attributes.empty(), | ||
| Resource.create( | ||
| createMapAttributes("foo_bar", "b", "foo-bar", "c", "foo.bar", "a"))), |
There was a problem hiding this comment.
I eventually put together that createMapAttributes intentionally creates a non-default Attributes implementation to confirm the sorting logic.
- Let's add a comment explaining this because its not immediately apparent why we're going out of the way to add a new attributes implementation
- Is it possible / reasonable to avoid the extra allocation / overhead by detecting when the Attributes implementation is default ArrayBackedAttributes?
There was a problem hiding this comment.
Thinking about this more, I don't think we should try to check if Attributes is instance of ArrayBackedAttributes. But I am still worried about the excessive new allocation here which occurs regardless of whether a collision occurs. To mitigate, let's adjust to a strategy which assumes that no collision occurs, but if one does occur, triggers the code path with all this additional new machinery.
| labelNameToValue.put( | ||
| convertLabelName(key.getKey()), toLabelValue(key.getType(), value))); | ||
| Map<String, String> labelNameToValue = new LinkedHashMap<>(); | ||
| normalizeAndMergeAttributeLabels(labelNameToValue, attributes); |
There was a problem hiding this comment.
This is just looking at attributes, but I think the collisions also have to consider collisions potentially caused by normalization of other sources of attributes: resource, scope, additionalAttributes
4 participants
Summary
Fixes #8236