diff --git a/internal/checks/promql_fragile.go b/internal/checks/promql_fragile.go index fc58743f..870227bd 100644 --- a/internal/checks/promql_fragile.go +++ b/internal/checks/promql_fragile.go @@ -53,7 +53,7 @@ func (c FragileCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rul return nil } - for _, problem := range c.checkAggregation(expr.Query) { + for _, problem := range c.checkAggregation(expr.Value.Value, expr.Query) { problems = append(problems, Problem{ Lines: expr.Value.Lines, Reporter: c.Reporter(), @@ -78,7 +78,7 @@ func (c FragileCheck) Check(_ context.Context, _ discovery.Path, rule parser.Rul return problems } -func (c FragileCheck) checkAggregation(node *parser.PromQLNode) (problems []exprProblem) { +func (c FragileCheck) checkAggregation(query string, node *parser.PromQLNode) (problems []exprProblem) { if n := utils.HasOuterBinaryExpr(node); n != nil && n.Op != promParser.LOR && n.Op != promParser.LUNLESS { if n.VectorMatching != nil && n.VectorMatching.On { goto NEXT @@ -91,8 +91,8 @@ func (c FragileCheck) checkAggregation(node *parser.PromQLNode) (problems []expr } var isFragile bool for _, child := range node.Children { - for _, agg := range utils.HasOuterAggregation(child) { - if agg.Without { + for _, src := range utils.LabelsSource(query, child.Expr) { + if src.Type == utils.AggregateSource && !src.FixedLabels { isFragile = true } } @@ -120,7 +120,7 @@ func (c FragileCheck) checkAggregation(node *parser.PromQLNode) (problems []expr NEXT: for _, child := range node.Children { - problems = append(problems, c.checkAggregation(child)...) + problems = append(problems, c.checkAggregation(query, child)...) } return problems diff --git a/internal/parser/utils/aggregation.go b/internal/parser/utils/aggregation.go deleted file mode 100644 index 3158b851..00000000 --- a/internal/parser/utils/aggregation.go +++ /dev/null @@ -1,103 +0,0 @@ -package utils - -import ( - "log/slog" - - "github.com/cloudflare/pint/internal/parser" - - promParser "github.com/prometheus/prometheus/promql/parser" -) - -func HasOuterAggregation(node *parser.PromQLNode) (aggs []*promParser.AggregateExpr) { - if n, ok := node.Expr.(*promParser.AggregateExpr); ok { - switch n.Op { - case promParser.SUM: - case promParser.MIN: - case promParser.MAX: - case promParser.AVG: - case promParser.GROUP: - case promParser.STDDEV: - case promParser.STDVAR: - case promParser.COUNT: - case promParser.COUNT_VALUES: - case promParser.BOTTOMK: - goto NEXT - case promParser.TOPK: - goto NEXT - case promParser.QUANTILE: - default: - slog.Warn("Unsupported aggregation operation", slog.String("op", n.Op.String())) - } - aggs = append(aggs, n) - return aggs - } - -NEXT: - if n, ok := node.Expr.(*promParser.BinaryExpr); ok { - if n.VectorMatching != nil { - switch n.VectorMatching.Card { - case promParser.CardOneToOne: - case promParser.CardOneToMany: - for i, child := range node.Children { - if i == len(node.Children)-1 { - a := HasOuterAggregation(child) - if len(a) > 0 && !a[0].Without { - a[0].Grouping = append(a[0].Grouping, n.VectorMatching.Include...) - } - return a - } - } - case promParser.CardManyToOne: - a := HasOuterAggregation(node.Children[0]) - if len(a) > 0 && !a[0].Without { - a[0].Grouping = append(a[0].Grouping, n.VectorMatching.Include...) - } - return a - case promParser.CardManyToMany: - default: - slog.Warn("Unsupported VectorMatching operation", slog.String("matching", n.VectorMatching.Card.String())) - } - } - - if n.Op.IsComparisonOperator() { - for i, child := range node.Children { - if n.VectorMatching != nil { - a := HasOuterAggregation(child) - if len(a) > 0 && !a[0].Without { - a[0].Grouping = append(a[0].Grouping, n.VectorMatching.Include...) - } - return a - } - if i == 0 { - return HasOuterAggregation(child) - } - } - } else { - switch n.Op { - case promParser.LOR: - for _, child := range node.Children { - aggs = append(aggs, HasOuterAggregation(child)...) - } - return aggs - case promParser.LUNLESS, promParser.LAND: - for _, child := range node.Children { - return HasOuterAggregation(child) - } - case promParser.DIV, promParser.SUB, promParser.ADD: - if _, ok := n.LHS.(*promParser.NumberLiteral); ok { - goto CHILDREN - } - for _, child := range node.Children { - return HasOuterAggregation(child) - } - } - } - } - -CHILDREN: - for _, child := range node.Children { - aggs = append(aggs, HasOuterAggregation(child)...) - } - - return aggs -} diff --git a/internal/parser/utils/aggregation_test.go b/internal/parser/utils/aggregation_test.go deleted file mode 100644 index 065adf2c..00000000 --- a/internal/parser/utils/aggregation_test.go +++ /dev/null @@ -1,163 +0,0 @@ -package utils_test - -import ( - "testing" - - "github.com/stretchr/testify/require" - - "github.com/cloudflare/pint/internal/parser" - "github.com/cloudflare/pint/internal/parser/utils" -) - -func TestHasOuterAggregation(t *testing.T) { - type testCaseT struct { - expr string - output []string - } - - testCases := []testCaseT{ - { - expr: "foo", - }, - { - expr: "sum(foo)", - output: []string{"sum(foo)"}, - }, - { - expr: "sum(foo) by(job)", - output: []string{"sum by (job) (foo)"}, - }, - { - expr: "sum(foo) without(job)", - output: []string{"sum without (job) (foo)"}, - }, - { - expr: "1 + sum(foo)", - output: []string{"sum(foo)"}, - }, - { - expr: "vector(0) or sum(foo)", - output: []string{"sum(foo)"}, - }, - { - expr: "sum(foo) or vector(0)", - output: []string{"sum(foo)"}, - }, - { - expr: "sum(foo) + sum(bar)", - output: []string{"sum(foo)"}, - }, - { - expr: "foo / on(bbb) sum(bar)", - }, - { - expr: "sum(foo) / on(bbb) sum(bar)", - output: []string{"sum(foo)"}, - }, - { - expr: "sum(foo) OR sum(bar) by(job)", - output: []string{"sum(foo)", "sum by (job) (bar)"}, - }, - { - expr: "foo OR sum(foo) OR sum(bar) by(job)", - output: []string{"sum(foo)", "sum by (job) (bar)"}, - }, - { - expr: "1 + sum(foo) by(job) + sum(foo) by(notjob)", - output: []string{"sum by (job) (foo)"}, - }, - { - expr: "sum(foo) by (job) > count(bar)", - output: []string{"sum by (job) (foo)"}, - }, - { - expr: "sum(foo) by (job) > count(foo) / 2 or sum(bar) by (job) > count(bar)", - output: []string{"sum by (job) (foo)", "sum by (job) (bar)"}, - }, - { - expr: "(foo unless on(instance, version, package) bar) and on(instance) (sum(enabled) by(instance) > 0)", - output: []string{}, - }, - { - expr: "count(build_info) by (instance, version) != ignoring(bar) group_left(package) count(foo) by (instance, version, package)", - output: []string{"count by (instance, version, package) (build_info)"}, - }, - { - expr: "sum(foo) without() != on() group_left(instance) sum(vector(0))", - output: []string{"sum without () (foo)"}, - }, - { - expr: "sum(foo) != on() group_right(instance) sum(vector(0))", - output: []string{"sum by (instance) (vector(0))"}, - }, - { - expr: "min(foo) by(bar) and max by(bar) (foo)", - output: []string{"min by (bar) (foo)"}, - }, - { - expr: "max(foo)", - output: []string{"max(foo)"}, - }, - { - expr: "avg(foo) by(bar)", - output: []string{"avg by (bar) (foo)"}, - }, - { - expr: "group(foo)", - output: []string{"group(foo)"}, - }, - { - expr: "stddev(foo)", - output: []string{"stddev(foo)"}, - }, - { - expr: "(stdvar(foo))", - output: []string{"stdvar(foo)"}, - }, - { - expr: `(1 - count_values("job", foo))`, - output: []string{`count_values("job", foo)`}, - }, - { - expr: "(topk(5, foo))", - }, - { - expr: "(bottomk(5, foo))", - }, - { - expr: "(quantile(0.9, foo))", - output: []string{"quantile(0.9, foo)"}, - }, - { - expr: `abs(rate(scylla_cql_updates{conditional="no"}[1m]) - scalar(avg(rate(scylla_cql_updates{conditional="no"}[1m]))))`, - }, - { - expr: `abs(rate(scylla_cql_updates{conditional="no"}[1m]) + scalar(avg(rate(scylla_cql_updates{conditional="no"}[1m]))))`, - }, - { - expr: `abs(rate(scylla_cql_updates{conditional="no"}[1m]) / scalar(avg(rate(scylla_cql_updates{conditional="no"}[1m]))))`, - }, - } - - for _, tc := range testCases { - t.Run(tc.expr, func(t *testing.T) { - n, err := parser.DecodeExpr(tc.expr) - if err != nil { - t.Error(err) - t.FailNow() - } - aggs := utils.HasOuterAggregation(n) - if len(aggs) == 0 { - if len(tc.output) > 0 { - t.Errorf("HasOuterAggregation() returned nil, expected %s", tc.output) - } - } else { - output := []string{} - for _, a := range aggs { - output = append(output, a.String()) - } - require.Equal(t, tc.output, output, "HasOuterAggregation() returned wrong output") - } - }) - } -}