From 5ec086dc56807acd1929ba110e92812650b0ecc1 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Sun, 30 Sep 2018 21:52:50 +0200 Subject: [PATCH] don't notify if notification state pending If notification state is pending and last update of state was made less than a minute ago. In the case of a grafana instance is shut down/crashes between setting pending state and before sending the notification/marks as complete this logic should allow the notification to be sent after some time instead of being left in an inconsistent state where no notifications are being sent. --- pkg/models/alert_notifications.go | 2 +- pkg/services/alerting/notifiers/base.go | 10 +++++++++- pkg/services/alerting/notifiers/base_test.go | 16 ++++++++++++++++ pkg/services/sqlstore/alert_notification.go | 13 ++++++++----- pkg/services/sqlstore/alert_notification_test.go | 12 ++++++++++++ pkg/services/sqlstore/migrations/alert_mig.go | 1 + 6 files changed, 47 insertions(+), 7 deletions(-) diff --git a/pkg/models/alert_notifications.go b/pkg/models/alert_notifications.go index bf52e10499a..8608303ddde 100644 --- a/pkg/models/alert_notifications.go +++ b/pkg/models/alert_notifications.go @@ -93,6 +93,7 @@ type AlertNotificationState struct { SentAt int64 State AlertNotificationStateType Version int64 + UpdatedAt int64 } type SetAlertNotificationStateToPendingCommand struct { @@ -110,4 +111,3 @@ type GetNotificationStateQuery struct { Result *AlertNotificationState } - diff --git a/pkg/services/alerting/notifiers/base.go b/pkg/services/alerting/notifiers/base.go index 6dce8494569..b13725f1138 100644 --- a/pkg/services/alerting/notifiers/base.go +++ b/pkg/services/alerting/notifiers/base.go @@ -54,7 +54,7 @@ func defaultShouldNotify(context *alerting.EvalContext, sendReminder bool, frequ 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()) { + if notificationState.SentAt != 0 && lastNotify.Add(frequency).After(time.Now()) { return false } @@ -74,6 +74,14 @@ func defaultShouldNotify(context *alerting.EvalContext, sendReminder bool, frequ return false } + // Do not notifu if state pending and it have been updated last minute + if notificationState.State == models.AlertNotificationStatePending { + lastUpdated := time.Unix(notificationState.UpdatedAt, 0) + if lastUpdated.Add(1 * time.Minute).After(time.Now()) { + return false + } + } + return true } diff --git a/pkg/services/alerting/notifiers/base_test.go b/pkg/services/alerting/notifiers/base_test.go index 3645255e385..581ff6550db 100644 --- a/pkg/services/alerting/notifiers/base_test.go +++ b/pkg/services/alerting/notifiers/base_test.go @@ -116,6 +116,22 @@ func TestShouldSendAlertNotification(t *testing.T) { sendReminder: true, state: &m.AlertNotificationState{SentAt: 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()}, + + 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()}, + expect: true, }, } diff --git a/pkg/services/sqlstore/alert_notification.go b/pkg/services/sqlstore/alert_notification.go index 97db569c214..a69168d094a 100644 --- a/pkg/services/sqlstore/alert_notification.go +++ b/pkg/services/sqlstore/alert_notification.go @@ -242,11 +242,12 @@ func SetAlertNotificationStateToCompleteCommand(ctx context.Context, cmd *m.SetA sql := `UPDATE alert_notification_state SET state = ?, version = ?, - sent_at = ? + sent_at = ?, + updated_at = ? WHERE id = ?` - _, err := sess.Exec(sql, cmd.State.State, cmd.State.Version, cmd.State.SentAt, cmd.State.Id) + _, err := sess.Exec(sql, cmd.State.State, cmd.State.Version, cmd.State.SentAt, timeNow().Unix(), cmd.State.Id) if err != nil { return err @@ -268,12 +269,13 @@ func SetAlertNotificationStateToPendingCommand(ctx context.Context, cmd *m.SetAl sql := `UPDATE alert_notification_state SET state = ?, - version = ? + version = ?, + updated_at = ? WHERE id = ? AND version = ?` - res, err := sess.Exec(sql, cmd.State.State, cmd.State.Version, cmd.State.Id, currentVersion) + res, err := sess.Exec(sql, cmd.State.State, cmd.State.Version, timeNow().Unix(), cmd.State.Id, currentVersion) if err != nil { return err @@ -310,6 +312,7 @@ func GetAlertNotificationState(ctx context.Context, cmd *m.GetNotificationStateQ AlertId: cmd.AlertId, NotifierId: cmd.NotifierId, State: "unknown", + UpdatedAt: timeNow().Unix(), } if _, err := sess.Insert(notificationState); err != nil { @@ -337,7 +340,7 @@ func GetAlertNotificationState(ctx context.Context, cmd *m.GetNotificationStateQ } func getAlertNotificationState(sess *DBSession, cmd *m.GetNotificationStateQuery, nj *m.AlertNotificationState) (bool, error) { - exist, err := sess.Desc("alert_notification_state.sent_at"). + exist, err := sess. Where("alert_notification_state.org_id = ?", cmd.OrgId). Where("alert_notification_state.alert_id = ?", cmd.AlertId). Where("alert_notification_state.notifier_id = ?", cmd.NotifierId). diff --git a/pkg/services/sqlstore/alert_notification_test.go b/pkg/services/sqlstore/alert_notification_test.go index daed5a8cd7f..f82022fd18b 100644 --- a/pkg/services/sqlstore/alert_notification_test.go +++ b/pkg/services/sqlstore/alert_notification_test.go @@ -18,6 +18,9 @@ func TestAlertNotificationSQLAccess(t *testing.T) { var alertID int64 = 7 var orgID int64 = 5 var notifierID int64 = 10 + oldTimeNow := timeNow + now := time.Date(2018, 9, 30, 0, 0, 0, 0, time.UTC) + timeNow = func() time.Time { return now } Convey("Get no existing state should create a new state", func() { query := &models.GetNotificationStateQuery{AlertId: alertID, OrgId: orgID, NotifierId: notifierID} @@ -26,6 +29,7 @@ func TestAlertNotificationSQLAccess(t *testing.T) { So(query.Result, ShouldNotBeNil) So(query.Result.State, ShouldEqual, "unknown") So(query.Result.Version, ShouldEqual, 0) + So(query.Result.UpdatedAt, ShouldEqual, now.Unix()) Convey("Get existing state should not create a new state", func() { query2 := &models.GetNotificationStateQuery{AlertId: alertID, OrgId: orgID, NotifierId: notifierID} @@ -33,6 +37,7 @@ func TestAlertNotificationSQLAccess(t *testing.T) { So(err, ShouldBeNil) So(query2.Result, ShouldNotBeNil) So(query2.Result.Id, ShouldEqual, query.Result.Id) + So(query2.Result.UpdatedAt, ShouldEqual, now.Unix()) }) Convey("Update existing state to pending with correct version should update database", func() { @@ -50,6 +55,7 @@ func TestAlertNotificationSQLAccess(t *testing.T) { So(err, ShouldBeNil) So(query2.Result.Version, ShouldEqual, 1) So(query2.Result.State, ShouldEqual, models.AlertNotificationStatePending) + So(query2.Result.UpdatedAt, ShouldEqual, now.Unix()) Convey("Update existing state to completed should update database", func() { s := *cmd.State @@ -64,6 +70,7 @@ func TestAlertNotificationSQLAccess(t *testing.T) { So(err, ShouldBeNil) So(query3.Result.Version, ShouldEqual, 2) So(query3.Result.State, ShouldEqual, models.AlertNotificationStateCompleted) + So(query3.Result.UpdatedAt, ShouldEqual, now.Unix()) }) Convey("Update existing state to completed should update database, but return version mismatch", func() { @@ -80,6 +87,7 @@ func TestAlertNotificationSQLAccess(t *testing.T) { So(err, ShouldBeNil) So(query3.Result.Version, ShouldEqual, 1001) So(query3.Result.State, ShouldEqual, models.AlertNotificationStateCompleted) + So(query3.Result.UpdatedAt, ShouldEqual, now.Unix()) }) }) @@ -93,6 +101,10 @@ func TestAlertNotificationSQLAccess(t *testing.T) { So(err, ShouldEqual, models.ErrAlertNotificationStateVersionConflict) }) }) + + Reset(func() { + timeNow = oldTimeNow + }) }) Convey("Alert notifications should be empty", func() { diff --git a/pkg/services/sqlstore/migrations/alert_mig.go b/pkg/services/sqlstore/migrations/alert_mig.go index 877dafcf1e1..bd42bb0343d 100644 --- a/pkg/services/sqlstore/migrations/alert_mig.go +++ b/pkg/services/sqlstore/migrations/alert_mig.go @@ -120,6 +120,7 @@ func addAlertMigrations(mg *Migrator) { {Name: "sent_at", Type: DB_BigInt, Nullable: false}, {Name: "state", Type: DB_NVarchar, Length: 50, Nullable: false}, {Name: "version", Type: DB_BigInt, Nullable: false}, + {Name: "updated_at", Type: DB_BigInt, Nullable: false}, }, Indices: []*Index{ {Cols: []string{"org_id", "alert_id", "notifier_id"}, Type: UniqueIndex},