From f0c35f7ca1883641dcf2bbfc0dfc139e8ad2db00 Mon Sep 17 00:00:00 2001 From: machine424 Date: Wed, 8 Jan 2025 14:48:30 +0100 Subject: [PATCH] feat(parser): offer assistance for a smooth Prom 3 upgrade after the "quantile" and "le" normalization breaking change See https://prometheus.io/docs/prometheus/latest/migration/#le-and-quantile-label-values --- cmd/prometheus/main.go | 12 ++--- model/relabel/relabel.go | 17 ++++++- promql/bench_test.go | 4 ++ promql/engine.go | 2 + promql/parser/parse.go | 91 +++++++++++++++++++++++++++++++++++++ promql/parser/parse_test.go | 63 +++++++++++++++++++++++++ 6 files changed, 179 insertions(+), 10 deletions(-) diff --git a/cmd/prometheus/main.go b/cmd/prometheus/main.go index bcfbe24a6ae9..19ccb4d06362 100644 --- a/cmd/prometheus/main.go +++ b/cmd/prometheus/main.go @@ -539,6 +539,9 @@ func main() { logger := promslog.New(&cfg.promslogConfig) slog.SetDefault(logger) + // Hacky but temporary + parser.Logger = logger.With("component", "parser") + relabel.Logger = logger.With("component", "relabel") notifs := notifications.NewNotifications(cfg.maxNotificationsSubscribers, prometheus.DefaultRegisterer) cfg.web.NotificationsSub = notifs.Sub @@ -994,18 +997,12 @@ func main() { listeners, err := webHandler.Listeners() if err != nil { logger.Error("Unable to start web listener", "err", err) - if err := queryEngine.Close(); err != nil { - logger.Warn("Closing query engine failed", "err", err) - } os.Exit(1) } err = toolkit_web.Validate(*webConfig) if err != nil { logger.Error("Unable to validate web configuration file", "err", err) - if err := queryEngine.Close(); err != nil { - logger.Warn("Closing query engine failed", "err", err) - } os.Exit(1) } @@ -1027,9 +1024,6 @@ func main() { case <-cancel: reloadReady.Close() } - if err := queryEngine.Close(); err != nil { - logger.Warn("Closing query engine failed", "err", err) - } return nil }, func(err error) { diff --git a/model/relabel/relabel.go b/model/relabel/relabel.go index eb79f7be21c2..7b293af263b8 100644 --- a/model/relabel/relabel.go +++ b/model/relabel/relabel.go @@ -22,11 +22,15 @@ import ( "github.com/grafana/regexp" "github.com/prometheus/common/model" + "github.com/prometheus/common/promslog" "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/promql/parser" ) var ( + Logger = promslog.NewNopLogger() + relabelTarget = regexp.MustCompile(`^(?:(?:[a-zA-Z_]|\$(?:\{\w+\}|\w+))+\w*)+$`) DefaultRelabelConfig = Config{ @@ -254,7 +258,18 @@ func relabel(cfg *Config, lb *labels.Builder) (keep bool) { if len(cfg.SourceLabels) > cap(values) { values = make([]string, 0, len(cfg.SourceLabels)) } - for _, ln := range cfg.SourceLabels { + for i, ln := range cfg.SourceLabels { + if ln == model.BucketLabel || ln == model.QuantileLabel { + vals := strings.Split(cfg.Regex.String(), cfg.Separator) + if len(vals) <= i { + continue + } + // Supposing regexes ending with (\.0)? to be already adjusted, excluding them to avoid noise. + if !strings.HasSuffix(vals[i], `(\.0)?`) { + parser.NarrowSelectors.WithLabelValues("relabel").Inc() + Logger.Debug("relabel_config concerns 'le' or 'quantile' labels, it may need to be adjusted to count for float values", "source_labels", cfg.SourceLabels, "regex", cfg.Regex) + } + } values = append(values, lb.Get(string(ln))) } val := strings.Join(values, cfg.Separator) diff --git a/promql/bench_test.go b/promql/bench_test.go index 943baceecb55..a737461e2772 100644 --- a/promql/bench_test.go +++ b/promql/bench_test.go @@ -539,6 +539,10 @@ func generateNativeHistogramSeries(app storage.Appender, numSeries int) error { func BenchmarkParser(b *testing.B) { cases := []string{ + `foo_bucket{a="b",c="d",le=~"1.0|2.0|3.0|4"}`, + `bar_bucket{a="b",c="d",le="1.0"}`, + `foo{a="b",c="d",quantile="0"}`, + `bar{a="b",c="d",quantile="0.99"}`, "a", "metric", "1", diff --git a/promql/engine.go b/promql/engine.go index eecb4bcc4e78..5a9eeb19f771 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -436,6 +436,8 @@ func NewEngine(opts EngineOpts) *Engine { } // Close closes ng. +// Callers must ensure the engine is really no longer in use before calling this to avoid +// issues failures like in https://github.com/prometheus/prometheus/issues/15232 func (ng *Engine) Close() error { if ng == nil { return nil diff --git a/promql/parser/parse.go b/promql/parser/parse.go index 05549eaac8d4..169798499ae4 100644 --- a/promql/parser/parse.go +++ b/promql/parser/parse.go @@ -24,7 +24,9 @@ import ( "sync" "time" + "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" + "github.com/prometheus/common/promslog" "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" @@ -33,6 +35,92 @@ import ( "github.com/prometheus/prometheus/util/strutil" ) +var ( + Logger = promslog.NewNopLogger() + NarrowSelectors = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "prometheus_narrow_selectors_count", + Help: "Number of selectors that may unintentionally only match integers on 'quantile' and 'le' labels", + }, + []string{"component"}, + ) +) + +func init() { + prometheus.MustRegister(NarrowSelectors) +} + +// isInteger is a light strconv.Atoi that avoids allocations due to Atoi's returned error and doesn't +// consider as integers invalid, big, or underscored integers. +func isInteger(s string) (int, bool) { + sLen := len(s) + if strconv.IntSize == 32 && (0 < sLen && sLen < 10) || + strconv.IntSize == 64 && (0 < sLen && sLen < 19) { + // Fast path for small integers that fit int type. + s0 := s + if s[0] == '-' || s[0] == '+' { + s = s[1:] + if len(s) < 1 { + return 0, false + } + } + + n := 0 + for _, ch := range []byte(s) { + ch -= '0' + if ch > 9 { + return 0, false + } + n = n*10 + int(ch) + } + if s0[0] == '-' { + n = -n + } + return n, true + } + + // Slow path for invalid, big, or underscored integers. + i64, err := strconv.ParseInt(s, 10, 0) + return int(i64), err == nil +} + +// checkLabelMatchers helps to detect unintentional misuses of matchers on "quantile" and "le" labels after normalization during scraping +// introduced in Prometheus3. For more details, see https://prometheus.io/docs/prometheus/latest/migration/#le-and-quantile-label-values. +// It specifically detects integers-only label matchers formatted as "integer" and "integer|integer|integer" (as they're the most likely to contain them). +// Refer to Test_checkLabelMatchers for more details. +func checkLabelMatchers(vs *VectorSelector) { +Outer: + for _, lm := range vs.LabelMatchers { + if lm == nil { + continue + } + if lm.Name == model.QuantileLabel || (strings.HasSuffix(vs.Name, "_bucket") && lm.Name == model.BucketLabel) { + switch lm.Type { + case labels.MatchEqual, labels.MatchNotEqual: + if n, ok := isInteger(lm.Value); ok { + NarrowSelectors.WithLabelValues("parser").Inc() + Logger.Debug("selector set to explicitly match an integer, but values could be floats", "narrow_matcher_label", lm.Name, "integer", n, "matchers", vs.LabelMatchers) + } + break Outer + case labels.MatchRegexp, labels.MatchNotRegexp: + if len(lm.Value) == 0 { + break Outer + } + vals := strings.Split(lm.Value, "|") + for _, val := range vals { + if _, ok := isInteger(val); !ok { + break Outer + } + } + + NarrowSelectors.WithLabelValues("parser").Inc() + Logger.Debug("selector set to explicitly match integers only, but values could be floats", "narrow_matcher_label", lm.Name, "matchers", vs.LabelMatchers) + break Outer + } + } + } +} + var parserPool = sync.Pool{ New: func() interface{} { return &parser{} @@ -198,6 +286,7 @@ func ParseMetricSelector(input string) (m []*labels.Matcher, err error) { parseResult := p.parseGenerated(START_METRIC_SELECTOR) if parseResult != nil { + checkLabelMatchers(parseResult.(*VectorSelector)) m = parseResult.(*VectorSelector).LabelMatchers } @@ -839,6 +928,8 @@ func (p *parser) checkAST(node Node) (typ ValueType) { } } + checkLabelMatchers(n) + // Skip the check for non-empty matchers because an explicit // metric name is a non-empty matcher. break diff --git a/promql/parser/parse_test.go b/promql/parser/parse_test.go index 0e5e2f638bbc..897ffae498ef 100644 --- a/promql/parser/parse_test.go +++ b/promql/parser/parse_test.go @@ -28,6 +28,8 @@ import ( "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/util/testutil" + client_testutil "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/prometheus/prometheus/promql/parser/posrange" ) @@ -4607,3 +4609,64 @@ func TestParseCustomFunctions(t *testing.T) { require.True(t, ok) require.Equal(t, "custom_func", call.Func.Name) } + +func Test_checkLabelMatchers(t *testing.T) { + parseExpr := func(s string) error { + _, err := ParseExpr(s) + return err + } + + parseMetricSelector := func(s string) error { + _, err := ParseMetricSelector(s) + return err + } + + cases := []struct { + expr string + shouldIncrementCounter bool + fn func(string) error + }{ + {`rate(foo_bucket{le="1"}[1d])`, true, parseExpr}, + {`sum(baz) - sum(foo_bucket{a="b",le=~"1|2|3"})`, true, parseExpr}, + + {`foo_bucket{le="1"}`, true, parseMetricSelector}, + {`foo_bucket{le="-1"}`, true, parseMetricSelector}, + {`foo_bucket{a="b",le="1"}`, true, parseMetricSelector}, + {`foo_bucket{le=~"5|1|3"}`, true, parseMetricSelector}, + {`foo_bucket{bar="1"}`, false, parseMetricSelector}, + {`foo_bucket{le=~"0.5|1.5"}`, false, parseMetricSelector}, + {`foo_bucket{le="0.5"}`, false, parseMetricSelector}, + {`foo{le="0"}`, false, parseMetricSelector}, + {`foo_bucket{le=~"0.5|1"}`, false, parseMetricSelector}, + {`foo_bucket{le=~"1|0.6"}`, false, parseMetricSelector}, + {`foo_bucket{le=""}`, false, parseMetricSelector}, + // name inside the braces is not supported + {`{le="1",__name__="foo_bucket"}`, false, parseMetricSelector}, + + {`bar{quantile="0"}`, true, parseMetricSelector}, + {`bar{quantile="-0"}`, true, parseMetricSelector}, + {`bar{a="b",quantile="0"}`, true, parseMetricSelector}, + {`bar{quantile=~"0|1"}`, true, parseMetricSelector}, + {`{quantile="1",__name__="bar"}`, true, parseMetricSelector}, + {`foo_bucket{bar="0"}`, false, parseMetricSelector}, + {`bar{quantile="0.95"}`, false, parseMetricSelector}, + {`bar{quantile=~"0.5|0.95"}`, false, parseMetricSelector}, + {`bar{quantile=~"0.5|0"}`, false, parseMetricSelector}, + {`bar{quantile=~"0|0.6"}`, false, parseMetricSelector}, + {`bar{quantile=""}`, false, parseMetricSelector}, + } + + for _, tt := range cases { + t.Run(tt.expr, func(t *testing.T) { + counter := client_testutil.ToFloat64(NarrowSelectors.WithLabelValues("parser")) + err := tt.fn(tt.expr) + require.NoError(t, err) + + if tt.shouldIncrementCounter { + counter++ + } + + require.Equal(t, counter, client_testutil.ToFloat64(NarrowSelectors.WithLabelValues("parser"))) + }) + } +}