Merge pull request #801 from cloudflare/deps

Add rule/dependency check

Makefile

@@ -33,6 +33,10 @@ format: $(GOBIN)/gofumpt $(GOBIN)/goimports
 	$(GOBIN)/gofumpt -extra -l -w .
 	$(GOBIN)/goimports -local github.com/cloudflare/pint -w .
 
+tidy:
+	go mod tidy
+	@for f in $(wildcard tools/*/go.mod) ; do echo ">>> $$f" && cd $(CURDIR)/`dirname "$$f"` && go mod tidy && cd $(CURDIR) ; done
+
 
 .PHONY: test
 test:

cmd/pint/scan.go

@@ -102,6 +102,12 @@ func checkRules(ctx context.Context, workers int, gen *config.PrometheusGenerato
 	go func() {
 		for _, entry := range entries {
 			switch {
+			case entry.State == discovery.Excluded:
+				continue
+			case entry.PathError != nil && entry.State == discovery.Removed:
+				continue
+			case entry.Rule.Error.Err != nil && entry.State == discovery.Removed:
+				continue
 			case entry.PathError == nil && entry.Rule.Error.Err == nil:
 				if entry.Rule.RecordingRule != nil {
 					rulesParsedTotal.WithLabelValues(config.RecordingRuleType).Inc()
@@ -120,7 +126,7 @@ func checkRules(ctx context.Context, workers int, gen *config.PrometheusGenerato
 					)
 				}
 
-				checkList := cfg.GetChecksForRule(ctx, gen, entry.SourcePath, entry.Rule, entry.DisabledChecks)
+				checkList := cfg.GetChecksForRule(ctx, gen, entry, entry.DisabledChecks)
 				for _, check := range checkList {
 					checkIterationChecks.Inc()
 					check := check
@@ -174,8 +180,6 @@ func scanWorker(ctx context.Context, jobs <-chan scanJob, results chan<- reporte
 			return
 		default:
 			switch {
-			case job.entry.State == discovery.Removed:
-				// FIXME check if it breaks any other rule?
 			case errors.Is(job.entry.PathError, discovery.ErrFileIsIgnored):
 				results <- reporter.Report{
 					ReportedPath:  job.entry.ReportedPath,
@@ -225,6 +229,15 @@ This usually means that it's missing some required fields.`,
 					Owner: job.entry.Owner,
 				}
 			default:
+				if job.entry.State == discovery.Unknown {
+					slog.Warn(
+						"Bug: unknown rule state",
+						slog.String("path", job.entry.ReportedPath),
+						slog.Int("line", job.entry.Rule.Lines()[0]),
+						slog.String("name", job.entry.Rule.Name()),
+					)
+				}
+
 				start := time.Now()
 				problems := job.check.Check(ctx, job.entry.ReportedPath, job.entry.Rule, job.allEntries)
 				checkDuration.WithLabelValues(job.check.Reporter()).Observe(time.Since(start).Seconds())

cmd/pint/tests/0025_config.txt

@@ -112,6 +112,7 @@ level=INFO msg="Loading configuration file" path=.pint.hcl
       "promql/vector_matching",
       "query/cost",
       "promql/series",
+      "rule/dependency",
       "rule/duplicate",
       "rule/for",
       "rule/label",

cmd/pint/tests/0113_config_env_expand.txt

@@ -49,6 +49,7 @@ level=INFO msg="Loading configuration file" path=.pint.hcl
       "promql/vector_matching",
       "query/cost",
       "promql/series",
+      "rule/dependency",
       "rule/duplicate",
       "rule/for",
       "rule/label",

cmd/pint/tests/0162_ci_deleted_dependency.txt

@@ -0,0 +1,56 @@
+mkdir testrepo
+cd testrepo
+exec git init --initial-branch=main .
+
+cp ../src/v1.yml rules.yml
+cp ../src/.pint.hcl .
+env GIT_AUTHOR_NAME=pint
+env GIT_AUTHOR_EMAIL=pint@example.com
+env GIT_COMMITTER_NAME=pint
+env GIT_COMMITTER_EMAIL=pint@example.com
+exec git add .
+exec git commit -am 'import rules and config'
+
+exec git checkout -b v2
+cp ../src/v2.yml rules.yml
+exec git commit -am 'v2'
+
+pint.ok -l error --offline --no-color ci
+! stdout .
+cmp stderr ../stderr.txt
+
+-- stderr.txt --
+rules.yml:5 Warning: This rule uses a metric produced by recording rule `up:sum` which was removed from rules.yml. (rule/dependency)
+ 5 |     expr: 'up:sum == 0'
+
+-- src/v1.yml --
+groups:
+- name: g1
+  rules:
+  - alert: Alert
+    expr: 'up:sum == 0'
+    annotations:
+      summary: 'Service is down'
+    labels:
+      cluster: dev
+  - record: up:sum
+    expr: sum(up)
+-- src/v2.yml --
+groups:
+- name: g1
+  rules:
+  - alert: Alert
+    expr: 'up:sum == 0'
+    annotations:
+      summary: 'Service is down'
+    labels:
+      cluster: dev
+-- src/.pint.hcl --
+ci {
+  baseBranch = "main"
+}
+prometheus "prom" {
+  uri      = "http://127.0.0.1:7162"
+  timeout  = "5s"
+  required = true
+}

docs/changelog.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## v0.51.0
+
+### Added
+
+- Added [rule/dependency](checks/rule/dependency.md) check.
+
 ## v0.50.1
 
 ### Fixed

docs/checks/rule/dependency.md

@@ -0,0 +1,124 @@
+---
+layout: default
+parent: Checks
+grand_parent: Documentation
+---
+
+# rule/dependency
+
+This check only works when running `pint ci` and will validate that any
+removed recording rule isn't still being used by other rules.
+
+Removing any recording rule that is a dependency of other rules is likely
+to make them stop working, unless there's some other source of the metric
+that was produced by the removed rule.
+
+Example: consider this two rules, one generates `down:count` metric
+that is then used by the alert rule:
+
+```yaml
+groups:
+- name: ...
+  rules:
+  - record: down:count
+    expr: count(up == 0) by(job)
+  - alert: Job is down
+    expr: down:count > 0
+```
+
+If we were to edit this file and delete the recording rule:
+
+```yaml
+groups:
+- name: ...
+  rules:
+  - alert: Job is down
+    expr: down:count > 0
+```
+
+This would leave our alert rule broken because Prometheus would
+no longer have `down:count` metric.
+
+This check tries to detect scenarios like this but works across all
+files.
+
+## Configuration
+
+This check doesn't have any configuration options.
+
+## How to enable it
+
+This check is enabled by default for all configured Prometheus servers.
+
+Example:
+
+```js
+prometheus "prod" {
+  uri     = "https://prometheus-prod.example.com"
+  timeout = "60s"
+  include = [
+    "rules/prod/.*",
+    "rules/common/.*",
+  ]
+}
+
+prometheus "dev" {
+  uri     = "https://prometheus-dev.example.com"
+  timeout = "30s"
+  include = [
+    "rules/dev/.*",
+    "rules/common/.*",
+  ]
+}
+```
+
+## How to disable it
+
+You can disable this check globally by adding this config block:
+
+```js
+checks {
+  disabled = ["rule/dependency"]
+}
+```
+
+You can also disable it for all rules inside given file by adding
+a comment anywhere in that file. Example:
+
+```yaml
+# pint file/disable rule/dependency
+```
+
+Or you can disable it per rule by adding a comment to it. Example:
+
+```yaml
+# pint disable rule/dependency
+```
+
+If you want to disable only individual instances of this check
+you can add a more specific comment.
+
+```yaml
+# pint disable rule/dependency($prometheus)
+```
+
+Where `$prometheus` is the name of Prometheus server to disable.
+
+Example:
+
+```yaml
+# pint disable rule/dependency(prod)
+```
+
+## How to snooze it
+
+You can disable this check until given time by adding a comment to it. Example:
+
+```yaml
+# pint snooze $TIMESTAMP rule/dependency
+```
+
+Where `$TIMESTAMP` is either use [RFC3339](https://www.rfc-editor.org/rfc/rfc3339)
+formatted  or `YYYY-MM-DD`.
+Adding this comment will disable `rule/dependency` *until* `$TIMESTAMP`, after that
+check will be re-enabled.

go.mod

@@ -1,6 +1,6 @@
 module github.com/cloudflare/pint
 
-go 1.21.3
+go 1.21.4
 
 require (
 	github.com/cespare/xxhash/v2 v2.2.0

internal/checks/alerts_annotation.go

@@ -24,7 +24,15 @@ type AnnotationCheck struct {
 }
 
 func (c AnnotationCheck) Meta() CheckMeta {
-	return CheckMeta{IsOnline: false}
+	return CheckMeta{
+		States: []discovery.ChangeType{
+			discovery.Noop,
+			discovery.Added,
+			discovery.Modified,
+			discovery.Moved,
+		},
+		IsOnline: false,
+	}
 }
 
 func (c AnnotationCheck) String() string {

internal/checks/alerts_comparison.go

@@ -26,7 +26,15 @@ func NewComparisonCheck() ComparisonCheck {
 type ComparisonCheck struct{}
 
 func (c ComparisonCheck) Meta() CheckMeta {
-	return CheckMeta{IsOnline: false}
+	return CheckMeta{
+		States: []discovery.ChangeType{
+			discovery.Noop,
+			discovery.Added,
+			discovery.Modified,
+			discovery.Moved,
+		},
+		IsOnline: false,
+	}
 }
 
 func (c ComparisonCheck) String() string {

internal/checks/alerts_count.go

@@ -41,7 +41,15 @@ type AlertsCheck struct {
 }
 
 func (c AlertsCheck) Meta() CheckMeta {
-	return CheckMeta{IsOnline: true}
+	return CheckMeta{
+		States: []discovery.ChangeType{
+			discovery.Noop,
+			discovery.Added,
+			discovery.Modified,
+			discovery.Moved,
+		},
+		IsOnline: true,
+	}
 }
 
 func (c AlertsCheck) String() string {

internal/checks/alerts_external_labels.go

@@ -24,7 +24,15 @@ type AlertsExternalLabelsCheck struct {
 }
 
 func (c AlertsExternalLabelsCheck) Meta() CheckMeta {
-	return CheckMeta{IsOnline: true}
+	return CheckMeta{
+		States: []discovery.ChangeType{
+			discovery.Noop,
+			discovery.Added,
+			discovery.Modified,
+			discovery.Moved,
+		},
+		IsOnline: true,
+	}
 }
 
 func (c AlertsExternalLabelsCheck) String() string {

internal/checks/alerts_for.go

@@ -22,7 +22,15 @@ func NewAlertsForCheck() AlertsForChecksFor {
 type AlertsForChecksFor struct{}
 
 func (c AlertsForChecksFor) Meta() CheckMeta {
-	return CheckMeta{IsOnline: false}
+	return CheckMeta{
+		States: []discovery.ChangeType{
+			discovery.Noop,
+			discovery.Added,
+			discovery.Modified,
+			discovery.Moved,
+		},
+		IsOnline: false,
+	}
 }
 
 func (c AlertsForChecksFor) String() string {

internal/checks/alerts_template.go

@@ -92,7 +92,15 @@ func NewTemplateCheck() TemplateCheck {
 type TemplateCheck struct{}
 
 func (c TemplateCheck) Meta() CheckMeta {
-	return CheckMeta{IsOnline: false}
+	return CheckMeta{
+		States: []discovery.ChangeType{
+			discovery.Noop,
+			discovery.Added,
+			discovery.Modified,
+			discovery.Moved,
+		},
+		IsOnline: false,
+	}
 }
 
 func (c TemplateCheck) String() string {