Merge pull request #1171 from cloudflare/rules

Allow disabling/enabling checks via rules
cloudflare · Oct 26, 2024 · e6ef73b · e6ef73b
2 parents 4fc90c8 + 4a23865
commit e6ef73b
Show file tree

Hide file tree

Showing 7 changed files with 448 additions and 23 deletions.
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -11,6 +11,27 @@
   to disable reports on smelly selector - [#1096](https://github.com/cloudflare/pint/issues/1096).
 - Added `--json` flag to both `pint lint` and `pint ci` commands, this enables writing a JSON file
   with the report of all problems [#606](https://github.com/cloudflare/pint/issues/606).
+- Checks can be enabled or disabled specifically for some Prometheus rules via `rule {}` config blocks.
+  Adding `enable` or `disable` option with a list of checks names allows to selectively enable or disable
+  checks only for Prometheus rules that match given `rule {}` definition.
+  Enabling checks only for matching rules will only work if these checks are disabled globally via
+  `check { disabled = [] }` config block.
+  For example to disable `promql/rate` check for all rules except alerting rules in the `rules/critical` folder:
+
+  ```js
+  checks {
+    # This will disable promql/rate by default.
+    disabled = [ "promql/rate" ]
+  }
+  rule {
+    match {
+      path = "rules/critical/.*"
+      kind = "alerting"
+    }
+    # This will enable promql/rate only for Prometheus rules matching all our match conditions above.
+    enable = [ "promql/rate" ]
+  }
+  ```
 
 ### Fixed
 

diff --git a/docs/configuration.md b/docs/configuration.md
@@ -551,6 +551,9 @@ rule {
   ignore { ... }
   ignore { ... }
 
+  enable  = [ "..." ]
+  disable = [ "..." ]
+
   [ check definition ]
   ...
   [ check definition ]
@@ -586,6 +589,11 @@ rule {
   way as `for` match filter.
 - `ignore` - works exactly like `match` but does the opposite - any alerting or recording rule
   matching all conditions defined on `ignore` will not be checked by this `rule` block.
+- `enable` - list of check names to enable for any Prometheus rule matching this block.
+  Enabling checks here will overwrite `check { disable = [...] }` settings, but won't
+  enable checks disable specifically for some Prometheus rule via `# pint disable ...` comments.
+- `disable` - list of check names to disable for any Prometheus rule matching this block.
+  This takes precedence over `enable` option above.
 
 Note: both `match` and `ignore` require all defined filters to be satisfied to work.
 If multiple `match` and/or `ignore` rules are present any of them needs to match for the rule to
@@ -657,3 +665,20 @@ rule {
   check { ... }
 }
 ```
+
+Disable `promql/rate` check for all rules except alerting rules in the `rules/critical` folder:
+
+```js
+checks {
+  # This will disable promql/rate by default.
+  disabled = [ "promql/rate" ]
+}
+rule {
+  match {
+    path = "rules/critical/.*"
+    kind = "alerting"
+  }
+  # This will enable promql/rate only for Prometheus rules matching all our match conditions above.
+  enable = [ "promql/rate" ]
+}
+```
diff --git a/internal/config/__snapshots__/config_test.snap b/internal/config/__snapshots__/config_test.snap
@@ -2969,3 +2969,216 @@
   ]
 }
 ---
+
+[TestGetChecksForRule/check_disabled_globally_but_enabled_via_rule{} - 1]
+{
+  "ci": {
+    "baseBranch": "master",
+    "maxCommits": 20
+  },
+  "parser": {},
+  "checks": {
+    "enabled": [
+      "alerts/absent",
+      "alerts/annotation",
+      "alerts/count",
+      "alerts/external_labels",
+      "alerts/for",
+      "alerts/template",
+      "labels/conflict",
+      "promql/aggregate",
+      "alerts/comparison",
+      "promql/fragile",
+      "promql/range_query",
+      "promql/rate",
+      "promql/regexp",
+      "promql/syntax",
+      "promql/vector_matching",
+      "query/cost",
+      "promql/counter",
+      "promql/series",
+      "rule/dependency",
+      "rule/duplicate",
+      "rule/for",
+      "rule/name",
+      "rule/label",
+      "rule/link",
+      "rule/reject"
+    ],
+    "disabled": [
+      "alerts/template",
+      "alerts/external_labels",
+      "rule/duplicate",
+      "alerts/absent",
+      "promql/series",
+      "promql/vector_matching"
+    ]
+  },
+  "owners": {},
+  "prometheus": [
+    {
+      "name": "prom",
+      "uri": "http://localhost",
+      "timeout": "1s",
+      "uptime": "up",
+      "concurrency": 16,
+      "rateLimit": 100,
+      "required": false
+    }
+  ],
+  "rules": [
+    {
+      "disable": [
+        "rule/duplicate"
+      ]
+    },
+    {
+      "match": [
+        {
+          "kind": "alerting"
+        }
+      ],
+      "disable": [
+        "promql/series"
+      ]
+    },
+    {
+      "enable": [
+        "promql/series"
+      ]
+    }
+  ]
+}
+---
+
+[TestGetChecksForRule/check_enabled_globally_but_disabled_via_rule{} - 1]
+{
+  "ci": {
+    "baseBranch": "master",
+    "maxCommits": 20
+  },
+  "parser": {},
+  "checks": {
+    "enabled": [
+      "alerts/absent",
+      "alerts/annotation",
+      "alerts/count",
+      "alerts/external_labels",
+      "alerts/for",
+      "alerts/template",
+      "labels/conflict",
+      "promql/aggregate",
+      "alerts/comparison",
+      "promql/fragile",
+      "promql/range_query",
+      "promql/rate",
+      "promql/regexp",
+      "promql/syntax",
+      "promql/vector_matching",
+      "query/cost",
+      "promql/counter",
+      "promql/series",
+      "rule/dependency",
+      "rule/duplicate",
+      "rule/for",
+      "rule/name",
+      "rule/label",
+      "rule/link",
+      "rule/reject"
+    ]
+  },
+  "owners": {},
+  "rules": [
+    {
+      "match": [
+        {
+          "kind": "recording"
+        }
+      ],
+      "disable": [
+        "rule/duplicate"
+      ]
+    }
+  ]
+}
+---
+
+[TestGetChecksForRule/two_prometheus_servers_/_check_disable_via_rule_{} - 1]
+{
+  "ci": {
+    "baseBranch": "master",
+    "maxCommits": 20
+  },
+  "parser": {},
+  "checks": {
+    "enabled": [
+      "alerts/absent",
+      "alerts/annotation",
+      "alerts/count",
+      "alerts/external_labels",
+      "alerts/for",
+      "alerts/template",
+      "labels/conflict",
+      "promql/aggregate",
+      "alerts/comparison",
+      "promql/fragile",
+      "promql/range_query",
+      "promql/rate",
+      "promql/regexp",
+      "promql/syntax",
+      "promql/vector_matching",
+      "query/cost",
+      "promql/counter",
+      "promql/series",
+      "rule/dependency",
+      "rule/duplicate",
+      "rule/for",
+      "rule/name",
+      "rule/label",
+      "rule/link",
+      "rule/reject"
+    ],
+    "disabled": [
+      "alerts/template",
+      "promql/regexp"
+    ]
+  },
+  "owners": {},
+  "prometheus": [
+    {
+      "name": "prom1",
+      "uri": "http://localhost/1",
+      "timeout": "1s",
+      "uptime": "up",
+      "concurrency": 16,
+      "rateLimit": 100,
+      "required": false
+    },
+    {
+      "name": "prom2",
+      "uri": "http://localhost/2",
+      "timeout": "1s",
+      "uptime": "up",
+      "concurrency": 16,
+      "rateLimit": 100,
+      "required": false
+    }
+  ],
+  "rules": [
+    {
+      "match": [
+        {
+          "path": "rules.yml"
+        }
+      ],
+      "disable": [
+        "promql/series",
+        "promql/range_query",
+        "rule/duplicate",
+        "promql/vector_matching",
+        "promql/counter"
+      ]
+    }
+  ]
+}
+---
diff --git a/internal/config/config.go b/internal/config/config.go
@@ -86,21 +86,26 @@ func (cfg *Config) GetChecksForEntry(ctx context.Context, gen *PrometheusGenerat
 	defaultMatch := []Match{{State: defaultStates}}
 	proms := gen.ServersForPath(entry.Path.Name)
 
+	parsedRules := make([]parsedRule, 0, len(cfg.Rules))
 	if entry.PathError != nil || entry.Rule.Error.Err != nil {
 		check := checks.NewErrorCheck(entry)
-		enabled = parsedRule{
+		parsedRules = append(parsedRules, parsedRule{
 			match: defaultMatch,
 			name:  check.Reporter(),
 			check: check,
-		}.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry)
+		})
 	} else {
-		for _, pr := range baseRules(proms, defaultMatch) {
-			enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry)
-		}
+		parsedRules = append(parsedRules, baseRules(proms, defaultMatch)...)
 		for _, rule := range cfg.Rules {
-			for _, pr := range parseRule(rule, proms, defaultStates) {
-				enabled = pr.entryChecks(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry)
-			}
+			parsedRules = append(parsedRules, parseRule(rule, proms, defaultStates)...)
+		}
+	}
+	for _, pr := range parsedRules {
+		if !isMatch(ctx, entry, pr.ignore, pr.match) {
+			continue
+		}
+		if pr.isEnabled(ctx, cfg.Checks.Enabled, cfg.Checks.Disabled, enabled, entry, cfg.Rules) {
+			enabled = append(enabled, pr.check)
 		}
 	}