Merge pull request #104 from cloudflare/aggregate

Refactor aggregation checks

CHANGELOG.md

@@ -1,5 +1,18 @@
 # Changelog
 
+## v0.6.0
+
+### Changed
+
+- `aggregate` check was refactored and uses to run a single test for both
+  `by` and `without` conditions. As a result this check might now find issues
+  previously undetected.
+  Check suppression comments will need to be migrated:
+  * `# pint disable promql/by` becomes `# pint disable promql/aggregate`
+  * `# pint disable promql/without` becomes `# pint disable promql/aggregate`
+  * `# pint ignore promql/by` becomes `# pint ignore promql/aggregate`
+  * `# pint ignore promql/without` becomes `# pint ignore promql/aggregate`
+
 ## v0.5.3
 
 ### Fixed

cmd/pint/tests/0001_match_path.txt

@@ -6,7 +6,7 @@ cmp stderr stderr.txt
 level=info msg="Loading configuration file" path=.pint.hcl
 level=info msg="File parsed" path=rules/0001.yml rules=1
 level=info msg="File parsed" path=rules/0002.yml rules=1
-rules/0002.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0002.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(foo) without(job)
 
 level=info msg="Problems found" Bug=1

cmd/pint/tests/0003_lint_workdir.txt

@@ -7,61 +7,61 @@ level=info msg="Loading configuration file" path=.pint.hcl
 level=info msg="File parsed" path=rules/0001.yml rules=3
 level=info msg="File parsed" path=rules/0002.yaml rules=1
 level=info msg="File parsed" path=rules/0003.yaml rules=10
-rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(rate(fl_cf_html_bytes_in[10m])) WITHOUT (colo_id, instance, node_type, region, node_status, job, colo_name)
 
-rules/0001.yml:6: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0001.yml:6: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: sum(irate(foo[3m])) WITHOUT (colo_id)
 
-rules/0003.yaml:11: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0003.yaml:11: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: sum(foo) without(job)
 
-rules/0003.yaml:11: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:11: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(foo) without(job)
 
 rules/0003.yaml:14: syntax error: unexpected right parenthesis ')' (promql/syntax)
   expr: sum(foo) by ())
 
-rules/0003.yaml:22-25: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:22-25: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: |
     sum(
       multiline
     ) without(job, instance)
 
-rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:34-37: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:34-37: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: >-
     sum(
       multiline2
     ) without(job, instance)
 
-rules/0003.yaml:40: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/by)
+rules/0003.yaml:40: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/aggregate)
   expr: sum(byinstance) by(instance)
 
-rules/0003.yaml:40: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/by)
+rules/0003.yaml:40: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/aggregate)
   expr: sum(byinstance) by(instance)
 
 level=info msg="Problems found" Fatal=1 Warning=12

cmd/pint/tests/0008_recording_rule_prometheus.txt

@@ -5,10 +5,10 @@ cmp stderr stderr.txt
 -- stderr.txt --
 level=info msg="Loading configuration file" path=.pint.hcl
 level=info msg="File parsed" path=rules/0001.yml rules=1
-rules/0001.yml:5: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/by)
+rules/0001.yml:5: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/aggregate)
       expr: sum by (instance) (http_inprogress_requests)
 
-rules/0001.yml:5: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/by)
+rules/0001.yml:5: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/aggregate)
       expr: sum by (instance) (http_inprogress_requests)
 
 level=info msg="Problems found" Bug=1 Warning=1

cmd/pint/tests/0009_alerting_rule_prometheus.txt

@@ -10,10 +10,10 @@ rules/0001.yml:11-13: link annotation is required (alerts/annotation)
       summary: "Instance {{ $labels.instance }} down"
       description: "{{ $labels.instance }} of job {{ $labels.job }} has been down for more than 5 minutes."
 
-rules/0001.yml:17: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/by)
+rules/0001.yml:17: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/aggregate)
     expr: sum by (instance) (http_inprogress_requests) > 0
 
-rules/0001.yml:17: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/by)
+rules/0001.yml:17: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/aggregate)
     expr: sum by (instance) (http_inprogress_requests) > 0
 
 rules/0001.yml:19-21: link annotation is required (alerts/annotation)

cmd/pint/tests/0011_ignore_rules.txt

