From 97a767d05871ee17ecadf5b9f25d5be22d1e62d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Tue, 26 Nov 2024 11:21:31 +0100 Subject: [PATCH] frontend: add new query pruning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current method of excluding/including sub query results in PromQL by comparing to -Inf or +Inf is no longer valid after https://github.com/prometheus/prometheus/pull/15245 due to comparison of native histograms to a float with < or > result in Jeanette's warning, not empty set. To ease migrating to the correct version, I'm not removing the old prune code yet, just adding the new method. The new method uses logical AND operation to intersect the sub query with either a const vector() or an empty vector(). E.g. subquery and on() (vector(1)==1) subquery and on() (vector(-1)==1) which become: subquery (vector(-1)==1) Signed-off-by: György Krajcsovits --- CHANGELOG.md | 1 + .../querymiddleware/astmapper/pruning.go | 212 +++++------------ .../astmapper/pruning_deprecated.go | 217 ++++++++++++++++++ .../astmapper/pruning_deprecated_test.go | 207 +++++++++++++++++ .../querymiddleware/astmapper/pruning_test.go | 162 ++++++------- .../querymiddleware/prune_deprecated.go | 74 ++++++ .../querymiddleware/prune_deprecated_test.go | 98 ++++++++ pkg/frontend/querymiddleware/prune_test.go | 14 +- pkg/frontend/querymiddleware/roundtrip.go | 3 + .../querymiddleware/roundtrip_test.go | 1 + 10 files changed, 728 insertions(+), 261 deletions(-) create mode 100644 pkg/frontend/querymiddleware/astmapper/pruning_deprecated.go create mode 100644 pkg/frontend/querymiddleware/astmapper/pruning_deprecated_test.go create mode 100644 pkg/frontend/querymiddleware/prune_deprecated.go create mode 100644 pkg/frontend/querymiddleware/prune_deprecated_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index cb3c177074a..0c4aa60fae9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ * [ENHANCEMENT] Distributor: Initialize ha_tracker cache before ha_tracker and distributor reach running state and begin serving writes. #9826 #9976 * [ENHANCEMENT] Ingester: `-ingest-storage.kafka.max-buffered-bytes` to limit the memory for buffered records when using concurrent fetching. #9892 * [ENHANCEMENT] Querier: improve performance and memory consumption of queries that select many series. #9914 +* [ENHANCEMENT] Query-Frontend: prune ` and on() (vector(x)==y)` style queries. Triggered by https://github.com/prometheus/prometheus/pull/15245. #10026 * [BUGFIX] Fix issue where functions such as `rate()` over native histograms could return incorrect values if a float stale marker was present in the selected range. #9508 * [BUGFIX] Fix issue where negation of native histograms (eg. `-some_native_histogram_series`) did nothing. #9508 * [BUGFIX] Fix issue where `metric might not be a counter, name does not end in _total/_sum/_count/_bucket` annotation would be emitted even if `rate` or `increase` did not have enough samples to compute a result. #9508 diff --git a/pkg/frontend/querymiddleware/astmapper/pruning.go b/pkg/frontend/querymiddleware/astmapper/pruning.go index 3040e44f83d..3b94438f2a0 100644 --- a/pkg/frontend/querymiddleware/astmapper/pruning.go +++ b/pkg/frontend/querymiddleware/astmapper/pruning.go @@ -4,8 +4,6 @@ package astmapper import ( "context" - "math" - "strconv" "github.com/go-kit/log" "github.com/prometheus/prometheus/promql/parser" @@ -33,185 +31,89 @@ func (pruner *queryPruner) MapExpr(expr parser.Expr) (mapped parser.Expr, finish return nil, false, err } - switch e := expr.(type) { - case *parser.ParenExpr: - mapped, finished, err = pruner.MapExpr(e.Expr) - if err != nil { - return e, false, err - } - return &parser.ParenExpr{Expr: mapped, PosRange: e.PosRange}, finished, nil - case *parser.BinaryExpr: - return pruner.pruneBinOp(e) - default: - return e, false, nil + e, ok := expr.(*parser.BinaryExpr) + if !ok { + return expr, false, nil } -} -func (pruner *queryPruner) pruneBinOp(expr *parser.BinaryExpr) (mapped parser.Expr, finished bool, err error) { - switch expr.Op { - case parser.MUL: - return pruner.handleMultiplyOp(expr), false, nil - case parser.GTR, parser.LSS: - return pruner.handleCompOp(expr), false, nil - case parser.LOR: - return pruner.handleOrOp(expr), false, nil - case parser.LAND: - return pruner.handleAndOp(expr), false, nil - case parser.LUNLESS: - return pruner.handleUnlessOp(expr), false, nil - default: + if e.Op != parser.LAND || e.VectorMatching == nil || + !e.VectorMatching.On || len(e.VectorMatching.MatchingLabels) != 0 { + // Return if not " and on() " return expr, false, nil } -} -// The bool signifies if the number evaluates to infinity, and if it does -// we return the infinity of the correct sign. -func calcInf(isPositive bool, num string) (*parser.NumberLiteral, bool) { - coeff, err := strconv.Atoi(num) - if err != nil || coeff == 0 { - return nil, false + isConst, isEmpty := pruner.isConst(e.RHS) + if !isConst { + return expr, false, nil } - switch { - case isPositive && coeff > 0: - return &parser.NumberLiteral{Val: math.Inf(1)}, true - case isPositive && coeff < 0: - return &parser.NumberLiteral{Val: math.Inf(-1)}, true - case !isPositive && coeff > 0: - return &parser.NumberLiteral{Val: math.Inf(-1)}, true - case !isPositive && coeff < 0: - return &parser.NumberLiteral{Val: math.Inf(1)}, true - default: - return nil, false + if isEmpty { + // The right hand side is empty, so the whole expression is empty due to + // "and on()", return the right hand side. + return e.RHS, false, nil } + // The right hand side is const no empty, so the whole expression is just the + // left side. + return e.LHS, false, nil } -func (pruner *queryPruner) handleMultiplyOp(expr *parser.BinaryExpr) parser.Expr { - isInfR, signR := pruner.isInfinite(expr.RHS) - if isInfR { - newExpr, ok := calcInf(signR, expr.LHS.String()) - if ok { - return newExpr - } - } - isInfL, signL := pruner.isInfinite(expr.LHS) - if isInfL { - newExpr, ok := calcInf(signL, expr.RHS.String()) - if ok { - return newExpr +func (pruner *queryPruner) isConst(expr parser.Expr) (isConst, isEmpty bool) { + var lhs, rhs parser.Expr + switch e := expr.(type) { + case *parser.ParenExpr: + return pruner.isConst(e.Expr) + case *parser.BinaryExpr: + if e.Op != parser.EQLC || e.ReturnBool { + return false, false } - } - return expr -} - -func (pruner *queryPruner) handleCompOp(expr *parser.BinaryExpr) parser.Expr { - var refNeg, refPos parser.Expr - switch expr.Op { - case parser.LSS: - refNeg = expr.RHS - refPos = expr.LHS - case parser.GTR: - refNeg = expr.LHS - refPos = expr.RHS + lhs = e.LHS + rhs = e.RHS default: - return expr + return false, false } - // foo < -Inf or -Inf > foo => vector(0) < -Inf - isInf, sign := pruner.isInfinite(refNeg) - if isInf && !sign { - return &parser.BinaryExpr{ - LHS: &parser.Call{ - Func: parser.Functions["vector"], - Args: []parser.Expr{&parser.NumberLiteral{Val: 0}}, - }, - Op: parser.LSS, - RHS: &parser.NumberLiteral{Val: math.Inf(-1)}, - ReturnBool: false, + lIsVector, lValue := pruner.isConstVector(lhs) + if lIsVector { + rIsConst, rValue := pruner.isNumber(rhs) + if rIsConst { + return true, rValue != lValue } + return false, false } - - // foo > +Inf or +Inf < foo => vector(0) > +Inf => vector(0) < -Inf - isInf, sign = pruner.isInfinite(refPos) - if isInf && sign { - return &parser.BinaryExpr{ - LHS: &parser.Call{ - Func: parser.Functions["vector"], - Args: []parser.Expr{&parser.NumberLiteral{Val: 0}}, - }, - Op: parser.LSS, - RHS: &parser.NumberLiteral{Val: math.Inf(-1)}, - ReturnBool: false, - } + var lIsConst bool + lIsConst, lValue = pruner.isNumber(lhs) + if !lIsConst { + return false, false } - - return expr + rIsVector, rValue := pruner.isConstVector(rhs) + if !rIsVector { + return false, false + } + return true, lValue != rValue } -// 1st bool is true if the number is infinite. -// 2nd bool is true if the number is positive infinity. -func (pruner *queryPruner) isInfinite(expr parser.Expr) (bool, bool) { - mapped, _, err := pruner.MapExpr(expr) - if err == nil { - expr = mapped - } +func (pruner *queryPruner) isConstVector(expr parser.Expr) (isVector bool, value float64) { switch e := expr.(type) { case *parser.ParenExpr: - return pruner.isInfinite(e.Expr) - case *parser.NumberLiteral: - if math.IsInf(e.Val, 1) { - return true, true + return pruner.isConstVector(e.Expr) + case *parser.Call: + if e.Func.Name != "vector" || len(e.Args) != 1 { + return false, 0 } - if math.IsInf(e.Val, -1) { - return true, false + lit, ok := e.Args[0].(*parser.NumberLiteral) + if !ok { + return false, 0 } - return false, false - default: - return false, false - } -} - -func (pruner *queryPruner) handleOrOp(expr *parser.BinaryExpr) parser.Expr { - switch { - case pruner.isEmpty(expr.LHS): - return expr.RHS - case pruner.isEmpty(expr.RHS): - return expr.LHS - } - return expr -} - -func (pruner *queryPruner) handleAndOp(expr *parser.BinaryExpr) parser.Expr { - switch { - case pruner.isEmpty(expr.LHS): - return expr.LHS - case pruner.isEmpty(expr.RHS): - return expr.RHS + return true, lit.Val } - return expr + return false, 0 } -func (pruner *queryPruner) handleUnlessOp(expr *parser.BinaryExpr) parser.Expr { - switch { - case pruner.isEmpty(expr.LHS): - return expr.LHS - case pruner.isEmpty(expr.RHS): - return expr.LHS - } - return expr -} - -func (pruner *queryPruner) isEmpty(expr parser.Expr) bool { - mapped, _, err := pruner.MapExpr(expr) - if err == nil { - expr = mapped - } +func (pruner *queryPruner) isNumber(expr parser.Expr) (isNumber bool, value float64) { switch e := expr.(type) { case *parser.ParenExpr: - return pruner.isEmpty(e.Expr) - default: - if e.String() == `vector(0) < -Inf` { - return true - } - return false + return pruner.isNumber(e.Expr) + case *parser.NumberLiteral: + return true, e.Val } + return false, 0 } diff --git a/pkg/frontend/querymiddleware/astmapper/pruning_deprecated.go b/pkg/frontend/querymiddleware/astmapper/pruning_deprecated.go new file mode 100644 index 00000000000..322e1264a93 --- /dev/null +++ b/pkg/frontend/querymiddleware/astmapper/pruning_deprecated.go @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package astmapper + +import ( + "context" + "math" + "strconv" + + "github.com/go-kit/log" + "github.com/prometheus/prometheus/promql/parser" +) + +func NewQueryPrunerDeprecated(ctx context.Context, logger log.Logger) ASTMapper { + pruner := newQueryPrunerDeprecated(ctx, logger) + return NewASTExprMapper(pruner) +} + +type queryPrunerDeprecated struct { + ctx context.Context + logger log.Logger +} + +func newQueryPrunerDeprecated(ctx context.Context, logger log.Logger) *queryPrunerDeprecated { + return &queryPrunerDeprecated{ + ctx: ctx, + logger: logger, + } +} + +func (pruner *queryPrunerDeprecated) MapExpr(expr parser.Expr) (mapped parser.Expr, finished bool, err error) { + if err := pruner.ctx.Err(); err != nil { + return nil, false, err + } + + switch e := expr.(type) { + case *parser.ParenExpr: + mapped, finished, err = pruner.MapExpr(e.Expr) + if err != nil { + return e, false, err + } + return &parser.ParenExpr{Expr: mapped, PosRange: e.PosRange}, finished, nil + case *parser.BinaryExpr: + return pruner.pruneBinOp(e) + default: + return e, false, nil + } +} + +func (pruner *queryPrunerDeprecated) pruneBinOp(expr *parser.BinaryExpr) (mapped parser.Expr, finished bool, err error) { + switch expr.Op { + case parser.MUL: + return pruner.handleMultiplyOp(expr), false, nil + case parser.GTR, parser.LSS: + return pruner.handleCompOp(expr), false, nil + case parser.LOR: + return pruner.handleOrOp(expr), false, nil + case parser.LAND: + return pruner.handleAndOp(expr), false, nil + case parser.LUNLESS: + return pruner.handleUnlessOp(expr), false, nil + default: + return expr, false, nil + } +} + +// The bool signifies if the number evaluates to infinity, and if it does +// we return the infinity of the correct sign. +func calcInf(isPositive bool, num string) (*parser.NumberLiteral, bool) { + coeff, err := strconv.Atoi(num) + if err != nil || coeff == 0 { + return nil, false + } + switch { + case isPositive && coeff > 0: + return &parser.NumberLiteral{Val: math.Inf(1)}, true + case isPositive && coeff < 0: + return &parser.NumberLiteral{Val: math.Inf(-1)}, true + case !isPositive && coeff > 0: + return &parser.NumberLiteral{Val: math.Inf(-1)}, true + case !isPositive && coeff < 0: + return &parser.NumberLiteral{Val: math.Inf(1)}, true + default: + return nil, false + } +} + +func (pruner *queryPrunerDeprecated) handleMultiplyOp(expr *parser.BinaryExpr) parser.Expr { + isInfR, signR := pruner.isInfinite(expr.RHS) + if isInfR { + newExpr, ok := calcInf(signR, expr.LHS.String()) + if ok { + return newExpr + } + } + isInfL, signL := pruner.isInfinite(expr.LHS) + if isInfL { + newExpr, ok := calcInf(signL, expr.RHS.String()) + if ok { + return newExpr + } + } + return expr +} + +func (pruner *queryPrunerDeprecated) handleCompOp(expr *parser.BinaryExpr) parser.Expr { + var refNeg, refPos parser.Expr + switch expr.Op { + case parser.LSS: + refNeg = expr.RHS + refPos = expr.LHS + case parser.GTR: + refNeg = expr.LHS + refPos = expr.RHS + default: + return expr + } + + // foo < -Inf or -Inf > foo => vector(0) < -Inf + isInf, sign := pruner.isInfinite(refNeg) + if isInf && !sign { + return &parser.BinaryExpr{ + LHS: &parser.Call{ + Func: parser.Functions["vector"], + Args: []parser.Expr{&parser.NumberLiteral{Val: 0}}, + }, + Op: parser.LSS, + RHS: &parser.NumberLiteral{Val: math.Inf(-1)}, + ReturnBool: false, + } + } + + // foo > +Inf or +Inf < foo => vector(0) > +Inf => vector(0) < -Inf + isInf, sign = pruner.isInfinite(refPos) + if isInf && sign { + return &parser.BinaryExpr{ + LHS: &parser.Call{ + Func: parser.Functions["vector"], + Args: []parser.Expr{&parser.NumberLiteral{Val: 0}}, + }, + Op: parser.LSS, + RHS: &parser.NumberLiteral{Val: math.Inf(-1)}, + ReturnBool: false, + } + } + + return expr +} + +// 1st bool is true if the number is infinite. +// 2nd bool is true if the number is positive infinity. +func (pruner *queryPrunerDeprecated) isInfinite(expr parser.Expr) (bool, bool) { + mapped, _, err := pruner.MapExpr(expr) + if err == nil { + expr = mapped + } + switch e := expr.(type) { + case *parser.ParenExpr: + return pruner.isInfinite(e.Expr) + case *parser.NumberLiteral: + if math.IsInf(e.Val, 1) { + return true, true + } + if math.IsInf(e.Val, -1) { + return true, false + } + return false, false + default: + return false, false + } +} + +func (pruner *queryPrunerDeprecated) handleOrOp(expr *parser.BinaryExpr) parser.Expr { + switch { + case pruner.isEmpty(expr.LHS): + return expr.RHS + case pruner.isEmpty(expr.RHS): + return expr.LHS + } + return expr +} + +func (pruner *queryPrunerDeprecated) handleAndOp(expr *parser.BinaryExpr) parser.Expr { + switch { + case pruner.isEmpty(expr.LHS): + return expr.LHS + case pruner.isEmpty(expr.RHS): + return expr.RHS + } + return expr +} + +func (pruner *queryPrunerDeprecated) handleUnlessOp(expr *parser.BinaryExpr) parser.Expr { + switch { + case pruner.isEmpty(expr.LHS): + return expr.LHS + case pruner.isEmpty(expr.RHS): + return expr.LHS + } + return expr +} + +func (pruner *queryPrunerDeprecated) isEmpty(expr parser.Expr) bool { + mapped, _, err := pruner.MapExpr(expr) + if err == nil { + expr = mapped + } + switch e := expr.(type) { + case *parser.ParenExpr: + return pruner.isEmpty(e.Expr) + default: + if e.String() == `vector(0) < -Inf` { + return true + } + return false + } +} diff --git a/pkg/frontend/querymiddleware/astmapper/pruning_deprecated_test.go b/pkg/frontend/querymiddleware/astmapper/pruning_deprecated_test.go new file mode 100644 index 00000000000..52005ea28ff --- /dev/null +++ b/pkg/frontend/querymiddleware/astmapper/pruning_deprecated_test.go @@ -0,0 +1,207 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package astmapper + +import ( + "context" + "testing" + + "github.com/go-kit/log" + "github.com/prometheus/prometheus/promql/parser" + "github.com/stretchr/testify/require" +) + +func TestQueryPrunerDeprecated(t *testing.T) { + pruner := NewQueryPrunerDeprecated(context.Background(), log.NewNopLogger()) + + for _, tt := range []struct { + in string + out string + }{ + { + `quantile(0.9,foo)`, + `quantile(0.9,foo)`, + }, + { + `histogram_quantile(0.5, rate(bar1{baz="blip"}[30s]))`, + `histogram_quantile(0.5, rate(bar1{baz="blip"}[30s]))`, + }, + { + `count(up)`, + `count(up)`, + }, + { + `avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `vector(0)`, + `vector(0)`, + }, + { + `up < -Inf`, + `vector(0) < -Inf`, + }, + { + `-Inf > up`, + `vector(0) < -Inf`, + }, + { + `up > +Inf`, + `vector(0) < -Inf`, + }, + { + `+Inf < up`, + `vector(0) < -Inf`, + }, + { + `up < +Inf`, + `up < +Inf`, + }, + { + `+Inf > up`, + `+Inf > up`, + }, + { + `up > -Inf`, + `up > -Inf`, + }, + { + `-Inf < up`, + `-Inf < up`, + }, + { + `avg(rate(foo[1m])) < (-Inf)`, + `vector(0) < -Inf`, + }, + { + `Inf * -1`, + `-Inf`, + }, + { + `+1 * -Inf`, + `-Inf`, + }, + { + `1 * +Inf`, + `Inf`, + }, + { + `-Inf * -1`, + `+Inf`, + }, + { + `avg(rate(foo[1m])) < (-1 * +Inf)`, + `vector(0) < -Inf`, + }, + { + `avg(rate(foo[1m])) < (+1 * +Inf)`, + `avg(rate(foo[1m])) < (+Inf)`, + }, + { + `avg(rate(foo[1m])) < (-1 * -Inf)`, + `avg(rate(foo[1m])) < (+Inf)`, + }, + { + `avg(rate(foo[1m])) < (+1 * -Inf)`, + `vector(0) < -Inf`, + }, + { + `(-1 * -Inf) < avg(rate(foo[1m]))`, + `vector(0) < -Inf`, + }, + { + `vector(0) < -Inf or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `avg(rate(foo[1m])) or vector(0) < -Inf`, + `avg(rate(foo[1m]))`, + }, + { + `avg(rate(foo[1m])) or (vector(0) < -Inf)`, + `avg(rate(foo[1m]))`, + }, + { + `((-1 * -Inf) < avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `((-1 * -Inf) <= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((+Inf) <= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + }, + { + `((-1 * -Inf) >= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((+Inf) >= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + }, + { + `((-1 * -Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((+Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + }, + { + `((-1 * +Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `((+1 * -Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `(avg(rate(foo[2m])) < (+1 * -Inf)) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `((-1 * -Inf) == avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((+Inf) == avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + }, + { + `((-1 * -Inf) != avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((+Inf) != avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + }, + { + `((-1 * -Inf) < avg(rate(foo[2m]))) and avg(rate(foo[1m]))`, + `(vector(0) < -Inf)`, + }, + { + `((-1 * -Inf) < avg(rate(foo[2m]))) unless avg(rate(foo[1m]))`, + `(vector(0) < -Inf)`, + }, + { + `avg(rate(foo[1m])) unless ((-1 * -Inf) < avg(rate(foo[2m])))`, + `avg(rate(foo[1m]))`, + }, + { + `((2 * +Inf) < avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `(((-1 * -Inf) < avg(rate(foo[3m]))) unless avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `((((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m]))) and avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `(((-1 * -Inf) < avg(rate(foo[4m]))) unless (avg(rate(foo[3m])) and avg(rate(foo[2m])))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + { + `(((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m])) and avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m]))`, + }, + } { + tt := tt + + t.Run(tt.in, func(t *testing.T) { + expr, err := parser.ParseExpr(tt.in) + require.NoError(t, err) + out, err := parser.ParseExpr(tt.out) + require.NoError(t, err) + + mapped, err := pruner.Map(expr) + require.NoError(t, err) + require.Equal(t, out.String(), mapped.String()) + }) + } +} diff --git a/pkg/frontend/querymiddleware/astmapper/pruning_test.go b/pkg/frontend/querymiddleware/astmapper/pruning_test.go index 963c97e29d6..791d75d1daa 100644 --- a/pkg/frontend/querymiddleware/astmapper/pruning_test.go +++ b/pkg/frontend/querymiddleware/astmapper/pruning_test.go @@ -18,6 +18,23 @@ func TestQueryPruner(t *testing.T) { in string out string }{ + // Non-prunable expressions. + { + `foo`, + `foo`, + }, + { + `foo[1m]`, + `foo[1m]`, + }, + { + `foo{bar="baz"}[1m]`, + `foo{bar="baz"}[1m]`, + }, + { + `foo{bar="baz"}`, + `foo{bar="baz"}`, + }, { `quantile(0.9,foo)`, `quantile(0.9,foo)`, @@ -40,155 +57,110 @@ func TestQueryPruner(t *testing.T) { }, { `up < -Inf`, - `vector(0) < -Inf`, - }, - { - `-Inf > up`, - `vector(0) < -Inf`, - }, - { - `up > +Inf`, - `vector(0) < -Inf`, - }, - { - `+Inf < up`, - `vector(0) < -Inf`, - }, - { - `up < +Inf`, - `up < +Inf`, - }, - { - `+Inf > up`, - `+Inf > up`, - }, - { - `up > -Inf`, - `up > -Inf`, - }, - { - `-Inf < up`, - `-Inf < up`, + `up < -Inf`, }, { `avg(rate(foo[1m])) < (-Inf)`, - `vector(0) < -Inf`, + `avg(rate(foo[1m])) < (-Inf)`, }, { `Inf * -1`, - `-Inf`, - }, - { - `+1 * -Inf`, - `-Inf`, - }, - { - `1 * +Inf`, - `Inf`, - }, - { - `-Inf * -1`, - `+Inf`, + `Inf * -1`, }, { `avg(rate(foo[1m])) < (-1 * +Inf)`, - `vector(0) < -Inf`, - }, - { - `avg(rate(foo[1m])) < (+1 * +Inf)`, - `avg(rate(foo[1m])) < (+Inf)`, - }, - { - `avg(rate(foo[1m])) < (-1 * -Inf)`, - `avg(rate(foo[1m])) < (+Inf)`, - }, - { - `avg(rate(foo[1m])) < (+1 * -Inf)`, - `vector(0) < -Inf`, + `avg(rate(foo[1m])) < (-1 * +Inf)`, }, { `(-1 * -Inf) < avg(rate(foo[1m]))`, - `vector(0) < -Inf`, + `(-1 * -Inf) < avg(rate(foo[1m]))`, }, { `vector(0) < -Inf or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `vector(0) < -Inf or avg(rate(foo[1m]))`, }, { `avg(rate(foo[1m])) or vector(0) < -Inf`, - `avg(rate(foo[1m]))`, + `avg(rate(foo[1m])) or vector(0) < -Inf`, }, { `avg(rate(foo[1m])) or (vector(0) < -Inf)`, - `avg(rate(foo[1m]))`, + `avg(rate(foo[1m])) or (vector(0) < -Inf)`, }, { `((-1 * -Inf) < avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `((-1 * -Inf) < avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, }, { - `((-1 * -Inf) <= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `((+Inf) <= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `(avg(rate(foo[2m])) < (+1 * -Inf)) or avg(rate(foo[1m]))`, + `(avg(rate(foo[2m])) < (+1 * -Inf)) or avg(rate(foo[1m]))`, }, { - `((-1 * -Inf) >= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `((+Inf) >= avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((-1 * -Inf) == avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((-1 * -Inf) == avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, }, { - `((-1 * -Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `((+Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m])) unless ((-1 * -Inf) < avg(rate(foo[2m])))`, + `avg(rate(foo[1m])) unless ((-1 * -Inf) < avg(rate(foo[2m])))`, }, { - `((-1 * +Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `((2 * +Inf) < avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((2 * +Inf) < avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, }, { - `((+1 * -Inf) > avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `(((-1 * -Inf) < avg(rate(foo[3m]))) unless avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `(((-1 * -Inf) < avg(rate(foo[3m]))) unless avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, }, { - `(avg(rate(foo[2m])) < (+1 * -Inf)) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `((((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m]))) and avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `((((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m]))) and avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, }, { - `((-1 * -Inf) == avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `((+Inf) == avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + `avg(rate(foo[1m])) or avg(rate(bar[1m]))`, + `avg(rate(foo[1m])) or avg(rate(bar[1m]))`, }, - { - `((-1 * -Inf) != avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `((+Inf) != avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, + { // The const expression is on the wrong side. + `(vector(0) == 1) and on() (avg(rate(foo[1m])))`, + `(vector(0) == 1) and on() (avg(rate(foo[1m])))`, }, - { - `((-1 * -Inf) < avg(rate(foo[2m]))) and avg(rate(foo[1m]))`, - `(vector(0) < -Inf)`, + { // Matching on labels. + `(avg(rate(foo[1m]))) and on(id) (vector(0) == 1)`, + `(avg(rate(foo[1m]))) and on(id) (vector(0) == 1)`, }, + { // Not "on" expression. + `(avg(rate(foo[1m]))) and ignoring() (vector(0) == 1)`, + `(avg(rate(foo[1m]))) and ignoring() (vector(0) == 1)`, + }, + // Pruned expressions. { - `((-1 * -Inf) < avg(rate(foo[2m]))) unless avg(rate(foo[1m]))`, - `(vector(0) < -Inf)`, + `(avg(rate(foo[1m]))) and on() (vector(0) == 1)`, + `(vector(0) == 1)`, }, { - `avg(rate(foo[1m])) unless ((-1 * -Inf) < avg(rate(foo[2m])))`, - `avg(rate(foo[1m]))`, + `(avg(rate(foo[1m]))) and on() (vector(1) == 1)`, + `(avg(rate(foo[1m])))`, }, { - `((2 * +Inf) < avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `(avg(rate(foo[1m]))) and on() (vector(3) == 4.5)`, + `(vector(3) == 4.5)`, }, { - `(((-1 * -Inf) < avg(rate(foo[3m]))) unless avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `(avg(rate(foo[1m]))) and on() (vector(5.5) == 5.5)`, + `(avg(rate(foo[1m])))`, }, { - `((((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m]))) and avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + // "and on()" is not on top level, "or" has lower precedence. + `(avg(rate(foo[1m]))) and on() (vector(0) == 1) or avg(rate(bar[1m]))`, + `(vector(0) == 1) or avg(rate(bar[1m]))`, }, { - `(((-1 * -Inf) < avg(rate(foo[4m]))) unless (avg(rate(foo[3m])) and avg(rate(foo[2m])))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + // "and on()" is not on top level, due to left-right associativity. + `(avg(rate(foo[1m]))) and on() (vector(0) == 1) and avg(rate(bar[1m]))`, + `(vector(0) == 1) and avg(rate(bar[1m]))`, }, { - `(((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m])) and avg(rate(foo[2m]))) or avg(rate(foo[1m]))`, - `avg(rate(foo[1m]))`, + `(avg(rate(foo[1m]))) and on() (vector(0) == 1) or avg(rate(bar[1m])) and on() (vector(1) == 1)`, + `(vector(0) == 1) or avg(rate(bar[1m]))`, }, } { tt := tt diff --git a/pkg/frontend/querymiddleware/prune_deprecated.go b/pkg/frontend/querymiddleware/prune_deprecated.go new file mode 100644 index 00000000000..c1300b62672 --- /dev/null +++ b/pkg/frontend/querymiddleware/prune_deprecated.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package querymiddleware + +import ( + "context" + + "github.com/go-kit/log" + "github.com/go-kit/log/level" + "github.com/prometheus/prometheus/promql/parser" + + apierror "github.com/grafana/mimir/pkg/api/error" + "github.com/grafana/mimir/pkg/frontend/querymiddleware/astmapper" + "github.com/grafana/mimir/pkg/util/spanlogger" +) + +type pruneMiddlewareDeprecated struct { + next MetricsQueryHandler + logger log.Logger +} + +// newPruneMiddleware creates a middleware that optimises queries by rewriting them to prune away +// unnecessary computations. +func newPruneMiddlewareDeprecated(logger log.Logger) MetricsQueryMiddleware { + return MetricsQueryMiddlewareFunc(func(next MetricsQueryHandler) MetricsQueryHandler { + return &pruneMiddlewareDeprecated{ + next: next, + logger: logger, + } + }) +} + +func (p *pruneMiddlewareDeprecated) Do(ctx context.Context, r MetricsQueryRequest) (Response, error) { + log := spanlogger.FromContext(ctx, p.logger) + + prunedQuery, success, err := p.pruneQuery(ctx, r.GetQuery()) + if err != nil { + level.Warn(log).Log("msg", "failed to prune the input query, falling back to the original query", "query", r.GetQuery(), "err", err) + + return p.next.Do(ctx, r) + } + + if !success { + level.Debug(log).Log("msg", "query pruning had no effect", "query", r.GetQuery()) + return p.next.Do(ctx, r) + } + + level.Debug(log).Log("msg", "query has been rewritten by pruning", "original", r.GetQuery(), "rewritten", prunedQuery) + + updatedReq, err := r.WithQuery(prunedQuery) + if err != nil { + return nil, err + } + + return p.next.Do(ctx, updatedReq) +} + +func (p *pruneMiddlewareDeprecated) pruneQuery(ctx context.Context, query string) (string, bool, error) { + // Parse the query. + expr, err := parser.ParseExpr(query) + if err != nil { + return "", false, apierror.New(apierror.TypeBadData, DecorateWithParamName(err, "query").Error()) + } + origQueryString := expr.String() + + mapper := astmapper.NewQueryPruner(ctx, p.logger) + prunedQuery, err := mapper.Map(expr) + if err != nil { + return "", false, err + } + prunedQueryString := prunedQuery.String() + + return prunedQueryString, origQueryString != prunedQueryString, nil +} diff --git a/pkg/frontend/querymiddleware/prune_deprecated_test.go b/pkg/frontend/querymiddleware/prune_deprecated_test.go new file mode 100644 index 00000000000..9ffb5fcffd2 --- /dev/null +++ b/pkg/frontend/querymiddleware/prune_deprecated_test.go @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package querymiddleware + +import ( + "context" + "fmt" + "strings" + "testing" + "time" + + "github.com/go-kit/log" + "github.com/grafana/dskit/user" + "github.com/prometheus/prometheus/promql/promqltest" + "github.com/stretchr/testify/require" +) + +func TestQueryPruningDeprecated(t *testing.T) { + numSamples := 100 + seriesName := `test_float` + replacer := strings.NewReplacer("", seriesName, "", fmt.Sprintf("%d", numSamples)) + data := replacer.Replace(` + load 1m + {series="1"} 0+1x + {series="2"} 0+2x + {series="3"} 0+3x + {series="4"} 0+4x + {series="5"} 0+5x + `) + queryable := promqltest.LoadedStorage(t, data) + + const step = 20 * time.Second + + engine := newEngine() + pruningware := newPruneMiddlewareDeprecated( + log.NewNopLogger(), + ) + downstream := &downstreamHandler{ + engine: engine, + queryable: queryable, + } + + type template struct { + query string + IsEmpty bool + } + + templates := []template{ + {`avg(rate(%s[1m])) < (-1 * +Inf)`, true}, + {`avg(rate(%s[1m])) < (+1 * +Inf)`, false}, + {`avg(rate(%s[1m])) < (-1 * -Inf)`, false}, + {`avg(rate(%s[1m])) < (+1 * -Inf)`, true}, + {`(-1 * -Inf) < avg(rate(%s[1m]))`, true}, + {`((-1 * -Inf) < avg(rate(foo[2m]))) or avg(rate(%s[1m]))`, false}, + {`((-1 * -Inf) < avg(rate(foo[2m]))) and avg(rate(%s[1m]))`, true}, + {`((-1 * -Inf) < avg(rate(foo[2m]))) unless avg(rate(%s[1m]))`, true}, + {`avg(rate(%s[1m])) unless ((-1 * -Inf) < avg(rate(foo[2m])))`, false}, + {`(((-1 * -Inf) < avg(rate(foo[3m]))) unless avg(rate(foo[2m]))) or avg(rate(%s[1m]))`, true}, + {`((((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m]))) and avg(rate(foo[2m]))) or avg(rate(%s[1m]))`, true}, + } + for _, template := range templates { + t.Run(template.query, func(t *testing.T) { + query := fmt.Sprintf(template.query, seriesName) + req := &PrometheusRangeQueryRequest{ + path: "/query_range", + start: 0, + end: int64(numSamples) * time.Minute.Milliseconds(), + step: step.Milliseconds(), + queryExpr: parseQuery(t, query), + } + + injectedContext := user.InjectOrgID(context.Background(), "test") + + // Run the query without pruning. + expectedRes, err := downstream.Do(injectedContext, req) + require.Nil(t, err) + + if !template.IsEmpty { + // Ensure the query produces some results. + require.NotEmpty(t, expectedRes.(*PrometheusResponse).Data.Result) + requireValidSamples(t, expectedRes.(*PrometheusResponse).Data.Result) + } + + // Run the query with pruning. + prunedRes, err := pruningware.Wrap(downstream).Do(injectedContext, req) + require.Nil(t, err) + + if !template.IsEmpty { + // Ensure the query produces some results. + require.NotEmpty(t, prunedRes.(*PrometheusResponse).Data.Result) + requireValidSamples(t, prunedRes.(*PrometheusResponse).Data.Result) + } + + // Ensure the results are approximately equal. + approximatelyEqualsSamples(t, expectedRes.(*PrometheusResponse), prunedRes.(*PrometheusResponse)) + }) + } +} diff --git a/pkg/frontend/querymiddleware/prune_test.go b/pkg/frontend/querymiddleware/prune_test.go index d0ea3cdda5a..35e06b69112 100644 --- a/pkg/frontend/querymiddleware/prune_test.go +++ b/pkg/frontend/querymiddleware/prune_test.go @@ -46,17 +46,9 @@ func TestQueryPruning(t *testing.T) { } templates := []template{ - {`avg(rate(%s[1m])) < (-1 * +Inf)`, true}, - {`avg(rate(%s[1m])) < (+1 * +Inf)`, false}, - {`avg(rate(%s[1m])) < (-1 * -Inf)`, false}, - {`avg(rate(%s[1m])) < (+1 * -Inf)`, true}, - {`(-1 * -Inf) < avg(rate(%s[1m]))`, true}, - {`((-1 * -Inf) < avg(rate(foo[2m]))) or avg(rate(%s[1m]))`, false}, - {`((-1 * -Inf) < avg(rate(foo[2m]))) and avg(rate(%s[1m]))`, true}, - {`((-1 * -Inf) < avg(rate(foo[2m]))) unless avg(rate(%s[1m]))`, true}, - {`avg(rate(%s[1m])) unless ((-1 * -Inf) < avg(rate(foo[2m])))`, false}, - {`(((-1 * -Inf) < avg(rate(foo[3m]))) unless avg(rate(foo[2m]))) or avg(rate(%s[1m]))`, true}, - {`((((-1 * -Inf) < avg(rate(foo[4m]))) unless avg(rate(foo[3m]))) and avg(rate(foo[2m]))) or avg(rate(%s[1m]))`, true}, + {`(avg(rate(%s[1m]))) and on() (vector(0) == 1)`, true}, + {`(avg(rate(%s[1m]))) and on() (vector(1) == 1)`, false}, + {`avg(rate(%s[1m])) or (avg(rate(test_float[1m]))) and on() (vector(0) == 1)`, false}, } for _, template := range templates { t.Run(template.query, func(t *testing.T) { diff --git a/pkg/frontend/querymiddleware/roundtrip.go b/pkg/frontend/querymiddleware/roundtrip.go index 2df612e6994..048b3746400 100644 --- a/pkg/frontend/querymiddleware/roundtrip.go +++ b/pkg/frontend/querymiddleware/roundtrip.go @@ -437,15 +437,18 @@ func newQueryMiddlewares( if cfg.PrunedQueries { pruneMiddleware := newPruneMiddleware(log) + pruneMiddlewareDeprecated := newPruneMiddlewareDeprecated(log) queryRangeMiddleware = append( queryRangeMiddleware, newInstrumentMiddleware("pruning", metrics), pruneMiddleware, + pruneMiddlewareDeprecated, ) queryInstantMiddleware = append( queryInstantMiddleware, newInstrumentMiddleware("pruning", metrics), pruneMiddleware, + pruneMiddlewareDeprecated, ) } diff --git a/pkg/frontend/querymiddleware/roundtrip_test.go b/pkg/frontend/querymiddleware/roundtrip_test.go index 65f5216ffcb..59faae27a14 100644 --- a/pkg/frontend/querymiddleware/roundtrip_test.go +++ b/pkg/frontend/querymiddleware/roundtrip_test.go @@ -587,6 +587,7 @@ func TestMiddlewaresConsistency(t *testing.T) { "splitInstantQueryByIntervalMiddleware", // Not applicable because specific to instant queries. "stepAlignMiddleware", // Not applicable because remote read requests don't take step in account when running in Mimir. "pruneMiddleware", // No query pruning support. + "pruneMiddlewareDeprecated", // No query pruning support. "experimentalFunctionsMiddleware", // No blocking for PromQL experimental functions as it is executed remotely. }, },