From 44b7f3ea1c31af9587702d5dbe1138211ed0a8f6 Mon Sep 17 00:00:00 2001 From: Dima Ryskin <8168970+aSapien@users.noreply.github.com> Date: Wed, 18 Mar 2020 15:00:56 +0200 Subject: [PATCH] AlertNotifications: Translate notifications IDs to UIDs in Rule builder (#19882) * AlertNotifications: Translate notifications IDs to UIDs in alert Rule builder * Avoid shadowing errors, raise validation error on non-existing notification id * create a cache for notification Uids to minimize db overhead * add cache usage test * avoid caching empty notification Uids * isolate db in alert notificationUid caching tests --- pkg/models/alert_notifications.go | 7 ++ pkg/services/alerting/rule.go | 34 ++++++++-- pkg/services/alerting/rule_test.go | 43 ++++++++++-- pkg/services/sqlstore/alert_notification.go | 55 ++++++++++++++++ .../sqlstore/alert_notification_test.go | 66 +++++++++++++++++++ pkg/services/sqlstore/sqlstore.go | 1 + 6 files changed, 198 insertions(+), 8 deletions(-) diff --git a/pkg/models/alert_notifications.go b/pkg/models/alert_notifications.go index 9bcee74312c..a4fc4259d78 100644 --- a/pkg/models/alert_notifications.go +++ b/pkg/models/alert_notifications.go @@ -91,6 +91,13 @@ type DeleteAlertNotificationWithUidCommand struct { OrgId int64 } +type GetAlertNotificationUidQuery struct { + Id int64 + OrgId int64 + + Result string +} + type GetAlertNotificationsQuery struct { Name string Id int64 diff --git a/pkg/services/alerting/rule.go b/pkg/services/alerting/rule.go index 4ad1b4a0389..77a79fbbc2e 100644 --- a/pkg/services/alerting/rule.go +++ b/pkg/services/alerting/rule.go @@ -7,6 +7,7 @@ import ( "strconv" "time" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" ) @@ -137,13 +138,15 @@ func NewRuleFromDBAlert(ruleDef *models.Alert) (*Rule, error) { for _, v := range ruleDef.Settings.Get("notifications").MustArray() { jsonModel := simplejson.NewFromAny(v) if id, err := jsonModel.Get("id").Int64(); err == nil { - model.Notifications = append(model.Notifications, fmt.Sprintf("%09d", id)) - } else { - uid, err := jsonModel.Get("uid").String() + uid, err := translateNotificationIDToUID(id, ruleDef.OrgId) if err != nil { - return nil, ValidationError{Reason: "Neither id nor uid is specified in 'notifications' block, " + err.Error(), DashboardID: model.DashboardID, AlertID: model.ID, PanelID: model.PanelID} + return nil, ValidationError{Reason: "Unable to translate notification id to uid, " + err.Error(), DashboardID: model.DashboardID, AlertID: model.ID, PanelID: model.PanelID} } model.Notifications = append(model.Notifications, uid) + } else if uid, err := jsonModel.Get("uid").String(); err == nil { + model.Notifications = append(model.Notifications, uid) + } else { + return nil, ValidationError{Reason: "Neither id nor uid is specified in 'notifications' block, " + err.Error(), DashboardID: model.DashboardID, AlertID: model.ID, PanelID: model.PanelID} } } model.AlertRuleTags = ruleDef.GetTagsFromSettings() @@ -169,6 +172,29 @@ func NewRuleFromDBAlert(ruleDef *models.Alert) (*Rule, error) { return model, nil } +func translateNotificationIDToUID(id int64, orgID int64) (string, error) { + notificationUid, err := getAlertNotificationUidByIDAndOrgID(id, orgID) + if err != nil { + logger.Debug("Failed to translate Notification Id to Uid", "orgID", orgID, "Id", id) + return "", err + } + + return notificationUid, nil +} + +func getAlertNotificationUidByIDAndOrgID(notificationID int64, orgID int64) (string, error) { + query := &models.GetAlertNotificationUidQuery{ + OrgId: orgID, + Id: notificationID, + } + + if err := bus.Dispatch(query); err != nil { + return "", err + } + + return query.Result, nil +} + // ConditionFactory is the function signature for creating `Conditions`. type ConditionFactory func(model *simplejson.Json, index int) (Condition, error) diff --git a/pkg/services/alerting/rule_test.go b/pkg/services/alerting/rule_test.go index 84ce6fbfe3d..f8e44d16302 100644 --- a/pkg/services/alerting/rule_test.go +++ b/pkg/services/alerting/rule_test.go @@ -60,7 +60,7 @@ func TestAlertRuleModel(t *testing.T) { }) Convey("can construct alert rule model", func() { - firstNotification := models.CreateAlertNotificationCommand{OrgId: 1, Name: "1"} + firstNotification := models.CreateAlertNotificationCommand{Uid: "notifier1", OrgId: 1, Name: "1"} err := sqlstore.CreateAlertNotificationCommand(&firstNotification) So(err, ShouldBeNil) secondNotification := models.CreateAlertNotificationCommand{Uid: "notifier2", OrgId: 1, Name: "2"} @@ -105,15 +105,50 @@ func TestAlertRuleModel(t *testing.T) { So(err, ShouldBeNil) So(len(alertRule.Conditions), ShouldEqual, 1) + So(len(alertRule.Notifications), ShouldEqual, 2) - Convey("Can read notifications", func() { - So(len(alertRule.Notifications), ShouldEqual, 2) - So(alertRule.Notifications, ShouldContain, "000000001") + Convey("Can read Id and Uid notifications (translate Id to Uid)", func() { So(alertRule.Notifications, ShouldContain, "notifier2") + So(alertRule.Notifications, ShouldContain, "notifier1") }) }) }) + Convey("with non existing notification id", func() { + json := ` + { + "name": "name3", + "description": "desc3", + "handler": 0, + "noDataMode": "critical", + "enabled": true, + "frequency": "60s", + "conditions": [{"type": "test", "prop": 123 }], + "notifications": [ + {"id": 999} + ] + } + ` + + alertJSON, jsonErr := simplejson.NewJson([]byte(json)) + So(jsonErr, ShouldBeNil) + + alert := &models.Alert{ + Id: 1, + OrgId: 1, + DashboardId: 1, + PanelId: 1, + + Settings: alertJSON, + } + + _, err := NewRuleFromDBAlert(alert) + Convey("raises an error", func() { + So(err, ShouldNotBeNil) + So(err.Error(), ShouldEqual, "Alert validation error: Unable to translate notification id to uid, Alert notification [ Id: 999, OrgId: 1 ] not found AlertId: 1 PanelId: 1 DashboardId: 1") + }) + }) + Convey("can construct alert rule model with invalid frequency", func() { json := ` { diff --git a/pkg/services/sqlstore/alert_notification.go b/pkg/services/sqlstore/alert_notification.go index a75fc50f8c0..bf0bf35cac3 100644 --- a/pkg/services/sqlstore/alert_notification.go +++ b/pkg/services/sqlstore/alert_notification.go @@ -67,6 +67,32 @@ func GetAlertNotifications(query *models.GetAlertNotificationsQuery) error { return getAlertNotificationInternal(query, newSession()) } +func (ss *SqlStore) addAlertNotificationUidByIdHandler() { + bus.AddHandler("sql", ss.GetAlertNotificationUidWithId) +} + +func (ss *SqlStore) GetAlertNotificationUidWithId(query *models.GetAlertNotificationUidQuery) error { + cacheKey := newAlertNotificationUidCacheKey(query.OrgId, query.Id) + + if cached, found := ss.CacheService.Get(cacheKey); found { + query.Result = cached.(string) + return nil + } + + err := getAlertNotificationUidInternal(query, newSession()) + if err != nil { + return err + } + + ss.CacheService.Set(cacheKey, query.Result, -1) //Infinite, never changes + + return nil +} + +func newAlertNotificationUidCacheKey(orgID, notificationId int64) string { + return fmt.Sprintf("notification-uid-by-org-%d-and-id-%d", orgID, notificationId) +} + func GetAlertNotificationsWithUid(query *models.GetAlertNotificationsWithUidQuery) error { return getAlertNotificationWithUidInternal(query, newSession()) } @@ -124,6 +150,35 @@ func GetAlertNotificationsWithUidToSend(query *models.GetAlertNotificationsWithU return nil } +func getAlertNotificationUidInternal(query *models.GetAlertNotificationUidQuery, sess *DBSession) error { + var sql bytes.Buffer + params := make([]interface{}, 0) + + sql.WriteString(`SELECT + alert_notification.uid + FROM alert_notification + `) + + sql.WriteString(` WHERE alert_notification.org_id = ?`) + params = append(params, query.OrgId) + + sql.WriteString(` AND alert_notification.id = ?`) + params = append(params, query.Id) + + results := make([]string, 0) + if err := sess.SQL(sql.String(), params...).Find(&results); err != nil { + return err + } + + if len(results) == 0 { + return fmt.Errorf("Alert notification [ Id: %v, OrgId: %v ] not found", query.Id, query.OrgId) + } + + query.Result = results[0] + + return nil +} + func getAlertNotificationInternal(query *models.GetAlertNotificationsQuery, sess *DBSession) error { var sql bytes.Buffer params := make([]interface{}, 0) diff --git a/pkg/services/sqlstore/alert_notification_test.go b/pkg/services/sqlstore/alert_notification_test.go index 91b84cb91d0..75d55820226 100644 --- a/pkg/services/sqlstore/alert_notification_test.go +++ b/pkg/services/sqlstore/alert_notification_test.go @@ -321,5 +321,71 @@ func TestAlertNotificationSQLAccess(t *testing.T) { So(len(query.Result), ShouldEqual, 4) }) }) + + Convey("Notification Uid by Id Caching", func() { + ss := InitTestDB(t) + + notification := &models.CreateAlertNotificationCommand{Uid: "aNotificationUid", OrgId: 1, Name: "aNotificationUid"} + err := CreateAlertNotificationCommand(notification) + So(err, ShouldBeNil) + + byUidQuery := &models.GetAlertNotificationsWithUidQuery{ + Uid: notification.Uid, + OrgId: notification.OrgId, + } + + notificationByUidErr := GetAlertNotificationsWithUid(byUidQuery) + So(notificationByUidErr, ShouldBeNil) + + Convey("Can cache notification Uid", func() { + byIdQuery := &models.GetAlertNotificationUidQuery{ + Id: byUidQuery.Result.Id, + OrgId: byUidQuery.Result.OrgId, + } + + cacheKey := newAlertNotificationUidCacheKey(byIdQuery.OrgId, byIdQuery.Id) + + resultBeforeCaching, foundBeforeCaching := ss.CacheService.Get(cacheKey) + So(foundBeforeCaching, ShouldBeFalse) + So(resultBeforeCaching, ShouldBeNil) + + notificationByIdErr := ss.GetAlertNotificationUidWithId(byIdQuery) + So(notificationByIdErr, ShouldBeNil) + + resultAfterCaching, foundAfterCaching := ss.CacheService.Get(cacheKey) + So(foundAfterCaching, ShouldBeTrue) + So(resultAfterCaching, ShouldEqual, notification.Uid) + }) + + Convey("Retrieves from cache when exists", func() { + query := &models.GetAlertNotificationUidQuery{ + Id: 999, + OrgId: 100, + } + cacheKey := newAlertNotificationUidCacheKey(query.OrgId, query.Id) + ss.CacheService.Set(cacheKey, "a-cached-uid", -1) + + err := ss.GetAlertNotificationUidWithId(query) + So(err, ShouldBeNil) + So(query.Result, ShouldEqual, "a-cached-uid") + }) + + Convey("Returns an error without populating cache when the notification doesn't exist in the database", func() { + query := &models.GetAlertNotificationUidQuery{ + Id: -1, + OrgId: 100, + } + + err := ss.GetAlertNotificationUidWithId(query) + So(query.Result, ShouldEqual, "") + So(err, ShouldNotBeNil) + So(err.Error(), ShouldEqual, "Alert notification [ Id: -1, OrgId: 100 ] not found") + + cacheKey := newAlertNotificationUidCacheKey(query.OrgId, query.Id) + result, found := ss.CacheService.Get(cacheKey) + So(found, ShouldBeFalse) + So(result, ShouldBeNil) + }) + }) }) } diff --git a/pkg/services/sqlstore/sqlstore.go b/pkg/services/sqlstore/sqlstore.go index dd75c09f4fe..b0023e0d841 100644 --- a/pkg/services/sqlstore/sqlstore.go +++ b/pkg/services/sqlstore/sqlstore.go @@ -99,6 +99,7 @@ func (ss *SqlStore) Init() error { // Register handlers ss.addUserQueryAndCommandHandlers() + ss.addAlertNotificationUidByIdHandler() if ss.skipEnsureDefaultOrgAndUser { return nil