Skip to content
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

Update module github.com/prometheus/common to v0.62.0 and fix tests #6198

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dashpole
Copy link
Contributor

Supersedes #6171

The update to prometheus/common changes the default escaping scheme to NoEscaping. Use of underscores vs original UTF-8 is still determined by content negotiation.

@dashpole
Copy link
Contributor Author

@ywwg I found one potential breaking change with the prometheus/common underscore escaping logic. The current behavior OTel-Go implemented handles colliding keys by concatenating the values with a ;.

The diff from the unit test is:

-foo_total{A_B="Q",C_D="Y",C_D="Z",otel_scope_name="testmeter",otel_scope_version="v0.1.0"} 24.3
+foo_total{A_B="Q",C_D="Y;Z",otel_scope_name="testmeter",otel_scope_version="v0.1.0"} 24.3

The unit test does this:

opt := otelmetric.WithAttributes(
    // exact match, value should be overwritten
    attribute.Key("A.B").String("X"),
    attribute.Key("A.B").String("Q"),

    // unintended match due to sanitization, values should be concatenated
    attribute.Key("C.D").String("Y"),
    attribute.Key("C/D").String("Z"),
)

It seems like there are a few paths forward (cc @ArthurSens):

  1. Accept this change as-is. If users have collisions, they can either enable scraping with UTF-8 or they can set the NameValidationScheme to LegacyValidation fix it.
  2. Update prometheus client_golang to handle duplicate label keys similarly to what OTel Go did (e.g. merge with delimiter).

I'm inclined to accept this change as-is and document how users can revert to prior behavior. I've had to work around this in our unit test for now.

@dashpole dashpole marked this pull request as ready for review January 22, 2025 18:17
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.2%. Comparing base (a85d0c1) to head (2bff18e).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6198     +/-   ##
=======================================
- Coverage   82.3%   82.2%   -0.1%     
=======================================
  Files        273     273             
  Lines      23735   23741      +6     
=======================================
+ Hits       19534   19535      +1     
- Misses      3853    3857      +4     
- Partials     348     349      +1     

see 3 files with indirect coverage changes

@MrAlias
Copy link
Contributor

MrAlias commented Jan 22, 2025

The bench test is failing. Are you waiting on feedback before it is resolved? Should I hold off on reviewing this?

@dashpole
Copy link
Contributor Author

working on fixing it

@dashpole dashpole marked this pull request as draft January 22, 2025 19:12
@dashpole
Copy link
Contributor Author

fixed

@dashpole dashpole marked this pull request as ready for review January 22, 2025 19:30
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

🚀

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.

2 participants