Improve rule/duplicate

docs/changelog.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## v0.66.0
+
+### Fixed
+
+- Improved accuracy of the [rule/duplicate](checks/rule/duplicate.md) check.
+
 ## v0.65.1
 
 ### Fixed

internal/checks/rule_duplicate.go

@@ -44,8 +44,9 @@ func (c RuleDuplicateCheck) Reporter() string {
 }
 
 func (c RuleDuplicateCheck) Check(ctx context.Context, path discovery.Path, rule parser.Rule, entries []discovery.Entry) (problems []Problem) {
-	if rule.RecordingRule == nil || rule.RecordingRule.Expr.SyntaxError != nil {
-		return nil
+	expr := rule.Expr()
+	if expr.SyntaxError != nil {
+		return problems
 	}
 
 	for _, entry := range entries {
@@ -55,6 +56,9 @@ func (c RuleDuplicateCheck) Check(ctx context.Context, path discovery.Path, rule
 		if entry.Rule.Error.Err != nil {
 			continue
 		}
+		if entry.Rule.Expr().SyntaxError != nil {
+			continue
+		}
 		if entry.Rule.RecordingRule == nil {
 			continue
 		}
@@ -67,23 +71,24 @@ func (c RuleDuplicateCheck) Check(ctx context.Context, path discovery.Path, rule
 		if !c.prom.IsEnabledForPath(entry.Path.Name) {
 			continue
 		}
-		if entry.Rule.RecordingRule.Record.Value != rule.RecordingRule.Record.Value {
-			continue
+
+		// Look for identical recording rules.
+		if entry.Rule.RecordingRule != nil && rule.RecordingRule != nil && entry.Rule.RecordingRule.Record.Value == rule.RecordingRule.Record.Value {
+			problems = append(problems, c.compareRules(ctx, rule.RecordingRule, entry, rule.Lines)...)
 		}
-		problems = append(problems, c.compareRules(ctx, rule.RecordingRule, entry, rule.Lines)...)
 	}
 	return problems
 }
 
 func (c RuleDuplicateCheck) compareRules(_ context.Context, rule *parser.RecordingRule, entry discovery.Entry, lines parser.LineRange) (problems []Problem) {
-	ruleALabels := buildRuleLabels(rule)
-	ruleBLabels := buildRuleLabels(entry.Rule.RecordingRule)
+	ruleALabels := buildRuleLabels(rule.Labels)
+	ruleBLabels := buildRuleLabels(entry.Rule.RecordingRule.Labels)
 
 	if ruleALabels.Hash() != ruleBLabels.Hash() {
 		return nil
 	}
 
-	if rule.Expr.Value.Value == entry.Rule.RecordingRule.Expr.Value.Value {
+	if rule.Expr.Query.Expr.String() == entry.Rule.RecordingRule.Expr.Query.Expr.String() {
 		problems = append(problems, Problem{
 			Lines:    lines,
 			Reporter: c.Reporter(),
@@ -95,14 +100,14 @@ func (c RuleDuplicateCheck) compareRules(_ context.Context, rule *parser.Recordi
 	return problems
 }
 
-func buildRuleLabels(rule *parser.RecordingRule) labels.Labels {
-	if rule.Labels == nil || len(rule.Labels.Items) == 0 {
+func buildRuleLabels(l *parser.YamlMap) labels.Labels {
+	if l == nil || len(l.Items) == 0 {
 		return labels.EmptyLabels()
 	}
 
-	pairs := make([]string, 0, len(rule.Labels.Items))
-	for _, l := range rule.Labels.Items {
-		pairs = append(pairs, l.Key.Value, l.Value.Value)
+	pairs := make([]string, 0, len(l.Items))
+	for _, label := range l.Items {
+		pairs = append(pairs, label.Key.Value, label.Value.Value)
 	}
 	return labels.FromStrings(pairs...)
 }

internal/checks/rule_duplicate_test.go

@@ -173,6 +173,35 @@ func TestRuleDuplicateCheck(t *testing.T) {
 			problems: noProblems,
 			entries:  xxxEntries,
 		},
+		{
+			description: "same expr but formatted differently",
+			content:     "- record: job:up:sum\n  expr: sum(up) by(job)\n",
+			checker: func(prom *promapi.FailoverGroup) checks.RuleChecker {
+				return checks.NewRuleDuplicateCheck(prom)
+			},
+			prometheus: newSimpleProm,
+			problems: func(_ string) []checks.Problem {
+				return []checks.Problem{
+					{
+						Lines: parser.LineRange{
+							First: 1,
+							Last:  2,
+						},
+						Reporter: checks.RuleDuplicateCheckName,
+						Text:     textDuplicateRule("fake.yml", 6),
+						Severity: checks.Bug,
+					},
+				}
+			},
+			entries: mustParseContent(`
+- record: job:up:sum
+  expr: sum by(job) (up)
+  labels:
+    cluster: prod
+- record: job:up:sum
+  expr: sum by(job) (up)
+`),
+		},
 	}
 
 	runTests(t, testCases)