@@ -8,16 +8,16 @@ level=info msg="File parsed" path=rules/1.yaml rules=10
 rules/1.yaml:5: syntax error: unexpected right parenthesis ')' (promql/syntax)
   expr: sum(errors_total) by )
 
-rules/1.yaml:16: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/1.yaml:16: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(errors_total) without(job)
 
 rules/1.yaml:22: syntax error: unexpected right parenthesis ')' (promql/syntax)
   expr: sum(errors_total) by )
 
-rules/1.yaml:33: alert query doesn't have any condition, it will always fire if the metric exists (promql/comparison)
+rules/1.yaml:33: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(errors_total) without(job)
 
-rules/1.yaml:33: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/1.yaml:33: alert query doesn't have any condition, it will always fire if the metric exists (promql/comparison)
   expr: sum(errors_total) without(job)
 
 level=info msg="Problems found" Fatal=2 Warning=3
@@ -30,11 +30,11 @@ level=fatal msg="Fatal error" error="problems found"
   expr: sum(errors_total) by )
 
 - record: disabled
-  # pint disable promql/without(job:true)
+  # pint disable promql/aggregate(job:true)
   expr: sum(errors_total) without(job)
 
 - record: disabled
-  # pint disable promql/without
+  # pint disable promql/aggregate
   expr: sum(errors_total) without(job)
 
 - record: active
@@ -47,11 +47,11 @@ level=fatal msg="Fatal error" error="problems found"
   expr: sum(errors_total) by )
 
 - alert: disabled
-  # pint disable promql/without(job:true)
+  # pint disable promql/aggregate(job:true)
   expr: sum(errors_total) without(job) > 0
 
 - alert: disabled
-  # pint disable promql/without
+  # pint disable promql/aggregate
   expr: sum(errors_total) without(job) > 0
 
 - alert: active

cmd/pint/tests/0018_match_alerting.txt

@@ -8,11 +8,11 @@ level=info msg="File parsed" path=rules/0001.yml rules=2
 level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
 level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template"] path=rules/0001.yml rule=colo:recording
 level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
-level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/without(job:true)","promql/by(job:true)"] path=rules/0001.yml rule=colo:alerting
-rules/0001.yml:5: alert query doesn't have any condition, it will always fire if the metric exists (promql/comparison)
+level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:alerting
+rules/0001.yml:5: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(bar) without(job)
 
-rules/0001.yml:5: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0001.yml:5: alert query doesn't have any condition, it will always fire if the metric exists (promql/comparison)
   expr: sum(bar) without(job)
 
 -- rules/0001.yml --

cmd/pint/tests/0019_match_recording.txt

@@ -6,10 +6,10 @@ cmp stderr stderr.txt
 level=info msg="Loading configuration file" path=.pint.hcl
 level=info msg="File parsed" path=rules/0001.yml rules=2
 level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
-level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/without(job:true)","promql/by(job:true)"] path=rules/0001.yml rule=colo:recording
+level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
 level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
 level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template"] path=rules/0001.yml rule=colo:alerting
-rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(foo) without(job)
 
 -- rules/0001.yml --

cmd/pint/tests/0020_ignore_kind.txt

@@ -6,10 +6,10 @@ cmp stderr stderr.txt
 level=info msg="Loading configuration file" path=.pint.hcl
 level=info msg="File parsed" path=rules/0001.yml rules=2
 level=debug msg="Found recording rule" lines=1-2 path=rules/0001.yml record=colo:recording
-level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/without(job:true)","promql/by(job:true)"] path=rules/0001.yml rule=colo:recording
+level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template","promql/aggregate(job:true)"] path=rules/0001.yml rule=colo:recording
 level=debug msg="Found alerting rule" alert=colo:alerting lines=4-5 path=rules/0001.yml
 level=debug msg="Configured checks for rule" enabled=["promql/syntax","alerts/for","promql/comparison","alerts/template"] path=rules/0001.yml rule=colo:alerting
-rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(foo) without(job)
 
 rules/0001.yml:5: alert query doesn't have any condition, it will always fire if the metric exists (promql/comparison)

cmd/pint/tests/0022_ignore_multi.txt

