From 8551ffa0b003270da3121df2eb1110d9710d172f Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Fri, 28 Sep 2018 18:34:20 +0200 Subject: [PATCH] alert -> ok with reminders enabled should send --- pkg/services/alerting/notifiers/base.go | 25 ++++--- pkg/services/alerting/notifiers/base_test.go | 79 +++++++++++--------- 2 files changed, 61 insertions(+), 43 deletions(-) diff --git a/pkg/services/alerting/notifiers/base.go b/pkg/services/alerting/notifiers/base.go index e1fc2969154..6dce8494569 100644 --- a/pkg/services/alerting/notifiers/base.go +++ b/pkg/services/alerting/notifiers/base.go @@ -51,19 +51,26 @@ func defaultShouldNotify(context *alerting.EvalContext, sendReminder bool, frequ return false } - // Do not notify if interval has not elapsed - lastNotify := time.Unix(notificationState.SentAt, 0) - if sendReminder && !lastNotify.IsZero() && lastNotify.Add(frequency).After(time.Now()) { - return false - } + if context.PrevAlertState == context.Rule.State && sendReminder { + // Do not notify if interval has not elapsed + lastNotify := time.Unix(notificationState.SentAt, 0) + if !lastNotify.IsZero() && lastNotify.Add(frequency).After(time.Now()) { + return false + } - // Do not notify if alert state if OK or pending even on repeated notify - if sendReminder && (context.Rule.State == models.AlertStateOK || context.Rule.State == models.AlertStatePending) { - return false + // 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 { + return false + } } // Do not notify when we become OK for the first time. - if (context.PrevAlertState == models.AlertStatePending) && (context.Rule.State == models.AlertStateOK) { + if context.PrevAlertState == models.AlertStatePending && context.Rule.State == models.AlertStateOK { + return false + } + + // Do not notify when we OK -> Pending + if context.PrevAlertState == models.AlertStateOK && context.Rule.State == models.AlertStatePending { return false } diff --git a/pkg/services/alerting/notifiers/base_test.go b/pkg/services/alerting/notifiers/base_test.go index 50cfbef7387..3645255e385 100644 --- a/pkg/services/alerting/notifiers/base_test.go +++ b/pkg/services/alerting/notifiers/base_test.go @@ -20,34 +20,34 @@ func TestShouldSendAlertNotification(t *testing.T) { newState m.AlertStateType sendReminder bool frequency time.Duration - journals *m.AlertNotificationState + state *m.AlertNotificationState expect bool }{ { name: "pending -> ok should not trigger an notification", - newState: m.AlertStatePending, - prevState: m.AlertStateOK, + newState: m.AlertStateOK, + prevState: m.AlertStatePending, sendReminder: false, - journals: &m.AlertNotificationState{}, + state: &m.AlertNotificationState{}, expect: false, }, { name: "ok -> alerting should trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStateAlerting, + newState: m.AlertStateAlerting, + prevState: m.AlertStateOK, sendReminder: false, - journals: &m.AlertNotificationState{}, + state: &m.AlertNotificationState{}, expect: true, }, { name: "ok -> pending should not trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStatePending, + newState: m.AlertStatePending, + prevState: m.AlertStateOK, sendReminder: false, - journals: &m.AlertNotificationState{}, + state: &m.AlertNotificationState{}, expect: false, }, @@ -56,66 +56,77 @@ func TestShouldSendAlertNotification(t *testing.T) { newState: m.AlertStateOK, prevState: m.AlertStateOK, sendReminder: false, - journals: &m.AlertNotificationState{}, + state: &m.AlertNotificationState{}, expect: false, }, - { - name: "ok -> alerting should trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStateAlerting, - sendReminder: true, - journals: &m.AlertNotificationState{}, - - expect: true, - }, { name: "ok -> ok with reminder should not trigger an notification", newState: m.AlertStateOK, prevState: m.AlertStateOK, sendReminder: true, - journals: &m.AlertNotificationState{}, + state: &m.AlertNotificationState{}, expect: false, }, { - name: "alerting -> alerting with reminder and no journaling should trigger", - newState: m.AlertStateAlerting, + name: "alerting -> ok should trigger an notification", + newState: m.AlertStateOK, prevState: m.AlertStateAlerting, - frequency: time.Minute * 10, - sendReminder: true, - journals: &m.AlertNotificationState{}, + sendReminder: false, + state: &m.AlertNotificationState{}, expect: true, }, { - name: "alerting -> alerting with reminder and successful recent journal event should not trigger", + name: "alerting -> ok should trigger an notification when reminders enabled", + newState: m.AlertStateOK, + prevState: m.AlertStateAlerting, + frequency: time.Minute * 10, + sendReminder: true, + state: &m.AlertNotificationState{SentAt: tnow.Add(-time.Minute).Unix()}, + + expect: true, + }, + { + name: "alerting -> alerting with reminder and no state should trigger", newState: m.AlertStateAlerting, prevState: m.AlertStateAlerting, frequency: time.Minute * 10, sendReminder: true, - journals: &m.AlertNotificationState{SentAt: tnow.Add(-time.Minute).Unix()}, + state: &m.AlertNotificationState{}, + + expect: true, + }, + { + name: "alerting -> alerting with reminder and last notification sent 1 minute ago should not trigger", + newState: m.AlertStateAlerting, + prevState: m.AlertStateAlerting, + frequency: time.Minute * 10, + sendReminder: true, + state: &m.AlertNotificationState{SentAt: tnow.Add(-time.Minute).Unix()}, expect: false, }, { - name: "alerting -> alerting with reminder and failed recent journal event should trigger", + name: "alerting -> alerting with reminder and last notifciation sent 11 minutes ago should trigger", newState: m.AlertStateAlerting, prevState: m.AlertStateAlerting, frequency: time.Minute * 10, sendReminder: true, - expect: true, - journals: &m.AlertNotificationState{SentAt: tnow.Add(-time.Hour).Unix()}, + state: &m.AlertNotificationState{SentAt: tnow.Add(-11 * time.Minute).Unix()}, + + expect: true, }, } for _, tc := range tcs { evalContext := alerting.NewEvalContext(context.TODO(), &alerting.Rule{ - State: tc.newState, + State: tc.prevState, }) - evalContext.Rule.State = tc.prevState - if defaultShouldNotify(evalContext, true, tc.frequency, tc.journals) != tc.expect { + evalContext.Rule.State = tc.newState + if defaultShouldNotify(evalContext, tc.sendReminder, tc.frequency, tc.state) != tc.expect { t.Errorf("failed test %s.\n expected \n%+v \nto return: %v", tc.name, tc, tc.expect) } }