From f001815d9d9910cf4286f4360a88ccb46a1c27c8 Mon Sep 17 00:00:00 2001 From: Carl Bergquist Date: Tue, 7 May 2019 10:34:00 +0200 Subject: [PATCH] alerting: no notification when going from nodata -> pending (#16905) ref #16496 --- pkg/services/alerting/eval_context_test.go | 12 +-- pkg/services/alerting/notifiers/base.go | 28 +++--- pkg/services/alerting/notifiers/base_test.go | 98 +++++++++++--------- 3 files changed, 76 insertions(+), 62 deletions(-) diff --git a/pkg/services/alerting/eval_context_test.go b/pkg/services/alerting/eval_context_test.go index af7e66b2f07..06d3ae16ed8 100644 --- a/pkg/services/alerting/eval_context_test.go +++ b/pkg/services/alerting/eval_context_test.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/grafana/grafana/pkg/models" ) @@ -195,12 +197,10 @@ func TestGetStateFromEvalContext(t *testing.T) { } for _, tc := range tcs { - ctx := NewEvalContext(context.TODO(), &Rule{Conditions: []Condition{&conditionStub{firing: true}}}) + evalContext := NewEvalContext(context.Background(), &Rule{Conditions: []Condition{&conditionStub{firing: true}}}) - tc.applyFn(ctx) - have := ctx.GetNewState() - if have != tc.expected { - t.Errorf("failed: %s \n expected '%s' have '%s'\n", tc.name, tc.expected, string(have)) - } + tc.applyFn(evalContext) + newState := evalContext.GetNewState() + assert.Equal(t, tc.expected, newState, "failed: %s \n expected '%s' have '%s'\n", tc.name, tc.expected, string(newState)) } } diff --git a/pkg/services/alerting/notifiers/base.go b/pkg/services/alerting/notifiers/base.go index 9616c2ab7cf..800505b6380 100644 --- a/pkg/services/alerting/notifiers/base.go +++ b/pkg/services/alerting/notifiers/base.go @@ -48,12 +48,15 @@ func NewNotifierBase(model *models.AlertNotification) NotifierBase { // ShouldNotify checks this evaluation should send an alert notification func (n *NotifierBase) ShouldNotify(ctx context.Context, context *alerting.EvalContext, notiferState *models.AlertNotificationState) bool { + prevState := context.PrevAlertState + newState := context.Rule.State + // Only notify on state change. - if context.PrevAlertState == context.Rule.State && !n.SendReminder { + if prevState == newState && !n.SendReminder { return false } - if context.PrevAlertState == context.Rule.State && n.SendReminder { + if prevState == newState && n.SendReminder { // Do not notify if interval has not elapsed lastNotify := time.Unix(notiferState.UpdatedAt, 0) if notiferState.UpdatedAt != 0 && lastNotify.Add(n.Frequency).After(time.Now()) { @@ -61,32 +64,35 @@ func (n *NotifierBase) ShouldNotify(ctx context.Context, context *alerting.EvalC } // Do not notify if alert state is OK or pending even on repeated notify - if context.Rule.State == models.AlertStateOK || context.Rule.State == models.AlertStatePending { + if newState == models.AlertStateOK || newState == models.AlertStatePending { return false } } - // Do not notify when we become OK for the first time. - if context.PrevAlertState == models.AlertStateUnknown && context.Rule.State == models.AlertStateOK { + unknownOrNoData := prevState == models.AlertStateUnknown || prevState == models.AlertStateNoData + okOrPending := newState == models.AlertStatePending || newState == models.AlertStateOK + + // Do not notify when new state is ok/pending when previous is unknown or no_data + if unknownOrNoData && okOrPending { return false } - // Do not notify when we become OK for the first time. - if context.PrevAlertState == models.AlertStateUnknown && context.Rule.State == models.AlertStatePending { + // Do not notify when we become Pending for the first + if prevState == models.AlertStateNoData && newState == models.AlertStatePending { return false } // Do not notify when we become OK from pending - if context.PrevAlertState == models.AlertStatePending && context.Rule.State == models.AlertStateOK { + if prevState == models.AlertStatePending && newState == models.AlertStateOK { return false } // Do not notify when we OK -> Pending - if context.PrevAlertState == models.AlertStateOK && context.Rule.State == models.AlertStatePending { + if prevState == models.AlertStateOK && newState == models.AlertStatePending { return false } - // Do not notifu if state pending and it have been updated last minute + // Do not notify if state pending and it have been updated last minute if notiferState.State == models.AlertNotificationStatePending { lastUpdated := time.Unix(notiferState.UpdatedAt, 0) if lastUpdated.Add(1 * time.Minute).After(time.Now()) { @@ -95,7 +101,7 @@ func (n *NotifierBase) ShouldNotify(ctx context.Context, context *alerting.EvalC } // Do not notify when state is OK if DisableResolveMessage is set to true - if context.Rule.State == models.AlertStateOK && n.DisableResolveMessage { + if newState == models.AlertStateOK && n.DisableResolveMessage { return false } diff --git a/pkg/services/alerting/notifiers/base_test.go b/pkg/services/alerting/notifiers/base_test.go index bf09ecab0cd..84294bfb29d 100644 --- a/pkg/services/alerting/notifiers/base_test.go +++ b/pkg/services/alerting/notifiers/base_test.go @@ -5,8 +5,10 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/grafana/grafana/pkg/components/simplejson" - m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/alerting" . "github.com/smartystreets/goconvey/convey" ) @@ -16,76 +18,76 @@ func TestShouldSendAlertNotification(t *testing.T) { tcs := []struct { name string - prevState m.AlertStateType - newState m.AlertStateType + prevState models.AlertStateType + newState models.AlertStateType sendReminder bool frequency time.Duration - state *m.AlertNotificationState + state *models.AlertNotificationState expect bool }{ { name: "pending -> ok should not trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStatePending, + newState: models.AlertStateOK, + prevState: models.AlertStatePending, sendReminder: false, expect: false, }, { name: "ok -> alerting should trigger an notification", - newState: m.AlertStateAlerting, - prevState: m.AlertStateOK, + newState: models.AlertStateAlerting, + prevState: models.AlertStateOK, sendReminder: false, expect: true, }, { name: "ok -> pending should not trigger an notification", - newState: m.AlertStatePending, - prevState: m.AlertStateOK, + newState: models.AlertStatePending, + prevState: models.AlertStateOK, sendReminder: false, expect: false, }, { name: "ok -> ok should not trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStateOK, + newState: models.AlertStateOK, + prevState: models.AlertStateOK, sendReminder: false, expect: false, }, { name: "ok -> ok with reminder should not trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStateOK, + newState: models.AlertStateOK, + prevState: models.AlertStateOK, sendReminder: true, expect: false, }, { name: "alerting -> ok should trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStateAlerting, + newState: models.AlertStateOK, + prevState: models.AlertStateAlerting, sendReminder: false, expect: true, }, { name: "alerting -> ok should trigger an notification when reminders enabled", - newState: m.AlertStateOK, - prevState: m.AlertStateAlerting, + newState: models.AlertStateOK, + prevState: models.AlertStateAlerting, frequency: time.Minute * 10, sendReminder: true, - state: &m.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()}, + state: &models.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()}, expect: true, }, { name: "alerting -> alerting with reminder and no state should trigger", - newState: m.AlertStateAlerting, - prevState: m.AlertStateAlerting, + newState: models.AlertStateAlerting, + prevState: models.AlertStateAlerting, frequency: time.Minute * 10, sendReminder: true, @@ -93,78 +95,84 @@ func TestShouldSendAlertNotification(t *testing.T) { }, { name: "alerting -> alerting with reminder and last notification sent 1 minute ago should not trigger", - newState: m.AlertStateAlerting, - prevState: m.AlertStateAlerting, + newState: models.AlertStateAlerting, + prevState: models.AlertStateAlerting, frequency: time.Minute * 10, sendReminder: true, - state: &m.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()}, + state: &models.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()}, expect: false, }, { name: "alerting -> alerting with reminder and last notifciation sent 11 minutes ago should trigger", - newState: m.AlertStateAlerting, - prevState: m.AlertStateAlerting, + newState: models.AlertStateAlerting, + prevState: models.AlertStateAlerting, frequency: time.Minute * 10, sendReminder: true, - state: &m.AlertNotificationState{UpdatedAt: tnow.Add(-11 * time.Minute).Unix()}, + state: &models.AlertNotificationState{UpdatedAt: tnow.Add(-11 * time.Minute).Unix()}, expect: true, }, { name: "OK -> alerting with notifciation state pending and updated 30 seconds ago should not trigger", - newState: m.AlertStateAlerting, - prevState: m.AlertStateOK, - state: &m.AlertNotificationState{State: m.AlertNotificationStatePending, UpdatedAt: tnow.Add(-30 * time.Second).Unix()}, + newState: models.AlertStateAlerting, + prevState: models.AlertStateOK, + state: &models.AlertNotificationState{State: models.AlertNotificationStatePending, UpdatedAt: tnow.Add(-30 * time.Second).Unix()}, expect: false, }, { name: "OK -> alerting with notifciation state pending and updated 2 minutes ago should trigger", - newState: m.AlertStateAlerting, - prevState: m.AlertStateOK, - state: &m.AlertNotificationState{State: m.AlertNotificationStatePending, UpdatedAt: tnow.Add(-2 * time.Minute).Unix()}, + newState: models.AlertStateAlerting, + prevState: models.AlertStateOK, + state: &models.AlertNotificationState{State: models.AlertNotificationStatePending, UpdatedAt: tnow.Add(-2 * time.Minute).Unix()}, expect: true, }, { name: "unknown -> ok", - prevState: m.AlertStateUnknown, - newState: m.AlertStateOK, + prevState: models.AlertStateUnknown, + newState: models.AlertStateOK, expect: false, }, { name: "unknown -> pending", - prevState: m.AlertStateUnknown, - newState: m.AlertStatePending, + prevState: models.AlertStateUnknown, + newState: models.AlertStatePending, expect: false, }, { name: "unknown -> alerting", - prevState: m.AlertStateUnknown, - newState: m.AlertStateAlerting, + prevState: models.AlertStateUnknown, + newState: models.AlertStateAlerting, expect: true, }, + { + name: "no_data -> pending", + prevState: models.AlertStateNoData, + newState: models.AlertStatePending, + + expect: false, + }, } for _, tc := range tcs { - evalContext := alerting.NewEvalContext(context.TODO(), &alerting.Rule{ + evalContext := alerting.NewEvalContext(context.Background(), &alerting.Rule{ State: tc.prevState, }) if tc.state == nil { - tc.state = &m.AlertNotificationState{} + tc.state = &models.AlertNotificationState{} } evalContext.Rule.State = tc.newState nb := &NotifierBase{SendReminder: tc.sendReminder, Frequency: tc.frequency} - if nb.ShouldNotify(evalContext.Ctx, evalContext, tc.state) != tc.expect { - t.Errorf("failed test %s.\n expected \n%+v \nto return: %v", tc.name, tc, tc.expect) - } + r := nb.ShouldNotify(evalContext.Ctx, evalContext, tc.state) + assert.Equal(t, r, tc.expect, "failed test %s. expected %+v to return: %v", tc.name, tc, tc.expect) } } @@ -172,7 +180,7 @@ func TestBaseNotifier(t *testing.T) { Convey("default constructor for notifiers", t, func() { bJson := simplejson.New() - model := &m.AlertNotification{ + model := &models.AlertNotification{ Uid: "1", Name: "name", Type: "email",