From 6779009fe096fa261b21714a81b8e27914bb3b4d Mon Sep 17 00:00:00 2001 From: George Robinson Date: Mon, 11 Dec 2023 21:41:47 +0000 Subject: [PATCH 1/7] Add metrics to matchers compat package This commit adds the following metrics to the compat package: alertmanager_matchers_parse_total alertmanager_matchers_disagree_total alertmanager_matchers_incompatible_total alertmanager_matchers_invalid_total With a label called origin to differentiate the different sources of inputs: the configuration file, the API, and amtool. The disagree_total metric is incremented when an input is invalid in both parsers, but results in different parsed representations, then there is disagreement. This should not happen, and suggests their is either a bug in one of the parsers or a mistake in the backwards compatible guarantees of the matchers/parse parser. The incompatible_total metric is incremented when an input is valid in pkg/labels, but not the UTF-8 parser in matchers/parse. In such case, the matcher should be updated to be compatible. This often means adding double quotes around the right hand side of the matcher. For example, foo="bar". The invalid_total metric is incremented when an input is invalid in both parsers. This was never a valid input. The tests have been updated to check the metrics are incremented as expected. Signed-off-by: George Robinson --- api/v2/api.go | 2 +- cli/alert_add.go | 6 +- cli/alert_query.go | 2 +- cli/root.go | 2 +- cli/silence_add.go | 4 +- cli/silence_query.go | 2 +- cli/test_routing.go | 2 +- cmd/alertmanager/main.go | 2 +- config/config.go | 4 +- matchers/compat/metrics.go | 51 ++++++++ matchers/compat/parse.go | 230 +++++++++++++++++++++------------- matchers/compat/parse_test.go | 76 +++++++---- 12 files changed, 261 insertions(+), 122 deletions(-) create mode 100644 matchers/compat/metrics.go diff --git a/api/v2/api.go b/api/v2/api.go index 3e0bc0fd9b..9d70625cd8 100644 --- a/api/v2/api.go +++ b/api/v2/api.go @@ -684,7 +684,7 @@ func (api *API) postSilencesHandler(params silence_ops.PostSilencesParams) middl func parseFilter(filter []string) ([]*labels.Matcher, error) { matchers := make([]*labels.Matcher, 0, len(filter)) for _, matcherString := range filter { - matcher, err := compat.Matcher(matcherString) + matcher, err := compat.Matcher(matcherString, "api") if err != nil { return nil, err } diff --git a/cli/alert_add.go b/cli/alert_add.go index 6018b95654..433afc0765 100644 --- a/cli/alert_add.go +++ b/cli/alert_add.go @@ -77,14 +77,14 @@ func (a *alertAddCmd) addAlert(ctx context.Context, _ *kingpin.ParseContext) err if len(a.labels) > 0 { // Allow the alertname label to be defined implicitly as the first argument rather // than explicitly as a key=value pair. - if _, err := compat.Matcher(a.labels[0]); err != nil { + if _, err := compat.Matcher(a.labels[0], "cli"); err != nil { a.labels[0] = fmt.Sprintf("alertname=%s", strconv.Quote(a.labels[0])) } } ls := make(models.LabelSet, len(a.labels)) for _, l := range a.labels { - matcher, err := compat.Matcher(l) + matcher, err := compat.Matcher(l, "cli") if err != nil { return err } @@ -96,7 +96,7 @@ func (a *alertAddCmd) addAlert(ctx context.Context, _ *kingpin.ParseContext) err annotations := make(models.LabelSet, len(a.annotations)) for _, a := range a.annotations { - matcher, err := compat.Matcher(a) + matcher, err := compat.Matcher(a, "cli") if err != nil { return err } diff --git a/cli/alert_query.go b/cli/alert_query.go index e5f5ba6ac8..e4bddaa651 100644 --- a/cli/alert_query.go +++ b/cli/alert_query.go @@ -81,7 +81,7 @@ func (a *alertQueryCmd) queryAlerts(ctx context.Context, _ *kingpin.ParseContext // the user wants alertname= and prepend `alertname=` to // the front. m := a.matcherGroups[0] - _, err := compat.Matcher(m) + _, err := compat.Matcher(m, "cli") if err != nil { a.matcherGroups[0] = fmt.Sprintf("alertname=%s", strconv.Quote(m)) } diff --git a/cli/root.go b/cli/root.go index 69c1022c6a..e1e5ac59aa 100644 --- a/cli/root.go +++ b/cli/root.go @@ -61,7 +61,7 @@ func initMatchersCompat(_ *kingpin.ParseContext) error { if err != nil { kingpin.Fatalf("error parsing the feature flag list: %v\n", err) } - compat.InitFromFlags(logger, featureConfig) + compat.InitFromFlags(logger, compat.RegisteredMetrics, featureConfig) return nil } diff --git a/cli/silence_add.go b/cli/silence_add.go index d30a523431..4456ddec9a 100644 --- a/cli/silence_add.go +++ b/cli/silence_add.go @@ -95,7 +95,7 @@ func (c *silenceAddCmd) add(ctx context.Context, _ *kingpin.ParseContext) error // If the parser fails then we likely don't have a (=|=~|!=|!~) so lets // assume that the user wants alertname= and prepend `alertname=` // to the front. - _, err := compat.Matcher(c.matchers[0]) + _, err := compat.Matcher(c.matchers[0], "cli") if err != nil { c.matchers[0] = fmt.Sprintf("alertname=%s", strconv.Quote(c.matchers[0])) } @@ -103,7 +103,7 @@ func (c *silenceAddCmd) add(ctx context.Context, _ *kingpin.ParseContext) error matchers := make([]labels.Matcher, 0, len(c.matchers)) for _, s := range c.matchers { - m, err := compat.Matcher(s) + m, err := compat.Matcher(s, "cli") if err != nil { return err } diff --git a/cli/silence_query.go b/cli/silence_query.go index b25569f818..5eb33a27d7 100644 --- a/cli/silence_query.go +++ b/cli/silence_query.go @@ -99,7 +99,7 @@ func (c *silenceQueryCmd) query(ctx context.Context, _ *kingpin.ParseContext) er // If the parser fails then we likely don't have a (=|=~|!=|!~) so lets // assume that the user wants alertname= and prepend `alertname=` // to the front. - _, err := compat.Matcher(c.matchers[0]) + _, err := compat.Matcher(c.matchers[0], "cli") if err != nil { c.matchers[0] = fmt.Sprintf("alertname=%s", strconv.Quote(c.matchers[0])) } diff --git a/cli/test_routing.go b/cli/test_routing.go index 85c29a7e2f..589a2e8cf8 100644 --- a/cli/test_routing.go +++ b/cli/test_routing.go @@ -84,7 +84,7 @@ func (c *routingShow) routingTestAction(ctx context.Context, _ *kingpin.ParseCon // Parse labels to LabelSet. ls := make(models.LabelSet, len(c.labels)) for _, l := range c.labels { - matcher, err := compat.Matcher(l) + matcher, err := compat.Matcher(l, "cli") if err != nil { kingpin.Fatalf("Failed to parse labels: %v\n", err) } diff --git a/cmd/alertmanager/main.go b/cmd/alertmanager/main.go index 15716c985c..d44a69d154 100644 --- a/cmd/alertmanager/main.go +++ b/cmd/alertmanager/main.go @@ -180,7 +180,7 @@ func run() int { level.Error(logger).Log("msg", "error parsing the feature flag list", "err", err) return 1 } - compat.InitFromFlags(logger, ff) + compat.InitFromFlags(logger, compat.RegisteredMetrics, ff) err = os.MkdirAll(*dataDir, 0o777) if err != nil { diff --git a/config/config.go b/config/config.go index 355209b467..7f3602e066 100644 --- a/config/config.go +++ b/config/config.go @@ -1006,7 +1006,7 @@ func (m *Matchers) UnmarshalYAML(unmarshal func(interface{}) error) error { return err } for _, line := range lines { - pm, err := compat.Matchers(line) + pm, err := compat.Matchers(line, "config") if err != nil { return err } @@ -1032,7 +1032,7 @@ func (m *Matchers) UnmarshalJSON(data []byte) error { return err } for _, line := range lines { - pm, err := compat.Matchers(line) + pm, err := compat.Matchers(line, "config") if err != nil { return err } diff --git a/matchers/compat/metrics.go b/matchers/compat/metrics.go new file mode 100644 index 0000000000..800695d4fc --- /dev/null +++ b/matchers/compat/metrics.go @@ -0,0 +1,51 @@ +package compat + +import ( + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promauto" +) + +var ( + RegisteredMetrics = NewMetrics(prometheus.DefaultRegisterer) +) + +const ( + OriginAPI = "api" + OriginConfig = "config" +) + +var ( + DefaultOrigins = []string{ + OriginAPI, + OriginConfig, + } +) + +type Metrics struct { + Total *prometheus.GaugeVec + DisagreeTotal *prometheus.GaugeVec + IncompatibleTotal *prometheus.GaugeVec + InvalidTotal *prometheus.GaugeVec +} + +func NewMetrics(r prometheus.Registerer) *Metrics { + m := &Metrics{ + Total: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ + Name: "alertmanager_matchers_parse_total", + Help: "Total number of matcher inputs parsed, including invalid inputs.", + }, []string{"origin"}), + DisagreeTotal: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ + Name: "alertmanager_matchers_disagree_total", + Help: "Total number of matcher inputs which produce different parsings (disagreement).", + }, []string{"origin"}), + IncompatibleTotal: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ + Name: "alertmanager_matchers_incompatible_total", + Help: "Total number of matcher inputs that are incompatible with the UTF-8 parser.", + }, []string{"origin"}), + InvalidTotal: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ + Name: "alertmanager_matchers_invalid_total", + Help: "Total number of matcher inputs that could not be parsed.", + }, []string{"origin"}), + } + return m +} diff --git a/matchers/compat/parse.go b/matchers/compat/parse.go index f2eab755c8..35950e9227 100644 --- a/matchers/compat/parse.go +++ b/matchers/compat/parse.go @@ -15,11 +15,13 @@ package compat import ( "fmt" + "reflect" "strings" "unicode/utf8" "github.com/go-kit/log" "github.com/go-kit/log/level" + "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" "github.com/prometheus/alertmanager/featurecontrol" @@ -29,8 +31,8 @@ import ( var ( isValidLabelName = isValidClassicLabelName(log.NewNopLogger()) - parseMatcher = ClassicMatcherParser(log.NewNopLogger()) - parseMatchers = ClassicMatchersParser(log.NewNopLogger()) + parseMatcher = ClassicMatcherParser(log.NewNopLogger(), RegisteredMetrics) + parseMatchers = ClassicMatchersParser(log.NewNopLogger(), RegisteredMetrics) ) // IsValidLabelName returns true if the string is a valid label name. @@ -38,143 +40,195 @@ func IsValidLabelName(name model.LabelName) bool { return isValidLabelName(name) } -type ParseMatcher func(s string) (*labels.Matcher, error) +type ParseMatcher func(input, origin string) (*labels.Matcher, error) -type ParseMatchers func(s string) (labels.Matchers, error) +type ParseMatchers func(input, origin string) (labels.Matchers, error) // Matcher parses the matcher in the input string. It returns an error // if the input is invalid or contains two or more matchers. -func Matcher(s string) (*labels.Matcher, error) { - return parseMatcher(s) +func Matcher(input, origin string) (*labels.Matcher, error) { + return parseMatcher(input, origin) } // Matchers parses one or more matchers in the input string. It returns // an error if the input is invalid. -func Matchers(s string) (labels.Matchers, error) { - return parseMatchers(s) +func Matchers(input, origin string) (labels.Matchers, error) { + return parseMatchers(input, origin) } // InitFromFlags initializes the compat package from the flagger. -func InitFromFlags(l log.Logger, f featurecontrol.Flagger) { +func InitFromFlags(l log.Logger, m *Metrics, f featurecontrol.Flagger) { if f.ClassicMode() { isValidLabelName = isValidClassicLabelName(l) - parseMatcher = ClassicMatcherParser(l) - parseMatchers = ClassicMatchersParser(l) + parseMatcher = ClassicMatcherParser(l, m) + parseMatchers = ClassicMatchersParser(l, m) } else if f.UTF8StrictMode() { isValidLabelName = isValidUTF8LabelName(l) - parseMatcher = UTF8MatcherParser(l) - parseMatchers = UTF8MatchersParser(l) + parseMatcher = UTF8MatcherParser(l, m) + parseMatchers = UTF8MatchersParser(l, m) } else { isValidLabelName = isValidUTF8LabelName(l) - parseMatcher = FallbackMatcherParser(l) - parseMatchers = FallbackMatchersParser(l) + parseMatcher = FallbackMatcherParser(l, m) + parseMatchers = FallbackMatchersParser(l, m) } } -// ClassicMatcherParser uses the old pkg/labels parser to parse the matcher in +// ClassicMatcherParser uses the pkg/labels parser to parse the matcher in // the input string. -func ClassicMatcherParser(l log.Logger) ParseMatcher { - return func(s string) (*labels.Matcher, error) { - level.Debug(l).Log("msg", "Parsing with classic matchers parser", "input", s) - return labels.ParseMatcher(s) +func ClassicMatcherParser(l log.Logger, m *Metrics) ParseMatcher { + return func(input, origin string) (matcher *labels.Matcher, err error) { + defer func() { + lbs := prometheus.Labels{"origin": origin} + m.Total.With(lbs).Inc() + if err != nil { + m.InvalidTotal.With(lbs).Inc() + } + }() + level.Debug(l).Log("msg", "Parsing with classic matchers parser", "input", input) + return labels.ParseMatcher(input) } } -// ClassicMatchersParser uses the old pkg/labels parser to parse zero or more +// ClassicMatchersParser uses the pkg/labels parser to parse zero or more // matchers in the input string. It returns an error if the input is invalid. -func ClassicMatchersParser(l log.Logger) ParseMatchers { - return func(s string) (labels.Matchers, error) { - level.Debug(l).Log("msg", "Parsing with classic matchers parser", "input", s) - return labels.ParseMatchers(s) +func ClassicMatchersParser(l log.Logger, m *Metrics) ParseMatchers { + return func(input, origin string) (matchers labels.Matchers, err error) { + defer func() { + lbs := prometheus.Labels{"origin": origin} + m.Total.With(lbs).Inc() + if err != nil { + m.InvalidTotal.With(lbs).Inc() + } + }() + level.Debug(l).Log("msg", "Parsing with classic matchers parser", "input", input) + return labels.ParseMatchers(input) } } -// UTF8MatcherParser uses the new matchers/parse parser to parse -// the matcher in the input string. If this fails it does not fallback -// to the old pkg/labels parser. -func UTF8MatcherParser(l log.Logger) ParseMatcher { - return func(s string) (*labels.Matcher, error) { - level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser", "input", s) - if strings.HasPrefix(s, "{") || strings.HasSuffix(s, "}") { - return nil, fmt.Errorf("unexpected open or close brace: %s", s) +// UTF8MatcherParser uses the new matchers/parse parser to parse the matcher +// in the input string. If this fails it does not revert to the pkg/labels parser. +func UTF8MatcherParser(l log.Logger, m *Metrics) ParseMatcher { + return func(input, origin string) (matcher *labels.Matcher, err error) { + defer func() { + lbs := prometheus.Labels{"origin": origin} + m.Total.With(lbs).Inc() + if err != nil { + m.InvalidTotal.With(lbs).Inc() + } + }() + level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser", "input", input) + if strings.HasPrefix(input, "{") || strings.HasSuffix(input, "}") { + return nil, fmt.Errorf("unexpected open or close brace: %s", input) } - return parse.Matcher(s) + return parse.Matcher(input) } } -// UTF8MatchersParser uses the new matchers/parse parser to parse -// zero or more matchers in the input string. If this fails it -// does not fallback to the old pkg/labels parser. -func UTF8MatchersParser(l log.Logger) ParseMatchers { - return func(s string) (labels.Matchers, error) { - level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser", "input", s) - return parse.Matchers(s) +// UTF8MatchersParser uses the new matchers/parse parser to parse zero or more +// matchers in the input string. If this fails it does not revert to the +// pkg/labels parser. +func UTF8MatchersParser(l log.Logger, m *Metrics) ParseMatchers { + return func(input, origin string) (matchers labels.Matchers, err error) { + defer func() { + lbs := prometheus.Labels{"origin": origin} + m.Total.With(lbs).Inc() + if err != nil { + m.InvalidTotal.With(lbs).Inc() + } + }() + level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser", "input", input) + return parse.Matchers(input) } } -// FallbackMatcherParser uses the new matchers/parse parser to parse -// zero or more matchers in the string. If this fails it falls back to -// the old pkg/labels parser and emits a warning log line. -func FallbackMatcherParser(l log.Logger) ParseMatcher { - return func(s string) (*labels.Matcher, error) { - var ( - m *labels.Matcher - err error - invalidErr error - ) - level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser, with fallback to classic matchers parser", "input", s) - if strings.HasPrefix(s, "{") || strings.HasSuffix(s, "}") { - return nil, fmt.Errorf("unexpected open or close brace: %s", s) +// FallbackMatcherParser uses the new matchers/parse parser to parse zero or more +// matchers in the string. If this fails it reverts to the pkg/labels parser and +// emits a warning log line. +func FallbackMatcherParser(l log.Logger, m *Metrics) ParseMatcher { + return func(input, origin string) (matcher *labels.Matcher, err error) { + lbs := prometheus.Labels{"origin": origin} + defer func() { + m.Total.With(lbs).Inc() + if err != nil { + m.InvalidTotal.With(lbs).Inc() + } + }() + level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser, with fallback to classic matchers parser", "input", input) + if strings.HasPrefix(input, "{") || strings.HasSuffix(input, "}") { + return nil, fmt.Errorf("unexpected open or close brace: %s", input) } - m, err = parse.Matcher(s) - if err != nil { - m, invalidErr = labels.ParseMatcher(s) - if invalidErr != nil { - // The input is not valid in the old pkg/labels parser either, - // it cannot be valid input. - return nil, invalidErr + // Parse the input in both parsers to look for disagreement and incompatible + // inputs. + nMatcher, nErr := parse.Matcher(input) + cMatcher, cErr := labels.ParseMatcher(input) + if nErr != nil { + // If the input is invalid in both parsers, return the error. + if cErr != nil { + return nil, cErr } - // The input is valid in the old pkg/labels parser, but not the - // new matchers/parse parser. - suggestion := m.String() - level.Warn(l).Log("msg", "Alertmanager is moving to a new parser for labels and matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue.", "input", s, "err", err, "suggestion", suggestion) + // The input is valid in the pkg/labels parser, but not the matchers/parse + // parser. This means the input is not forwards compatible. + m.IncompatibleTotal.With(lbs).Inc() + suggestion := cMatcher.String() + level.Warn(l).Log("msg", "Alertmanager is moving to a new parser for labels and matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue.", "input", input, "err", err, "suggestion", suggestion) + return cMatcher, nil + } + // If the input is valid in both parsers, but produces different results, + // then there is disagreement. + if nErr == nil && cErr == nil && !reflect.DeepEqual(nMatcher, cMatcher) { + m.DisagreeTotal.With(lbs).Inc() + level.Warn(l).Log("msg", "Matchers input has disagreement", "input", input) } - return m, nil + return nMatcher, nil } } // FallbackMatchersParser uses the new matchers/parse parser to parse the -// matcher in the input string. If this fails it falls back to the old -// pkg/labels parser and emits a warning log line. -func FallbackMatchersParser(l log.Logger) ParseMatchers { - return func(s string) (labels.Matchers, error) { - var ( - m []*labels.Matcher - err error - invalidErr error - ) - level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser, with fallback to classic matchers parser", "input", s) - m, err = parse.Matchers(s) - if err != nil { - m, invalidErr = labels.ParseMatchers(s) - if invalidErr != nil { - // The input is not valid in the old pkg/labels parser either, - // it cannot be valid input. - return nil, invalidErr +// matcher in the input string. If this fails it falls back to the pkg/labels +// parser and emits a warning log line. +func FallbackMatchersParser(l log.Logger, m *Metrics) ParseMatchers { + return func(input, origin string) (matchers labels.Matchers, err error) { + lbs := prometheus.Labels{"origin": origin} + defer func() { + m.Total.With(lbs).Inc() + if err != nil { + m.InvalidTotal.With(lbs).Inc() } + }() + level.Debug(l).Log("msg", "Parsing with UTF-8 matchers parser, with fallback to classic matchers parser", "input", input) + // Parse the input in both parsers to look for disagreement and incompatible + // inputs. + nMatchers, nErr := parse.Matchers(input) + cMatchers, cErr := labels.ParseMatchers(input) + if nErr != nil { + // If the input is invalid in both parsers, return the error. + if cErr != nil { + return nil, cErr + } + // The input is valid in the pkg/labels parser, but not the matchers/parse + // parser. This means the input is not forwards compatible. + m.IncompatibleTotal.With(lbs).Inc() var sb strings.Builder - for i, n := range m { + for i, n := range cMatchers { sb.WriteString(n.String()) - if i < len(m)-1 { + if i < len(cMatchers)-1 { sb.WriteRune(',') } } suggestion := sb.String() - // The input is valid in the old pkg/labels parser, but not the + // The input is valid in the pkg/labels parser, but not the // new matchers/parse parser. - level.Warn(l).Log("msg", "Alertmanager is moving to a new parser for labels and matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue.", "input", s, "err", err, "suggestion", suggestion) + level.Warn(l).Log("msg", "Alertmanager is moving to a new parser for labels and matchers, and this input is incompatible. Alertmanager has instead parsed the input using the old matchers parser as a fallback. To make this input compatible with the new parser please make sure all regular expressions and values are double-quoted. If you are still seeing this message please open an issue.", "input", input, "err", err, "suggestion", suggestion) + return cMatchers, nil + } + // If the input is valid in both parsers, but produces different results, + // then there is disagreement. We need to compare to labels.Matchers(cMatchers) + // as cMatchers is a []*labels.Matcher not labels.Matchers. + if nErr == nil && cErr == nil && !reflect.DeepEqual(nMatchers, labels.Matchers(cMatchers)) { + m.DisagreeTotal.With(lbs).Inc() + level.Warn(l).Log("msg", "Matchers input has disagreement", "input", input) } - return m, nil + return nMatchers, nil } } diff --git a/matchers/compat/parse_test.go b/matchers/compat/parse_test.go index 8343ea17f3..3b006de00e 100644 --- a/matchers/compat/parse_test.go +++ b/matchers/compat/parse_test.go @@ -17,6 +17,8 @@ import ( "testing" "github.com/go-kit/log" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" "github.com/prometheus/common/model" "github.com/stretchr/testify/require" @@ -25,47 +27,67 @@ import ( func TestFallbackMatcherParser(t *testing.T) { tests := []struct { - name string - input string - expected *labels.Matcher - err string + name string + input string + expected *labels.Matcher + err string + total int + disagreeTotal int + incompatibleTotal int + invalidTotal int }{{ name: "is accepted in both", input: "foo=bar", expected: mustNewMatcher(t, labels.MatchEqual, "foo", "bar"), + total: 1, }, { name: "is accepted in new parser but not old", input: "foo🙂=bar", expected: mustNewMatcher(t, labels.MatchEqual, "foo🙂", "bar"), + total: 1, }, { - name: "is accepted in old parser but not new", - input: "foo=!bar\\n", - expected: mustNewMatcher(t, labels.MatchEqual, "foo", "!bar\n"), + name: "is accepted in old parser but not new", + input: "foo=!bar\\n", + expected: mustNewMatcher(t, labels.MatchEqual, "foo", "!bar\n"), + total: 1, + incompatibleTotal: 1, }, { - name: "is accepted in neither", - input: "foo!bar", - err: "bad matcher format: foo!bar", + name: "is accepted in neither", + input: "foo!bar", + err: "bad matcher format: foo!bar", + total: 1, + invalidTotal: 1, }} - f := FallbackMatcherParser(log.NewNopLogger()) + for _, test := range tests { + m := NewMetrics(prometheus.NewRegistry()) + f := FallbackMatcherParser(log.NewNopLogger(), m) t.Run(test.name, func(t *testing.T) { - matcher, err := f(test.input) + matcher, err := f(test.input, "test") if test.err != "" { require.EqualError(t, err, test.err) } else { require.NoError(t, err) require.EqualValues(t, test.expected, matcher) } + require.Equal(t, test.total, testutil.CollectAndCount(m.Total)) + require.Equal(t, test.disagreeTotal, testutil.CollectAndCount(m.DisagreeTotal)) + require.Equal(t, test.incompatibleTotal, testutil.CollectAndCount(m.IncompatibleTotal)) + require.Equal(t, test.invalidTotal, testutil.CollectAndCount(m.InvalidTotal)) }) } } func TestFallbackMatchersParser(t *testing.T) { tests := []struct { - name string - input string - expected labels.Matchers - err string + name string + input string + expected labels.Matchers + err string + total int + disagreeTotal int + incompatibleTotal int + invalidTotal int }{{ name: "is accepted in both", input: "{foo=bar,bar=baz}", @@ -73,6 +95,7 @@ func TestFallbackMatchersParser(t *testing.T) { mustNewMatcher(t, labels.MatchEqual, "foo", "bar"), mustNewMatcher(t, labels.MatchEqual, "bar", "baz"), }, + total: 1, }, { name: "is accepted in new parser but not old", input: "{foo🙂=bar,bar=baz🙂}", @@ -80,6 +103,7 @@ func TestFallbackMatchersParser(t *testing.T) { mustNewMatcher(t, labels.MatchEqual, "foo🙂", "bar"), mustNewMatcher(t, labels.MatchEqual, "bar", "baz🙂"), }, + total: 1, }, { name: "is accepted in old parser but not new", input: "{foo=!bar,bar=$baz\\n}", @@ -87,21 +111,31 @@ func TestFallbackMatchersParser(t *testing.T) { mustNewMatcher(t, labels.MatchEqual, "foo", "!bar"), mustNewMatcher(t, labels.MatchEqual, "bar", "$baz\n"), }, + total: 1, + incompatibleTotal: 1, }, { - name: "is accepted in neither", - input: "{foo!bar}", - err: "bad matcher format: foo!bar", + name: "is accepted in neither", + input: "{foo!bar}", + err: "bad matcher format: foo!bar", + total: 1, + invalidTotal: 1, }} - f := FallbackMatchersParser(log.NewNopLogger()) + for _, test := range tests { + m := NewMetrics(prometheus.NewRegistry()) + f := FallbackMatchersParser(log.NewNopLogger(), m) t.Run(test.name, func(t *testing.T) { - matchers, err := f(test.input) + matchers, err := f(test.input, "test") if test.err != "" { require.EqualError(t, err, test.err) } else { require.NoError(t, err) require.EqualValues(t, test.expected, matchers) } + require.Equal(t, test.total, testutil.CollectAndCount(m.Total)) + require.Equal(t, test.disagreeTotal, testutil.CollectAndCount(m.DisagreeTotal)) + require.Equal(t, test.incompatibleTotal, testutil.CollectAndCount(m.IncompatibleTotal)) + require.Equal(t, test.invalidTotal, testutil.CollectAndCount(m.InvalidTotal)) }) } } From 7cf1ba6c40721866cae49227231ba1c195600baa Mon Sep 17 00:00:00 2001 From: George Robinson Date: Wed, 3 Jan 2024 21:43:16 +0000 Subject: [PATCH 2/7] Fix license Signed-off-by: George Robinson --- matchers/compat/metrics.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/matchers/compat/metrics.go b/matchers/compat/metrics.go index 800695d4fc..7551fc3516 100644 --- a/matchers/compat/metrics.go +++ b/matchers/compat/metrics.go @@ -1,3 +1,16 @@ +// Copyright 2023 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package compat import ( From b2930f09114c1e3b825734ba01a23e369f32719e Mon Sep 17 00:00:00 2001 From: George Robinson Date: Thu, 4 Jan 2024 11:47:15 +0000 Subject: [PATCH 3/7] Fix lint Signed-off-by: George Robinson --- matchers/compat/metrics.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/matchers/compat/metrics.go b/matchers/compat/metrics.go index 7551fc3516..5b6a559a14 100644 --- a/matchers/compat/metrics.go +++ b/matchers/compat/metrics.go @@ -18,21 +18,17 @@ import ( "github.com/prometheus/client_golang/prometheus/promauto" ) -var ( - RegisteredMetrics = NewMetrics(prometheus.DefaultRegisterer) -) - const ( OriginAPI = "api" OriginConfig = "config" ) -var ( - DefaultOrigins = []string{ - OriginAPI, - OriginConfig, - } -) +var DefaultOrigins = []string{ + OriginAPI, + OriginConfig, +} + +var RegisteredMetrics = NewMetrics(prometheus.DefaultRegisterer) type Metrics struct { Total *prometheus.GaugeVec From 1c8c1f094ce51bc5dc8b0ebc1180a6301a89a56c Mon Sep 17 00:00:00 2001 From: George Robinson Date: Thu, 4 Jan 2024 18:52:47 +0000 Subject: [PATCH 4/7] Add tests for disagreement Signed-off-by: George Robinson --- matchers/compat/parse_test.go | 101 ++++++++++++++++++++++------------ 1 file changed, 65 insertions(+), 36 deletions(-) diff --git a/matchers/compat/parse_test.go b/matchers/compat/parse_test.go index 3b006de00e..a026b8f570 100644 --- a/matchers/compat/parse_test.go +++ b/matchers/compat/parse_test.go @@ -31,38 +31,47 @@ func TestFallbackMatcherParser(t *testing.T) { input string expected *labels.Matcher err string - total int - disagreeTotal int - incompatibleTotal int - invalidTotal int + total float64 + disagreeTotal float64 + incompatibleTotal float64 + invalidTotal float64 }{{ - name: "is accepted in both", + name: "input is accepted", input: "foo=bar", expected: mustNewMatcher(t, labels.MatchEqual, "foo", "bar"), total: 1, }, { - name: "is accepted in new parser but not old", + name: "input is accepted in neither", + input: "foo!bar", + err: "bad matcher format: foo!bar", + total: 1, + invalidTotal: 1, + }, { + name: "input is accepted in matchers/parse but not pkg/labels", input: "foo🙂=bar", expected: mustNewMatcher(t, labels.MatchEqual, "foo🙂", "bar"), total: 1, }, { - name: "is accepted in old parser but not new", + name: "input is accepted in pkg/labels but not matchers/parse", input: "foo=!bar\\n", expected: mustNewMatcher(t, labels.MatchEqual, "foo", "!bar\n"), total: 1, incompatibleTotal: 1, }, { - name: "is accepted in neither", - input: "foo!bar", - err: "bad matcher format: foo!bar", - total: 1, - invalidTotal: 1, + // This input causes disagreement because \xf0\x9f\x99\x82 is the byte sequence for 🙂, + // which is not understood by pkg/labels but is understood by matchers/parse. In such cases, + // the fallback parser returns the result from pkg/labels. + name: "input causes disagreement", + input: "foo=\"\\xf0\\x9f\\x99\\x82\"", + expected: mustNewMatcher(t, labels.MatchEqual, "foo", "\\xf0\\x9f\\x99\\x82"), + total: 1, + disagreeTotal: 1, }} for _, test := range tests { - m := NewMetrics(prometheus.NewRegistry()) - f := FallbackMatcherParser(log.NewNopLogger(), m) t.Run(test.name, func(t *testing.T) { + m := NewMetrics(prometheus.NewRegistry()) + f := FallbackMatcherParser(log.NewNopLogger(), m) matcher, err := f(test.input, "test") if test.err != "" { require.EqualError(t, err, test.err) @@ -70,10 +79,10 @@ func TestFallbackMatcherParser(t *testing.T) { require.NoError(t, err) require.EqualValues(t, test.expected, matcher) } - require.Equal(t, test.total, testutil.CollectAndCount(m.Total)) - require.Equal(t, test.disagreeTotal, testutil.CollectAndCount(m.DisagreeTotal)) - require.Equal(t, test.incompatibleTotal, testutil.CollectAndCount(m.IncompatibleTotal)) - require.Equal(t, test.invalidTotal, testutil.CollectAndCount(m.InvalidTotal)) + requireMetric(t, test.total, m.Total) + requireMetric(t, test.disagreeTotal, m.DisagreeTotal) + requireMetric(t, test.incompatibleTotal, m.IncompatibleTotal) + requireMetric(t, test.invalidTotal, m.InvalidTotal) }) } } @@ -84,12 +93,12 @@ func TestFallbackMatchersParser(t *testing.T) { input string expected labels.Matchers err string - total int - disagreeTotal int - incompatibleTotal int - invalidTotal int + total float64 + disagreeTotal float64 + incompatibleTotal float64 + invalidTotal float64 }{{ - name: "is accepted in both", + name: "input is accepted", input: "{foo=bar,bar=baz}", expected: labels.Matchers{ mustNewMatcher(t, labels.MatchEqual, "foo", "bar"), @@ -97,7 +106,13 @@ func TestFallbackMatchersParser(t *testing.T) { }, total: 1, }, { - name: "is accepted in new parser but not old", + name: "input is accepted in neither", + input: "{foo!bar}", + err: "bad matcher format: foo!bar", + total: 1, + invalidTotal: 1, + }, { + name: "input is accepted in matchers/parse but not pkg/labels", input: "{foo🙂=bar,bar=baz🙂}", expected: labels.Matchers{ mustNewMatcher(t, labels.MatchEqual, "foo🙂", "bar"), @@ -105,7 +120,7 @@ func TestFallbackMatchersParser(t *testing.T) { }, total: 1, }, { - name: "is accepted in old parser but not new", + name: "is accepted in pkg/labels but not matchers/parse", input: "{foo=!bar,bar=$baz\\n}", expected: labels.Matchers{ mustNewMatcher(t, labels.MatchEqual, "foo", "!bar"), @@ -114,17 +129,22 @@ func TestFallbackMatchersParser(t *testing.T) { total: 1, incompatibleTotal: 1, }, { - name: "is accepted in neither", - input: "{foo!bar}", - err: "bad matcher format: foo!bar", - total: 1, - invalidTotal: 1, + // This input causes disagreement because \xf0\x9f\x99\x82 is the byte sequence for 🙂, + // which is not understood by pkg/labels but is understood by matchers/parse. In such cases, + // the fallback parser returns the result from pkg/labels. + name: "input causes disagreement", + input: "{foo=\"\\xf0\\x9f\\x99\\x82\"}", + expected: labels.Matchers{ + mustNewMatcher(t, labels.MatchEqual, "foo", "\\xf0\\x9f\\x99\\x82"), + }, + total: 1, + disagreeTotal: 1, }} for _, test := range tests { - m := NewMetrics(prometheus.NewRegistry()) - f := FallbackMatchersParser(log.NewNopLogger(), m) t.Run(test.name, func(t *testing.T) { + m := NewMetrics(prometheus.NewRegistry()) + f := FallbackMatchersParser(log.NewNopLogger(), m) matchers, err := f(test.input, "test") if test.err != "" { require.EqualError(t, err, test.err) @@ -132,10 +152,10 @@ func TestFallbackMatchersParser(t *testing.T) { require.NoError(t, err) require.EqualValues(t, test.expected, matchers) } - require.Equal(t, test.total, testutil.CollectAndCount(m.Total)) - require.Equal(t, test.disagreeTotal, testutil.CollectAndCount(m.DisagreeTotal)) - require.Equal(t, test.incompatibleTotal, testutil.CollectAndCount(m.IncompatibleTotal)) - require.Equal(t, test.invalidTotal, testutil.CollectAndCount(m.InvalidTotal)) + requireMetric(t, test.total, m.Total) + requireMetric(t, test.disagreeTotal, m.DisagreeTotal) + requireMetric(t, test.incompatibleTotal, m.IncompatibleTotal) + requireMetric(t, test.invalidTotal, m.InvalidTotal) }) } } @@ -207,3 +227,12 @@ func TestIsValidUTF8LabelName(t *testing.T) { }) } } + +func requireMetric(t *testing.T, expected float64, m *prometheus.GaugeVec) { + if expected == 0 { + require.Equal(t, 0, testutil.CollectAndCount(m)) + } else { + require.Equal(t, 1, testutil.CollectAndCount(m)) + require.Equal(t, expected, testutil.ToFloat64(m)) + } +} From 852cb76e334d05d9edf6b4ecac97f1db1dd4a58f Mon Sep 17 00:00:00 2001 From: George Robinson Date: Thu, 4 Jan 2024 18:52:55 +0000 Subject: [PATCH 5/7] Fix bug where disagreement returned wrong matchers This commit fixes a bug where an input that caused disagreement would return the result of matchers/parse and not pkg/labels. This is to prevent disagreement breaking configurations such as routes should unexpected disagreement occur (i.e. disagreement that is not due to added support for UTF-8). Signed-off-by: George Robinson --- matchers/compat/parse.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/matchers/compat/parse.go b/matchers/compat/parse.go index 35950e9227..9eb09f836b 100644 --- a/matchers/compat/parse.go +++ b/matchers/compat/parse.go @@ -178,6 +178,7 @@ func FallbackMatcherParser(l log.Logger, m *Metrics) ParseMatcher { if nErr == nil && cErr == nil && !reflect.DeepEqual(nMatcher, cMatcher) { m.DisagreeTotal.With(lbs).Inc() level.Warn(l).Log("msg", "Matchers input has disagreement", "input", input) + return cMatcher, nil } return nMatcher, nil } @@ -227,6 +228,7 @@ func FallbackMatchersParser(l log.Logger, m *Metrics) ParseMatchers { if nErr == nil && cErr == nil && !reflect.DeepEqual(nMatchers, labels.Matchers(cMatchers)) { m.DisagreeTotal.With(lbs).Inc() level.Warn(l).Log("msg", "Matchers input has disagreement", "input", input) + return cMatchers, nil } return nMatchers, nil } From e52adb717d391693f90758e8e47ccbc54d7f1ed1 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Fri, 5 Jan 2024 09:52:31 +0000 Subject: [PATCH 6/7] Fix metric names as no longer counters Signed-off-by: George Robinson --- matchers/compat/metrics.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/matchers/compat/metrics.go b/matchers/compat/metrics.go index 5b6a559a14..a3d47abc85 100644 --- a/matchers/compat/metrics.go +++ b/matchers/compat/metrics.go @@ -40,19 +40,19 @@ type Metrics struct { func NewMetrics(r prometheus.Registerer) *Metrics { m := &Metrics{ Total: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ - Name: "alertmanager_matchers_parse_total", + Name: "alertmanager_matchers_parsed", Help: "Total number of matcher inputs parsed, including invalid inputs.", }, []string{"origin"}), DisagreeTotal: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ - Name: "alertmanager_matchers_disagree_total", + Name: "alertmanager_matchers_disagree", Help: "Total number of matcher inputs which produce different parsings (disagreement).", }, []string{"origin"}), IncompatibleTotal: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ - Name: "alertmanager_matchers_incompatible_total", + Name: "alertmanager_matchers_incompatible", Help: "Total number of matcher inputs that are incompatible with the UTF-8 parser.", }, []string{"origin"}), InvalidTotal: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ - Name: "alertmanager_matchers_invalid_total", + Name: "alertmanager_matchers_invalid", Help: "Total number of matcher inputs that could not be parsed.", }, []string{"origin"}), } From 37934cd1d5f8e0d869faa6d73e0f2f306d72977b Mon Sep 17 00:00:00 2001 From: George Robinson Date: Fri, 5 Jan 2024 09:56:01 +0000 Subject: [PATCH 7/7] Use present tense Signed-off-by: George Robinson --- matchers/compat/metrics.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/matchers/compat/metrics.go b/matchers/compat/metrics.go index a3d47abc85..4741cf182b 100644 --- a/matchers/compat/metrics.go +++ b/matchers/compat/metrics.go @@ -40,7 +40,7 @@ type Metrics struct { func NewMetrics(r prometheus.Registerer) *Metrics { m := &Metrics{ Total: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{ - Name: "alertmanager_matchers_parsed", + Name: "alertmanager_matchers_parse", Help: "Total number of matcher inputs parsed, including invalid inputs.", }, []string{"origin"}), DisagreeTotal: promauto.With(r).NewGaugeVec(prometheus.GaugeOpts{