Cherry-pick of pkg/rules: fix deduplication of equal alerts with different labels (#3960)

CHANGELOG.md

@@ -35,7 +35,8 @@ We use _breaking :warning:_ to mark changes that are not backward compatible (re
 - [#3815](https://github.com/thanos-io/thanos/pull/3815) Receive: Improve handling of empty time series from clients
 - [#3795](https://github.com/thanos-io/thanos/pull/3795) s3: A truncated "get object" response is reported as error.
 - [#3899](https://github.com/thanos-io/thanos/pull/3899) Receive: Correct the inference of client gRPC configuration.
-- [#3943](https://github.com/thanos-io/thanos/pull/3943): Receive: Fixed memory regression introduced in v0.17.0.
+- [#3943](https://github.com/thanos-io/thanos/pull/3943) Receive: Fixed memory regression introduced in v0.17.0.
+- [#3960](https://github.com/thanos-io/thanos/pull/3960) Query: Fixed deduplication of equal alerts with different labels.
 
 ### Changed
 

pkg/rules/rules.go

@@ -68,23 +68,22 @@ func dedupRules(rules []*rulespb.Rule, replicaLabels map[string]struct{}) []*rul
 		return rules
 	}
 
-	// Sort each rule's label names such that they are comparable.
+	// Remove replica labels and sort each rule's label names such that they are comparable.
 	for _, r := range rules {
+		removeReplicaLabels(r, replicaLabels)
 		sort.Slice(r.GetLabels(), func(i, j int) bool {
 			return r.GetLabels()[i].Name < r.GetLabels()[j].Name
 		})
 	}
 
-	// Sort rules globally based on synthesized deduplication labels, also considering replica labels and their values.
+	// Sort rules globally.
 	sort.Slice(rules, func(i, j int) bool {
 		return rules[i].Compare(rules[j]) < 0
 	})
 
-	// Remove rules based on synthesized deduplication labels, this time ignoring replica labels and last evaluation.
+	// Remove rules based on synthesized deduplication labels.
 	i := 0
-	removeReplicaLabels(rules[i], replicaLabels)
 	for j := 1; j < len(rules); j++ {
-		removeReplicaLabels(rules[j], replicaLabels)
 		if rules[i].Compare(rules[j]) != 0 {
 			// Effectively retain rules[j] in the resulting slice.
 			i++

pkg/rules/rules_test.go

@@ -448,14 +448,14 @@ func TestDedupRules(t *testing.T) {
 				rulespb.NewAlertingRule(&rulespb.Alert{
 					Name:            "a1",
 					Query:           "up",
-					DurationSeconds: 2.0,
+					DurationSeconds: 1.0,
 					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
 						{Name: "a", Value: "1"},
 					}}}),
 				rulespb.NewAlertingRule(&rulespb.Alert{
 					Name:            "a1",
 					Query:           "up",
-					DurationSeconds: 1.0,
+					DurationSeconds: 2.0,
 					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
 						{Name: "a", Value: "1"},
 					}}}),
@@ -505,13 +505,13 @@ func TestDedupRules(t *testing.T) {
 				}}}),
 			},
 			want: []*rulespb.Rule{
+				rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
 				rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1", Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
 					{Name: "a", Value: "1"},
 				}}}),
 				rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1", Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
 					{Name: "a", Value: "2"},
 				}}}),
-				rulespb.NewRecordingRule(&rulespb.RecordingRule{Name: "a1"}),
 			},
 			replicaLabels: []string{"replica"},
 		},
@@ -613,6 +613,11 @@ func TestDedupRules(t *testing.T) {
 				}),
 			},
 			want: []*rulespb.Rule{
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					State:          rulespb.AlertState_FIRING,
+					Name:           "a1",
+					LastEvaluation: time.Unix(2, 0),
+				}),
 				rulespb.NewAlertingRule(&rulespb.Alert{
 					State: rulespb.AlertState_FIRING,
 					Name:  "a1",
@@ -621,11 +626,6 @@ func TestDedupRules(t *testing.T) {
 					}},
 					LastEvaluation: time.Unix(1, 0),
 				}),
-				rulespb.NewAlertingRule(&rulespb.Alert{
-					State:          rulespb.AlertState_FIRING,
-					Name:           "a1",
-					LastEvaluation: time.Unix(2, 0),
-				}),
 				rulespb.NewAlertingRule(&rulespb.Alert{
 					State:          rulespb.AlertState_PENDING,
 					Name:           "a2",
@@ -634,6 +634,98 @@ func TestDedupRules(t *testing.T) {
 			},
 			replicaLabels: []string{"replica"},
 		},
+		{
+			name: "alerts with different severity",
+			rules: []*rulespb.Rule{
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "replica", Value: "1"},
+						{Name: "severity", Value: "warning"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "replica", Value: "1"},
+						{Name: "severity", Value: "critical"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "replica", Value: "2"},
+						{Name: "severity", Value: "warning"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "replica", Value: "2"},
+						{Name: "severity", Value: "critical"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+			},
+			want: []*rulespb.Rule{
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "severity", Value: "critical"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "severity", Value: "warning"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+			},
+			replicaLabels: []string{"replica"},
+		},
+		{
+			name: "alerts with missing replica labels",
+			rules: []*rulespb.Rule{
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "replica", Value: "1"},
+						{Name: "label", Value: "foo"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "replica", Value: "2"},
+						{Name: "label", Value: "foo"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "label", Value: "foo"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+			},
+			want: []*rulespb.Rule{
+				rulespb.NewAlertingRule(&rulespb.Alert{
+					Name: "a1",
+					Labels: labelpb.ZLabelSet{Labels: []labelpb.ZLabel{
+						{Name: "label", Value: "foo"},
+					}},
+					LastEvaluation: time.Unix(1, 0),
+				}),
+			},
+			replicaLabels: []string{"replica"},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			replicaLabels := make(map[string]struct{})