From 6435c9c85943e90408a8df7c22b06ee7d4acb1a7 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Thu, 24 Oct 2024 15:42:00 +0100 Subject: [PATCH] Use LabelsSource for alerts/absent --- internal/checks/alerts_absent.go | 12 ++- internal/checks/alerts_absent_test.go | 33 ++++++ internal/parser/utils/absent.go | 78 -------------- internal/parser/utils/absent_test.go | 150 -------------------------- 4 files changed, 43 insertions(+), 230 deletions(-) delete mode 100644 internal/parser/utils/absent.go delete mode 100644 internal/parser/utils/absent_test.go diff --git a/internal/checks/alerts_absent.go b/internal/checks/alerts_absent.go index c40870b3..4e8f963b 100644 --- a/internal/checks/alerts_absent.go +++ b/internal/checks/alerts_absent.go @@ -8,10 +8,10 @@ import ( "github.com/cloudflare/pint/internal/discovery" "github.com/cloudflare/pint/internal/output" "github.com/cloudflare/pint/internal/parser" + "github.com/cloudflare/pint/internal/parser/utils" "github.com/cloudflare/pint/internal/promapi" "github.com/prometheus/common/model" - promParser "github.com/prometheus/prometheus/promql/parser" ) const ( @@ -58,7 +58,15 @@ func (c AlertsAbsentCheck) Check(ctx context.Context, _ discovery.Path, rule par return problems } - if n, ok := rule.AlertingRule.Expr.Query.Expr.(*promParser.Call); !ok || n.Func.Name != "absent" { + var hasAbsent bool + src := utils.LabelsSource(rule.AlertingRule.Expr.Query) + for _, s := range append(src.Alternatives, src) { + if s.Operation == "absent" { + hasAbsent = true + } + } + + if !hasAbsent { return problems } diff --git a/internal/checks/alerts_absent_test.go b/internal/checks/alerts_absent_test.go index 761e5c4b..68749806 100644 --- a/internal/checks/alerts_absent_test.go +++ b/internal/checks/alerts_absent_test.go @@ -34,6 +34,13 @@ func TestAlertsAbsentCheck(t *testing.T) { prometheus: newSimpleProm, problems: noProblems, }, + { + description: "ignores rules with no absent()", + content: "- alert: foo\n expr: count(foo)\n for: 2m\n", + checker: newAlertsAbsentCheck, + prometheus: newSimpleProm, + problems: noProblems, + }, { description: "ignores rules with invalid duration", content: "- alert: foo\n expr: absent(foo)\n for: abc\n", @@ -47,6 +54,32 @@ func TestAlertsAbsentCheck(t *testing.T) { }, }, }, + { + description: "count() or absent() without for", + content: "- alert: foo\n expr: count(foo) > 5 or absent(foo)\n", + checker: newAlertsAbsentCheck, + prometheus: newSimpleProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Lines: parser.LineRange{ + First: 2, + Last: 2, + }, + Reporter: checks.AlertsAbsentCheckName, + Text: absentForNeeded("prom", uri, "2m"), + Details: checks.AlertsAbsentCheckDetails, + Severity: checks.Warning, + }, + } + }, + mocks: []*prometheusMock{ + { + conds: []requestCondition{requireConfigPath}, + resp: configResponse{yaml: "global:\n scrape_interval: 1m\n"}, + }, + }, + }, { description: "absent() without for", content: "- alert: foo\n expr: absent(foo)\n", diff --git a/internal/parser/utils/absent.go b/internal/parser/utils/absent.go deleted file mode 100644 index f7f52cb7..00000000 --- a/internal/parser/utils/absent.go +++ /dev/null @@ -1,78 +0,0 @@ -package utils - -import ( - "log/slog" - - "github.com/cloudflare/pint/internal/parser" - - promParser "github.com/prometheus/prometheus/promql/parser" -) - -type PromQLFragment struct { - Fragment *parser.PromQLNode - BinExpr *promParser.BinaryExpr -} - -func HasOuterAbsent(node *parser.PromQLNode) (calls []PromQLFragment) { - if n, ok := node.Expr.(*promParser.Call); ok && n.Func.Name == "absent" { - calls = append(calls, PromQLFragment{Fragment: node}) - return calls - } - - if n, ok := node.Expr.(*promParser.BinaryExpr); ok { - if n.VectorMatching != nil { - switch n.VectorMatching.Card { - // bar / absent(foo) - // absent(foo) / bar - case promParser.CardOneToOne: - - // absent(foo{job="bar"}) * on(job) group_left(xxx) bar - // bar * on() group_left(xxx) absent(foo{job="bar"}) - case promParser.CardManyToOne: - if ln, ok := n.LHS.(*promParser.Call); ok && ln.Func.Name == "absent" { - calls = append(calls, PromQLFragment{ - Fragment: node.Children[0], - BinExpr: n, - }) - } - - // bar * on() group_right(xxx) absent(foo{job="bar"}) - // absent(foo{job="bar"}) * on(job) group_right(xxx) bar - case promParser.CardOneToMany: - if rn, ok := n.RHS.(*promParser.Call); ok && rn.Func.Name == "absent" { - calls = append(calls, PromQLFragment{ - Fragment: node.Children[1], - BinExpr: n, - }) - } - - // bar AND absent(foo{job="bar"}) - // bar OR absent(foo{job="bar"}) - // bar UNLESS absent(foo{job="bar"}) - case promParser.CardManyToMany: - if n.Op == promParser.LOR { - for _, child := range node.Children { - calls = append(calls, HasOuterAbsent(child)...) - } - } else { - if ln, ok := n.LHS.(*promParser.Call); ok && ln.Func.Name == "absent" { - calls = append(calls, PromQLFragment{ - Fragment: node.Children[0], - BinExpr: n, - }) - } - } - - default: - slog.Warn("Unsupported VectorMatching operation", slog.String("matching", n.VectorMatching.Card.String())) - } - return calls - } - } - - for _, child := range node.Children { - calls = append(calls, HasOuterAbsent(child)...) - } - - return calls -} diff --git a/internal/parser/utils/absent_test.go b/internal/parser/utils/absent_test.go deleted file mode 100644 index 38e5a0d5..00000000 --- a/internal/parser/utils/absent_test.go +++ /dev/null @@ -1,150 +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 TestHasOuterAbsent(t *testing.T) { - type callT struct { - call string - binExpr string - } - - type testCaseT struct { - expr string - output []callT - } - - testCases := []testCaseT{ - { - expr: "foo", - }, - { - expr: "absent(foo)", - output: []callT{{call: "absent(foo)"}}, - }, - { - expr: `absent(foo{job="bar"})`, - output: []callT{{call: `absent(foo{job="bar"})`}}, - }, - { - expr: `absent(foo{job="bar"}) AND on(job) bar`, - output: []callT{{ - call: `absent(foo{job="bar"})`, - binExpr: `absent(foo{job="bar"}) and on (job) bar`, - }}, - }, - { - expr: `vector(1) or absent(foo{job="bar"}) AND on(job) bar`, - output: []callT{{ - call: `absent(foo{job="bar"})`, - binExpr: `absent(foo{job="bar"}) and on (job) bar`, - }}, - }, - { - expr: `up == 0 or absent(foo{job="bar"}) AND on(job) bar`, - output: []callT{{ - call: `absent(foo{job="bar"})`, - binExpr: `absent(foo{job="bar"}) and on (job) bar`, - }}, - }, - { - expr: `up == 0 or absent(foo{job="bar"}) or absent(bar)`, - output: []callT{ - {call: `absent(foo{job="bar"})`}, - {call: `absent(bar)`}, - }, - }, - { - expr: `absent(sum(nonexistent{job="myjob"}))`, - output: []callT{ - {call: `absent(sum(nonexistent{job="myjob"}))`}, - }, - }, - { - expr: `up == 0 or absent(foo{job="bar"}) * on(job) group_left(xxx) bar`, - output: []callT{{ - call: `absent(foo{job="bar"})`, - binExpr: `absent(foo{job="bar"}) * on (job) group_left (xxx) bar`, - }}, - }, - { - expr: `bar * on() group_left(xxx) absent(foo{job="bar"})`, - output: []callT{}, - }, - { - expr: `up == 0 or absent(foo{job="bar"}) * on(job) group_left() bar`, - output: []callT{{ - call: `absent(foo{job="bar"})`, - binExpr: `absent(foo{job="bar"}) * on (job) group_left () bar`, - }}, - }, - { - expr: `bar * on() group_right(xxx) absent(foo{job="bar"})`, - output: []callT{{ - call: `absent(foo{job="bar"})`, - binExpr: `bar * on () group_right (xxx) absent(foo{job="bar"})`, - }}, - }, - { - expr: `absent(foo{job="bar"}) * on(job) group_right(xxx) bar`, - output: []callT{}, - }, - { - expr: `absent(foo{job="bar"}) OR bar`, - output: []callT{{ - call: `absent(foo{job="bar"})`, - }}, - }, - { - expr: `absent(foo{job="bar"}) OR absent(foo{job="bob"})`, - output: []callT{ - {call: `absent(foo{job="bar"})`}, - {call: `absent(foo{job="bob"})`}, - }, - }, - { - expr: `absent(foo{job="bar"}) UNLESS absent(foo{job="bob"})`, - output: []callT{ - { - call: `absent(foo{job="bar"})`, - binExpr: `absent(foo{job="bar"}) unless absent(foo{job="bob"})`, - }, - }, - }, - } - - 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() - } - calls := utils.HasOuterAbsent(n) - if len(calls) == 0 { - if len(tc.output) > 0 { - t.Errorf("HasOuterAbsent() returned nil, expected %s", tc.output) - } - } else { - output := []callT{} - for _, a := range calls { - var c callT - if a.Fragment != nil { - c.call = a.Fragment.Expr.String() - } - if a.BinExpr != nil { - c.binExpr = a.BinExpr.String() - } - output = append(output, c) - } - require.Equal(t, tc.output, output, "HasOuterAbsent() returned wrong output") - } - }) - } -}