diff --git a/docs/changelog.md b/docs/changelog.md index 900d9dd0..987c6eb5 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -20,6 +20,9 @@ {% endraw %} +- `alerts/comparison` check can now warn if alerting rules use a query + with `foo > 0 OR vector(1)`, which would always fire. + ## v0.46.0 ### Added diff --git a/internal/checks/alerts_comparison.go b/internal/checks/alerts_comparison.go index ec152f8e..bec9ea51 100644 --- a/internal/checks/alerts_comparison.go +++ b/internal/checks/alerts_comparison.go @@ -5,6 +5,7 @@ import ( "github.com/cloudflare/pint/internal/discovery" "github.com/cloudflare/pint/internal/parser" + "github.com/cloudflare/pint/internal/parser/utils" promParser "github.com/prometheus/prometheus/promql/parser" ) @@ -42,6 +43,18 @@ func (c ComparisonCheck) Check(_ context.Context, _ string, rule parser.Rule, _ expr := rule.Expr().Query + if n := utils.HasOuterBinaryExpr(expr); n != nil && n.Op == promParser.LOR { + if (hasComparision(n.LHS) == nil || hasComparision(n.RHS) == nil) && !isAbsent(n.LHS) && !isAbsent(n.RHS) { + problems = append(problems, Problem{ + Fragment: rule.AlertingRule.Expr.Value.Value, + Lines: rule.AlertingRule.Expr.Lines(), + Reporter: c.Reporter(), + Text: "alert query uses 'or' operator with one side of the query that will always return a result, this alert will always fire", + Severity: rewriteSeverity(Warning, n.LHS, n.RHS), + }) + } + } + if n := hasComparision(expr.Node); n != nil { if n.ReturnBool && hasComparision(n.LHS) == nil && hasComparision(n.RHS) == nil { problems = append(problems, Problem{ @@ -89,8 +102,15 @@ func hasComparision(n promParser.Node) *promParser.BinaryExpr { return nil } +func isAbsent(node promParser.Node) bool { + if node, ok := node.(*promParser.Call); ok && (node.Func.Name == "absent") { + return true + } + return false +} + func hasAbsent(n *parser.PromQLNode) bool { - if node, ok := n.Node.(*promParser.Call); ok && (node.Func.Name == "absent") { + if isAbsent(n.Node) { return true } for _, child := range n.Children { @@ -100,3 +120,12 @@ func hasAbsent(n *parser.PromQLNode) bool { } return false } + +func rewriteSeverity(s Severity, nodes ...promParser.Node) Severity { + for _, node := range nodes { + if n, ok := node.(*promParser.Call); ok && n.Func.Name == "vector" { + return Bug + } + } + return s +} diff --git a/internal/checks/alerts_comparison_test.go b/internal/checks/alerts_comparison_test.go index a8d51762..6dc7ccd5 100644 --- a/internal/checks/alerts_comparison_test.go +++ b/internal/checks/alerts_comparison_test.go @@ -158,6 +158,57 @@ func TestComparisonCheck(t *testing.T) { prometheus: noProm, problems: noProblems, }, + { + description: "vector(0) or (foo > 0)", + content: "- alert: Foo Is Down\n expr: (foo > 0) or vector(0)\n", + checker: newComparisonCheck, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `(foo > 0) or vector(0)`, + Lines: []int{2}, + Reporter: checks.ComparisonCheckName, + Text: "alert query uses 'or' operator with one side of the query that will always return a result, this alert will always fire", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "(foo > 0) or vector(0)", + content: "- alert: Foo Is Down\n expr: (foo > 0) or vector(0)\n", + checker: newComparisonCheck, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `(foo > 0) or vector(0)`, + Lines: []int{2}, + Reporter: checks.ComparisonCheckName, + Text: "alert query uses 'or' operator with one side of the query that will always return a result, this alert will always fire", + Severity: checks.Bug, + }, + } + }, + }, + { + description: "absent(foo) or vector(0)", + content: "- alert: Foo Is Down\n expr: (foo > 0) or vector(0)\n", + checker: newComparisonCheck, + prometheus: noProm, + problems: func(uri string) []checks.Problem { + return []checks.Problem{ + { + Fragment: `(foo > 0) or vector(0)`, + Lines: []int{2}, + Reporter: checks.ComparisonCheckName, + Text: "alert query uses 'or' operator with one side of the query that will always return a result, this alert will always fire", + Severity: checks.Bug, + }, + } + }, + }, } runTests(t, testCases)