Copy previous series and alert state more intelligently.

Usually rules don't more around, and if they do it's likely
that rules/alerts with the same name stay in the same order.

If rules/alerts with the same name are added/removed this
could cause a blip for one cycle, but this is unavoidable
without requiring rule and alert names to be unique - which we don't
want to do.

rules/manager.go

@@ -221,25 +221,37 @@ func (g *Group) offset() time.Duration {
 }
 
 // copyState copies the alerting rule and staleness related state from the given group.
+//
+// Rules are matched based on their name. If there are duplicates, the
+// first is matched with the first, second with the second etc.
 func (g *Group) copyState(from *Group) {
-	g.seriesInPreviousEval = from.seriesInPreviousEval
-	for _, fromRule := range from.rules {
-		far, ok := fromRule.(*AlertingRule)
+	ruleMap := make(map[string][]int, len(from.rules))
+
+	for fi, fromRule := range from.rules {
+		l, _ := ruleMap[fromRule.Name()]
+		ruleMap[fromRule.Name()] = append(l, fi)
+	}
+
+	for i, rule := range g.rules {
+		indexes, ok := ruleMap[rule.Name()]
+		if len(indexes) == 0 {
+			continue
+		}
+		fi := indexes[0]
+		g.seriesInPreviousEval[i] = from.seriesInPreviousEval[fi]
+		ruleMap[rule.Name()] = indexes[1:]
+
+		ar, ok := rule.(*AlertingRule)
 		if !ok {
 			continue
 		}
-		for _, rule := range g.rules {
-			ar, ok := rule.(*AlertingRule)
-			if !ok {
-				continue
-			}
-			// TODO(fabxc): forbid same alert definitions that are not unique by
-			// at least on static label or alertname?
-			if far.equal(ar) {
-				for fp, a := range far.active {
-					ar.active[fp] = a
-				}
-			}
+		far, ok := from.rules[fi].(*AlertingRule)
+		if !ok {
+			continue
+		}
+
+		for fp, a := range far.active {
+			ar.active[fp] = a
 		}
 	}
 }

rules/manager_test.go

@@ -183,15 +183,7 @@ func TestStaleness(t *testing.T) {
 	// A time series that has two samples and then goes stale.
 	app, _ := storage.Appender()
 	app.Add(labels.FromStrings(model.MetricNameLabel, "a"), 0, 1)
-	if err = app.Commit(); err != nil {
-		t.Fatal(err)
-	}
-	app, _ = storage.Appender()
 	app.Add(labels.FromStrings(model.MetricNameLabel, "a"), 1000, 2)
-	if err = app.Commit(); err != nil {
-		t.Fatal(err)
-	}
-	app, _ = storage.Appender()
 	app.Add(labels.FromStrings(model.MetricNameLabel, "a"), 2000, math.Float64frombits(value.StaleNaN))
 	if err = app.Commit(); err != nil {
 		t.Fatal(err)
@@ -208,8 +200,7 @@ func TestStaleness(t *testing.T) {
 		t.Fatal(err)
 	}
 	matcher, _ := labels.NewMatcher(labels.MatchEqual, model.MetricNameLabel, "a_plus_one")
-	seriesSet := querier.Select(matcher)
-	samples, err := readSeriesSet(seriesSet)
+	samples, err := readSeriesSet(querier.Select(matcher))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -249,9 +240,53 @@ func readSeriesSet(ss storage.SeriesSet) (map[string][]promql.Point, error) {
 
 		name := series.Labels().String()
 		result[name] = points
-		if err := ss.Err(); err != nil {
-			return nil, err
-		}
 	}
-	return result, nil
+	return result, ss.Err()
+}
+
+func TestCopyState(t *testing.T) {
+	oldGroup := &Group{
+		rules: []Rule{
+			NewAlertingRule("alert", nil, 0, nil, nil),
+			NewRecordingRule("rule1", nil, nil),
+			NewRecordingRule("rule2", nil, nil),
+			NewRecordingRule("rule3", nil, nil),
+			NewRecordingRule("rule3", nil, nil),
+		},
+		seriesInPreviousEval: []map[string]labels.Labels{
+			map[string]labels.Labels{"a": nil},
+			map[string]labels.Labels{"r1": nil},
+			map[string]labels.Labels{"r2": nil},
+			map[string]labels.Labels{"r3a": nil},
+			map[string]labels.Labels{"r3b": nil},
+		},
+	}
+	oldGroup.rules[0].(*AlertingRule).active[42] = nil
+	newGroup := &Group{
+		rules: []Rule{
+			NewRecordingRule("rule3", nil, nil),
+			NewRecordingRule("rule3", nil, nil),
+			NewRecordingRule("rule3", nil, nil),
+			NewAlertingRule("alert", nil, 0, nil, nil),
+			NewRecordingRule("rule1", nil, nil),
+			NewRecordingRule("rule4", nil, nil),
+		},
+		seriesInPreviousEval: make([]map[string]labels.Labels, 6),
+	}
+	newGroup.copyState(oldGroup)
+
+	want := []map[string]labels.Labels{
+		map[string]labels.Labels{"r3a": nil},
+		map[string]labels.Labels{"r3b": nil},
+		nil,
+		map[string]labels.Labels{"a": nil},
+		map[string]labels.Labels{"r1": nil},
+		nil,
+	}
+	if !reflect.DeepEqual(want, newGroup.seriesInPreviousEval) {
+		t.Fatalf("seriesInPreviousEval not as expected. Wanted: %+v Got: %+v", want, newGroup.seriesInPreviousEval)
+	}
+	if !reflect.DeepEqual(oldGroup.rules[0], newGroup.rules[3]) {
+		t.Fatalf("Active alerts not as expected. Wanted: %+v Got: %+v", oldGroup.rules[0], oldGroup.rules[3])
+	}
 }