Merge colliding Prometheus label values#8364
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8364 +/- ##
============================================
- Coverage 91.61% 91.61% -0.01%
- Complexity 10320 10343 +23
============================================
Files 1013 1013
Lines 27287 27352 +65
Branches 3203 3215 +12
============================================
+ Hits 25000 25059 +59
- Misses 1559 1566 +7
+ Partials 728 727 -1 ☔ View full report in Codecov by Harness. 🚀 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. |
|
@jack-berg Thanks for the review. Those are good points — I’ll make sure to optimize the allocation with a fast path for the no-collision case, extend the collision check to resource/scope/additionalAttributes sources, and add the comment explaining the sorting logic.\n\nThis PR is currently parked per @zeitlinger’s request pending #8346. Once that settles, I’ll rebase, apply these changes, and re-request review. |
@ADITYA-CODE-SOURCE this is unblocked now |
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
5c8f7e1 to
6e3a97f
Compare
|
Rebased this parked PR onto current Latest update:
Local verification completed with:
The branch was force-pushed after rebase. |
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
|
Pushed a follow-up for the failing CI. The Linux/macOS I regenerated the apidiffs with Local verification:
At this point the code-side maintainer feedback is addressed and CI should rerun cleanly on the updated branch. The remaining merge blocker after CI is the stale review state from the earlier requested-changes review, which will still need maintainer re-review or dismissal. |
|
@psx95 @jack-berg CI is green now, and I’ve addressed the earlier review feedback in the rebased branch. The main implementation update is in Could you please take another look |
…y-java into issue-8236-prometheus-label-collisions
|
@psx95 @jack-berg All checks are green now, and the branch has been rebased onto current At this point the remaining merge blocker appears to be the old |
|
Hey @ADITYA-CODE-SOURCE - thanks for the update. I was it challenging to grok what was actually happening with all the helper methods doing similar but slightly different things. Also, the fast path was still allocating an inner LinkedHashMap inside normalizeAndMergeAttributeLabels, so we were still missing the mark on avoiding allocations for the common case of no collisions. I took a crack at trying to simplify and avoid the allocations, and opened a PR against your branch on your fork for you to consider: ADITYA-CODE-SOURCE#1 If you approve, go ahead and merge, or go ahead and tweak whatever you see fit and we'll go from there. |
|
Applied the simplification suggested in ADITYA-CODE-SOURCE#1 in 98f2820.\n\nThis keeps the current behavior unchanged:\n- metric and resource collisions still merge within their own source\n- values are still joined in original-key sort order\n- existing cross-source precedence remains unchanged\n- non-escaping strategies are unchanged\n\nThe main change is implementation shape: the common no-collision path now avoids the extra helper/map allocation that was still present before.\n\nLocal verification:\n- ./gradlew :exporters:prometheus:test --tests "*Otel2PrometheusConverterTest"\n- ./gradlew :exporters:prometheus:spotlessCheck\n- ./gradlew :exporters:prometheus:check\n\nCould you please take another look? |
jack-berg
left a comment
There was a problem hiding this comment.
Thanks for working through this!
|
@psx95 I've dismissed your change request as stale. Any remaining issues with this? Will wait until tomorrow and then merge |
Hi @jack-berg , thanks! |
psx95
left a comment
There was a problem hiding this comment.
Thanks for working through this!
|
Thank you for your contribution @ADITYA-CODE-SOURCE! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Summary
Fixes #8236