mimirtool: fail on duplicate rules with --strict (#5035)

* mimirtool: fail on duplicate rules with --strict

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update CHANGELOG.md

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Check rules with mimirtool in CI

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Apply suggestions from code review

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

* Close on cleanup

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Remove duplicated testcase

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

---------

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

.github/workflows/test-build-deploy.yml

@@ -80,6 +80,8 @@ jobs:
         run: make BUILD_IN_CONTAINER=false check-mixin
       - name: Check Mixin Tests
         run: make BUILD_IN_CONTAINER=false check-mixin-tests
+      - name: Check Mixin with Mimirtool rules check
+        run: make BUILD_IN_CONTAINER=false check-mixin-mimirtool-rules
       - name: Check Jsonnet Manifests
         run: make BUILD_IN_CONTAINER=false check-jsonnet-manifests
       - name: Check Jsonnet Getting Started

CHANGELOG.md

@@ -82,6 +82,7 @@
 
 ### Mimirtool
 
+* [CHANGE] check rules: will fail on duplicate rules when `--strict` is provided. #5035
 * [ENHANCEMENT] analyze prometheus: allow to specify `-prometheus-http-prefix`. #4966
 * [ENHANCEMENT] analyze grafana: allow to specify `--folder-title` to limit dashboards analysis based on their exact folder title. #4973
 

Makefile

@@ -10,7 +10,7 @@ help:
 # WARNING: do not commit to a repository!
 -include Makefile.local
 
-.PHONY: all test test-with-race integration-tests cover clean images protos exes dist doc clean-doc check-doc push-multiarch-build-image license check-license format check-mixin check-mixin-jb check-mixin-mixtool check-mixin-runbooks build-mixin format-mixin check-jsonnet-manifests format-jsonnet-manifests push-multiarch-mimir list-image-targets check-jsonnet-getting-started mixin-screenshots
+.PHONY: all test test-with-race integration-tests cover clean images protos exes dist doc clean-doc check-doc push-multiarch-build-image license check-license format check-mixin check-mixin-jb check-mixin-mixtool check-mixin-runbooks check-mixin-mimirtool-rules build-mixin format-mixin check-jsonnet-manifests format-jsonnet-manifests push-multiarch-mimir list-image-targets check-jsonnet-getting-started mixin-screenshots
 .DEFAULT_GOAL := all
 
 # Version number
@@ -528,6 +528,12 @@ check-mixin-mixtool: check-mixin-jb
 check-mixin-runbooks: build-mixin
 	@tools/lint-runbooks.sh
 
+check-mixin-mimirtool-rules: build-mixin
+	@echo "Checking 'mimirtool rules check':"
+	@for suffix in $(MIXIN_OUT_PATH_SUFFIXES); do \
+		go run ./cmd/mimirtool rules check --rule-dirs "$(MIXIN_OUT_PATH)$$suffix"; \
+	done
+
 mixin-serve: ## Runs Grafana loading the mixin dashboards compiled at operations/mimir-mixin-compiled.
 	@./operations/mimir-mixin-tools/serve/run.sh
 

pkg/mimirtool/commands/rules.go

@@ -149,7 +149,7 @@ func (r *RuleCommand) Register(app *kingpin.Application, envVars EnvVarNames, re
 		Action(r.lint)
 	checkCmd := rulesCmd.
 		Command("check", "Run various best practice checks against rules.").
-		Action(r.checkRecordingRuleNames)
+		Action(r.checkRules)
 	deleteNamespaceCmd := rulesCmd.
 		Command("delete-namespace", "Delete a namespace from the ruler.").
 		Action(r.deleteNamespace)
@@ -722,7 +722,7 @@ func (r *RuleCommand) lint(k *kingpin.ParseContext) error {
 	return nil
 }
 
-func (r *RuleCommand) checkRecordingRuleNames(k *kingpin.ParseContext) error {
+func (r *RuleCommand) checkRules(_ *kingpin.ParseContext) error {
 	err := r.setupFiles()
 	if err != nil {
 		return errors.Wrap(err, "check operation unsuccessful, unable to load rules files")
@@ -733,21 +733,36 @@ func (r *RuleCommand) checkRecordingRuleNames(k *kingpin.ParseContext) error {
 		return errors.Wrap(err, "check operation unsuccessful, unable to parse rules files")
 	}
 
+	lvl := log.WarnLevel
+	if r.Strict {
+		lvl = log.ErrorLevel
+	}
+
 	for _, ruleNamespace := range namespaces {
 		n := ruleNamespace.CheckRecordingRules(r.Strict)
 		if n != 0 {
 			return fmt.Errorf("%d erroneous recording rule names", n)
 		}
 		duplicateRules := checkDuplicates(ruleNamespace.Groups)
 		if len(duplicateRules) != 0 {
-			fmt.Printf("%d duplicate rule(s) found.\n", len(duplicateRules))
+			log.WithFields(log.Fields{
+				"namespace":       ruleNamespace.Namespace,
+				"error":           "rules should emit unique series, to avoid producing inconsistencies while recording expressions",
+				"duplicate_rules": len(duplicateRules),
+			}).Logf(lvl, "duplicate rules found")
 			for _, n := range duplicateRules {
-				fmt.Printf("Metric: %s\nLabel(s):\n", n.metric)
+				fields := log.Fields{
+					"namespace": ruleNamespace.Namespace,
+					"metric":    n.metric,
+				}
 				for i, l := range n.label {
-					fmt.Printf("\t%s: %s\n", i, l)
+					fields["label["+i+"]"] = l
 				}
+				log.WithFields(fields).Logf(lvl, "duplicate rule")
+			}
+			if r.Strict {
+				return fmt.Errorf("%d duplicate rules found in namespace %q", len(duplicateRules), ruleNamespace.Namespace)
 			}
-			fmt.Println("Might cause inconsistency while recording expressions.")
 		}
 	}
 

pkg/mimirtool/commands/rules_test.go

@@ -8,6 +8,8 @@ package commands
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/prometheus/prometheus/model/rulefmt"
@@ -115,6 +117,101 @@ func TestCheckDuplicates(t *testing.T) {
 	}
 }
 
+func TestRuleCommand_checkRules(t *testing.T) {
+	completelyBadRuleName := rulefmt.RuleNode{
+		Record: yaml.Node{Value: "up", Kind: yaml.ScalarNode},
+		Expr:   yaml.Node{Value: "up==1", Kind: yaml.ScalarNode},
+	}
+	strictlyBadRuleName := rulefmt.RuleNode{
+		Record: yaml.Node{Value: "up:onecolonmissing", Kind: yaml.ScalarNode},
+		Expr:   yaml.Node{Value: "up==1", Kind: yaml.ScalarNode},
+	}
+	validRule1 := rulefmt.RuleNode{
+		Record: yaml.Node{Value: "rule:up:nothing", Kind: yaml.ScalarNode},
+		Expr:   yaml.Node{Value: "up==1", Kind: yaml.ScalarNode},
+	}
+	validRule2 := rulefmt.RuleNode{
+		Record: yaml.Node{Value: "rule:down:nothing", Kind: yaml.ScalarNode},
+		Expr:   yaml.Node{Value: "up==0", Kind: yaml.ScalarNode},
+	}
+	for _, tc := range []struct {
+		name       string
+		rules      []rulefmt.RuleNode
+		strict     bool
+		shouldFail bool
+	}{
+		{
+			name:       "completely bad rule name, not strict fails too",
+			rules:      []rulefmt.RuleNode{validRule1, completelyBadRuleName, validRule2},
+			strict:     false,
+			shouldFail: true,
+		},
+		{
+			name:       "strictly bad rule name, strict",
+			rules:      []rulefmt.RuleNode{validRule1, strictlyBadRuleName, validRule2},
+			strict:     true,
+			shouldFail: true,
+		},
+		{
+			name:       "strictly bad rule name, not strict",
+			rules:      []rulefmt.RuleNode{validRule1, strictlyBadRuleName, validRule2},
+			strict:     false,
+			shouldFail: false,
+		},
+		{
+			name:       "no duplicates, strict",
+			rules:      []rulefmt.RuleNode{validRule1, validRule2},
+			strict:     true,
+			shouldFail: false,
+		},
+		{
+			name:       "with duplicates, not strict",
+			rules:      []rulefmt.RuleNode{validRule1, validRule1},
+			strict:     false,
+			shouldFail: false,
+		},
+		{
+			name:       "with duplicates, strict",
+			rules:      []rulefmt.RuleNode{validRule1, validRule1},
+			strict:     true,
+			shouldFail: true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			file := filepath.Join(t.TempDir(), "rules.yaml")
+			{
+				// Setup.
+				f, err := os.Create(file)
+				require.NoError(t, err)
+				t.Cleanup(func() { _ = f.Close() })
+
+				contents := rules.RuleNamespace{
+					Namespace: "test",
+					Groups: []rwrulefmt.RuleGroup{{
+						RuleGroup: rulefmt.RuleGroup{
+							Name:  "rulegroup",
+							Rules: tc.rules,
+						},
+					}},
+				}
+				require.NoError(t, yaml.NewEncoder(f).Encode(contents))
+				require.NoError(t, f.Close())
+			}
+
+			{
+				// Test.
+				cmd := &RuleCommand{Strict: tc.strict, RuleFilesList: []string{file}, Backend: rules.MimirBackend}
+				err := cmd.checkRules(nil)
+				if tc.shouldFail {
+					require.Error(t, err)
+				} else {
+					require.NoError(t, err)
+				}
+			}
+		})
+	}
+}
+
 type ruleCommandClientMock struct {
 	mock.Mock
 }