pkg/rules: consider group name and file for deduplication (#3024)

Currently only the group name is considered for group deduplication.
However prometheus uses also the group file according to [1].

This fixes it.

[1] https://github.com/prometheus/prometheus/blob/ce838ad6fcbd14b0cf9971a4d093ad672e1469fe/rules/manager.go#L1047-L1055

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

CHANGELOG.md

@@ -28,6 +28,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 - [#2957](https://github.com/thanos-io/thanos/pull/2957) Rule: now sets all of the relevant fields properly; avoids a panic when `/api/v1/rules` is called and the time zone is _not_ UTC; `rules` field is an empty array now if no rules have been defined in a rule group.
 - [#2976](https://github.com/thanos-io/thanos/pull/2976) Query: Better rounding for incoming query timestamps.
 - [#2929](https://github.com/thanos-io/thanos/pull/2929) Mixin: Fix expression for 'unhealthy sidecar' alert and also increase the timeout for 10 minutes.
+- [#3024](https://github.com/thanos-io/thanos/pull/3024) Query: consider group name and file for deduplication
 
 ### Added
 

docs/proposals/202003_thanos_rules_federation.md

@@ -61,7 +61,7 @@ Thanos Querier fans-out to all know Rules endpoints configured via `--rule` comm
 
 Generally the deduplication logic is less complex than with time series, specifically:
 
-* Deduplication happens first at the rule group level. The identifier is the group name.
+* Deduplication happens first at the rule group level. The identifier is the group name and the group file.
 * Then, per group name deduplication happens on the rule level, where:
 
 1. the rule type (recording rule vs. alerting rule)
@@ -171,6 +171,26 @@ Given the following stream of incoming alerting rules will also result in two in
       severity: critical
 ```
 
+Scenario 4:
+
+As specified, the group name and file fields are used for deduplication.
+
+Given the following stream of incoming rule groups:
+```text
+group: a/file1
+group: b/file1
+group: a/file2
+```
+
+The output becomes:
+```text
+group: a/file1
+group: a/file2
+group: b/file1
+```
+
+Deduplication of included alerting/recording rules inside groups is described in the previous scenarios.
+
 ## Alternatives
 
 * Cortex contains a sharded Ruler. Assigning rules to shards is done via Consul, though a gossip implementation is under development. Shards do not communicate with other shards. Rules come from a store (e.g. a Postgres database).

pkg/rules/rules.go

@@ -130,11 +130,11 @@ func dedupGroups(groups []*rulespb.RuleGroup) []*rulespb.RuleGroup {
 	}
 
 	// Sort groups such that they appear next to each other.
-	sort.Slice(groups, func(i, j int) bool { return groups[i].Name < groups[j].Name })
+	sort.Slice(groups, func(i, j int) bool { return groups[i].Compare(groups[j]) < 0 })
 
 	i := 0
 	for _, g := range groups[1:] {
-		if g.Name == groups[i].Name {
+		if g.Compare(groups[i]) == 0 {
 			groups[i].Rules = append(groups[i].Rules, g.Rules...)
 		} else {
 			i++

pkg/rules/rules_test.go

@@ -5,6 +5,7 @@ package rules
 
 import (
 	"context"
+	"path"
 	"path/filepath"
 	"testing"
 	"time"
@@ -53,7 +54,6 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 			File: filepath.Join(dir, "alerts.yaml"),
 			Rules: []*rulespb.Rule{
 				someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert,
-				someRecording, someRecording, someRecording, someRecording, someRecording,
 			},
 			Interval:                60,
 			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
@@ -63,7 +63,6 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 			File: filepath.Join(dir, "alerts.yaml"),
 			Rules: []*rulespb.Rule{
 				someAlert, someAlert, someAlert, someAlert, someAlert, someAlert, someAlert,
-				someRecording, someRecording, someRecording, someRecording, someRecording, someRecording, someRecording,
 			},
 			Interval:                60,
 			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
@@ -87,6 +86,39 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 			File: filepath.Join(dir, "alerts.yaml"),
 			Rules: []*rulespb.Rule{
 				someAlert, someAlert, someAlert, someAlert,
+			},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+		},
+		{
+			Name:                    "thanos-bucket-replicate.rules",
+			File:                    filepath.Join(dir, "rules.yaml"),
+			Rules:                   nil,
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+		},
+		{
+			Name: "thanos-query.rules",
+			File: filepath.Join(dir, "rules.yaml"),
+			Rules: []*rulespb.Rule{
+				someRecording, someRecording, someRecording, someRecording, someRecording,
+			},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+		},
+		{
+			Name: "thanos-receive.rules",
+			File: filepath.Join(dir, "rules.yaml"),
+			Rules: []*rulespb.Rule{
+				someRecording, someRecording, someRecording, someRecording, someRecording, someRecording, someRecording,
+			},
+			Interval:                60,
+			PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+		},
+		{
+			Name: "thanos-store.rules",
+			File: filepath.Join(dir, "rules.yaml"),
+			Rules: []*rulespb.Rule{
 				someRecording, someRecording, someRecording, someRecording,
 			},
 			Interval:                60,
@@ -165,7 +197,9 @@ func testRulesAgainstExamples(t *testing.T, dir string, server rulespb.RulesServ
 				got[i].EvaluationDurationSeconds = 0
 				got[i].LastEvaluation = time.Time{}
 
-				testutil.Equals(t, expectedForType[i], got[i])
+				t.Run(got[i].Name+" "+path.Base(got[i].File), func(t *testing.T) {
+					testutil.Equals(t, expectedForType[i], got[i])
+				})
 			}
 			testutil.Equals(t, expectedForType, got)
 		})
@@ -742,6 +776,79 @@ func TestDedupGroups(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "distinct file names",
+			groups: []*rulespb.RuleGroup{
+				{
+					Name: "a",
+					File: "foo.yaml",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a2"}),
+					},
+				},
+				{
+					Name: "a",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a2"}),
+					},
+				},
+				{
+					Name: "b",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "b1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "b2"}),
+					},
+				},
+				{
+					Name: "c",
+				},
+				{
+					Name: "a",
+					File: "bar.yaml",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a2"}),
+					},
+				},
+			},
+			want: []*rulespb.RuleGroup{
+				{
+					Name: "a",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a2"}),
+					},
+				},
+				{
+					Name: "b",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "b1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "b2"}),
+					},
+				},
+				{
+					Name: "c",
+				},
+				{
+					Name: "a",
+					File: "bar.yaml",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a2"}),
+					},
+				},
+				{
+					Name: "a",
+					File: "foo.yaml",
+					Rules: []*rulespb.Rule{
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
+						rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a2"}),
+					},
+				},
+			},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Run(tc.name, func(t *testing.T) {

pkg/rules/rulespb/custom.go

@@ -186,6 +186,25 @@ func (r *RuleGroups) MarshalJSON() ([]byte, error) {
 	return json.Marshal((*plain)(r))
 }
 
+// Compare compares rule group x and y and returns:
+//
+//   < 0 if x < y   if rule group r1 is not equal and lexically before rule group r2
+//     0 if x == y  if rule group r1 is logically equal to r2 (r1 and r2 are the "same" rule groups)
+//   > 0 if x > y   if rule group r1 is not equal and lexically after rule group r2
+func (r1 *RuleGroup) Compare(r2 *RuleGroup) int {
+	return strings.Compare(r1.Key(), r2.Key())
+}
+
+// Key returns the group key similar resembling Prometheus logic.
+// See https://github.com/prometheus/prometheus/blob/869f1bc587e667b79721852d5badd9f70a39fc3f/rules/manager.go#L1062-L1065
+func (r *RuleGroup) Key() string {
+	if r == nil {
+		return ""
+	}
+
+	return r.File + ";" + r.Name
+}
+
 func (m *Rule) UnmarshalJSON(entry []byte) error {
 	decider := struct {
 		Type string `json:"type"`