model.ValidationScheme: Support encoding as YAML #799
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances ValidationScheme to support YAML encoding/decoding, introduces a default UnsetValidation, and adds a String method for human-readable output.
- Adds
UnsetValidationas the zero-value scheme and shifts existing constants - Implements
String(),MarshalYAML(), andUnmarshalYAML()onValidationScheme - Provides unit tests covering string conversion and YAML round‐trip behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| model/metric.go | Define UnsetValidation, implement String, YAML marshal/unmarshal, and add interface assertions |
| model/metric_test.go | Add tests for default value, String(), MarshalYAML(), and UnmarshalYAML() behaviors |
Comments suppressed due to low confidence (3)
model/metric.go:68
- Introducing
UnsetValidationshifts the numeric values ofLegacyValidationandUTF8Validation, which may break existing consumers; consider assigning explicit values (e.g.,LegacyValidation = 0,UTF8Validation = 1) or documenting the breaking change.
UnsetValidation ValidationScheme = iota
model/metric.go:27
- The code calls
fmt.Errorfandfmt.Sprintfbutfmtis not imported, causing a compile error; addimport "fmt".
"gopkg.in/yaml.v2"
model/metric.go:120
- [nitpick] Relying on the pre-existing value of
*swhen the input is empty can be unclear; explicitly setting*s = UnsetValidationmay improve readability and avoid unintended state retention.
case "":
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
7de91e0 to
ec69d9c
Compare
| 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())) |
There was a problem hiding this comment.
nit: since we implement fmt.Stringer anyways
| panic(fmt.Sprintf("Invalid name validation scheme requested: %s", NameValidationScheme.String())) | |
| panic(fmt.Sprintf("Invalid name validation scheme requested: %v", NameValidationScheme)) |
There was a problem hiding this comment.
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.
jesusvazquez
left a comment
There was a problem hiding this comment.
Overall looks good to me, left a few questions, lets have a round with the answers and then i can have another look.
| // conform to the original Prometheus character requirements described by | ||
| // MetricNameRE and LabelNameRE. | ||
| LegacyValidation ValidationScheme = iota | ||
| LegacyValidation |
There was a problem hiding this comment.
Did you check if the change of ordering here is going to cause any issues?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks @jesusvazquez - I will try to look a bit.
There was a problem hiding this comment.
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.
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Modify
model.ValidationSchemeto support encoding as YAML. To facilitate this, I add a default value for the type:UnsetValidation. I also add aStringmethod to the type, for use in error messages etc (exemplified inIsValidMetricName).The motivation is to consolidate Prometheus around
model.ValidationScheme, dropping the duplicated representationsconfig.LegacyValidationConfigandconfig.UTF8ValidationConfig. This will in turn facilitate moving Prometheus (and e.g. Grafana Mimir) off the deprecatedmodel.NameValidationSchemeglobal.See prometheus/prometheus#16806 for reference, where I implement the consolidation around
model.ValidationScheme.Default value changing to
UnsetValidationFrom my testing, without changing the default value to
UnsetValidation, you'll getLegacyValidationwhen YAML unmarshalling an empty string. This means you can't tell whether the user has set the validation scheme in a configuration file or not.