model: fix EscapeName incorrectly preserving colon in label names#889
model: fix EscapeName incorrectly preserving colon in label names#889shankalpaLamichhane wants to merge 1 commit into
Conversation
Colons are only valid in metric names, not label names. EscapeName was using the same isValidLegacyRune for both contexts, causing colons to be preserved when escaping label names. Added ValidationContext parameter to isValidLegacyRune and EscapeName so callers can specify metric or label context. EscapeMetricFamily updated to pass the correct context at each call site. Fixes prometheus/prometheus#18380 Signed-off-by: shankalpaLamichhane <shankalpa.lamichhane@gmail.com>
|
Hello from the Bug Scrub Please not mark this issue as |
ywwg
left a comment
ywwg
left a comment
There was a problem hiding this comment.
Thank you for putting this together. I made some notes -- please make sure to find all the places we are incorrectly calling IsValidLegacyMetricName on labels and make sure the tests are covering those cases as well.
|
|
||
| for _, l := range m.Label { | ||
| if l.GetName() == MetricNameLabel { | ||
| if l.Value == nil || IsValidLegacyMetricName(l.GetValue()) { |
| // Colons are reserved for metric names (e.g. recording rules) only. | ||
| // They have never been valid in label names. |
There was a problem hiding this comment.
No need to talk about history in comments
| // Colons are reserved for metric names (e.g. recording rules) only. | |
| // They have never been valid in label names. | |
| // Colons are reserved for metric names (e.g. recording rules) only. |
|
|
||
| func isValidLegacyRune(b rune, i int) bool { | ||
| return (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || b == '_' || b == ':' || (b >= '0' && b <= '9' && i > 0) | ||
| func isValidLegacyRune(r rune, i int, ctx ValidationContext) bool { |
There was a problem hiding this comment.
ctx is go idiomatic for a context.Context, and it's confusing to use it for this simple enum value. this is more of a NameType than a validation context.
| // ValidationContext determines whether a name being validated or escaped | ||
| // is a metric name or a label name. Colons are only valid in metric names. | ||
| type ValidationContext int | ||
|
|
||
| const ( | ||
| ContextMetric ValidationContext = iota | ||
| ContextLabel | ||
| ) | ||
|
|
There was a problem hiding this comment.
"context" is not quite the right word here, we want something that describes a concrete object, not the context that it exists in. How about:
| // ValidationContext determines whether a name being validated or escaped | |
| // is a metric name or a label name. Colons are only valid in metric names. | |
| type ValidationContext int | |
| const ( | |
| ContextMetric ValidationContext = iota | |
| ContextLabel | |
| ) | |
| // NameClass determines whether a name being validated or escaped | |
| // is a metric name or a label name. | |
| type NameClass int | |
| const ( | |
| MetricClass NameClass = iota | |
| LabelClass | |
| ) | |
A few more options: AttributeClass, NameType, ...
|
|
||
| // TestEscapeNameLabelContext verifies that colons are correctly escaped when | ||
| // EscapeName is called with ContextLabel. | ||
| // Colons are reserved for metric names only and must never be preserved in escaped label names. |
There was a problem hiding this comment.
Not strictly true. We do preserve colons in label names when we are doing NoEscaping. (whereas quote marks must be escaped even during "NoEscaping"...)
| // Colons are reserved for metric names only and must never be preserved in escaped label names. | |
| // Colons are legacy valid in metric names, but not label names. |
3 participants
Fixes prometheus/prometheus#18380
Colons are only valid in metric names, not label names. EscapeName was using the same isValidLegacyRune for both contexts, causing colons to be preserved when escaping label names.
Added ValidationContext parameter to isValidLegacyRune and EscapeName so callers can specify metric or label context. EscapeMetricFamily updated to pass the correct context at each call site.
@roidelapluie @gotjosh