WIP: Add logic and command to lint recording rules (#46)

* Add logic and command to lint recording rules based on naming best
practices.

Signed-off-by: Callum Styan <[email protected]>

* Rename recording rules names check, since we're not linting based on the
definition of linting for the existing function. Also adds strict bool
to the check.

Signed-off-by: Callum Styan <[email protected]>

* Don't try to evaluate recording rule names if the name is empty (because
the rule is actually an alerting rule).

Signed-off-by: Callum Styan <[email protected]>

Author: cstyan

pkg/commands/rules.go

@@ -63,6 +63,9 @@ type RuleCommand struct {
 	// Lint Rules Config
 	LintDryRun bool
 
+	// Rules check flags
+	Strict bool
+
 	DisableColor bool
 }
 
@@ -99,6 +102,9 @@ func (r *RuleCommand) Register(app *kingpin.Application) {
 	lintCmd := rulesCmd.
 		Command("lint", "formats a set of rule files. It reorders keys alphabetically, uses 4 spaces as indentantion, and formats PromQL expressions to a single line.").
 		Action(r.lint)
+	checkCmd := rulesCmd.
+		Command("check", "runs various best practice checks against rules.").
+		Action(r.checkRecordingRuleNames)
 
 	// Require Cortex cluster address and tentant ID on all these commands
 	for _, c := range []*kingpin.CmdClause{listCmd, printRulesCmd, getRuleGroupCmd, deleteRuleGroupCmd, loadRulesCmd, diffRulesCmd, syncRulesCmd} {
@@ -166,6 +172,14 @@ func (r *RuleCommand) Register(app *kingpin.Application) {
 		"Comma separated list of paths to directories containing rules yaml files. Each file in a directory with a .yml or .yaml suffix will be parsed.",
 	).StringVar(&r.RuleFilesPath)
 	lintCmd.Flag("dry-run", "Performs a trial run that doesn't make any changes and (mostly) produces the same outpupt as a real run.").Short('n').BoolVar(&r.LintDryRun)
+
+	checkCmd.Arg("rule-files", "The rule files to check.").Required().ExistingFilesVar(&r.RuleFilesList)
+	checkCmd.Flag("rule-files", "The rule files to check. Flag can be reused to load multiple files.").StringVar(&r.RuleFiles)
+	checkCmd.Flag(
+		"rule-dirs",
+		"Comma separated list of paths to directories containing rules yaml files. Each file in a directory with a .yml or .yaml suffix will be parsed.",
+	).StringVar(&r.RuleFilesPath)
+	checkCmd.Flag("strict", "fails rules checks that do not match best practices exactly").BoolVar(&r.Strict)
 }
 
 func (r *RuleCommand) setup(k *kingpin.ParseContext) error {
@@ -556,6 +570,27 @@ func (r *RuleCommand) lint(k *kingpin.ParseContext) error {
 	return nil
 }
 
+func (r *RuleCommand) checkRecordingRuleNames(k *kingpin.ParseContext) error {
+	err := r.setupFiles()
+	if err != nil {
+		return errors.Wrap(err, "check operation unsuccessful, unable to load rules files")
+	}
+
+	namespaces, err := rules.ParseFiles(r.RuleFilesList)
+	if err != nil {
+		return errors.Wrap(err, "check operation unsuccessful, unable to parse rules files")
+	}
+
+	for _, ruleNamespace := range namespaces {
+		n := ruleNamespace.CheckRecordingRules(r.Strict)
+		if n != 0 {
+			return fmt.Errorf("%d erroneous recording rule names", n)
+		}
+	}
+
+	return nil
+}
+
 // save saves a set of rule files to to disk. You can specify whenever you want the
 // file(s) to be edited in-place.
 func save(nss map[string]rules.RuleNamespace, i bool) error {

pkg/rules/rules.go

@@ -21,7 +21,7 @@ type RuleNamespace struct {
 }
 
 // LintPromQLExpressions runs the `expr` from a rule through the PromQL parser and
-// compares its output. if it differs from the parser, it uses the parser's instead.
+// compares its output. If it differs from the parser, it uses the parser's instead.
 func (r RuleNamespace) LintPromQLExpressions() (int, int, error) {
 	// `count` represents the number of rules we evalated.
 	// `mod` represents the number of rules linted.
@@ -51,6 +51,39 @@ func (r RuleNamespace) LintPromQLExpressions() (int, int, error) {
 	return count, mod, nil
 }
 
+// CheckRecordingRules checks that recording rules have at least one colon in their name, this is based
+// on the recording rules best practices here: https://prometheus.io/docs/practices/rules/
+// Returns the number of rules that don't match the requirements, and error if that number is not 0.
+func (r RuleNamespace) CheckRecordingRules(strict bool) int {
+	var name string
+	var count int
+	reqChunks := 2
+	if strict {
+		reqChunks = 3
+	}
+	for _, group := range r.Groups {
+		for _, rule := range group.Rules {
+			// Assume if there is a rule.Record that this is a recording rule.
+			if rule.Record == "" {
+				continue
+			}
+			name = rule.Record
+			log.WithFields(log.Fields{"rule": name}).Debugf("linting recording rule name")
+			chunks := strings.Split(name, ":")
+			if len(chunks) < reqChunks {
+				count++
+				log.WithFields(log.Fields{
+					"rule":      getRuleName(rule),
+					"ruleGroup": group.Name,
+					"file":      r.Filepath,
+					"error":     "recording rule name does not match level:metric:operation format, must contain at least one colon",
+				}).Errorf("bad recording rule name")
+			}
+		}
+	}
+	return count
+}
+
 // AggregateBy modifies the aggregation rules in groups to include a given Label.
 func (r RuleNamespace) AggregateBy(label string) (int, int, error) {
 	// `count` represents the number of rules we evalated.

pkg/rules/rules_test.go

@@ -155,3 +155,50 @@ func TestLintPromQLExpressions(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckRecordingRules(t *testing.T) {
+	tt := []struct {
+		name     string
+		ruleName string
+		count    int
+		strict   bool
+	}{
+		{
+			name:     "follows rule name conventions",
+			ruleName: "level:metric:operation",
+			count:    0,
+		},
+		{
+			name:     "doesn't follow rule name conventions",
+			ruleName: "level_metric_operation",
+			count:    1,
+		},
+		{
+			name:     "almost follows rule name conventions",
+			ruleName: "level:metric_operation",
+			count:    1,
+			strict:   true,
+		},
+		{
+			name:     "almost follows rule name conventions",
+			ruleName: "level:metric_operation",
+			count:    0,
+		},
+		{
+			name:     "follows rule name conventions extra",
+			ruleName: "level:metric:something_else:operation",
+			count:    0,
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			r := RuleNamespace{Groups: []rulefmt.RuleGroup{{Rules: []rulefmt.Rule{
+				{Record: tc.ruleName, Expr: "rate(some_metric_total)[5m]"},
+			}}}}
+
+			n := r.CheckRecordingRules(tc.strict)
+			require.Equal(t, tc.count, n, "failed rule: %s", tc.ruleName)
+		})
+	}
+}