Skip to content

model: fix EscapeName incorrectly preserving colon in label names #889

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.

Sign up for GitHub

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

Open
shankalpaLamichhane wants to merge 1 commit into
base: main
Choose a base branch
from
Open

model: fix EscapeName incorrectly preserving colon in label names #889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 31 additions & 11 deletions model/metric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,15 @@ var _ interface {
fmt.Stringer
} = new(ValidationScheme)

// 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
)

Comment on lines +89 to +97
Copy link
Copy Markdown
Member

@ywwg ywwg May 12, 2026

Choose a reason for hiding this comment

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

"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:

Suggested change
// 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, ...

// String returns the string representation of s.
func (s ValidationScheme) String() string {
switch s {
Expand Down Expand Up @@ -165,7 +174,7 @@ func (s ValidationScheme) IsValidMetricName(metricName string) bool {
return false
}
for i, b := range metricName {
if !isValidLegacyRune(b, i) {
if !isValidLegacyRune(b, i, ContextMetric) {
return false
}
}
Expand Down Expand Up @@ -353,7 +362,7 @@ func EscapeMetricFamily(v *dto.MetricFamily, scheme EscapingScheme) *dto.MetricF
if v.Name == nil || IsValidLegacyMetricName(v.GetName()) {
out.Name = v.Name
} else {
out.Name = proto.String(EscapeName(v.GetName(), scheme))
out.Name = proto.String(EscapeName(v.GetName(), scheme, ContextMetric))
}
for _, m := range v.Metric {
if !metricNeedsEscaping(m) {
Expand All @@ -378,7 +387,7 @@ func EscapeMetricFamily(v *dto.MetricFamily, scheme EscapingScheme) *dto.MetricF
}
escaped.Label = append(escaped.Label, &dto.LabelPair{
Name: proto.String(MetricNameLabel),
Value: proto.String(EscapeName(l.GetValue(), scheme)),
Value: proto.String(EscapeName(l.GetValue(), scheme, ContextMetric)),
})
continue
}
Expand All @@ -387,7 +396,7 @@ func EscapeMetricFamily(v *dto.MetricFamily, scheme EscapingScheme) *dto.MetricF
continue
}
escaped.Label = append(escaped.Label, &dto.LabelPair{
Name: proto.String(EscapeName(l.GetName(), scheme)),
Name: proto.String(EscapeName(l.GetName(), scheme, ContextLabel)),
Value: l.Value,
})
}
Expand All @@ -412,7 +421,7 @@ func metricNeedsEscaping(m *dto.Metric) bool {
// scheme. Depending on the rules of escaping, this may cause no change in the
// string that is returned. (Especially NoEscaping, which by definition is a
// noop). This function does not do any validation of the name.
func EscapeName(name string, scheme EscapingScheme) string {
func EscapeName(name string, scheme EscapingScheme, ctx ValidationContext) string {
if len(name) == 0 {
return name
}
Expand All @@ -421,11 +430,14 @@ func EscapeName(name string, scheme EscapingScheme) string {
case NoEscaping:
return name
case UnderscoreEscaping:
if IsValidLegacyMetricName(name) {
if ctx == ContextMetric && IsValidLegacyMetricName(name) {
return name
}
if ctx == ContextLabel && LegacyValidation.IsValidLabelName(name) {
return name
}
for i, b := range name {
if isValidLegacyRune(b, i) {
if isValidLegacyRune(b, i, ctx) {
escaped.WriteRune(b)
} else {
escaped.WriteRune('_')
Expand All @@ -440,7 +452,7 @@ func EscapeName(name string, scheme EscapingScheme) string {
escaped.WriteString("__")
case b == '.':
escaped.WriteString("_dot_")
case isValidLegacyRune(b, i):
case isValidLegacyRune(b, i, ctx):
escaped.WriteRune(b)
default:
escaped.WriteString("__")
Expand All @@ -456,7 +468,7 @@ func EscapeName(name string, scheme EscapingScheme) string {
switch {
case b == '_':
escaped.WriteString("__")
case isValidLegacyRune(b, i):
case isValidLegacyRune(b, i, ctx):
escaped.WriteRune(b)
case !utf8.ValidRune(b):
escaped.WriteString("_FFFD_")
Expand Down Expand Up @@ -555,8 +567,16 @@ func UnescapeName(name string, scheme EscapingScheme) string {
}
}

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 {
Copy link
Copy Markdown
Member

@ywwg ywwg May 12, 2026

Choose a reason for hiding this comment

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

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.

if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || r == '_' || (r >= '0' && r <= '9' && i > 0) {
return true
}
// Colons are reserved for metric names (e.g. recording rules) only.
// They have never been valid in label names.
Comment on lines +574 to +575
Copy link
Copy Markdown
Member

@ywwg ywwg May 12, 2026

Choose a reason for hiding this comment

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

No need to talk about history in comments

Suggested change
// 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.

if ctx == ContextMetric && r == ':' {
return true
}
return false
}

func (e EscapingScheme) String() string {
Expand Down
72 changes: 69 additions & 3 deletions model/metric_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,72 @@ func TestValidationScheme_IsMetricNameValid(t *testing.T) {
}
}

// 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.
Copy link
Copy Markdown
Member

@ywwg ywwg May 12, 2026

Choose a reason for hiding this comment

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

Not strictly true. We do preserve colons in label names when we are doing NoEscaping. (whereas quote marks must be escaped even during "NoEscaping"...)

Suggested change
// 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.

// Ref: https://github.com/prometheus/prometheus/issues/18380
func TestEscapeNameLabelContext(t *testing.T) {
scenarios := []struct {
name string
input string
scheme EscapingScheme
want string
}{
{
name: "colon and hyphen in label - underscores",
input: "app:instance-id",
scheme: UnderscoreEscaping,
want: "app_instance_id",
},
{
name: "colon and dot in label - underscores",
input: "http.status:sum",
scheme: UnderscoreEscaping,
want: "http_status_sum",
},
{
name: "colon and dot in label - dots",
input: "http.status:sum",
scheme: DotsEscaping,
want: "http_dot_status__sum",
},
{
name: "colon and hyphen in label - dots",
input: "app:instance-id",
scheme: DotsEscaping,
want: "app__instance__id",
},
{
name: "colon and hyphen in label - values",
input: "app:instance-id",
scheme: ValueEncodingEscaping,
want: "U__app_3a_instance_2d_id",
},
{
name: "colon only in label - underscores",
input: "app:service",
scheme: UnderscoreEscaping,
want: "app_service",
},
{
name: "no colon - label and metric should agree",
input: "my_metric_name",
scheme: UnderscoreEscaping,
want: "my_metric_name",
},
}

for _, scenario := range scenarios {
t.Run(scenario.name, func(t *testing.T) {
got := EscapeName(scenario.input, scenario.scheme, ContextLabel)
if got != scenario.want {
t.Errorf("EscapeName(%q, %v, ContextLabel) = %q, want %q",
scenario.input, scenario.scheme, got, scenario.want)
}
})
}
}

func TestMetricClone(t *testing.T) {
m := Metric{
"first_name": "electro",
Expand Down Expand Up @@ -531,7 +597,7 @@ func TestEscapeName(t *testing.T) {

for _, scenario := range scenarios {
t.Run(scenario.name, func(t *testing.T) {
got := EscapeName(scenario.input, UnderscoreEscaping)
got := EscapeName(scenario.input, UnderscoreEscaping, ContextMetric)
if got != scenario.expectedUnderscores {
t.Errorf("expected string output %s but got %s", scenario.expectedUnderscores, got)
}
Expand All @@ -541,7 +607,7 @@ func TestEscapeName(t *testing.T) {
t.Errorf("expected unescaped string output %s but got %s", scenario.expectedUnderscores, got)
}

got = EscapeName(scenario.input, DotsEscaping)
got = EscapeName(scenario.input, DotsEscaping, ContextMetric)
if got != scenario.expectedDots {
t.Errorf("expected string output %s but got %s", scenario.expectedDots, got)
}
Expand All @@ -550,7 +616,7 @@ func TestEscapeName(t *testing.T) {
t.Errorf("expected unescaped string output %s but got %s", scenario.expectedUnescapedDots, got)
}

got = EscapeName(scenario.input, ValueEncodingEscaping)
got = EscapeName(scenario.input, ValueEncodingEscaping, ContextMetric)
if got != scenario.expectedValue {
t.Errorf("expected string output %s but got %s", scenario.expectedValue, got)
}
Expand Down
Loading