From e15d8c5802f98b6a508022500acf44e91adc7d87 Mon Sep 17 00:00:00 2001 From: James Ravn Date: Fri, 15 Mar 2019 15:23:36 +0000 Subject: [PATCH] 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 --- rules/manager.go | 18 +++++++++++++----- rules/manager_test.go | 26 ++++++++++++++++---------- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/rules/manager.go b/rules/manager.go index 4f82f4ee0..4fa160940 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -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 { diff --git a/rules/manager_test.go b/rules/manager_test.go index d41466654..6dbff9c38 100644 --- a/rules/manager_test.go +++ b/rules/manager_test.go @@ -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])