From f69a5086657bc69049f207945702a7436cb93840 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Thu, 8 Feb 2024 09:59:03 +0000 Subject: [PATCH] Remove metrics from compat package (#3714) This commit removes the metrics from the compat package in favour of the existing logging and the additional tools at hand, such as amtool, to validate Alertmanager configurations. Due to the global nature of the compat package, a consequence of config.Load, these metrics have proven to be less useful in practice than expected, both in Alertmanager and other projects such as Mimir. There are a number of reasons for this: 1. Because the compat package is global, these metrics cannot be reset each time config.Load is called, as in multi-tenant projects like Mimir loading a config for one tenant would reset the metrics for all tenants. This is also the reason the metrics are counters and not gauges. 2. Since the metrics are counters, it is difficult to create meaningful dashboards for Alertmanager as, unlike in Mimir, configurations are not reloaded at fixed intervals, and as such, operators cannot use rate to track configuration changes over time. In Alertmanager, there are much better tools available to validate that an Alertmanager configuration is compatible with the UTF-8 parser, including both the existing logging from Alertmanager server and amtool check-config. In other projects like Mimir, we can track configurations for individual tenants using log aggregation and storage systems such as Loki. This gives operators far more information than what is possible with the metrics, including the timestamp, input and ID of tenant configurations that are incompatible or have disagreement. Signed-off-by: George Robinson --- cli/root.go | 2 +- cmd/alertmanager/main.go | 2 +- matchers/compat/metrics.go | 60 ----------------------- matchers/compat/parse.go | 77 ++++++------------------------ matchers/compat/parse_test.go | 89 +++++++++-------------------------- silence/silence_test.go | 4 +- types/types_test.go | 4 +- 7 files changed, 43 insertions(+), 195 deletions(-) delete mode 100644 matchers/compat/metrics.go diff --git a/cli/root.go b/cli/root.go index a94db391..69c1022c 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, compat.NewMetrics(nil), featureConfig) + compat.InitFromFlags(logger, featureConfig) return nil } diff --git a/cmd/alertmanager/main.go b/cmd/alertmanager/main.go index cb225b2e..b2938189 100644 --- a/cmd/alertmanager/main.go +++ b/cmd/alertmanager/main.go @@ -186,7 +186,7 @@ func run() int { level.Error(logger).Log("msg", "error parsing the feature flag list", "err", err) return 1 } - compat.InitFromFlags(logger, compat.RegisteredMetrics, ff) + compat.InitFromFlags(logger, ff) err = os.MkdirAll(*dataDir, 0o777) if err != nil { diff --git a/matchers/compat/metrics.go b/matchers/compat/metrics.go deleted file mode 100644 index 34b64099..00000000 --- a/matchers/compat/metrics.go +++ /dev/null @@ -1,60 +0,0 @@ -// 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 ( - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promauto" -) - -const ( - OriginAPI = "api" - OriginConfig = "config" -) - -var DefaultOrigins = []string{ - OriginAPI, - OriginConfig, -} - -var RegisteredMetrics = NewMetrics(prometheus.DefaultRegisterer) - -type Metrics struct { - Total *prometheus.CounterVec - DisagreeTotal *prometheus.CounterVec - IncompatibleTotal *prometheus.CounterVec - InvalidTotal *prometheus.CounterVec -} - -func NewMetrics(r prometheus.Registerer) *Metrics { - m := &Metrics{ - Total: promauto.With(r).NewCounterVec(prometheus.CounterOpts{ - Name: "alertmanager_matchers_parse_total", - Help: "Total number of matcher inputs parsed, including invalid inputs.", - }, []string{"origin"}), - DisagreeTotal: promauto.With(r).NewCounterVec(prometheus.CounterOpts{ - Name: "alertmanager_matchers_disagree_total", - Help: "Total number of matcher inputs which produce different parsings (disagreement).", - }, []string{"origin"}), - IncompatibleTotal: promauto.With(r).NewCounterVec(prometheus.CounterOpts{ - 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).NewCounterVec(prometheus.CounterOpts{ - 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 e6b2758b..7aa4e2d9 100644 --- a/matchers/compat/parse.go +++ b/matchers/compat/parse.go @@ -21,7 +21,6 @@ import ( "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" @@ -31,8 +30,8 @@ import ( var ( isValidLabelName = isValidClassicLabelName(log.NewNopLogger()) - parseMatcher = ClassicMatcherParser(log.NewNopLogger(), RegisteredMetrics) - parseMatchers = ClassicMatchersParser(log.NewNopLogger(), RegisteredMetrics) + parseMatcher = ClassicMatcherParser(log.NewNopLogger()) + parseMatchers = ClassicMatchersParser(log.NewNopLogger()) ) // IsValidLabelName returns true if the string is a valid label name. @@ -57,33 +56,26 @@ func Matchers(input, origin string) (labels.Matchers, error) { } // InitFromFlags initializes the compat package from the flagger. -func InitFromFlags(l log.Logger, m *Metrics, f featurecontrol.Flagger) { +func InitFromFlags(l log.Logger, f featurecontrol.Flagger) { if f.ClassicMode() { isValidLabelName = isValidClassicLabelName(l) - parseMatcher = ClassicMatcherParser(l, m) - parseMatchers = ClassicMatchersParser(l, m) + parseMatcher = ClassicMatcherParser(l) + parseMatchers = ClassicMatchersParser(l) } else if f.UTF8StrictMode() { isValidLabelName = isValidUTF8LabelName(l) - parseMatcher = UTF8MatcherParser(l, m) - parseMatchers = UTF8MatchersParser(l, m) + parseMatcher = UTF8MatcherParser(l) + parseMatchers = UTF8MatchersParser(l) } else { isValidLabelName = isValidUTF8LabelName(l) - parseMatcher = FallbackMatcherParser(l, m) - parseMatchers = FallbackMatchersParser(l, m) + parseMatcher = FallbackMatcherParser(l) + parseMatchers = FallbackMatchersParser(l) } } // ClassicMatcherParser uses the pkg/labels parser to parse the matcher in // the input string. -func ClassicMatcherParser(l log.Logger, m *Metrics) ParseMatcher { +func ClassicMatcherParser(l log.Logger) 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, "origin", origin) return labels.ParseMatcher(input) } @@ -91,15 +83,8 @@ func ClassicMatcherParser(l log.Logger, m *Metrics) ParseMatcher { // 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, m *Metrics) ParseMatchers { +func ClassicMatchersParser(l log.Logger) 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, "origin", origin) return labels.ParseMatchers(input) } @@ -107,15 +92,8 @@ func ClassicMatchersParser(l log.Logger, m *Metrics) ParseMatchers { // 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 { +func UTF8MatcherParser(l log.Logger) 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, "origin", origin) if strings.HasPrefix(input, "{") || strings.HasSuffix(input, "}") { return nil, fmt.Errorf("unexpected open or close brace: %s", input) @@ -127,15 +105,8 @@ func UTF8MatcherParser(l log.Logger, m *Metrics) ParseMatcher { // 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 { +func UTF8MatchersParser(l log.Logger) 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, "origin", origin) return parse.Matchers(input) } @@ -144,15 +115,8 @@ func UTF8MatchersParser(l log.Logger, m *Metrics) ParseMatchers { // 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 { +func FallbackMatcherParser(l log.Logger) 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, "origin", origin) if strings.HasPrefix(input, "{") || strings.HasSuffix(input, "}") { return nil, fmt.Errorf("unexpected open or close brace: %s", input) @@ -168,7 +132,6 @@ func FallbackMatcherParser(l log.Logger, m *Metrics) ParseMatcher { } // 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, "origin", origin, "err", nErr, "suggestion", suggestion) return cMatcher, nil @@ -176,7 +139,6 @@ func FallbackMatcherParser(l log.Logger, m *Metrics) ParseMatcher { // 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, "origin", origin) return cMatcher, nil } @@ -187,15 +149,8 @@ func FallbackMatcherParser(l log.Logger, m *Metrics) ParseMatcher { // FallbackMatchersParser uses the new matchers/parse parser to parse the // 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 { +func FallbackMatchersParser(l log.Logger) 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, "origin", origin) // Parse the input in both parsers to look for disagreement and incompatible // inputs. @@ -208,7 +163,6 @@ func FallbackMatchersParser(l log.Logger, m *Metrics) ParseMatchers { } // 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 cMatchers { sb.WriteString(n.String()) @@ -226,7 +180,6 @@ func FallbackMatchersParser(l log.Logger, m *Metrics) ParseMatchers { // 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, "origin", origin) return cMatchers, nil } diff --git a/matchers/compat/parse_test.go b/matchers/compat/parse_test.go index d1490437..bb417ffb 100644 --- a/matchers/compat/parse_test.go +++ b/matchers/compat/parse_test.go @@ -17,8 +17,6 @@ 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" @@ -27,51 +25,38 @@ import ( func TestFallbackMatcherParser(t *testing.T) { tests := []struct { - name string - input string - expected *labels.Matcher - err string - total float64 - disagreeTotal float64 - incompatibleTotal float64 - invalidTotal float64 + name string + input string + expected *labels.Matcher + err string }{{ name: "input is accepted", input: "foo=bar", expected: mustNewMatcher(t, labels.MatchEqual, "foo", "bar"), - total: 1, }, { - name: "input is accepted in neither", - input: "foo!bar", - err: "bad matcher format: foo!bar", - total: 1, - invalidTotal: 1, + name: "input is accepted in neither", + input: "foo!bar", + err: "bad matcher format: foo!bar", }, { name: "input is accepted in matchers/parse but not pkg/labels", input: "foo🙂=bar", expected: mustNewMatcher(t, labels.MatchEqual, "foo🙂", "bar"), - total: 1, }, { - 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: "input is accepted in pkg/labels but not matchers/parse", + input: "foo=!bar\\n", + expected: mustNewMatcher(t, labels.MatchEqual, "foo", "!bar\n"), }, { // 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, + name: "input causes disagreement", + input: "foo=\"\\xf0\\x9f\\x99\\x82\"", + expected: mustNewMatcher(t, labels.MatchEqual, "foo", "\\xf0\\x9f\\x99\\x82"), }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { - m := NewMetrics(prometheus.NewRegistry()) - f := FallbackMatcherParser(log.NewNopLogger(), m) + f := FallbackMatcherParser(log.NewNopLogger()) matcher, err := f(test.input, "test") if test.err != "" { require.EqualError(t, err, test.err) @@ -79,24 +64,16 @@ func TestFallbackMatcherParser(t *testing.T) { require.NoError(t, err) require.EqualValues(t, test.expected, matcher) } - requireMetric(t, test.total, m.Total) - requireMetric(t, test.disagreeTotal, m.DisagreeTotal) - requireMetric(t, test.incompatibleTotal, m.IncompatibleTotal) - requireMetric(t, test.invalidTotal, m.InvalidTotal) }) } } func TestFallbackMatchersParser(t *testing.T) { tests := []struct { - name string - input string - expected labels.Matchers - err string - total float64 - disagreeTotal float64 - incompatibleTotal float64 - invalidTotal float64 + name string + input string + expected labels.Matchers + err string }{{ name: "input is accepted", input: "{foo=bar,bar=baz}", @@ -104,13 +81,10 @@ func TestFallbackMatchersParser(t *testing.T) { mustNewMatcher(t, labels.MatchEqual, "foo", "bar"), mustNewMatcher(t, labels.MatchEqual, "bar", "baz"), }, - total: 1, }, { - name: "input is accepted in neither", - input: "{foo!bar}", - err: "bad matcher format: foo!bar", - total: 1, - invalidTotal: 1, + name: "input is accepted in neither", + input: "{foo!bar}", + err: "bad matcher format: foo!bar", }, { name: "input is accepted in matchers/parse but not pkg/labels", input: "{foo🙂=bar,bar=baz🙂}", @@ -118,7 +92,6 @@ func TestFallbackMatchersParser(t *testing.T) { mustNewMatcher(t, labels.MatchEqual, "foo🙂", "bar"), mustNewMatcher(t, labels.MatchEqual, "bar", "baz🙂"), }, - total: 1, }, { name: "is accepted in pkg/labels but not matchers/parse", input: "{foo=!bar,bar=$baz\\n}", @@ -126,8 +99,6 @@ func TestFallbackMatchersParser(t *testing.T) { mustNewMatcher(t, labels.MatchEqual, "foo", "!bar"), mustNewMatcher(t, labels.MatchEqual, "bar", "$baz\n"), }, - total: 1, - incompatibleTotal: 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, @@ -137,14 +108,11 @@ func TestFallbackMatchersParser(t *testing.T) { expected: labels.Matchers{ mustNewMatcher(t, labels.MatchEqual, "foo", "\\xf0\\x9f\\x99\\x82"), }, - total: 1, - disagreeTotal: 1, }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { - m := NewMetrics(prometheus.NewRegistry()) - f := FallbackMatchersParser(log.NewNopLogger(), m) + f := FallbackMatchersParser(log.NewNopLogger()) matchers, err := f(test.input, "test") if test.err != "" { require.EqualError(t, err, test.err) @@ -152,10 +120,6 @@ func TestFallbackMatchersParser(t *testing.T) { require.NoError(t, err) require.EqualValues(t, test.expected, matchers) } - requireMetric(t, test.total, m.Total) - requireMetric(t, test.disagreeTotal, m.DisagreeTotal) - requireMetric(t, test.incompatibleTotal, m.IncompatibleTotal) - requireMetric(t, test.invalidTotal, m.InvalidTotal) }) } } @@ -235,12 +199,3 @@ func TestIsValidUTF8LabelName(t *testing.T) { }) } } - -func requireMetric(t *testing.T, expected float64, m *prometheus.CounterVec) { - 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)) - } -} diff --git a/silence/silence_test.go b/silence/silence_test.go index da39b3a8..2a880dd3 100644 --- a/silence/silence_test.go +++ b/silence/silence_test.go @@ -1334,12 +1334,12 @@ func TestValidateUTF8Matcher(t *testing.T) { // Change the mode to UTF-8 mode. ff, err := featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureUTF8StrictMode) require.NoError(t, err) - compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + compat.InitFromFlags(log.NewNopLogger(), ff) // Restore the mode to classic at the end of the test. ff, err = featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureClassicMode) require.NoError(t, err) - defer compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + defer compat.InitFromFlags(log.NewNopLogger(), ff) for _, c := range cases { checkErr(t, c.err, validateMatcher(c.m)) diff --git a/types/types_test.go b/types/types_test.go index 438f3515..ece6fb5c 100644 --- a/types/types_test.go +++ b/types/types_test.go @@ -341,12 +341,12 @@ func TestValidateUTF8Ls(t *testing.T) { // Change the mode to UTF-8 mode. ff, err := featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureUTF8StrictMode) require.NoError(t, err) - compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + compat.InitFromFlags(log.NewNopLogger(), ff) // Restore the mode to classic at the end of the test. ff, err = featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureClassicMode) require.NoError(t, err) - defer compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + defer compat.InitFromFlags(log.NewNopLogger(), ff) for _, test := range tests { t.Run(test.name, func(t *testing.T) {