Merge pull request cloudflare#200 from cloudflare/fragile

Fix false positives from promql/fragile
linode-obs · Mar 21, 2022 · 8543d73 · 8543d73
2 parents bc531ee + 9906dde
commit 8543d73
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 4 deletions.
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -1,5 +1,12 @@
 # Changelog
 
+## v0.15.4
+
+### Fixed
+
+- Fixed false positive reports from `promql/fragile` when `foo OR bar` is used inside
+  aggregation.
+
 ## v0.15.3
 
 ### Fixed

diff --git a/internal/checks/promql_fragile.go b/internal/checks/promql_fragile.go
@@ -48,6 +48,12 @@ func (c FragileCheck) Check(ctx context.Context, rule parser.Rule, entries []dis
 
 func (c FragileCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem) {
 	if n := utils.HasOuterBinaryExpr(node); n != nil && n.Op != promParser.LOR && n.Op != promParser.LUNLESS {
+		if _, ok := n.LHS.(*promParser.NumberLiteral); ok {
+			goto NEXT
+		}
+		if _, ok := n.RHS.(*promParser.NumberLiteral); ok {
+			goto NEXT
+		}
 		var isFragile bool
 		for _, child := range node.Children {
 			for _, agg := range utils.HasOuterAggregation(child) {

diff --git a/internal/checks/promql_fragile_test.go b/internal/checks/promql_fragile_test.go
@@ -83,7 +83,7 @@ func TestFragileCheck(t *testing.T) {
 			problems: func(uri string) []checks.Problem {
 				return []checks.Problem{
 					{
-						Fragment: `(sum(foo) without(job) + sum(bar) without(job)) > 1`,
+						Fragment: `(sum without(job) (foo) + sum without(job) (bar))`,
 						Lines:    []int{2},
 						Reporter: "promql/fragile",
 						Text:     text,
@@ -115,7 +115,7 @@ func TestFragileCheck(t *testing.T) {
 			problems: func(uri string) []checks.Problem {
 				return []checks.Problem{
 					{
-						Fragment: `(sum without(job) (foo) + sum(bar)) > 1`,
+						Fragment: `(sum without(job) (foo) + sum(bar))`,
 						Lines:    []int{2},
 						Reporter: "promql/fragile",
 						Text:     text,
@@ -125,17 +125,55 @@ func TestFragileCheck(t *testing.T) {
 			},
 		},
 		{
-			description: "ignores safe division",
+			description: "ignores safe addition",
 			content:     "- record: foo\n  expr: sum(foo) + sum(bar)\n",
 			checker:     newFragileCheck,
 			problems:    noProblems,
 		},
 		{
-			description: "ignores division if source metric is the same",
+			description: "ignores addition if source metric is the same",
 			content:     "- record: foo\n  expr: sum(foo) without(bar) + sum(foo) without(bar)\n",
 			checker:     newFragileCheck,
 			problems:    noProblems,
 		},
+		{
+			description: "handles nested aggregations correctly / LHS",
+			content: `
+- alert: foo
+  expr: |
+    count without (foo) (
+        probe_success{job="foo"} == 0 or probe_duration_seconds{job="foo"} >= 15
+    ) > 3
+`,
+			checker:  newFragileCheck,
+			problems: noProblems,
+		},
+		{
+			description: "handles nested aggregations correctly / RHS",
+			content: `
+- alert: foo
+  expr: |
+    3 <
+    count without (foo) (
+        probe_success{job="foo"} == 0 or probe_duration_seconds{job="foo"} >= 15
+    )
+`,
+			checker:  newFragileCheck,
+			problems: noProblems,
+		},
+		{
+			description: "handles nested aggregations correctly / both",
+			content: `
+- alert: foo
+  expr: |
+    3 <
+    count without (foo) (
+        probe_success{job="foo"} == 0 or probe_duration_seconds{job="foo"} >= 15
+    ) > 2
+`,
+			checker:  newFragileCheck,
+			problems: noProblems,
+		},
 	}
 
 	runTests(t, testCases)