Check for bool midifier in comparision

CHANGELOG.md

@@ -1,15 +1,34 @@
 # Changelog
 
-## v0.3.1
+## v0.4.0
 
-### Fixed
+### Added
 
-- Fixed `template` check panic when alert query had a syntax error.
+- `comparison` check will now warn when alert query uses
+  [bool](https://prometheus.io/docs/prometheus/latest/querying/operators/#comparison-binary-operators)
+  modifier after condition, which can cause alert to always fire.
+  Example:
+
+  ```
+  - alert: Foo
+    expr: rate(error_count[5m]) > bool 5
+  ```
+
+  Having `bool` as part of `> 5` condition means that the query will return value `1` when condition
+  is met, and `0` when it's not. Rather than returning value of `rate(error_count[5m])` only when
+  that value is `> 5`. Since all results of an alerting rule `expr` are considered alerts such alert
+  rule could always fire, regardless of the value returned by `rate(error_count[5m])`.
+
+## v0.3.1
 
 ### Added
 
 - `--offline` flag for `pint ci` command.
 
+### Fixed
+
+- Fixed `template` check panic when alert query had a syntax error.
+
 ## v0.3.0
 
 ### Added

internal/checks/promql_comparison.go

@@ -31,7 +31,16 @@ func (c ComparisonCheck) Check(rule parser.Rule) (problems []Problem) {
 		return
 	}
 
-	if hasComparision(rule.Expr().Query) {
+	if expr := hasComparision(rule.Expr().Query); expr != nil {
+		if expr.ReturnBool {
+			problems = append(problems, Problem{
+				Fragment: rule.AlertingRule.Expr.Value.Value,
+				Lines:    rule.AlertingRule.Expr.Lines(),
+				Reporter: ComparisonCheckName,
+				Text:     "alert query uses bool modifier for comparison, this means it will always return a result and the alert will always fire",
+				Severity: c.severity,
+			})
+		}
 		return
 	}
 
@@ -46,21 +55,21 @@ func (c ComparisonCheck) Check(rule parser.Rule) (problems []Problem) {
 	return
 }
 
-func hasComparision(n *parser.PromQLNode) bool {
+func hasComparision(n *parser.PromQLNode) *promParser.BinaryExpr {
 	if node, ok := n.Node.(*promParser.BinaryExpr); ok {
 		if node.Op.IsComparisonOperator() {
-			return true
+			return node
 		}
 		if node.Op == promParser.LUNLESS {
-			return true
+			return node
 		}
 	}
 
 	for _, child := range n.Children {
-		if hasComparision(child) {
-			return true
+		if node := hasComparision(child); node != nil {
+			return node
 		}
 	}
 
-	return false
+	return nil
 }

internal/checks/promql_comparison_test.go

@@ -30,7 +30,8 @@ func TestComparisonCheck(t *testing.T) {
 		{
 			description: "alert expr with == condition",
 			content:     "- alert: Foo Is Down\n  for: 10m\n  expr: up{job=\"foo\"} == 1\n",
-			checker:     checks.NewComparisonCheck(checks.Bug)},
+			checker:     checks.NewComparisonCheck(checks.Bug),
+		},
 		{
 			description: "alert expr without any condition",
 			content:     "- alert: Foo Is Down\n  expr: up{job=\"foo\"}\n",
@@ -79,6 +80,25 @@ func TestComparisonCheck(t *testing.T) {
 			content:     "- alert: Foo Is Down\n  for: 10m\n  expr: foo unless bar\n",
 			checker:     checks.NewComparisonCheck(checks.Bug),
 		},
+		{
+			description: "alert expr with bool",
+			content:     "- alert: Error rate is high\n  expr: rate(error_count[5m]) > bool 5\n",
+			checker:     checks.NewComparisonCheck(checks.Warning),
+			problems: []checks.Problem{
+				{
+					Fragment: "rate(error_count[5m]) > bool 5",
+					Lines:    []int{2},
+					Reporter: "promql/comparison",
+					Text:     "alert query uses bool modifier for comparison, this means it will always return a result and the alert will always fire",
+					Severity: checks.Warning,
+				},
+			},
+		},
+		{
+			description: "alert expr with bool and condition",
+			content:     "- alert: Error rate is high\n  expr: rate(error_count[5m]) > bool 5 == 1\n",
+			checker:     checks.NewComparisonCheck(checks.Warning),
+		},
 	}
 
 	runTests(t, testCases)