From fca97535d121a0d316dfa0dc9e62d5d8839ee46a Mon Sep 17 00:00:00 2001 From: John Baublitz Date: Sun, 20 May 2018 12:12:10 -0400 Subject: [PATCH] Fix multiple bugs --- pkg/api/alerting.go | 57 ++++++++++++++++--- pkg/api/dtos/alerting.go | 19 ++++--- pkg/models/alert_notifications.go | 6 +- pkg/services/alerting/eval_context.go | 4 +- pkg/services/alerting/notifiers/base.go | 1 + pkg/services/alerting/notifiers/base_test.go | 13 +++-- pkg/services/sqlstore/alert_notification.go | 28 +++++++-- .../alerting/partials/notification_edit.html | 3 +- 8 files changed, 95 insertions(+), 36 deletions(-) diff --git a/pkg/api/alerting.go b/pkg/api/alerting.go index 961fc11b2dc..c5b47270f4d 100644 --- a/pkg/api/alerting.go +++ b/pkg/api/alerting.go @@ -193,12 +193,15 @@ func GetAlertNotifications(c *m.ReqContext) Response { for _, notification := range query.Result { result = append(result, &dtos.AlertNotification{ - Id: notification.Id, - Name: notification.Name, - Type: notification.Type, - IsDefault: notification.IsDefault, - Created: notification.Created, - Updated: notification.Updated, + Id: notification.Id, + Name: notification.Name, + Type: notification.Type, + IsDefault: notification.IsDefault, + Created: notification.Created, + Updated: notification.Updated, + Frequency: notification.Frequency.String(), + NotifyOnce: notification.NotifyOnce, + Settings: notification.Settings, }) } @@ -215,7 +218,19 @@ func GetAlertNotificationByID(c *m.ReqContext) Response { return Error(500, "Failed to get alert notifications", err) } - return JSON(200, query.Result) + result := &dtos.AlertNotification{ + Id: query.Result.Id, + Name: query.Result.Name, + Type: query.Result.Type, + IsDefault: query.Result.IsDefault, + Created: query.Result.Created, + Updated: query.Result.Updated, + Frequency: query.Result.Frequency.String(), + NotifyOnce: query.Result.NotifyOnce, + Settings: query.Result.Settings, + } + + return JSON(200, result) } func CreateAlertNotification(c *m.ReqContext, cmd m.CreateAlertNotificationCommand) Response { @@ -225,7 +240,19 @@ func CreateAlertNotification(c *m.ReqContext, cmd m.CreateAlertNotificationComma return Error(500, "Failed to create alert notification", err) } - return JSON(200, cmd.Result) + result := &dtos.AlertNotification{ + Id: cmd.Result.Id, + Name: cmd.Result.Name, + Type: cmd.Result.Type, + IsDefault: cmd.Result.IsDefault, + Created: cmd.Result.Created, + Updated: cmd.Result.Updated, + Frequency: cmd.Result.Frequency.String(), + NotifyOnce: cmd.Result.NotifyOnce, + Settings: cmd.Result.Settings, + } + + return JSON(200, result) } func UpdateAlertNotification(c *m.ReqContext, cmd m.UpdateAlertNotificationCommand) Response { @@ -235,7 +262,19 @@ func UpdateAlertNotification(c *m.ReqContext, cmd m.UpdateAlertNotificationComma return Error(500, "Failed to update alert notification", err) } - return JSON(200, cmd.Result) + result := &dtos.AlertNotification{ + Id: cmd.Result.Id, + Name: cmd.Result.Name, + Type: cmd.Result.Type, + IsDefault: cmd.Result.IsDefault, + Created: cmd.Result.Created, + Updated: cmd.Result.Updated, + Frequency: cmd.Result.Frequency.String(), + NotifyOnce: cmd.Result.NotifyOnce, + Settings: cmd.Result.Settings, + } + + return JSON(200, result) } func DeleteAlertNotification(c *m.ReqContext) Response { diff --git a/pkg/api/dtos/alerting.go b/pkg/api/dtos/alerting.go index 5e0196c20d1..7d4201fba87 100644 --- a/pkg/api/dtos/alerting.go +++ b/pkg/api/dtos/alerting.go @@ -24,14 +24,15 @@ type AlertRule struct { } type AlertNotification struct { - Id int64 `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - IsDefault bool `json:"isDefault"` - NotifyOnce bool `json:"notifyOnce"` - Frequency bool `json:"frequency"` - Created time.Time `json:"created"` - Updated time.Time `json:"updated"` + Id int64 `json:"id"` + Name string `json:"name"` + Type string `json:"type"` + IsDefault bool `json:"isDefault"` + NotifyOnce bool `json:"notifyOnce"` + Frequency string `json:"frequency"` + Created time.Time `json:"created"` + Updated time.Time `json:"updated"` + Settings *simplejson.Json `json:"settings"` } type AlertTestCommand struct { @@ -64,7 +65,7 @@ type NotificationTestCommand struct { Name string `json:"name"` Type string `json:"type"` NotifyOnce bool `json:"notifyOnce"` - Frequency time.Duration `json:"frequency"` + Frequency string `json:"frequency"` Settings *simplejson.Json `json:"settings"` } diff --git a/pkg/models/alert_notifications.go b/pkg/models/alert_notifications.go index cba62a51527..6715eb21395 100644 --- a/pkg/models/alert_notifications.go +++ b/pkg/models/alert_notifications.go @@ -22,8 +22,8 @@ type AlertNotification struct { type CreateAlertNotificationCommand struct { Name string `json:"name" binding:"Required"` Type string `json:"type" binding:"Required"` - NotifyOnce bool `json:"notifyOnce" binding:"Required"` - Frequency time.Duration `json:"frequency"` + NotifyOnce bool `json:"notifyOnce"` + Frequency string `json:"frequency"` IsDefault bool `json:"isDefault"` Settings *simplejson.Json `json:"settings"` @@ -35,7 +35,7 @@ type UpdateAlertNotificationCommand struct { Id int64 `json:"id" binding:"Required"` Name string `json:"name" binding:"Required"` Type string `json:"type" binding:"Required"` - NotifyOnce string `json:"notifyOnce" binding:"Required"` + NotifyOnce bool `json:"notifyOnce"` Frequency string `json:"frequency"` IsDefault bool `json:"isDefault"` Settings *simplejson.Json `json:"settings" binding:"Required"` diff --git a/pkg/services/alerting/eval_context.go b/pkg/services/alerting/eval_context.go index b451d188a64..3817f4b4a3c 100644 --- a/pkg/services/alerting/eval_context.go +++ b/pkg/services/alerting/eval_context.go @@ -151,8 +151,8 @@ func (c *EvalContext) LastNotify(notifierId int64) *time.Time { NotifierId: notifierId, } if err := bus.Dispatch(cmd); err != nil { - c.log.Warn("Could not determine last time alert", - c.Rule.Name, "notified") + c.log.Warn("Could not determine last time alert notifier fired", + "Alert name", c.Rule.Name, "Error", err) return nil } diff --git a/pkg/services/alerting/notifiers/base.go b/pkg/services/alerting/notifiers/base.go index e9e32020c6d..734b5e56b28 100644 --- a/pkg/services/alerting/notifiers/base.go +++ b/pkg/services/alerting/notifiers/base.go @@ -41,6 +41,7 @@ func defaultShouldNotify(context *alerting.EvalContext, notifyOnce bool, frequen if context.PrevAlertState == context.Rule.State && notifyOnce { return false } + // Do not notify if interval has not elapsed if !notifyOnce && lastNotify != nil && lastNotify.Add(frequency).After(time.Now()) { return false } diff --git a/pkg/services/alerting/notifiers/base_test.go b/pkg/services/alerting/notifiers/base_test.go index b7142d144cc..5f2d4989063 100644 --- a/pkg/services/alerting/notifiers/base_test.go +++ b/pkg/services/alerting/notifiers/base_test.go @@ -3,6 +3,7 @@ package notifiers import ( "context" "testing" + "time" "github.com/grafana/grafana/pkg/components/simplejson" m "github.com/grafana/grafana/pkg/models" @@ -18,19 +19,19 @@ func TestBaseNotifier(t *testing.T) { Convey("can parse false value", func() { bJson.Set("uploadImage", false) - base := NewNotifierBase(1, false, "name", "email", bJson) + base := NewNotifierBase(1, false, "name", "email", true, 0, bJson) So(base.UploadImage, ShouldBeFalse) }) Convey("can parse true value", func() { bJson.Set("uploadImage", true) - base := NewNotifierBase(1, false, "name", "email", bJson) + base := NewNotifierBase(1, false, "name", "email", true, 0, bJson) So(base.UploadImage, ShouldBeTrue) }) Convey("default value should be true for backwards compatibility", func() { - base := NewNotifierBase(1, false, "name", "email", bJson) + base := NewNotifierBase(1, false, "name", "email", true, 0, bJson) So(base.UploadImage, ShouldBeTrue) }) }) @@ -41,7 +42,8 @@ func TestBaseNotifier(t *testing.T) { State: m.AlertStatePending, }) context.Rule.State = m.AlertStateOK - So(defaultShouldNotify(context), ShouldBeFalse) + timeNow := time.Now() + So(defaultShouldNotify(context, true, 0, &timeNow), ShouldBeFalse) }) Convey("ok -> alerting", func() { @@ -49,7 +51,8 @@ func TestBaseNotifier(t *testing.T) { State: m.AlertStateOK, }) context.Rule.State = m.AlertStateAlerting - So(defaultShouldNotify(context), ShouldBeTrue) + timeNow := time.Now() + So(defaultShouldNotify(context, true, 0, &timeNow), ShouldBeTrue) }) }) }) diff --git a/pkg/services/sqlstore/alert_notification.go b/pkg/services/sqlstore/alert_notification.go index 8bb17143042..a2cfa37ce5c 100644 --- a/pkg/services/sqlstore/alert_notification.go +++ b/pkg/services/sqlstore/alert_notification.go @@ -56,7 +56,9 @@ func GetAlertNotificationsToSend(query *m.GetAlertNotificationsToSendQuery) erro alert_notification.created, alert_notification.updated, alert_notification.settings, - alert_notification.is_default + alert_notification.is_default, + alert_notification.notify_once, + alert_notification.frequency FROM alert_notification `) @@ -94,7 +96,9 @@ func getAlertNotificationInternal(query *m.GetAlertNotificationsQuery, sess *DBS alert_notification.created, alert_notification.updated, alert_notification.settings, - alert_notification.is_default + alert_notification.is_default, + alert_notification.notify_once, + alert_notification.frequency FROM alert_notification `) @@ -140,19 +144,24 @@ func CreateAlertNotificationCommand(cmd *m.CreateAlertNotificationCommand) error return fmt.Errorf("Alert notification name %s already exists", cmd.Name) } + frequency, err_convert := time.ParseDuration(cmd.Frequency) + if err_convert != nil { + return err + } + alertNotification := &m.AlertNotification{ OrgId: cmd.OrgId, Name: cmd.Name, Type: cmd.Type, Settings: cmd.Settings, NotifyOnce: cmd.NotifyOnce, - Frequency: cmd.Frequency, + Frequency: frequency, Created: time.Now(), Updated: time.Now(), IsDefault: cmd.IsDefault, } - if _, err = sess.Insert(alertNotification); err != nil { + if _, err = sess.MustCols("notify_once").Insert(alertNotification); err != nil { return err } @@ -184,8 +193,15 @@ func UpdateAlertNotification(cmd *m.UpdateAlertNotificationCommand) error { current.Name = cmd.Name current.Type = cmd.Type current.IsDefault = cmd.IsDefault + current.NotifyOnce = cmd.NotifyOnce - sess.UseBool("is_default") + frequency, err_convert := time.ParseDuration(cmd.Frequency) + if err_convert != nil { + return err + } + current.Frequency = frequency + + sess.UseBool("is_default", "notify_once") if affected, err := sess.ID(cmd.Id).Update(current); err != nil { return err @@ -219,7 +235,7 @@ func RecordNotificationJournal(cmd *m.RecordNotificationJournalCommand) error { func GetLatestNotification(cmd *m.GetLatestNotificationQuery) error { return inTransaction(func(sess *DBSession) error { notificationJournal := &m.NotificationJournal{} - _, err := sess.OrderBy("notification_journal.sent_at").Desc().Where("notification_journal.org_id = ? AND notification_journal.alert_id = ? AND notification_journal.notifier_id = ?", cmd.OrgId, cmd.AlertId, cmd.NotifierId).Get(notificationJournal) + _, err := sess.Desc("notification_journal.sent_at").Limit(1).Where("notification_journal.org_id = ? AND notification_journal.alert_id = ? AND notification_journal.notifier_id = ?", cmd.OrgId, cmd.AlertId, cmd.NotifierId).Get(notificationJournal) if err != nil { return err } diff --git a/public/app/features/alerting/partials/notification_edit.html b/public/app/features/alerting/partials/notification_edit.html index ccdb9ef1073..dd56564cb95 100644 --- a/public/app/features/alerting/partials/notification_edit.html +++ b/public/app/features/alerting/partials/notification_edit.html @@ -20,8 +20,7 @@
{{ ctrl.model.notifyOnce ? 'Notify on state change' : 'Notify at most every' }} - - seconds +