From 55b560a4a8610094168180b1a3830e1633fd6eab Mon Sep 17 00:00:00 2001 From: bergquist Date: Tue, 6 Sep 2016 13:19:05 +0200 Subject: [PATCH 1/6] feat(alerting): filter notifications based on severity closes #5851 --- pkg/services/alerting/interfaces.go | 7 +- pkg/services/alerting/notifier.go | 28 ++- pkg/services/alerting/notifier_test.go | 192 ++++++++---------- pkg/services/alerting/notifiers/base.go | 30 ++- pkg/services/alerting/notifiers/base_test.go | 36 ++++ pkg/services/alerting/notifiers/common.go | 1 - pkg/services/alerting/notifiers/email.go | 9 +- pkg/services/alerting/notifiers/slack.go | 9 +- pkg/services/alerting/notifiers/webhook.go | 13 +- pkg/services/alerting/test_notification.go | 2 +- .../alerting/notification_edit_ctrl.ts | 4 +- .../alerting/partials/notification_edit.html | 19 +- 12 files changed, 202 insertions(+), 148 deletions(-) create mode 100644 pkg/services/alerting/notifiers/base_test.go delete mode 100644 pkg/services/alerting/notifiers/common.go diff --git a/pkg/services/alerting/interfaces.go b/pkg/services/alerting/interfaces.go index 9688aba153a..e2549da2365 100644 --- a/pkg/services/alerting/interfaces.go +++ b/pkg/services/alerting/interfaces.go @@ -1,6 +1,10 @@ package alerting -import "time" +import ( + "time" + + "github.com/grafana/grafana/pkg/models" +) type EvalHandler interface { Eval(context *EvalContext) @@ -15,6 +19,7 @@ type Notifier interface { Notify(alertResult *EvalContext) GetType() string NeedsImage() bool + MatchSeverity(result models.AlertSeverityType) bool } type Condition interface { diff --git a/pkg/services/alerting/notifier.go b/pkg/services/alerting/notifier.go index b4b5cd819ff..9a81501833d 100644 --- a/pkg/services/alerting/notifier.go +++ b/pkg/services/alerting/notifier.go @@ -28,10 +28,14 @@ func (n *RootNotifier) NeedsImage() bool { return false } +func (n *RootNotifier) MatchSeverity(result m.AlertSeverityType) bool { + return false +} + func (n *RootNotifier) Notify(context *EvalContext) { n.log.Info("Sending notifications for", "ruleId", context.Rule.Id) - notifiers, err := n.getNotifiers(context.Rule.OrgId, context.Rule.Notifications) + notifiers, err := n.getNotifiers(context.Rule.OrgId, context.Rule.Notifications, context) if err != nil { n.log.Error("Failed to read notifications", "error", err) return @@ -87,7 +91,7 @@ func (n *RootNotifier) uploadImage(context *EvalContext) error { return nil } -func (n *RootNotifier) getNotifiers(orgId int64, notificationIds []int64) ([]Notifier, error) { +func (n *RootNotifier) getNotifiers(orgId int64, notificationIds []int64, context *EvalContext) ([]Notifier, error) { query := &m.GetAlertNotificationsToSendQuery{OrgId: orgId, Ids: notificationIds} if err := bus.Dispatch(query); err != nil { @@ -96,17 +100,19 @@ func (n *RootNotifier) getNotifiers(orgId int64, notificationIds []int64) ([]Not var result []Notifier for _, notification := range query.Result { - if not, err := n.getNotifierFor(notification); err != nil { + if not, err := n.createNotifierFor(notification); err != nil { return nil, err } else { - result = append(result, not) + if shouldUseNotification(not, context) { + result = append(result, not) + } } } return result, nil } -func (n *RootNotifier) getNotifierFor(model *m.AlertNotification) (Notifier, error) { +func (n *RootNotifier) createNotifierFor(model *m.AlertNotification) (Notifier, error) { factory, found := notifierFactories[model.Type] if !found { return nil, errors.New("Unsupported notification type") @@ -115,6 +121,18 @@ func (n *RootNotifier) getNotifierFor(model *m.AlertNotification) (Notifier, err return factory(model) } +func shouldUseNotification(notifier Notifier, context *EvalContext) bool { + if !context.Firing { + return true + } + + if context.Error != nil { + return true + } + + return notifier.MatchSeverity(context.Rule.Severity) +} + type NotifierFactory func(notification *m.AlertNotification) (Notifier, error) var notifierFactories map[string]NotifierFactory = make(map[string]NotifierFactory) diff --git a/pkg/services/alerting/notifier_test.go b/pkg/services/alerting/notifier_test.go index ebe305c6174..1354177f9b3 100644 --- a/pkg/services/alerting/notifier_test.go +++ b/pkg/services/alerting/notifier_test.go @@ -1,114 +1,82 @@ package alerting -// func TestAlertNotificationExtraction(t *testing.T) { -// Convey("Notifier tests", t, func() { -// Convey("rules for sending notifications", func() { -// dummieNotifier := NotifierImpl{} -// -// result := &AlertResult{ -// State: alertstates.Critical, -// } -// -// notifier := &Notification{ -// Name: "Test Notifier", -// Type: "TestType", -// SendCritical: true, -// SendWarning: true, -// } -// -// Convey("Should send notification", func() { -// So(dummieNotifier.ShouldDispath(result, notifier), ShouldBeTrue) -// }) -// -// Convey("warn:false and state:warn should not send", func() { -// result.State = alertstates.Warn -// notifier.SendWarning = false -// So(dummieNotifier.ShouldDispath(result, notifier), ShouldBeFalse) -// }) -// }) -// -// Convey("Parsing alert notification from settings", func() { -// Convey("Parsing email", func() { -// Convey("empty settings should return error", func() { -// json := `{ }` -// -// settingsJSON, _ := simplejson.NewJson([]byte(json)) -// model := &m.AlertNotification{ -// Name: "ops", -// Type: "email", -// Settings: settingsJSON, -// } -// -// _, err := NewNotificationFromDBModel(model) -// So(err, ShouldNotBeNil) -// }) -// -// Convey("from settings", func() { -// json := ` -// { -// "to": "ops@grafana.org" -// }` -// -// settingsJSON, _ := simplejson.NewJson([]byte(json)) -// model := &m.AlertNotification{ -// Name: "ops", -// Type: "email", -// Settings: settingsJSON, -// } -// -// not, err := NewNotificationFromDBModel(model) -// -// So(err, ShouldBeNil) -// So(not.Name, ShouldEqual, "ops") -// So(not.Type, ShouldEqual, "email") -// So(reflect.TypeOf(not.Notifierr).Elem().String(), ShouldEqual, "alerting.EmailNotifier") -// -// email := not.Notifierr.(*EmailNotifier) -// So(email.To, ShouldEqual, "ops@grafana.org") -// }) -// }) -// -// Convey("Parsing webhook", func() { -// Convey("empty settings should return error", func() { -// json := `{ }` -// -// settingsJSON, _ := simplejson.NewJson([]byte(json)) -// model := &m.AlertNotification{ -// Name: "ops", -// Type: "webhook", -// Settings: settingsJSON, -// } -// -// _, err := NewNotificationFromDBModel(model) -// So(err, ShouldNotBeNil) -// }) -// -// Convey("from settings", func() { -// json := ` -// { -// "url": "http://localhost:3000", -// "username": "username", -// "password": "password" -// }` -// -// settingsJSON, _ := simplejson.NewJson([]byte(json)) -// model := &m.AlertNotification{ -// Name: "slack", -// Type: "webhook", -// Settings: settingsJSON, -// } -// -// not, err := NewNotificationFromDBModel(model) -// -// So(err, ShouldBeNil) -// So(not.Name, ShouldEqual, "slack") -// So(not.Type, ShouldEqual, "webhook") -// So(reflect.TypeOf(not.Notifierr).Elem().String(), ShouldEqual, "alerting.WebhookNotifier") -// -// webhook := not.Notifierr.(*WebhookNotifier) -// So(webhook.Url, ShouldEqual, "http://localhost:3000") -// }) -// }) -// }) -// }) -// } +import ( + "testing" + + "fmt" + + "github.com/grafana/grafana/pkg/models" + . "github.com/smartystreets/goconvey/convey" +) + +type FakeNotifier struct { + FakeMatchResult bool +} + +func (fn *FakeNotifier) GetType() string { + return "FakeNotifier" +} + +func (fn *FakeNotifier) NeedsImage() bool { + return true +} + +func (fn *FakeNotifier) Notify(alertResult *EvalContext) {} + +func (fn *FakeNotifier) MatchSeverity(result models.AlertSeverityType) bool { + return fn.FakeMatchResult +} + +func TestAlertNotificationExtraction(t *testing.T) { + + Convey("Notifier tests", t, func() { + Convey("none firing alerts", func() { + ctx := &EvalContext{ + Firing: false, + Rule: &Rule{ + Severity: models.AlertSeverityCritical, + }, + } + notifier := &FakeNotifier{FakeMatchResult: false} + + So(shouldUseNotification(notifier, ctx), ShouldBeTrue) + }) + + Convey("exeuction error cannot be ignored", func() { + ctx := &EvalContext{ + Firing: true, + Error: fmt.Errorf("I used to be a programmer just like you"), + Rule: &Rule{ + Severity: models.AlertSeverityCritical, + }, + } + notifier := &FakeNotifier{FakeMatchResult: false} + + So(shouldUseNotification(notifier, ctx), ShouldBeTrue) + }) + + Convey("firing alert that match", func() { + ctx := &EvalContext{ + Firing: true, + Rule: &Rule{ + Severity: models.AlertSeverityCritical, + }, + } + notifier := &FakeNotifier{FakeMatchResult: true} + + So(shouldUseNotification(notifier, ctx), ShouldBeTrue) + }) + + Convey("firing alert that dont match", func() { + ctx := &EvalContext{ + Firing: true, + Rule: &Rule{ + Severity: models.AlertSeverityCritical, + }, + } + notifier := &FakeNotifier{FakeMatchResult: false} + + So(shouldUseNotification(notifier, ctx), ShouldBeFalse) + }) + }) +} diff --git a/pkg/services/alerting/notifiers/base.go b/pkg/services/alerting/notifiers/base.go index 48fe6c4eaa5..acfafd59ac4 100644 --- a/pkg/services/alerting/notifiers/base.go +++ b/pkg/services/alerting/notifiers/base.go @@ -1,8 +1,34 @@ package notifiers +import ( + "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/models" +) + type NotifierBase struct { - Name string - Type string + Name string + Type string + SeverityFilter models.AlertSeverityType +} + +func NewNotifierBase(name, notifierType string, model *simplejson.Json) NotifierBase { + base := NotifierBase{Name: name, Type: notifierType} + + severityFilter := models.AlertSeverityType(model.Get("severityFilter").MustString("")) + + if severityFilter == models.AlertSeverityCritical || severityFilter == models.AlertSeverityWarning { + base.SeverityFilter = severityFilter + } + + return base +} + +func (n *NotifierBase) MatchSeverity(result models.AlertSeverityType) bool { + if !n.SeverityFilter.IsValid() { + return true + } + + return n.SeverityFilter == result } func (n *NotifierBase) GetType() string { diff --git a/pkg/services/alerting/notifiers/base_test.go b/pkg/services/alerting/notifiers/base_test.go new file mode 100644 index 00000000000..af5dd902c87 --- /dev/null +++ b/pkg/services/alerting/notifiers/base_test.go @@ -0,0 +1,36 @@ +package notifiers + +import ( + "testing" + + "github.com/grafana/grafana/pkg/components/simplejson" + m "github.com/grafana/grafana/pkg/models" + . "github.com/smartystreets/goconvey/convey" +) + +func TestBaseNotifier(t *testing.T) { + Convey("Parsing base notification severity", t, func() { + + Convey("matches", func() { + json := ` + { + "severityFilter": "critical" + }` + + settingsJSON, _ := simplejson.NewJson([]byte(json)) + not := NewNotifierBase("ops", "email", settingsJSON) + So(not.MatchSeverity(m.AlertSeverityCritical), ShouldBeTrue) + }) + + Convey("does not match", func() { + json := ` + { + "severityFilter": "critical" + }` + + settingsJSON, _ := simplejson.NewJson([]byte(json)) + not := NewNotifierBase("ops", "email", settingsJSON) + So(not.MatchSeverity(m.AlertSeverityWarning), ShouldBeFalse) + }) + }) +} diff --git a/pkg/services/alerting/notifiers/common.go b/pkg/services/alerting/notifiers/common.go deleted file mode 100644 index 48b634c44d7..00000000000 --- a/pkg/services/alerting/notifiers/common.go +++ /dev/null @@ -1 +0,0 @@ -package notifiers diff --git a/pkg/services/alerting/notifiers/email.go b/pkg/services/alerting/notifiers/email.go index d63ca7d13f5..c754dd7985e 100644 --- a/pkg/services/alerting/notifiers/email.go +++ b/pkg/services/alerting/notifiers/email.go @@ -29,12 +29,9 @@ func NewEmailNotifier(model *m.AlertNotification) (alerting.Notifier, error) { } return &EmailNotifier{ - NotifierBase: NotifierBase{ - Name: model.Name, - Type: model.Type, - }, - Addresses: strings.Split(addressesString, "\n"), - log: log.New("alerting.notifier.email"), + NotifierBase: NewNotifierBase(model.Name, model.Type, model.Settings), + Addresses: strings.Split(addressesString, "\n"), + log: log.New("alerting.notifier.email"), }, nil } diff --git a/pkg/services/alerting/notifiers/slack.go b/pkg/services/alerting/notifiers/slack.go index 837621e80bb..d0d67ca5a88 100644 --- a/pkg/services/alerting/notifiers/slack.go +++ b/pkg/services/alerting/notifiers/slack.go @@ -23,12 +23,9 @@ func NewSlackNotifier(model *m.AlertNotification) (alerting.Notifier, error) { } return &SlackNotifier{ - NotifierBase: NotifierBase{ - Name: model.Name, - Type: model.Type, - }, - Url: url, - log: log.New("alerting.notifier.slack"), + NotifierBase: NewNotifierBase(model.Name, model.Type, model.Settings), + Url: url, + log: log.New("alerting.notifier.slack"), }, nil } diff --git a/pkg/services/alerting/notifiers/webhook.go b/pkg/services/alerting/notifiers/webhook.go index e725752778f..9a66d18c73c 100644 --- a/pkg/services/alerting/notifiers/webhook.go +++ b/pkg/services/alerting/notifiers/webhook.go @@ -20,14 +20,11 @@ func NewWebHookNotifier(model *m.AlertNotification) (alerting.Notifier, error) { } return &WebhookNotifier{ - NotifierBase: NotifierBase{ - Name: model.Name, - Type: model.Type, - }, - Url: url, - User: model.Settings.Get("user").MustString(), - Password: model.Settings.Get("password").MustString(), - log: log.New("alerting.notifier.webhook"), + NotifierBase: NewNotifierBase(model.Name, model.Type, model.Settings), + Url: url, + User: model.Settings.Get("user").MustString(), + Password: model.Settings.Get("password").MustString(), + log: log.New("alerting.notifier.webhook"), }, nil } diff --git a/pkg/services/alerting/test_notification.go b/pkg/services/alerting/test_notification.go index 026dfdd18a5..c3fbaeb4f1c 100644 --- a/pkg/services/alerting/test_notification.go +++ b/pkg/services/alerting/test_notification.go @@ -28,7 +28,7 @@ func handleNotificationTestCommand(cmd *NotificationTestCommand) error { Settings: cmd.Settings, } - notifiers, err := notifier.getNotifierFor(model) + notifiers, err := notifier.createNotifierFor(model) if err != nil { log.Error2("Failed to create notifier", "error", err.Error()) diff --git a/public/app/features/alerting/notification_edit_ctrl.ts b/public/app/features/alerting/notification_edit_ctrl.ts index d465a9021a6..3ef9e822333 100644 --- a/public/app/features/alerting/notification_edit_ctrl.ts +++ b/public/app/features/alerting/notification_edit_ctrl.ts @@ -17,7 +17,9 @@ export class AlertNotificationEditCtrl { } else { this.model = { type: 'email', - settings: {}, + settings: { + severityFilter: 'none' + }, isDefault: false }; } diff --git a/public/app/features/alerting/partials/notification_edit.html b/public/app/features/alerting/partials/notification_edit.html index c9f181eacf9..59f4607a2e7 100644 --- a/public/app/features/alerting/partials/notification_edit.html +++ b/public/app/features/alerting/partials/notification_edit.html @@ -12,11 +12,11 @@
- Name + Name
- Type + Type
+ +
+
@@ -91,5 +100,5 @@
- + From 22805ce56bd2bc89bb23a05d83c925771e46b2b3 Mon Sep 17 00:00:00 2001 From: bergquist Date: Tue, 6 Sep 2016 14:42:34 +0200 Subject: [PATCH 2/6] fix(alert_tab): fix broken alert history --- public/app/features/alerting/alert_tab_ctrl.ts | 7 +++++++ public/app/features/alerting/partials/alert_tab.html | 6 +++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/public/app/features/alerting/alert_tab_ctrl.ts b/public/app/features/alerting/alert_tab_ctrl.ts index d10265a3ab7..c0cb0936f62 100644 --- a/public/app/features/alerting/alert_tab_ctrl.ts +++ b/public/app/features/alerting/alert_tab_ctrl.ts @@ -106,6 +106,13 @@ export class AlertTabCtrl { })); } + changeTabIndex(newTabIndex) { + this.subTabIndex = newTabIndex; + + if (this.subTabIndex === 2) { + this.getAlertHistory(); + } + } notificationAdded() { var model = _.findWhere(this.notifications, {name: this.addNotificationSegment.value}); diff --git a/public/app/features/alerting/partials/alert_tab.html b/public/app/features/alerting/partials/alert_tab.html index 85919ef8a63..db2fa978869 100644 --- a/public/app/features/alerting/partials/alert_tab.html +++ b/public/app/features/alerting/partials/alert_tab.html @@ -2,15 +2,15 @@