@@ -5,10 +5,10 @@ cmp stderr stderr.txt
 -- stderr.txt --
 level=info msg="Loading configuration file" path=.pint.hcl
 level=info msg="File parsed" path=rules/1.yaml rules=3
-rules/1.yaml:2: dropped label should be removed when aggregating "^.+$" rules, remove dropped from by() (promql/by)
+rules/1.yaml:2: dropped label should be removed when aggregating "^.+$" rules, remove dropped from by() (promql/aggregate)
   expr: sum(errors_total) by(keep,dropped)
 
-rules/1.yaml:5: keep label is required and should be preserved when aggregating "^.+$" rules, remove keep from without() (promql/without)
+rules/1.yaml:5: keep label is required and should be preserved when aggregating "^.+$" rules, remove keep from without() (promql/aggregate)
   expr: sum(errors_total) without(keep,dropped)
 
 -- rules/1.yaml --
@@ -19,10 +19,8 @@ rules/1.yaml:5: keep label is required and should be preserved when aggregating
   expr: sum(errors_total) without(keep,dropped)
 
 - record: C
-  # pint disable promql/by(keep:true)
-  # pint disable promql/without(keep:true)
-  # pint disable promql/by(dropped:false)
-  # pint disable promql/without(dropped:false)
+  # pint disable promql/aggregate(keep:true)
+  # pint disable promql/aggregate(dropped:false)
   expr: sum(sum(errors_total) without(keep)) by(dropped)
 
 -- .pint.hcl --

cmd/pint/tests/0024_color_output.txt

@@ -7,61 +7,61 @@ level=info msg="Loading configuration file" path=.pint.hcl
 level=info msg="File parsed" path=rules/0001.yml rules=3
 level=info msg="File parsed" path=rules/0002.yaml rules=1
 level=info msg="File parsed" path=rules/0003.yaml rules=10
-rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0001.yml:2: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(rate(fl_cf_html_bytes_in[10m])) WITHOUT (colo_id, instance, node_type, region, node_status, job, colo_name)
 
-rules/0001.yml:6: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0001.yml:6: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: sum(irate(foo[3m])) WITHOUT (colo_id)
 
-rules/0003.yaml:11: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0003.yaml:11: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: sum(foo) without(job)
 
-rules/0003.yaml:11: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:11: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: sum(foo) without(job)
 
 rules/0003.yaml:14: syntax error: unexpected right parenthesis ')' (promql/syntax)
   expr: sum(foo) by ())
 
-rules/0003.yaml:22-25: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:22-25: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: |
     sum(
       multiline
     ) without(job, instance)
 
-rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/without)
+rules/0003.yaml:28-31: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, use without(instance, ...) (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:28-31: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: |
     sum(sum) without(job)
     +
     sum(sum) without(job)
 
-rules/0003.yaml:34-37: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/without)
+rules/0003.yaml:34-37: job label is required and should be preserved when aggregating "^.+$" rules, remove job from without() (promql/aggregate)
   expr: >-
     sum(
       multiline2
     ) without(job, instance)
 
-rules/0003.yaml:40: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/by)
+rules/0003.yaml:40: instance label should be removed when aggregating "^colo(?:_.+)?:.+$" rules, remove instance from by() (promql/aggregate)
   expr: sum(byinstance) by(instance)
 
-rules/0003.yaml:40: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/by)
+rules/0003.yaml:40: job label is required and should be preserved when aggregating "^.+$" rules, use by(job, ...) (promql/aggregate)
   expr: sum(byinstance) by(instance)
 
 level=info msg="Problems found" Fatal=1 Warning=12

cmd/pint/tests/0025_config.txt

@@ -18,20 +18,19 @@ level=info msg="Loading configuration file" path=.pint.hcl
   ],
   "checks": {
     "enabled": [
+      "alerts/annotation",
       "alerts/count",
+      "alerts/for",
+      "alerts/template",
+      "promql/aggregate",
       "promql/comparison",
-      "alerts/annotation",
-      "promql/by",
-      "query/cost",
-      "rule/label",
       "promql/rate",
-      "query/series",
       "promql/syntax",
-      "promql/without",
-      "rule/reject",
-      "alerts/template",
       "promql/vector_matching",
-      "alerts/for"
+      "query/cost",
+      "query/series",
+      "rule/label",
+      "rule/reject"
     ]
   },
   "rules": [