Merge pull request #753 from cloudflare/comp

More strict alerts/comparison check
cloudflare · Oct 18, 2023 · ee0835b · ee0835b
2 parents 6b679f1 + 7f6ebb7
commit ee0835b
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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)