From 008b4a93da8138b851cea77ec068a97f3a440ba2 Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Tue, 6 Nov 2018 16:58:08 +0100 Subject: [PATCH] types: fix alert merging Alert merging assumed that EndsAt would always be empty for firing alerts. This is no longer true starting with Prometheus v2.4.0: EndsAt is set to a multiple of the evaluation interval or resend interval (whichever is the largest). This change updates the merging logic to support both cases. Signed-off-by: Simon Pasquier --- types/types.go | 19 ++-- types/types_test.go | 206 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 213 insertions(+), 12 deletions(-) diff --git a/types/types.go b/types/types.go index 94545f82..1299f45a 100644 --- a/types/types.go +++ b/types/types.go @@ -295,9 +295,8 @@ func Alerts(alerts ...*Alert) model.Alerts { res := make(model.Alerts, 0, len(alerts)) for _, a := range alerts { v := a.Alert - // If the end timestamp was set as the expected value in case - // of a timeout but is not reached yet, do not expose it. - if a.Timeout && !a.Resolved() { + // If the end timestamp is not reached yet, do not expose it. + if !a.Resolved() { v.EndsAt = time.Time{} } res = append(res, &v) @@ -321,10 +320,16 @@ func (a *Alert) Merge(o *Alert) *Alert { res.StartsAt = a.StartsAt } - // A non-timeout resolved timestamp always rules. - // The latest explicit resolved timestamp wins. - if a.EndsAt.After(o.EndsAt) && !a.Timeout { - res.EndsAt = a.EndsAt + if o.Resolved() { + // The latest explicit resolved timestamp wins if both alerts are effectively resolved. + if a.Resolved() && a.EndsAt.After(o.EndsAt) { + res.EndsAt = a.EndsAt + } + } else { + // A non-timeout timestamp always rules if it is the latest. + if a.EndsAt.After(o.EndsAt) && !a.Timeout { + res.EndsAt = a.EndsAt + } } return &res diff --git a/types/types_test.go b/types/types_test.go index 4e3fc2d7..bb1970a7 100644 --- a/types/types_test.go +++ b/types/types_test.go @@ -16,6 +16,7 @@ package types import ( "reflect" "sort" + "strconv" "testing" "time" @@ -26,13 +27,17 @@ import ( func TestAlertMerge(t *testing.T) { now := time.Now() + // By convention, alert A is always older than alert B. pairs := []struct { A, B, Res *Alert }{ { + // Both alerts have the Timeout flag set. + // StartsAt is defined by Alert A. + // EndsAt is defined by Alert B. A: &Alert{ Alert: model.Alert{ - StartsAt: now.Add(-time.Minute), + StartsAt: now.Add(-2 * time.Minute), EndsAt: now.Add(2 * time.Minute), }, UpdatedAt: now, @@ -46,6 +51,61 @@ func TestAlertMerge(t *testing.T) { UpdatedAt: now.Add(time.Minute), Timeout: true, }, + Res: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-2 * time.Minute), + EndsAt: now.Add(3 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + Timeout: true, + }, + }, + { + // Alert A has the Timeout flag set while Alert B has it unset. + // StartsAt is defined by Alert A. + // EndsAt is defined by Alert B. + A: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(3 * time.Minute), + }, + UpdatedAt: now, + Timeout: true, + }, + B: &Alert{ + Alert: model.Alert{ + StartsAt: now, + EndsAt: now.Add(2 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + Res: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(2 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + }, + { + // Alert A has the Timeout flag unset while Alert B has it set. + // StartsAt is defined by Alert A. + // EndsAt is defined by Alert A. + A: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(3 * time.Minute), + }, + UpdatedAt: now, + }, + B: &Alert{ + Alert: model.Alert{ + StartsAt: now, + EndsAt: now.Add(2 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + Timeout: true, + }, Res: &Alert{ Alert: model.Alert{ StartsAt: now.Add(-time.Minute), @@ -55,12 +115,148 @@ func TestAlertMerge(t *testing.T) { Timeout: true, }, }, + { + // Both alerts have the Timeout flag unset and are not resolved. + // StartsAt is defined by Alert A. + // EndsAt is defined by Alert A. + A: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(3 * time.Minute), + }, + UpdatedAt: now, + }, + B: &Alert{ + Alert: model.Alert{ + StartsAt: now, + EndsAt: now.Add(2 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + Res: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(3 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + }, + { + // Both alerts have the Timeout flag unset and are not resolved. + // StartsAt is defined by Alert A. + // EndsAt is defined by Alert B. + A: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(3 * time.Minute), + }, + UpdatedAt: now, + }, + B: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(4 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + Res: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-time.Minute), + EndsAt: now.Add(4 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + }, + { + // Both alerts have the Timeout flag unset, A is resolved while B isn't. + // StartsAt is defined by Alert A. + // EndsAt is defined by Alert B. + A: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-3 * time.Minute), + EndsAt: now.Add(-time.Minute), + }, + UpdatedAt: now, + }, + B: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-2 * time.Minute), + EndsAt: now.Add(time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + Res: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-3 * time.Minute), + EndsAt: now.Add(time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + }, + { + // Both alerts have the Timeout flag unset, B is resolved while A isn't. + // StartsAt is defined by Alert A. + // EndsAt is defined by Alert B. + A: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-2 * time.Minute), + EndsAt: now.Add(3 * time.Minute), + }, + UpdatedAt: now, + }, + B: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-2 * time.Minute), + EndsAt: now, + }, + UpdatedAt: now.Add(time.Minute), + }, + Res: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-2 * time.Minute), + EndsAt: now, + }, + UpdatedAt: now.Add(time.Minute), + }, + }, + { + // Both alerts are resolved (EndsAt < now). + // StartsAt is defined by Alert B. + // EndsAt is defined by Alert A. + A: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-3 * time.Minute), + EndsAt: now.Add(-time.Minute), + }, + UpdatedAt: now.Add(-time.Minute), + }, + B: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-4 * time.Minute), + EndsAt: now.Add(-2 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + Res: &Alert{ + Alert: model.Alert{ + StartsAt: now.Add(-4 * time.Minute), + EndsAt: now.Add(-1 * time.Minute), + }, + UpdatedAt: now.Add(time.Minute), + }, + }, } - for _, p := range pairs { - if res := p.A.Merge(p.B); !reflect.DeepEqual(p.Res, res) { - t.Errorf("unexpected merged alert %#v", res) - } + for i, p := range pairs { + p := p + t.Run(strconv.Itoa(i), func(t *testing.T) { + if res := p.A.Merge(p.B); !reflect.DeepEqual(p.Res, res) { + t.Errorf("unexpected merged alert %#v", res) + } + if res := p.B.Merge(p.A); !reflect.DeepEqual(p.Res, res) { + t.Errorf("unexpected merged alert %#v", res) + } + }) } }