model: escape ':' in label names via EscapeLabelName / IsValidLegacyLabelName - #953
Open
sergioperezcheco wants to merge 1 commit into
Open
model: escape ':' in label names via EscapeLabelName / IsValidLegacyLabelName#953sergioperezcheco wants to merge 1 commit into
sergioperezcheco wants to merge 1 commit into
Conversation
…cyLabelName EscapeMetricFamily and metricNeedsEscaping validated label names with IsValidLegacyMetricName, which permits ':'. Colons are only valid in metric names, never in label names, so a label like 'app:instance-id' was rendered as 'app:instance_id' under UnderscoreEscaping (and 'U__app:instance_2d_id' under ValueEncodingEscaping) instead of being escaped. This adds: - IsValidLegacyLabelName / isValidLegacyLabelRune: label-name validity that rejects ':', mirroring LabelNameRE. - EscapeLabelName: an escaping entry point that uses the label-name rules, parallel to EscapeName (metric names). - EscapeMetricFamily and metricNeedsEscaping now use the label-name rules for label names and keep the metric-name rules for the metric family name and the __name__ label value. EscapeName is unchanged for backward compatibility. Relates to prometheus/prometheus#18380 Signed-off-by: sergioperezcheco <checo520@outlook.com>
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
1 participant
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.
Relates to prometheus/prometheus#18380 (previous attempt #889 was closed by the author before the requested changes were addressed).
EscapeMetricFamily and metricNeedsEscaping were validating label names with IsValidLegacyMetricName, which permits :. Colons are only valid in metric names and have never been valid in label names, so a label like app:instance-id was rendered as app:instance_id under UnderscoreEscaping (and U__app:instance_2d_id under ValueEncodingEscaping) instead of having the colon escaped.
Rather than threading a ValidationContext through EscapeName, this adds label-name-specific entry points so the metric-name and label-name rules stay cleanly separated:
EscapeMetricFamily and metricNeedsEscaping now use the label-name rules for label names and keep the metric-name rules for the metric family name and the name label value. EscapeName itself is unchanged for backward compatibility. I checked the rest of common for other places calling IsValidLegacyMetricName on labels and did not find any (the otlptranslator functions were already correct, as ywwg noted on the issue).
Tests: TestIsValidLegacyLabelName and TestEscapeLabelName cover the new helpers across all three schemes, and two new cases in TestEscapeMetricFamily verify that a colon in a label name is escaped under both UnderscoreEscaping and ValueEncodingEscaping while the name value (a metric name) keeps its colon. Full model package and go vet pass.