Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 57 additions & 2 deletions model/metric.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

dto "github.com/prometheus/client_model/go"
"google.golang.org/protobuf/proto"
"gopkg.in/yaml.v2"
Comment thread
aknuds1 marked this conversation as resolved.
)

var (
Expand Down Expand Up @@ -62,16 +63,70 @@ var (
type ValidationScheme int

const (
// UnsetValidation represents an undefined ValidationScheme.
// Should not be used in practice.
UnsetValidation ValidationScheme = iota

// LegacyValidation is a setting that requires that all metric and label names
// conform to the original Prometheus character requirements described by
// MetricNameRE and LabelNameRE.
LegacyValidation ValidationScheme = iota
LegacyValidation
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you check if the change of ordering here is going to cause any issues?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not really a change of ordering is it, but a change of default, right? It's a breaking change if anyone depends on LegacyValidation being the default, but that's a bad idea since you can't tell whether the enum is set or not. I'm not sure how to figure out if this will break anyone, but I can try to search for cases in Prometheus and Mimir.

Copy link
Copy Markdown
Member

@jesusvazquez jesusvazquez Jul 1, 2025

Choose a reason for hiding this comment

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

but that's a bad idea since you can't tell whether the enum is set or not.

I see that now,. Well if you cant find a dependency in mimir or prom to the current default then I'm happy with this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jesusvazquez - I will try to look a bit.

Copy link
Copy Markdown
Contributor Author

@aknuds1 aknuds1 Jul 3, 2025

Choose a reason for hiding this comment

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

I don't see anything breaking in Prometheus/Mimir from this change at least.

I also did some more testing, and found that YAML decoding depends on UnsetValidation being the default enum value. The reason being that when unmarshalling from an empty string, ValidationScheme.UnmarshalYAML isn't called, so you always get the default value. If LegacyValidation remains the default, you can't tell whether the user has configured the validation scheme or not in YAML.


// UTF8Validation only requires that metric and label names be valid UTF-8
// strings.
UTF8Validation
)

var (
_ yaml.Marshaler = UnsetValidation
_ fmt.Stringer = UnsetValidation
)

// String returns the string representation of s.
func (s ValidationScheme) String() string {
switch s {
case UnsetValidation:
return "unset"
case LegacyValidation:
return "legacy"
case UTF8Validation:
return "utf8"
default:
panic(fmt.Errorf("unhandled ValidationScheme: %d", s))
}
}

// MarshalYAML implements the yaml.Marshaler interface.
func (s ValidationScheme) MarshalYAML() (any, error) {
switch s {
Comment thread
aknuds1 marked this conversation as resolved.
case UnsetValidation:
return "", nil
case LegacyValidation, UTF8Validation:
return s.String(), nil
default:
panic(fmt.Errorf("unhandled ValidationScheme: %d", s))
Comment thread
aknuds1 marked this conversation as resolved.
}
}

// UnmarshalYAML implements the yaml.Unmarshaler interface.
func (s *ValidationScheme) UnmarshalYAML(unmarshal func(any) error) error {
var scheme string
if err := unmarshal(&scheme); err != nil {
return err
}
switch scheme {
case "":
// Don't change the value.
case "legacy":
*s = LegacyValidation
case "utf8":
*s = UTF8Validation
default:
return fmt.Errorf("unrecognized ValidationScheme: %q", scheme)
}
return nil
}

type EscapingScheme int

const (
Expand Down Expand Up @@ -185,7 +240,7 @@ func IsValidMetricName(n LabelValue) bool {
}
return utf8.ValidString(string(n))
default:
panic(fmt.Sprintf("Invalid name validation scheme requested: %d", NameValidationScheme))
panic(fmt.Sprintf("Invalid name validation scheme requested: %s", NameValidationScheme.String()))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: since we implement fmt.Stringer anyways

Suggested change
panic(fmt.Sprintf("Invalid name validation scheme requested: %s", NameValidationScheme.String()))
panic(fmt.Sprintf("Invalid name validation scheme requested: %v", NameValidationScheme))

Copy link
Copy Markdown
Contributor Author

@aknuds1 aknuds1 Jul 1, 2025

Choose a reason for hiding this comment

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

I don't understand the change to %v though, %s is more specific and thus better IMO. There's no real benefit to dropping the .String() call, it just becomes implicit.

}
}

Expand Down
113 changes: 113 additions & 0 deletions model/metric_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@
package model

import (
"errors"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
dto "github.com/prometheus/client_model/go"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"gopkg.in/yaml.v2"
)

func testMetric(t testing.TB) {
Expand Down Expand Up @@ -89,6 +93,115 @@ func BenchmarkMetric(b *testing.B) {
}
}

func TestValidationScheme(t *testing.T) {
var scheme ValidationScheme
require.Equal(t, UnsetValidation, scheme)
}

func TestValidationScheme_String(t *testing.T) {
for _, tc := range []struct {
name string
scheme ValidationScheme
want string
}{
{
name: "Unset",
scheme: UnsetValidation,
want: "unset",
},
{
name: "Legacy",
scheme: LegacyValidation,
want: "legacy",
},
{
name: "UTF8",
scheme: UTF8Validation,
want: "utf8",
},
} {
t.Run(tc.name, func(t *testing.T) {
require.Equal(t, tc.want, tc.scheme.String())
})
}
}

func TestValidationScheme_MarshalYAML(t *testing.T) {
for _, tc := range []struct {
name string
scheme ValidationScheme
want string
}{
{
name: "Unset",
scheme: UnsetValidation,
want: `""`,
},
{
name: "Legacy",
scheme: LegacyValidation,
want: "legacy",
},
{
name: "UTF8",
scheme: UTF8Validation,
want: "utf8",
},
} {
t.Run(tc.name, func(t *testing.T) {
marshaled, err := yaml.Marshal(tc.scheme)
require.NoError(t, err)
require.Equal(t, tc.want, strings.TrimSpace(string(marshaled)))
})
}
}

func TestValidationScheme_UnmarshalYAML(t *testing.T) {
for _, tc := range []struct {
name string
input string
want ValidationScheme
wantError error
}{
{
name: "Unset empty input",
input: "",
want: UnsetValidation,
},
{
name: "Unset quoted input",
input: `""`,
want: UnsetValidation,
},
{
name: "Legacy",
input: "legacy",
want: LegacyValidation,
},
{
name: "UTF8",
input: "utf8",
want: UTF8Validation,
},
{
name: "Invalid",
input: "invalid",
wantError: errors.New(`unrecognized ValidationScheme: "invalid"`),
},
} {
t.Run(tc.name, func(t *testing.T) {
scheme := UnsetValidation
err := yaml.Unmarshal([]byte(tc.input), &scheme)
if tc.wantError == nil {
require.NoError(t, err)
require.Equal(t, tc.want, scheme)
} else {
require.EqualError(t, err, tc.wantError.Error())
}
})
}
}

func TestMetricNameIsLegacyValid(t *testing.T) {
scenarios := []struct {
mn LabelValue
Expand Down
Loading