From 8474620b28d5cc8c8fe19c8404b7a1d826aa48db Mon Sep 17 00:00:00 2001
From: Lukasz Mierzwa <lukasz@cloudflare.com>
Date: Thu, 21 Mar 2024 12:04:49 +0000
Subject: [PATCH] More strict type checks for promql/counter

---
 docs/changelog.md                      |  8 +++++++
 internal/checks/promql_counter.go      | 30 +++++++++++++++-----------
 internal/checks/promql_counter_test.go | 18 ++++++++++++++++
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/docs/changelog.md b/docs/changelog.md
index 843491cf..84504f30 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -1,5 +1,13 @@
 # Changelog
 
+## v0.56.1
+
+### Fixed
+
+- [promql/counter](checks/promql/counter.md) check will now consider a metric
+  to be a counter only if all metadata entries for it use `TYPE counter`.
+  Previously it would check for at least one metadata entry with `TYPE counter`.
+
 ## v0.56.0
 
 ### Added
diff --git a/internal/checks/promql_counter.go b/internal/checks/promql_counter.go
index 3876838d..409e8ac1 100644
--- a/internal/checks/promql_counter.go
+++ b/internal/checks/promql_counter.go
@@ -105,22 +105,28 @@ LOOP:
 				Text:     text,
 				Severity: severity,
 			})
-			continue
+			continue LOOP
+		}
+		if len(metadata.Metadata) == 0 {
+			// No metadata so we don't know what type it uses.
+			continue LOOP
 		}
 		for _, m := range metadata.Metadata {
-			if m.Type == v1.MetricTypeCounter {
-				problems = append(problems, Problem{
-					Lines:    expr.Value.Lines,
-					Reporter: c.Reporter(),
-					Text: fmt.Sprintf("`%s` is a counter according to metrics metadata from %s, you can't use its value directly.",
-						selector.Name,
-						promText(c.prom.Name(), metadata.URI),
-					),
-					Details:  CounterCheckDetails,
-					Severity: Bug,
-				})
+			if m.Type != v1.MetricTypeCounter {
+				// There's metadata with non-counter type, so it's not always a counter.
+				continue LOOP
 			}
 		}
+		problems = append(problems, Problem{
+			Lines:    expr.Value.Lines,
+			Reporter: c.Reporter(),
+			Text: fmt.Sprintf("`%s` is a counter according to metrics metadata from %s, you can't use its value directly.",
+				selector.Name,
+				promText(c.prom.Name(), metadata.URI),
+			),
+			Details:  CounterCheckDetails,
+			Severity: Bug,
+		})
 
 		done[selector.Name] = struct{}{}
 	}
diff --git a/internal/checks/promql_counter_test.go b/internal/checks/promql_counter_test.go
index 548d08f1..4613ce30 100644
--- a/internal/checks/promql_counter_test.go
+++ b/internal/checks/promql_counter_test.go
@@ -368,6 +368,24 @@ func TestCounterCheck(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "counter > 1 / mixed metadata",
+			content: `
+- alert: my alert
+  expr:  http_requests_total{cluster="prod"} > 1
+`,
+			checker:    newCounterCheck,
+			prometheus: newSimpleProm,
+			problems:   noProblems,
+			mocks: []*prometheusMock{
+				{
+					conds: []requestCondition{requireMetadataPath},
+					resp: metadataResponse{metadata: map[string][]v1.Metadata{
+						"http_requests_total": {{Type: "counter"}, {Type: "gauge"}, {Type: "counter"}},
+					}},
+				},
+			},
+		},
 	}
 	runTests(t, testCases)
 }