Merge pull request #13402 from bergquist/alert_notifier_reminder

alerting: move all notification conditions to defaultShouldNotify

pkg/models/alert_notifications.go

@@ -98,7 +98,7 @@ type GetLatestNotificationQuery struct {
 	AlertId    int64
 	AlertId    int64
 	NotifierId int64
 	NotifierId int64
 
 
-	Result *AlertNotificationJournal
+	Result []AlertNotificationJournal
 }
 }
 
 
 type CleanNotificationJournalCommand struct {
 type CleanNotificationJournalCommand struct {

pkg/services/alerting/notifier.go

@@ -68,7 +68,7 @@ func (n *notificationService) sendNotifications(evalContext *EvalContext, notifi
 
 
 			// Verify that we can send the notification again
 			// Verify that we can send the notification again
 			// but this time within the same transaction.
 			// but this time within the same transaction.
-			if !evalContext.IsTestRun && !not.ShouldNotify(context.Background(), evalContext) {
+			if !evalContext.IsTestRun && !not.ShouldNotify(ctx, evalContext) {
 				return nil
 				return nil
 			}
 			}
 
 

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.
 	// Only notify on state change.
 	if context.PrevAlertState == context.Rule.State && !sendReminder {
 	if context.PrevAlertState == context.Rule.State && !sendReminder {
 		return false
 		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
 	// Do not notify if interval has not elapsed
 	if sendReminder && !lastNotify.IsZero() && lastNotify.Add(frequency).After(time.Now()) {
 	if sendReminder && !lastNotify.IsZero() && lastNotify.Add(frequency).After(time.Now()) {
 		return false
 		return false
@@ -75,20 +84,12 @@ func (n *NotifierBase) ShouldNotify(ctx context.Context, c *alerting.EvalContext
 	}
 	}
 
 
 	err := bus.DispatchCtx(ctx, cmd)
 	err := bus.DispatchCtx(ctx, cmd)
-	if err == models.ErrJournalingNotFound {
-		return true
-	}
-
 	if err != nil {
 	if err != nil {
 		n.log.Error("Could not determine last time alert notifier fired", "Alert name", c.Rule.Name, "Error", err)
 		n.log.Error("Could not determine last time alert notifier fired", "Alert name", c.Rule.Name, "Error", err)
 		return false
 		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 {
 func (n *NotifierBase) GetType() string {

pkg/services/alerting/notifiers/base_test.go

@@ -15,51 +15,105 @@ import (
 )
 )
 
 
 func TestShouldSendAlertNotification(t *testing.T) {
 func TestShouldSendAlertNotification(t *testing.T) {
+	tnow := time.Now()
+
 	tcs := []struct {
 	tcs := []struct {
 		name         string
 		name         string
 		prevState    m.AlertStateType
 		prevState    m.AlertStateType
 		newState     m.AlertStateType
 		newState     m.AlertStateType
-		expected     bool
 		sendReminder 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",
 			name:         "ok -> ok should not trigger an notification",
 			newState:     m.AlertStateOK,
 			newState:     m.AlertStateOK,
 			prevState:    m.AlertStateOK,
 			prevState:    m.AlertStateOK,
-			expected:     false,
 			sendReminder: 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,
 			newState:     m.AlertStateOK,
 			prevState:    m.AlertStateAlerting,
 			prevState:    m.AlertStateAlerting,
-			expected:     true,
 			sendReminder: true,
 			sendReminder: true,
+			journals:     []m.AlertNotificationJournal{},
+
+			expect: true,
 		},
 		},
 		{
 		{
 			name:         "ok -> ok with reminder should not trigger an notification",
 			name:         "ok -> ok with reminder should not trigger an notification",
 			newState:     m.AlertStateOK,
 			newState:     m.AlertStateOK,
 			prevState:    m.AlertStateOK,
 			prevState:    m.AlertStateOK,
-			expected:     false,
 			sendReminder: true,
 			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
 		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{})
 		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() {
 		Convey("should not notify query returns error", func() {
 			bus.AddHandlerCtx("", func(ctx context.Context, q *m.GetLatestNotificationQuery) error {
 			bus.AddHandlerCtx("", func(ctx context.Context, q *m.GetLatestNotificationQuery) error {
 				return errors.New("some kind of error unknown error")
 				return errors.New("some kind of error unknown error")

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 {
 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{
 		journalEntry := &m.AlertNotificationJournal{
 			OrgId:      cmd.OrgId,
 			OrgId:      cmd.OrgId,
 			AlertId:    cmd.AlertId,
 			AlertId:    cmd.AlertId,
@@ -245,21 +245,19 @@ func RecordNotificationJournal(ctx context.Context, cmd *m.RecordNotificationJou
 }
 }
 
 
 func GetLatestNotification(ctx context.Context, cmd *m.GetLatestNotificationQuery) error {
 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 {
 		if err != nil {
 			return err
 			return err
 		}
 		}
 
 
-		if nj.AlertId == 0 && nj.Id == 0 && nj.NotifierId == 0 && nj.OrgId == 0 {
-			return m.ErrJournalingNotFound
-		}
-
 		cmd.Result = nj
 		cmd.Result = nj
 		return nil
 		return nil
 	})
 	})

pkg/services/sqlstore/alert_notification_test.go

@@ -15,16 +15,21 @@ func TestAlertNotificationSQLAccess(t *testing.T) {
 		InitTestDB(t)
 		InitTestDB(t)
 
 
 		Convey("Alert notification journal", func() {
 		Convey("Alert notification journal", func() {
-			var alertId int64 = 5
+			var alertId int64 = 7
 			var orgId int64 = 5
 			var orgId int64 = 5
-			var notifierId int64 = 5
+			var notifierId int64 = 10
 
 
 			Convey("Getting last journal should raise error if no one exists", func() {
 			Convey("Getting last journal should raise error if no one exists", func() {
 				query := &m.GetLatestNotificationQuery{AlertId: alertId, OrgId: orgId, NotifierId: notifierId}
 				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}
 					createCmd := &m.RecordNotificationJournalCommand{AlertId: alertId, NotifierId: notifierId, OrgId: orgId, Success: true, SentAt: 1}
 
 
 					err := RecordNotificationJournal(context.Background(), createCmd)
 					err := RecordNotificationJournal(context.Background(), createCmd)
@@ -38,17 +43,20 @@ func TestAlertNotificationSQLAccess(t *testing.T) {
 					Convey("get last journaling event", func() {
 					Convey("get last journaling event", func() {
 						err := GetLatestNotification(context.Background(), query)
 						err := GetLatestNotification(context.Background(), query)
 						So(err, ShouldBeNil)
 						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() {
 						Convey("be able to clear all journaling for an notifier", func() {
 							cmd := &m.CleanNotificationJournalCommand{AlertId: alertId, NotifierId: notifierId, OrgId: orgId}
 							cmd := &m.CleanNotificationJournalCommand{AlertId: alertId, NotifierId: notifierId, OrgId: orgId}
 							err := CleanNotificationJournal(context.Background(), cmd)
 							err := CleanNotificationJournal(context.Background(), cmd)
 							So(err, ShouldBeNil)
 							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}
 								query := &m.GetLatestNotificationQuery{AlertId: alertId, OrgId: orgId, NotifierId: notifierId}
 								err := GetLatestNotification(context.Background(), query)
 								err := GetLatestNotification(context.Background(), query)
-								So(err, ShouldEqual, m.ErrJournalingNotFound)
+								So(err, ShouldBeNil)
+								So(len(query.Result), ShouldEqual, 0)
 							})
 							})
 						})
 						})
 					})
 					})