diff --git a/pkg/models/alert_notifications.go b/pkg/models/alert_notifications.go index 42d33d5ed22..b90b3d36ced 100644 --- a/pkg/models/alert_notifications.go +++ b/pkg/models/alert_notifications.go @@ -98,7 +98,7 @@ type GetLatestNotificationQuery struct { AlertId int64 NotifierId int64 - Result *AlertNotificationJournal + Result []AlertNotificationJournal } type CleanNotificationJournalCommand struct { diff --git a/pkg/services/alerting/notifier.go b/pkg/services/alerting/notifier.go index 353df1938a2..cbad5cbfdcf 100644 --- a/pkg/services/alerting/notifier.go +++ b/pkg/services/alerting/notifier.go @@ -68,7 +68,7 @@ func (n *notificationService) sendNotifications(evalContext *EvalContext, notifi // Verify that we can send the notification again // but this time within the same transaction. - if !evalContext.IsTestRun && !not.ShouldNotify(context.Background(), evalContext) { + if !evalContext.IsTestRun && !not.ShouldNotify(ctx, evalContext) { return nil } diff --git a/pkg/services/alerting/notifiers/base.go b/pkg/services/alerting/notifiers/base.go index ca011356247..24daa02bce8 100644 --- a/pkg/services/alerting/notifiers/base.go +++ b/pkg/services/alerting/notifiers/base.go @@ -42,12 +42,21 @@ func NewNotifierBase(model *models.AlertNotification) NotifierBase { } } -func defaultShouldNotify(context *alerting.EvalContext, sendReminder bool, frequency time.Duration, lastNotify time.Time) bool { +func defaultShouldNotify(context *alerting.EvalContext, sendReminder bool, frequency time.Duration, journals []models.AlertNotificationJournal) bool { // Only notify on state change. if context.PrevAlertState == context.Rule.State && !sendReminder { return false } + // get last successfully sent notification + lastNotify := time.Time{} + for _, j := range journals { + if j.Success { + lastNotify = time.Unix(j.SentAt, 0) + break + } + } + // Do not notify if interval has not elapsed if sendReminder && !lastNotify.IsZero() && lastNotify.Add(frequency).After(time.Now()) { return false @@ -75,20 +84,12 @@ func (n *NotifierBase) ShouldNotify(ctx context.Context, c *alerting.EvalContext } err := bus.DispatchCtx(ctx, cmd) - if err == models.ErrJournalingNotFound { - return true - } - if err != nil { n.log.Error("Could not determine last time alert notifier fired", "Alert name", c.Rule.Name, "Error", err) return false } - if !cmd.Result.Success { - return true - } - - return defaultShouldNotify(c, n.SendReminder, n.Frequency, time.Unix(cmd.Result.SentAt, 0)) + return defaultShouldNotify(c, n.SendReminder, n.Frequency, cmd.Result) } func (n *NotifierBase) GetType() string { diff --git a/pkg/services/alerting/notifiers/base_test.go b/pkg/services/alerting/notifiers/base_test.go index 57b82f32466..9ea4b82fd54 100644 --- a/pkg/services/alerting/notifiers/base_test.go +++ b/pkg/services/alerting/notifiers/base_test.go @@ -15,51 +15,105 @@ import ( ) func TestShouldSendAlertNotification(t *testing.T) { + tnow := time.Now() + tcs := []struct { name string prevState m.AlertStateType newState m.AlertStateType - expected bool sendReminder bool + frequency time.Duration + journals []m.AlertNotificationJournal + + expect bool }{ { - name: "pending -> ok should not trigger an notification", - newState: m.AlertStatePending, - prevState: m.AlertStateOK, - expected: false, + name: "pending -> ok should not trigger an notification", + newState: m.AlertStatePending, + prevState: m.AlertStateOK, + sendReminder: false, + journals: []m.AlertNotificationJournal{}, + + expect: false, }, { - name: "ok -> alerting should trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStateAlerting, - expected: true, + name: "ok -> alerting should trigger an notification", + newState: m.AlertStateOK, + prevState: m.AlertStateAlerting, + sendReminder: false, + journals: []m.AlertNotificationJournal{}, + + expect: true, }, { - name: "ok -> pending should not trigger an notification", - newState: m.AlertStateOK, - prevState: m.AlertStatePending, - expected: false, + name: "ok -> pending should not trigger an notification", + newState: m.AlertStateOK, + prevState: m.AlertStatePending, + sendReminder: false, + journals: []m.AlertNotificationJournal{}, + + expect: false, }, { name: "ok -> ok should not trigger an notification", newState: m.AlertStateOK, prevState: m.AlertStateOK, - expected: false, sendReminder: false, + journals: []m.AlertNotificationJournal{}, + + expect: false, }, { - name: "ok -> alerting should not trigger an notification", + name: "ok -> alerting should trigger an notification", newState: m.AlertStateOK, prevState: m.AlertStateAlerting, - expected: true, sendReminder: true, + journals: []m.AlertNotificationJournal{}, + + expect: true, }, { name: "ok -> ok with reminder should not trigger an notification", newState: m.AlertStateOK, prevState: m.AlertStateOK, - expected: false, sendReminder: true, + journals: []m.AlertNotificationJournal{}, + + expect: false, + }, + { + name: "alerting -> alerting with reminder and no journaling should trigger", + newState: m.AlertStateAlerting, + prevState: m.AlertStateAlerting, + frequency: time.Minute * 10, + sendReminder: true, + journals: []m.AlertNotificationJournal{}, + + expect: true, + }, + { + name: "alerting -> alerting with reminder and successful recent journal event should not trigger", + newState: m.AlertStateAlerting, + prevState: m.AlertStateAlerting, + frequency: time.Minute * 10, + sendReminder: true, + journals: []m.AlertNotificationJournal{ + {SentAt: tnow.Add(-time.Minute).Unix(), Success: true}, + }, + + expect: false, + }, + { + name: "alerting -> alerting with reminder and failed recent journal event should trigger", + newState: m.AlertStateAlerting, + prevState: m.AlertStateAlerting, + frequency: time.Minute * 10, + sendReminder: true, + expect: true, + journals: []m.AlertNotificationJournal{ + {SentAt: tnow.Add(-time.Minute).Unix(), Success: false}, // recent failed notification + {SentAt: tnow.Add(-time.Hour).Unix(), Success: true}, // old successful notification + }, }, } @@ -69,8 +123,8 @@ func TestShouldSendAlertNotification(t *testing.T) { }) evalContext.Rule.State = tc.prevState - if defaultShouldNotify(evalContext, true, 0, time.Now()) != tc.expected { - t.Errorf("failed %s. expected %+v to return %v", tc.name, tc, tc.expected) + if defaultShouldNotify(evalContext, true, tc.frequency, tc.journals) != tc.expect { + t.Errorf("failed test %s.\n expected \n%+v \nto return: %v", tc.name, tc, tc.expect) } } } @@ -87,16 +141,6 @@ func TestShouldNotifyWhenNoJournalingIsFound(t *testing.T) { }) evalContext := alerting.NewEvalContext(context.TODO(), &alerting.Rule{}) - Convey("should notify if no journaling is found", func() { - bus.AddHandlerCtx("", func(ctx context.Context, q *m.GetLatestNotificationQuery) error { - return m.ErrJournalingNotFound - }) - - if !notifier.ShouldNotify(context.Background(), evalContext) { - t.Errorf("should send notifications when ErrJournalingNotFound is returned") - } - }) - Convey("should not notify query returns error", func() { bus.AddHandlerCtx("", func(ctx context.Context, q *m.GetLatestNotificationQuery) error { return errors.New("some kind of error unknown error") diff --git a/pkg/services/sqlstore/alert_notification.go b/pkg/services/sqlstore/alert_notification.go index 31867910ddb..df247e6891d 100644 --- a/pkg/services/sqlstore/alert_notification.go +++ b/pkg/services/sqlstore/alert_notification.go @@ -230,7 +230,7 @@ func UpdateAlertNotification(cmd *m.UpdateAlertNotificationCommand) error { } func RecordNotificationJournal(ctx context.Context, cmd *m.RecordNotificationJournalCommand) error { - return inTransactionCtx(ctx, func(sess *DBSession) error { + return withDbSession(ctx, func(sess *DBSession) error { journalEntry := &m.AlertNotificationJournal{ OrgId: cmd.OrgId, AlertId: cmd.AlertId, @@ -245,21 +245,19 @@ func RecordNotificationJournal(ctx context.Context, cmd *m.RecordNotificationJou } func GetLatestNotification(ctx context.Context, cmd *m.GetLatestNotificationQuery) error { - return inTransactionCtx(ctx, func(sess *DBSession) error { - nj := &m.AlertNotificationJournal{} + return withDbSession(ctx, func(sess *DBSession) error { + nj := []m.AlertNotificationJournal{} - _, err := sess.Desc("alert_notification_journal.sent_at"). - Limit(1). - Where("alert_notification_journal.org_id = ? AND alert_notification_journal.alert_id = ? AND alert_notification_journal.notifier_id = ?", cmd.OrgId, cmd.AlertId, cmd.NotifierId).Get(nj) + err := sess.Desc("alert_notification_journal.sent_at"). + Where("alert_notification_journal.org_id = ?", cmd.OrgId). + Where("alert_notification_journal.alert_id = ?", cmd.AlertId). + Where("alert_notification_journal.notifier_id = ?", cmd.NotifierId). + Find(&nj) if err != nil { return err } - if nj.AlertId == 0 && nj.Id == 0 && nj.NotifierId == 0 && nj.OrgId == 0 { - return m.ErrJournalingNotFound - } - cmd.Result = nj return nil }) diff --git a/pkg/services/sqlstore/alert_notification_test.go b/pkg/services/sqlstore/alert_notification_test.go index 83fb42db9bb..1e3df45b5cf 100644 --- a/pkg/services/sqlstore/alert_notification_test.go +++ b/pkg/services/sqlstore/alert_notification_test.go @@ -15,16 +15,21 @@ func TestAlertNotificationSQLAccess(t *testing.T) { InitTestDB(t) Convey("Alert notification journal", func() { - var alertId int64 = 5 + var alertId int64 = 7 var orgId int64 = 5 - var notifierId int64 = 5 + var notifierId int64 = 10 Convey("Getting last journal should raise error if no one exists", func() { query := &m.GetLatestNotificationQuery{AlertId: alertId, OrgId: orgId, NotifierId: notifierId} - err := GetLatestNotification(context.Background(), query) - So(err, ShouldEqual, m.ErrJournalingNotFound) + GetLatestNotification(context.Background(), query) + So(len(query.Result), ShouldEqual, 0) - Convey("shoulbe be able to record two journaling events", func() { + // recording an journal entry in another org to make sure org filter works as expected. + journalInOtherOrg := &m.RecordNotificationJournalCommand{AlertId: alertId, NotifierId: notifierId, OrgId: 10, Success: true, SentAt: 1} + err := RecordNotificationJournal(context.Background(), journalInOtherOrg) + So(err, ShouldBeNil) + + Convey("should be able to record two journaling events", func() { createCmd := &m.RecordNotificationJournalCommand{AlertId: alertId, NotifierId: notifierId, OrgId: orgId, Success: true, SentAt: 1} err := RecordNotificationJournal(context.Background(), createCmd) @@ -38,17 +43,20 @@ func TestAlertNotificationSQLAccess(t *testing.T) { Convey("get last journaling event", func() { err := GetLatestNotification(context.Background(), query) So(err, ShouldBeNil) - So(query.Result.SentAt, ShouldEqual, 1001) + So(len(query.Result), ShouldEqual, 2) + last := query.Result[0] + So(last.SentAt, ShouldEqual, 1001) Convey("be able to clear all journaling for an notifier", func() { cmd := &m.CleanNotificationJournalCommand{AlertId: alertId, NotifierId: notifierId, OrgId: orgId} err := CleanNotificationJournal(context.Background(), cmd) So(err, ShouldBeNil) - Convey("querying for last junaling should raise error", func() { + Convey("querying for last journaling should return no journal entries", func() { query := &m.GetLatestNotificationQuery{AlertId: alertId, OrgId: orgId, NotifierId: notifierId} err := GetLatestNotification(context.Background(), query) - So(err, ShouldEqual, m.ErrJournalingNotFound) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 0) }) }) })