From ad75114d7d8c2095d574e166013b019ec00163dc Mon Sep 17 00:00:00 2001
From: Lukasz Mierzwa <lukasz@cloudflare.com>
Date: Mon, 31 Jul 2023 15:34:33 +0100
Subject: [PATCH] Fix false positive in alerts/template check

Fixes #681.
---
 docs/changelog.md                         |  1 +
 internal/checks/alerts_template_test.go   | 25 ++++++++++++++++++++++-
 internal/parser/utils/aggregation.go      | 10 ++++++++-
 internal/parser/utils/aggregation_test.go | 13 ++++++++++--
 4 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/docs/changelog.md b/docs/changelog.md
index 78de0fa2..4ffc449c 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -6,6 +6,7 @@
 
 - Fixed a crash in `promql/series` check when Prometheus instance becomes
   unavailable - #682.
+- Fixed false positive reports in `alerts/template` check - #681.
 
 ## v0.44.1
 
diff --git a/internal/checks/alerts_template_test.go b/internal/checks/alerts_template_test.go
index a539caeb..69ca9507 100644
--- a/internal/checks/alerts_template_test.go
+++ b/internal/checks/alerts_template_test.go
@@ -388,7 +388,7 @@ func TestTemplateCheck(t *testing.T) {
 		},
 		{
 			description: "annotation label missing from metrics (1+)",
-			content:     "- alert: Foo Is Down\n  expr: 1 + sum(foo) by(job) + sum(foo) by(notjob)\n  annotations:\n    summary: '{{ .Labels.job }}'\n",
+			content:     "- alert: Foo Is Down\n  expr: 1 + sum(foo) by(notjob)\n  annotations:\n    summary: '{{ .Labels.job }}'\n",
 			checker:     newTemplateCheck,
 			prometheus:  noProm,
 			problems: func(uri string) []checks.Problem {
@@ -989,6 +989,29 @@ func TestTemplateCheck(t *testing.T) {
     sum(foo:count) by(job) > 20
   labels:
     notify: "{{ $labels.notify }}"
+`,
+			checker:    newTemplateCheck,
+			prometheus: noProm,
+			problems:   noProblems,
+		},
+		{
+			description: "abs / scalar",
+			content: `
+- alert: ScyllaNonBalancedcqlTraffic
+  expr: >
+    abs(rate(scylla_cql_updates{conditional="no"}[1m]) - scalar(avg(rate(scylla_cql_updates{conditional="no"}[1m]))))
+    /
+    scalar(stddev(rate(scylla_cql_updates{conditional="no"}[1m])) + 100) > 2
+  for: 10s
+  labels:
+    advisor: balanced
+    dashboard: cql
+    severity: moderate
+    status: "1"
+    team: team_devops
+  annotations:
+    description: CQL queries are not balanced among shards {{ $labels.instance }} shard {{ $labels.shard }}
+    summary: CQL queries are not balanced
 `,
 			checker:    newTemplateCheck,
 			prometheus: noProm,
diff --git a/internal/parser/utils/aggregation.go b/internal/parser/utils/aggregation.go
index 9bcf4759..9e8d8059 100644
--- a/internal/parser/utils/aggregation.go
+++ b/internal/parser/utils/aggregation.go
@@ -78,7 +78,14 @@ NEXT:
 					aggs = append(aggs, HasOuterAggregation(child)...)
 				}
 				return aggs
-			case promParser.DIV, promParser.LUNLESS, promParser.LAND:
+			case promParser.LUNLESS, promParser.LAND:
+				for _, child := range node.Children {
+					return HasOuterAggregation(child)
+				}
+			case promParser.DIV, promParser.SUB, promParser.ADD:
+				if _, ok := n.LHS.(*promParser.NumberLiteral); ok {
+					goto CHILDREN
+				}
 				for _, child := range node.Children {
 					return HasOuterAggregation(child)
 				}
@@ -86,6 +93,7 @@ NEXT:
 		}
 	}
 
+CHILDREN:
 	for _, child := range node.Children {
 		aggs = append(aggs, HasOuterAggregation(child)...)
 	}
diff --git a/internal/parser/utils/aggregation_test.go b/internal/parser/utils/aggregation_test.go
index e1fe509d..065adf2c 100644
--- a/internal/parser/utils/aggregation_test.go
+++ b/internal/parser/utils/aggregation_test.go
@@ -45,7 +45,7 @@ func TestHasOuterAggregation(t *testing.T) {
 		},
 		{
 			expr:   "sum(foo) + sum(bar)",
-			output: []string{"sum(foo)", "sum(bar)"},
+			output: []string{"sum(foo)"},
 		},
 		{
 			expr: "foo / on(bbb) sum(bar)",
@@ -64,7 +64,7 @@ func TestHasOuterAggregation(t *testing.T) {
 		},
 		{
 			expr:   "1 + sum(foo) by(job) + sum(foo) by(notjob)",
-			output: []string{"sum by (job) (foo)", "sum by (notjob) (foo)"},
+			output: []string{"sum by (job) (foo)"},
 		},
 		{
 			expr:   "sum(foo) by (job) > count(bar)",
@@ -128,6 +128,15 @@ func TestHasOuterAggregation(t *testing.T) {
 			expr:   "(quantile(0.9, foo))",
 			output: []string{"quantile(0.9, foo)"},
 		},
+		{
+			expr: `abs(rate(scylla_cql_updates{conditional="no"}[1m]) - scalar(avg(rate(scylla_cql_updates{conditional="no"}[1m]))))`,
+		},
+		{
+			expr: `abs(rate(scylla_cql_updates{conditional="no"}[1m]) + scalar(avg(rate(scylla_cql_updates{conditional="no"}[1m]))))`,
+		},
+		{
+			expr: `abs(rate(scylla_cql_updates{conditional="no"}[1m]) / scalar(avg(rate(scylla_cql_updates{conditional="no"}[1m]))))`,
+		},
 	}
 
 	for _, tc := range testCases {