reload: copy state on both name and labels (#5368)

* reload: copy state on both name and labels

Fix https://github.com/prometheus/prometheus/issues/5193

Using just name causes the linked issue - if new rules are inserted with
the same name (but different labels), the reordering will cause stale
markers to be inserted in the next eval for all shifted rules, despite
them not being stale.

Ideally we want to avoid stale markers for time series that still exist
in the new rules, with name and labels being the unique identifer.

This change adds labels to the internal map when copying the old rule
data to the new rule data. This prevents the problem of staling rules
that simply shifted order.

If labels change, it is a new time series and the old series will stale
regardless. So it should be safe to always match on name and labels when
copying state.

Signed-off-by: James Ravn <james@r-vn.org>
This commit is contained in:
James Ravn 2019-03-15 15:23:36 +00:00 committed by Brian Brazil
parent 23069b87dc
commit e15d8c5802
2 changed files with 29 additions and 15 deletions

View File

@ -189,6 +189,8 @@ func EngineQueryFunc(engine *promql.Engine, q storage.Queryable) QueryFunc {
// interval and acted upon (currently either recorded or used for alerting).
type Rule interface {
Name() string
// Labels of the rule.
Labels() labels.Labels
// eval evaluates the rule, including any associated recording or alerting actions.
Eval(context.Context, time.Time, QueryFunc, *url.URL) (promql.Vector, error)
// String returns a human-readable string representation of the rule.
@ -404,9 +406,13 @@ func (g *Group) evalTimestamp() time.Time {
return time.Unix(0, base+offset)
}
func nameAndLabels(rule Rule) string {
return rule.Name() + rule.Labels().String()
}
// 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
// Rules are matched based on their name and labels. If there are duplicates, the
// first is matched with the first, second with the second etc.
func (g *Group) CopyState(from *Group) {
g.evaluationDuration = from.evaluationDuration
@ -414,18 +420,20 @@ func (g *Group) CopyState(from *Group) {
ruleMap := make(map[string][]int, len(from.rules))
for fi, fromRule := range from.rules {
l := ruleMap[fromRule.Name()]
ruleMap[fromRule.Name()] = append(l, fi)
nameAndLabels := nameAndLabels(fromRule)
l := ruleMap[nameAndLabels]
ruleMap[nameAndLabels] = append(l, fi)
}
for i, rule := range g.rules {
indexes := ruleMap[rule.Name()]
nameAndLabels := nameAndLabels(rule)
indexes := ruleMap[nameAndLabels]
if len(indexes) == 0 {
continue
}
fi := indexes[0]
g.seriesInPreviousEval[i] = from.seriesInPreviousEval[fi]
ruleMap[rule.Name()] = indexes[1:]
ruleMap[nameAndLabels] = indexes[1:]
ar, ok := rule.(*AlertingRule)
if !ok {

View File

@ -584,39 +584,45 @@ func TestCopyState(t *testing.T) {
NewAlertingRule("alert", nil, 0, nil, nil, true, nil),
NewRecordingRule("rule1", nil, nil),
NewRecordingRule("rule2", nil, nil),
NewRecordingRule("rule3", nil, nil),
NewRecordingRule("rule3", nil, nil),
NewRecordingRule("rule3", nil, labels.Labels{{Name: "l1", Value: "v1"}}),
NewRecordingRule("rule3", nil, labels.Labels{{Name: "l1", Value: "v2"}}),
NewAlertingRule("alert2", nil, 0, labels.Labels{{Name: "l2", Value: "v1"}}, nil, true, nil),
},
seriesInPreviousEval: []map[string]labels.Labels{
{"a": nil},
{"r1": nil},
{"r2": nil},
{"r3a": nil},
{"r3b": nil},
{"r3a": labels.Labels{{Name: "l1", Value: "v1"}}},
{"r3b": labels.Labels{{Name: "l1", Value: "v2"}}},
{"a2": labels.Labels{{Name: "l2", Value: "v1"}}},
},
evaluationDuration: time.Second,
}
oldGroup.rules[0].(*AlertingRule).active[42] = nil
newGroup := &Group{
rules: []Rule{
NewRecordingRule("rule3", nil, nil),
NewRecordingRule("rule3", nil, nil),
NewRecordingRule("rule3", nil, nil),
NewRecordingRule("rule3", nil, labels.Labels{{Name: "l1", Value: "v0"}}),
NewRecordingRule("rule3", nil, labels.Labels{{Name: "l1", Value: "v1"}}),
NewRecordingRule("rule3", nil, labels.Labels{{Name: "l1", Value: "v2"}}),
NewAlertingRule("alert", nil, 0, nil, nil, true, nil),
NewRecordingRule("rule1", nil, nil),
NewAlertingRule("alert2", nil, 0, labels.Labels{{Name: "l2", Value: "v0"}}, nil, true, nil),
NewAlertingRule("alert2", nil, 0, labels.Labels{{Name: "l2", Value: "v1"}}, nil, true, nil),
NewRecordingRule("rule4", nil, nil),
},
seriesInPreviousEval: make([]map[string]labels.Labels, 6),
seriesInPreviousEval: make([]map[string]labels.Labels, 8),
}
newGroup.CopyState(oldGroup)
want := []map[string]labels.Labels{
{"r3a": nil},
{"r3b": nil},
nil,
{"r3a": labels.Labels{{Name: "l1", Value: "v1"}}},
{"r3b": labels.Labels{{Name: "l1", Value: "v2"}}},
{"a": nil},
{"r1": nil},
nil,
{"a2": labels.Labels{{Name: "l2", Value: "v1"}}},
nil,
}
testutil.Equals(t, want, newGroup.seriesInPreviousEval)
testutil.Equals(t, oldGroup.rules[0], newGroup.rules[3])