Fix false positive in alerts/template check

Fixes #681.
cloudflare · Jul 31, 2023 · ad75114 · ad75114
1 parent 7ada3f9
commit ad75114
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 4 deletions.
diff --git 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
@@ -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
@@ -78,14 +78,22 @@ 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)
  }
  }
  }
  }
 
+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
@@ -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 {