Merge pull request #1703 from cloudflare/ignoring

Check ignoring() when validating joins

Author: prymitive

docs/changelog.md

@@ -10,6 +10,7 @@
 ### Fixed
 
 - Fixed a crash in [rule/label](checks/rule/label.md) - [#1696](https://github.com/cloudflare/pint/pull/1696).
+- Fixed false positive reports from [promql/impossible](checks/promql/impossible.md) check.
 
 ## v0.78.0
 

internal/checks/promql_impossible_test.go

@@ -703,6 +703,20 @@ func TestImpossibleCheck(t *testing.T) {
         , "workload", "", "owner_name")
       , "workload_type", "", "owner_kind")
     )
+`,
+			checker:    newImpossibleCheck,
+			prometheus: newSimpleProm,
+		},
+		{
+			description: "sum(...) by(a,b) / ignoring(b) sum(...) by(a)",
+			content: `
+- alert: foo
+  expr: |
+    (
+      sum(rate(panics_total{module_name=~".+"}[5m])) by (colo_name, module_name)
+      / ignoring(module_name) group_left
+      sum(colo:requests:rate5m) by (colo_name)
+    ) > 0.01
 `,
 			checker:    newImpossibleCheck,
 			prometheus: newSimpleProm,

internal/checks/promql_impossible_test.snap

@@ -3265,6 +3265,11 @@
 
 ---
 
+[TestImpossibleCheck/sum(...)_by(a,b)_/_ignoring(b)_sum(...)_by(a) - 1]
+[]
+
+---
+
 [TestImpossibleCheck/sum(foo_or_vector(0))_>_0 - 1]
 - description: sum(foo or vector(0)) > 0
   content: |4

internal/parser/source/source.go

@@ -1404,6 +1404,9 @@ func canJoin(ls, rs Source, vm *promParser.VectorMatching) (bool, string, posran
 			if l.Kind != GuaranteedLabel {
 				continue
 			}
+			if slices.Contains(vm.MatchingLabels, name) {
+				continue
+			}
 			if ls.CanHaveLabel(name) && !rs.CanHaveLabel(name) {
 				reason, fragment := rs.LabelExcludeReason(name)
 				return false, fmt.Sprintf("The %s hand side will never be matched because it doesn't have the `%s` label while the left hand side will. %s",

internal/parser/source/source_test.go

@@ -376,6 +376,14 @@ group by (cluster, namespace, workload, workload_type, pod) (
 	`foo{job="xxx"} + ignoring(job) group_right(instance) bar{job="zzz"}`,
 	`foo or ignoring(job) bar`,
 	`foo or on(job) bar`,
+	`
+(
+  sum(rate(panics_total{module_name=~".+"}[5m])) by (colo_name, module_name)
+  /
+  ignoring(module_name) group_left
+  sum(colo:requests:rate5m) by (colo_name)
+) > 0.01
+`,
 }
 
 func TestLabelsSource(t *testing.T) {

internal/parser/source/source_test.snap

@@ -11813,3 +11813,98 @@ output:
       type: selector
 
 ---
+
+[TestLabelsSource/184 - 1]
+expr: |4
+
+    (
+      sum(rate(panics_total{module_name=~".+"}[5m])) by (colo_name, module_name)
+      /
+      ignoring(module_name) group_left
+      sum(colo:requests:rate5m) by (colo_name)
+    ) > 0.01
+output:
+    - labels:
+        "":
+            reason: Query is using aggregation with `by(colo_name, module_name)`, only labels included inside `by(...)` will be present on the results.
+            kind: excluded
+            fragment:
+                start: 52
+                end: 55
+        __name__:
+            reason: Binary operation between two vectors removes metric names.
+            kind: excluded
+            fragment:
+                start: 5
+                end: 161
+        colo_name:
+            reason: Query is using aggregation with `by(colo_name, module_name)`, only labels included inside `by(...)` will be present on the results.
+            kind: included
+            fragment:
+                start: 56
+                end: 65
+        module_name:
+            reason: Query will only return series where these labels are present.
+            kind: guaranteed
+            fragment:
+                start: 9
+                end: 50
+      returns: vector
+      operations:
+        - node: '[*parser.VectorSelector] panics_total{module_name=~".+"}'
+          op: ""
+        - node: '[*parser.Call] rate(panics_total{module_name=~".+"}[5m])'
+          op: rate
+        - node: '[*parser.AggregateExpr] sum by (colo_name, module_name) (rate(panics_total{module_name=~".+"}[5m]))'
+          op: sum
+      joins:
+        - matchinglabels:
+            - module_name
+          addedlabels: []
+          src:
+            labels:
+                "":
+                    reason: Query is using aggregation with `by(colo_name)`, only labels included inside `by(...)` will be present on the results.
+                    kind: excluded
+                    fragment:
+                        start: 147
+                        end: 150
+                __name__:
+                    reason: Aggregation removes metric name.
+                    kind: excluded
+                    fragment:
+                        start: 121
+                        end: 161
+                colo_name:
+                    reason: Query is using aggregation with `by(colo_name)`, only labels included inside `by(...)` will be present on the results.
+                    kind: included
+                    fragment:
+                        start: 151
+                        end: 160
+            returns: vector
+            operations:
+                - node: '[*parser.VectorSelector] colo:requests:rate5m'
+                  op: ""
+                - node: '[*parser.AggregateExpr] sum by (colo_name) (colo:requests:rate5m)'
+                  op: sum
+            usedLabels:
+                - colo_name
+            position:
+                start: 125
+                end: 145
+            type: aggregation
+            fixedLabels: true
+          op: 57384
+          depth: 0
+          ison: false
+      usedLabels:
+        - colo_name
+        - module_name
+      position:
+        start: 14
+        end: 49
+      type: aggregation
+      fixedLabels: true
+      isConditional: true
+
+---