From 7ea4b4c4a6e7da22d04d4a76c89d5ac44fb25c86 Mon Sep 17 00:00:00 2001 From: Owen Williams Date: Mon, 4 Dec 2023 13:20:43 -0500 Subject: [PATCH] Try the library-wide setting more cleanup and simplification --- expfmt/decode.go | 13 +++---------- expfmt/decode_test.go | 12 +++++++----- expfmt/openmetrics_create.go | 6 +++--- expfmt/text_create.go | 6 +++--- model/alert.go | 6 +++--- model/alert_test.go | 2 +- model/labels.go | 35 +++++++++++++++++++++-------------- model/labels_test.go | 6 ++++-- model/labelset.go | 16 +++++++++++----- model/labelset_test.go | 9 +++------ model/metric.go | 35 +++++++++++++++++++++++++++++------ model/metric_test.go | 6 ++++-- model/silence.go | 6 +++--- model/silence_test.go | 7 +++++-- 14 files changed, 100 insertions(+), 65 deletions(-) diff --git a/expfmt/decode.go b/expfmt/decode.go index 6bf6e067..0ca86a3d 100644 --- a/expfmt/decode.go +++ b/expfmt/decode.go @@ -79,16 +79,9 @@ func NewDecoder(r io.Reader, format Format) Decoder { return &textDecoder{r: r} } -// WithUTF8Names enables support for UTF-8 metric and label names. -func (d *protoDecoder) WithUTF8Names(enable bool) Decoder { - d.utf8Names = enable - return d -} - // protoDecoder implements the Decoder interface for protocol buffers. type protoDecoder struct { - r io.Reader - utf8Names bool // if false, metric names will be constrained to MetricNameRE. Otherwise, strings are checked to be valid UTF8. + r io.Reader } // Decode implements the Decoder interface. @@ -97,7 +90,7 @@ func (d *protoDecoder) Decode(v *dto.MetricFamily) error { if err != nil { return err } - if !model.IsValidMetricName(model.LabelValue(v.GetName()), d.utf8Names) { + if !model.IsValidMetricName(model.LabelValue(v.GetName())) { return fmt.Errorf("invalid metric name %q", v.GetName()) } for _, m := range v.GetMetric() { @@ -111,7 +104,7 @@ func (d *protoDecoder) Decode(v *dto.MetricFamily) error { if !model.LabelValue(l.GetValue()).IsValid() { return fmt.Errorf("invalid label value %q", l.GetValue()) } - if !model.LabelName(l.GetName()).IsValid(d.utf8Names) { + if !model.LabelName(l.GetName()).IsValid() { return fmt.Errorf("invalid label name %q", l.GetName()) } } diff --git a/expfmt/decode_test.go b/expfmt/decode_test.go index 5e63cdef..ad3f0209 100644 --- a/expfmt/decode_test.go +++ b/expfmt/decode_test.go @@ -370,6 +370,7 @@ func TestProtoDecoder(t *testing.T) { var all model.Vector for { + model.NameValidationScheme = model.LegacyValidation var smpls model.Vector err := dec.Decode(&smpls) if err == io.EOF { @@ -379,11 +380,9 @@ func TestProtoDecoder(t *testing.T) { if err == nil { t.Fatal("Expected error when decoding without UTF-8 support enabled but got none") } - dec := &SampleDecoder{ - Dec: &protoDecoder{ - r: strings.NewReader(scenario.in), - utf8Names: true, - }, + model.NameValidationScheme = model.UTF8Validation + dec = &SampleDecoder{ + Dec: &protoDecoder{r: strings.NewReader(scenario.in)}, Opts: &DecodeOptions{ Timestamp: testTime, }, @@ -392,6 +391,9 @@ func TestProtoDecoder(t *testing.T) { if err == io.EOF { break } + if err != nil { + t.Fatalf("Unexpected error when decoding with UTF-8 support: %v", err) + } } if scenario.fail { if err == nil { diff --git a/expfmt/openmetrics_create.go b/expfmt/openmetrics_create.go index 768e0010..85579aaa 100644 --- a/expfmt/openmetrics_create.go +++ b/expfmt/openmetrics_create.go @@ -92,7 +92,7 @@ func MetricFamilyToOpenMetrics(out io.Writer, in *dto.MetricFamily) (written int } // If the name does not satisfy the legacy validity check, we must quote it. quotedName := shortName - if !model.IsValidMetricName(model.LabelValue(quotedName), false) { + if !model.IsValidLegacyMetricName(model.LabelValue(quotedName)) { quotedName = fmt.Sprintf(`"%s"`, quotedName) } @@ -375,7 +375,7 @@ func writeOpenMetricsNameAndLabelPairs( if name != "" { // If the name does not pass the legacy validity check, we must put the // metric name inside the braces, quoted. - if !model.IsValidMetricName(model.LabelValue(name), false) { + if !model.IsValidLegacyMetricName(model.LabelValue(name)) { metricInsideBraces = true err := w.WriteByte(separator) written++ @@ -410,7 +410,7 @@ func writeOpenMetricsNameAndLabelPairs( return written, err } labelName := lp.GetName() - if !model.IsValidMetricName(model.LabelValue(labelName), false) { + if !model.IsValidLegacyMetricName(model.LabelValue(labelName)) { labelName = fmt.Sprintf(`"%s"`, labelName) } n, err := w.WriteString(labelName) diff --git a/expfmt/text_create.go b/expfmt/text_create.go index e23414b4..d0e862ee 100644 --- a/expfmt/text_create.go +++ b/expfmt/text_create.go @@ -75,7 +75,7 @@ func MetricFamilyToText(out io.Writer, in *dto.MetricFamily) (written int, err e // If the name does not satisfy the legacy validity check, we must quote it. quotedName := name - if !model.IsValidMetricName(model.LabelValue(quotedName), false) { + if !model.IsValidLegacyMetricName(model.LabelValue(quotedName)) { quotedName = fmt.Sprintf(`"%s"`, quotedName) } // Try the interface upgrade. If it doesn't work, we'll use a @@ -345,7 +345,7 @@ func writeNameAndLabelPairs( if name != "" { // If the name does not pass the legacy validity check, we must put the // metric name inside the braces. Note, it will already have been - if !model.IsValidMetricName(model.LabelValue(name), false) { + if !model.IsValidLegacyMetricName(model.LabelValue(name)) { metricInsideBraces = true err := w.WriteByte(separator) written++ @@ -380,7 +380,7 @@ func writeNameAndLabelPairs( return written, err } labelName := lp.GetName() - if !model.IsValidMetricName(model.LabelValue(labelName), false) { + if !model.IsValidLegacyMetricName(model.LabelValue(labelName)) { labelName = fmt.Sprintf(`"%s"`, labelName) } n, err := w.WriteString(labelName) diff --git a/model/alert.go b/model/alert.go index daf6a807..35e739c7 100644 --- a/model/alert.go +++ b/model/alert.go @@ -82,20 +82,20 @@ func (a *Alert) Status() AlertStatus { } // Validate checks whether the alert data is inconsistent. -func (a *Alert) Validate(utf8Names bool) error { +func (a *Alert) Validate() error { if a.StartsAt.IsZero() { return fmt.Errorf("start time missing") } if !a.EndsAt.IsZero() && a.EndsAt.Before(a.StartsAt) { return fmt.Errorf("start time must be before end time") } - if err := a.Labels.Validate(utf8Names); err != nil { + if err := a.Labels.Validate(); err != nil { return fmt.Errorf("invalid label set: %s", err) } if len(a.Labels) == 0 { return fmt.Errorf("at least one label pair required") } - if err := a.Annotations.Validate(utf8Names); err != nil { + if err := a.Annotations.Validate(); err != nil { return fmt.Errorf("invalid annotations: %s", err) } return nil diff --git a/model/alert_test.go b/model/alert_test.go index ef6bacd0..8910da7a 100644 --- a/model/alert_test.go +++ b/model/alert_test.go @@ -101,7 +101,7 @@ func TestAlertValidate(t *testing.T) { } for i, c := range cases { - err := c.alert.Validate(false) + err := c.alert.Validate() if err == nil { if c.err == "" { continue diff --git a/model/labels.go b/model/labels.go index f7970fba..973d87ce 100644 --- a/model/labels.go +++ b/model/labels.go @@ -15,6 +15,7 @@ package model import ( "encoding/json" + "fmt" "regexp" "strings" "unicode/utf8" @@ -100,41 +101,47 @@ type LabelName string // for legacy names, and iff it's valid UTF-8 if isUtf8 is true. // This function, however, does not use LabelNameRE for the check but a much // faster hardcoded implementation. -func (ln LabelName) IsValid(isUtf8 bool) bool { +func (ln LabelName) IsValid() bool { if len(ln) == 0 { return false } - if isUtf8 { - return utf8.ValidString(string(ln)) - } - for i, b := range ln { - if !((b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || b == '_' || (b >= '0' && b <= '9' && i > 0)) { - return false - } + switch NameValidationScheme { + case LegacyValidation: + for i, b := range ln { + if !((b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || b == '_' || (b >= '0' && b <= '9' && i > 0)) { + return false + } + } + case UTF8Validation: + return utf8.ValidString(string(ln)) + default: + panic(fmt.Sprintf("Invalid name validation scheme requested: %d", NameValidationScheme)) } return true } -// UnmarshalYAML implements the yaml.Unmarshaler interface. It does not do -// validation of the names. Callers should call Validate on the resulting name -// themselves. +// UnmarshalYAML implements the yaml.Unmarshaler interface. func (ln *LabelName) UnmarshalYAML(unmarshal func(interface{}) error) error { var s string if err := unmarshal(&s); err != nil { return err } + if !LabelName(s).IsValid() { + return fmt.Errorf("%q is not a valid label name", s) + } *ln = LabelName(s) return nil } -// UnmarshalJSON implements the json.Unmarshaler interface. It does not do -// validation of the names. Callers should call Validate on the resulting name -// themselves. +// UnmarshalJSON implements the json.Unmarshaler interface. func (ln *LabelName) UnmarshalJSON(b []byte) error { var s string if err := json.Unmarshal(b, &s); err != nil { return err } + if !LabelName(s).IsValid() { + return fmt.Errorf("%q is not a valid label name", s) + } *ln = LabelName(s) return nil } diff --git a/model/labels_test.go b/model/labels_test.go index e232e8bb..4087dc71 100644 --- a/model/labels_test.go +++ b/model/labels_test.go @@ -144,13 +144,15 @@ func TestLabelNameIsValid(t *testing.T) { } for _, s := range scenarios { - if s.ln.IsValid(false) != s.legacyValid { + NameValidationScheme = LegacyValidation + if s.ln.IsValid() != s.legacyValid { t.Errorf("Expected %v for %q using legacy IsValid method", s.legacyValid, s.ln) } if LabelNameRE.MatchString(string(s.ln)) != s.legacyValid { t.Errorf("Expected %v for %q using legacy regexp match", s.legacyValid, s.ln) } - if s.ln.IsValid(true) != s.utf8Valid { + NameValidationScheme = UTF8Validation + if s.ln.IsValid() != s.utf8Valid { t.Errorf("Expected %v for %q using UTF8 IsValid method", s.legacyValid, s.ln) } } diff --git a/model/labelset.go b/model/labelset.go index ac161823..6eda08a7 100644 --- a/model/labelset.go +++ b/model/labelset.go @@ -29,9 +29,9 @@ type LabelSet map[LabelName]LabelValue // Validate checks whether all names and values in the label set // are valid. -func (ls LabelSet) Validate(utf8Names bool) error { +func (ls LabelSet) Validate() error { for ln, lv := range ls { - if !ln.IsValid(utf8Names) { + if !ln.IsValid() { return fmt.Errorf("invalid name %q", ln) } if !lv.IsValid() { @@ -150,14 +150,20 @@ func (ls LabelSet) FastFingerprint() Fingerprint { return labelSetToFastFingerprint(ls) } -// UnmarshalJSON implements the json.Unmarshaler interface. It does not do -// validation of the names. Callers should call Validate on the resulting set -// themselves. +// UnmarshalJSON implements the json.Unmarshaler interface. func (l *LabelSet) UnmarshalJSON(b []byte) error { var m map[LabelName]LabelValue if err := json.Unmarshal(b, &m); err != nil { return err } + // encoding/json only unmarshals maps of the form map[string]T. It treats + // LabelName as a string and does not call its UnmarshalJSON method. + // Thus, we have to replicate the behavior here. + for ln := range m { + if !ln.IsValid() { + return fmt.Errorf("%q is not a valid label name", ln) + } + } *l = LabelSet(m) return nil } diff --git a/model/labelset_test.go b/model/labelset_test.go index 734dfcd7..cfa70020 100644 --- a/model/labelset_test.go +++ b/model/labelset_test.go @@ -53,14 +53,11 @@ func TestUnmarshalJSONLabelSet(t *testing.T) { } }` + NameValidationScheme = LegacyValidation err = json.Unmarshal([]byte(invalidlabelSetJSON), &c) - if err != nil { - t.Errorf("expected no error, got: %v", err) - } - err = c.LabelSet.Validate(false) - expectedErr := `invalid name "1nvalid_23name"` + expectedErr := `"1nvalid_23name" is not a valid label name` if err == nil || err.Error() != expectedErr { - t.Errorf("expected an error with message '%s' to be thrown, got: %v", expectedErr, err) + t.Errorf("expected an error with message '%s' to be thrown", expectedErr) } } diff --git a/model/metric.go b/model/metric.go index c8a05a65..fc3a0c22 100644 --- a/model/metric.go +++ b/model/metric.go @@ -21,11 +21,19 @@ import ( "unicode/utf8" ) +type validationSchemeId int + +const ( + LegacyValidation = validationSchemeId(0) + UTF8Validation = validationSchemeId(1) +) + var ( // MetricNameRE is a regular expression matching valid metric // names. Note that the IsValidMetricName function performs the same // check but faster than a match with this regular expression. MetricNameRE = regexp.MustCompile(`^[a-zA-Z_:][a-zA-Z0-9_:]*$`) + NameValidationScheme = LegacyValidation ) // A Metric is similar to a LabelSet, but the key difference is that a Metric is @@ -87,17 +95,14 @@ func (m Metric) FastFingerprint() Fingerprint { return LabelSet(m).FastFingerprint() } -// IsValidMetricName returns true iff name matches the pattern of MetricNameRE -// for legacy names, and iff it's valid UTF-8 if isUtf8 is true. +// IsValidLegacyMetricName returns true iff name matches the pattern of MetricNameRE +// for legacy names. // This function, however, does not use MetricNameRE for the check but a much // faster hardcoded implementation. -func IsValidMetricName(n LabelValue, isUtf8 bool) bool { +func IsValidLegacyMetricName(n LabelValue) bool { if len(n) == 0 { return false } - if isUtf8 { - return utf8.ValidString(string(n)) - } for i, b := range n { if !((b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || b == '_' || b == ':' || (b >= '0' && b <= '9' && i > 0)) { return false @@ -105,3 +110,21 @@ func IsValidMetricName(n LabelValue, isUtf8 bool) bool { } return true } + +// IsValidMetricName returns true iff name matches the pattern of MetricNameRE +// for legacy names, and iff it's valid UTF-8 if isUtf8 is true. +// This function, however, does not use MetricNameRE for the check but a much +// faster hardcoded implementation. +func IsValidMetricName(n LabelValue) bool { + switch NameValidationScheme { + case LegacyValidation: + return IsValidLegacyMetricName(n) + case UTF8Validation: + if len(n) == 0 { + return false + } + return utf8.ValidString(string(n)) + default: + panic(fmt.Sprintf("Invalid name validation scheme requested: %d", NameValidationScheme)) + } +} diff --git a/model/metric_test.go b/model/metric_test.go index 46b0dbe4..0e2f9802 100644 --- a/model/metric_test.go +++ b/model/metric_test.go @@ -141,13 +141,15 @@ func TestMetricNameIsLegacyValid(t *testing.T) { } for _, s := range scenarios { - if IsValidMetricName(s.mn, false) != s.legacyValid { + NameValidationScheme = LegacyValidation + if IsValidMetricName(s.mn) != s.legacyValid { t.Errorf("Expected %v for %q using legacy IsValidMetricName method", s.legacyValid, s.mn) } if MetricNameRE.MatchString(string(s.mn)) != s.legacyValid { t.Errorf("Expected %v for %q using regexp matching", s.legacyValid, s.mn) } - if IsValidMetricName(s.mn, true) != s.utf8Valid { + NameValidationScheme = UTF8Validation + if IsValidMetricName(s.mn) != s.utf8Valid { t.Errorf("Expected %v for %q using utf8 IsValidMetricName method", s.legacyValid, s.mn) } } diff --git a/model/silence.go b/model/silence.go index 6fe37283..bb99889d 100644 --- a/model/silence.go +++ b/model/silence.go @@ -45,8 +45,8 @@ func (m *Matcher) UnmarshalJSON(b []byte) error { } // Validate returns true iff all fields of the matcher have valid values. -func (m *Matcher) Validate(utf8Names bool) error { - if !m.Name.IsValid(utf8Names) { +func (m *Matcher) Validate() error { + if !m.Name.IsValid() { return fmt.Errorf("invalid name %q", m.Name) } if m.IsRegex { @@ -80,7 +80,7 @@ func (s *Silence) Validate() error { return fmt.Errorf("at least one matcher required") } for _, m := range s.Matchers { - if err := m.Validate(false); err != nil { + if err := m.Validate(); err != nil { return fmt.Errorf("invalid matcher: %s", err) } } diff --git a/model/silence_test.go b/model/silence_test.go index 0a8bf6a4..5b79f86d 100644 --- a/model/silence_test.go +++ b/model/silence_test.go @@ -80,8 +80,10 @@ func TestMatcherValidate(t *testing.T) { } for i, c := range cases { - legacyErr := c.matcher.Validate(false) - utf8Err := c.matcher.Validate(true) + NameValidationScheme = LegacyValidation + legacyErr := c.matcher.Validate() + NameValidationScheme = UTF8Validation + utf8Err := c.matcher.Validate() if legacyErr == nil && utf8Err == nil { if c.legacyErr == "" && c.utf8Err == "" { continue @@ -246,6 +248,7 @@ func TestSilenceValidate(t *testing.T) { } for i, c := range cases { + NameValidationScheme = LegacyValidation err := c.sil.Validate() if err == nil { if c.err == "" {