fix: escape colons in metric names when escaping=underscores - #1180
Open
johnsaurabh wants to merge 3 commits into
Open
fix: escape colons in metric names when escaping=underscores#1180johnsaurabh wants to merge 3 commits into
johnsaurabh wants to merge 3 commits into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
In UNDERSCORES escaping mode, escape_metric_name was short-circuiting for names that matched the legacy Prometheus metric name regex, which allows colons. This caused # HELP and # TYPE lines to emit the raw name (e.g. sglang:token_usage) while sample lines, which use _is_legacy_labelname_rune, correctly replaced the colon with an underscore (e.g. sglang_token_usage). The mismatch violates the OpenMetrics standard and breaks strict parsers. Fix by requiring the name to also be colon-free before taking the no-op path, and switching the fallback _escape call to use _is_legacy_labelname_rune so colons are replaced consistently. Fixes prometheus#1177 Signed-off-by: John Saurabh <itsjohnsaurabh@gmail.com>
Add test_gauge_colon_in_name_escaped_underscores to verify that a gauge named sglang:token_usage produces consistent # HELP, # TYPE, and sample lines when escaping=underscores is in effect. Update two parametrized scenario expectations that documented the old buggy behaviour (colon preserved in UNDERSCORES mode): - "legacy valid metric name": no:escaping_required -> no_escaping_required - "metric name with dots and colon": http_status:sum -> http_status_sum Signed-off-by: John Saurabh <itsjohnsaurabh@gmail.com>
Mirror the same expectation corrections made to tests/openmetrics/test_exposition.py: under UNDERSCORES mode, colons in metric names are now replaced with underscores to match sample line output. Signed-off-by: John Saurabh <itsjohnsaurabh@gmail.com>
csmarchbanks
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Thanks! Generally this looks good to me, though you need to sign the DCO.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1177
When a metric name contains a colon (e.g. sglang:token_usage) and the client negotiates escaping=underscores, the # HELP and # TYPE lines were emitting the raw name while the sample line replaced the colon with an underscore:
This violates the OpenMetrics spec and causes strict parsers to treat the metadata and the sample as two separate metrics.
The bug is in escape_metric_name in prometheus_client/openmetrics/exposition.py. In UNDERSCORES mode, the function short-circuits for any name that matches the legacy Prometheus metric name regex. That regex allows colons, so names like sglang:token_usage were returned unchanged. The sample line uses _is_legacy_labelname_rune which does not allow colons, so it escaped them correctly.
The fix adds a colon check to the short-circuit condition and switches the fallback _escape call to use _is_legacy_labelname_rune, matching what the sample line already does.
Two existing test expectations that documented the old behavior are updated. A new end-to-end test reproduces the exact scenario from the issue.
@csmarchbanks