Sort alerts in correct order (#1349)

* Sort dispatched alerts by job+instance in the correct order (#1178)

Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>

* dispatch: add unit test for alerts sorting

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

dispatch/dispatch.go

@@ -431,32 +431,13 @@ func (ag *aggrGroup) flush(notify func(...*types.Alert) bool) {
 
 	var (
 		alerts      = make(map[model.Fingerprint]*types.Alert, len(ag.alerts))
-		alertsSlice = make([]*types.Alert, 0, len(ag.alerts))
+		alertsSlice = make(types.AlertSlice, 0, len(ag.alerts))
 	)
 	for fp, alert := range ag.alerts {
 		alerts[fp] = alert
 		alertsSlice = append(alertsSlice, alert)
 	}
-
-	sort.SliceStable(alertsSlice, func(i, j int) bool {
-		// Look at labels.job, then labels.instance.
-		for _, override_key := range [...]model.LabelName{"job", "instance"} {
-			key_i, ok_i := alertsSlice[i].Labels[override_key]
-			if !ok_i {
-				return true
-			}
-			key_j, ok_j := alertsSlice[j].Labels[override_key]
-			if !ok_j {
-				return false
-			}
-
-			if key_i != key_j {
-				return key_i > key_j
-			}
-		}
-
-		return alertsSlice[i].Labels.Before(alertsSlice[j].Labels)
-	})
+	sort.Stable(alertsSlice)
 
 	ag.mtx.Unlock()
 

types/types.go

@@ -266,9 +266,28 @@ type Alert struct {
 // AlertSlice is a sortable slice of Alerts.
 type AlertSlice []*Alert
 
-func (as AlertSlice) Less(i, j int) bool { return as[i].UpdatedAt.Before(as[j].UpdatedAt) }
-func (as AlertSlice) Swap(i, j int)      { as[i], as[j] = as[j], as[i] }
-func (as AlertSlice) Len() int           { return len(as) }
+func (as AlertSlice) Less(i, j int) bool {
+	// Look at labels.job, then labels.instance.
+	for _, overrideKey := range [...]model.LabelName{"job", "instance"} {
+		iVal, iOk := as[i].Labels[overrideKey]
+		jVal, jOk := as[j].Labels[overrideKey]
+		if !iOk && !jOk {
+			continue
+		}
+		if !iOk {
+			return false
+		}
+		if !jOk {
+			return true
+		}
+		if iVal != jVal {
+			return iVal < jVal
+		}
+	}
+	return as[i].Labels.Before(as[j].Labels)
+}
+func (as AlertSlice) Swap(i, j int) { as[i], as[j] = as[j], as[i] }
+func (as AlertSlice) Len() int      { return len(as) }
 
 // Alerts turns a sequence of internal alerts into a list of
 // exposable model.Alert structures.

types/types_test.go

@@ -15,6 +15,7 @@ package types
 
 import (
 	"reflect"
+	"sort"
 	"testing"
 	"time"
 
@@ -94,3 +95,78 @@ func TestSilenceExpired(t *testing.T) {
 	silence = Silence{StartsAt: now, EndsAt: now.Add(time.Hour)}
 	require.False(t, silence.Expired())
 }
+
+func TestAlertSliceSort(t *testing.T) {
+	var (
+		a1 = &Alert{
+			Alert: model.Alert{
+				Labels: model.LabelSet{
+					"job":       "j1",
+					"instance":  "i1",
+					"alertname": "an1",
+				},
+			},
+		}
+		a2 = &Alert{
+			Alert: model.Alert{
+				Labels: model.LabelSet{
+					"job":       "j1",
+					"instance":  "i1",
+					"alertname": "an2",
+				},
+			},
+		}
+		a3 = &Alert{
+			Alert: model.Alert{
+				Labels: model.LabelSet{
+					"job":       "j2",
+					"instance":  "i1",
+					"alertname": "an1",
+				},
+			},
+		}
+		a4 = &Alert{
+			Alert: model.Alert{
+				Labels: model.LabelSet{
+					"alertname": "an1",
+				},
+			},
+		}
+		a5 = &Alert{
+			Alert: model.Alert{
+				Labels: model.LabelSet{
+					"alertname": "an2",
+				},
+			},
+		}
+	)
+
+	cases := []struct {
+		alerts AlertSlice
+		exp    AlertSlice
+	}{
+		{
+			alerts: AlertSlice{a2, a1},
+			exp:    AlertSlice{a1, a2},
+		},
+		{
+			alerts: AlertSlice{a3, a2, a1},
+			exp:    AlertSlice{a1, a2, a3},
+		},
+		{
+			alerts: AlertSlice{a4, a2, a4},
+			exp:    AlertSlice{a2, a4, a4},
+		},
+		{
+			alerts: AlertSlice{a5, a4},
+			exp:    AlertSlice{a4, a5},
+		},
+	}
+
+	for _, tc := range cases {
+		sort.Stable(tc.alerts)
+		if !reflect.DeepEqual(tc.alerts, tc.exp) {
+			t.Fatalf("expected %v but got %v", tc.exp, tc.alerts)
+		}
+	}
+}