AlertNotifications: Translate notifications IDs to UIDs in Rule builder #19882

aSapien · 2019-10-17T14:34:24Z

What this PR does / why we need it:
Fixes alert notifications for dashboards that were saved with notification IDs

Which issue(s) this PR fixes:
Fixes #19771

Special notes for your reviewer:
I wasn't sure if rule.go is the correct place for this fix, but it seemed the easiest to put there. Please let me know if there's a better approach to fixing this.

Bug trace:
Currently, IDs are parsed, converted to strings and included in the same slice (of UIDs):

grafana/pkg/services/alerting/rule.go

Lines 139 to 147 in c674fa1

    
           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() 
        
           	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} 
        
           	} 
        
           	model.Notifications = append(model.Notifications, uid) 
        
           }

In a later stage they are used in

grafana/pkg/services/alerting/notifier.go

Lines 161 to 162 in c674fa1

    
           func (n *notificationService) getNeededNotifiers(orgID int64, notificationUids []string, evalContext *EvalContext) (notifierStateSlice, error) { 
        
           	query := &models.GetAlertNotificationsWithUidToSendQuery{OrgId: orgID, Uids: notificationUids}

Which in turn triggers a query, however, the SQL query expects UIDs and doesn't consider "stringified" IDs:

grafana/pkg/services/sqlstore/alert_notification.go

Line 111 in c674fa1

    
           sql.WriteString(` OR alert_notification.uid IN (?` + strings.Repeat(",?", len(query.Uids)-1) + ")")

This causes notification entities for IDs to not be returned and therefore to not be triggered.

… builder

CLAassistant · 2019-10-17T14:34:54Z

All committers have signed the CLA.

aSapien · 2019-10-17T14:52:42Z

@torkelo

torkelo · 2019-10-22T08:32:22Z

How are you getting alert rules with notification ids? new rules should use uids. Only legacy rules should still use ids, and we convert those to uids same way we migrated them in the DB

aSapien · 2019-10-22T08:52:45Z

@torkelo We have system that generates dashboards with ID based notification channels

aSapien · 2019-10-22T09:03:20Z

Is there a migration for dashboards that point to notifications by the old IDs? I'm only seeing the migration of the alerts_notifications table:

grafana/pkg/services/sqlstore/migrations/alert_mig.go

Lines 155 to 159 in 7a8f2cd

    
           mg.AddMigration("Add column uid in alert_notification", NewAddColumnMigration(alert_notification, &Column{ 
        
           	Name: "uid", Type: DB_NVarchar, Length: 40, Nullable: true, 
        
           })) 
        
           mg.AddMigration("Update uid column values in alert_notification", new(RawSqlMigration).

stale · 2019-12-12T00:30:25Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Tim-Schwalbe · 2020-01-06T08:44:07Z

any plans to solve this issue on the code base? I think a lot of people will run into this problem.
I have updated from 6.0.2 and luckily recognize the issue, but it could be a huge trap for others.

I am also not sure how to solve the issue on for all dashboards. can someone point out which ID, UID need to be used for the migration?

stale · 2020-01-20T09:28:39Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

papagian

Thank you for your contribution!
Please check my comments for some suggested improvements.

pkg/services/alerting/rule.go

pkg/services/alerting/rule_test.go

pkg/services/alerting/rule.go

aSapien · 2020-02-06T12:26:37Z

@papagian thank you for the comments. I'll try to complete the needed changes soon.

…cation id

papagian

Great! Thanks for the modifications.
I have a minor comment.

pkg/services/alerting/rule.go

papagian

Great!
I have a minor request before merging, please check my inline comment!

Additionally, it would be nice to do some UID caching.
Therefore I would recommend to change the GetAlertNotifications to save the result in the cache and check whether it exists in the cache before running the actual query. Here you can find an example of using the cache.

pkg/services/alerting/rule.go

aSapien · 2020-02-13T08:55:09Z

@papagian are you referring to caching the id->uid translation result? This makes perfect sense.

papagian · 2020-02-13T09:04:25Z

@papagian are you referring to caching the id->uid translation result? This makes perfect sense.

yes! sorry for not being clear

aSapien · 2020-02-13T09:05:16Z

great! I was concerned about caching values that might change in runtime 😅

aSapien · 2020-02-21T08:12:32Z

@papagian I'm not sure why mysql-integration-test failed but it looks like a timeout issue. Is there a way to run it locally? I can't re-run from CircleCI

papagian · 2020-02-24T09:51:46Z

@aSapien
It seems to be a timeout. I have rerun the CI workflow and hopefully it will be ok.
For your information for running mysql tests you can try:
./scripts/circle-test-mysql.sh
which actually sets environmental variable GRAFANA_TEST_DB=mysql and on top of this using the integration build tag for running integration tests.

aSapien · 2020-02-24T14:04:16Z

@papagian Yep, it's fine now, but failing on grafana-docker-ubuntu-pr which has nothing to do with this PR. Please review :)

papagian · 2020-02-24T14:08:16Z

@aSapien
FYI there is a fix for grafana-docker-ubuntu-pr in the master branch so for overcoming it you need to sync your branch with the master.
But I will review it anyway tomorrow.

papagian

Great work! Thank you for the modifications.
I have some suggestions for improving the tests.
Let me know if you want to work on them or you would prefer me to do it.

papagian · 2020-02-26T16:33:19Z

pkg/services/sqlstore/alert_notification_test.go

@@ -321,5 +321,54 @@ func TestAlertNotificationSQLAccess(t *testing.T) {
 				So(len(query.Result), ShouldEqual, 4)
 			})
 		})
+
+		Convey("Notification Uid by Id Caching", func() {
+			ss.CacheService.Flush()


I would prefer mocking the actual cache (and reverting after execution) instead of flushing:

actualCache := ss.CacheService ss.CacheService = localcache.New(5*time.Minute, 10*time.Minute) defer func() { ss.CacheService = actualCache }()

@papagian I saw that some tests initialize a separate TestDb for isolated test scopes. Perhaps I should use this technique instead? I like it because it's less verbose.

grafana/pkg/services/sqlstore/user_test.go

Lines 313 to 315 in 5aeb367

Convey("Should enable all users", func() {

ss = InitTestDB(t)

createFiveTestUsers(func(i int) *models.CreateUserCommand {

Then there's not even a need to flush :)

yes, you can do that instead

Not that important but my last impression was that you would use a separate test database for these tests and you would remove flushing.

pkg/services/sqlstore/alert_notification_test.go

aSapien · 2020-02-27T13:23:59Z

@papagian thanks for the comments! I'll do the modifications. They help me learn idiomatic Golang :)

papagian

The modifications look good to me only check my last comment.

papagian · 2020-03-13T12:12:51Z

pkg/services/sqlstore/alert_notification_test.go

@@ -321,5 +321,54 @@ func TestAlertNotificationSQLAccess(t *testing.T) {
 				So(len(query.Result), ShouldEqual, 4)
 			})
 		})
+
+		Convey("Notification Uid by Id Caching", func() {
+			ss.CacheService.Flush()


Not that important but my last impression was that you would use a separate test database for these tests and you would remove flushing.

aSapien · 2020-03-15T08:31:35Z

@papagian Thanks for pointing it out! It must've slipped my mind somehow... I made the necessary change

papagian · 2020-03-17T08:32:58Z

Thank you for contributing!

papagian · 2020-03-17T09:03:11Z

@aSapien could you synchronise your branch with master in order to trigger running the new build-pipeline workflow and be able to merge it?
I can do it myself if you prefer but I'm wasn't sure since the PR is targeting your repository's master.

aSapien · 2020-03-18T11:44:09Z

@papagian yeah working on a fork's master was poor choice. I synced the branches :)

* master: (113 commits) AlertNotifications: Translate notifications IDs to UIDs in Rule builder (#19882) CircleCI: Don't build Storybook on PR (#22865) Graphite: Rollup Indicator (#22738) Plugins: Return jsondetails as an json object instead of raw json on datasource healthchecks. (#22859) Backend plugins: Exclude plugin metrics in Grafana's metrics endpoint (#22857) Graphite: Fixed issue with query editor and next select metric now showing after selecting metric node (#22856) Webpack: Fix webpack for enterprise (#22863) Metrics: Storybook documented components (#22854) Search: Improve tags layout , #22804 (#22830) Stackdriver: Fix GCE auth bug when creating new data source (#22836) @grafana/runtime: Add cancellation of queries to DataSourceWithBackend (#22818) Rich history: Test coverage (#22852) Chore: Support Volta in package.json (#22849) CircleCI: Skip enterprise builds for forked PRs (#22851) Toolkit: docker image for plugin CI process (#22790) Revert "Explore: Add test coverage for Rich history (#22722)" (#22850) Datasource config was not mapped for datasource healthcheck (#22848) upgrades plugin sdk to 0.30.0 (#22846) Explore: Add test coverage for Rich history (#22722) Rich History: UX adjustments and fixes (#22729) ...

* master: (98 commits) NewPanelEdit: Refactor value mappings UI to work better with new panel edit (grafana#22808) FieldOverrides: Move FieldConfigSource from fieldOptions to PanelModel.fieldConfig (grafana#22600) Reporting: Update docs with correct logger name (grafana#22892) Design tweaks (grafana#22886) Remove duplicated localStorage mock (grafana#22872) Rich history UX fixes (grafana#22783) Storybook: Solve deployment issues (grafana#22873) Docs: Update templating.md (grafana#22881) AzureMonitor: support workspaces function for template variables (grafana#22882) Check if the datasource is of type loki using meta.id instead of name. (grafana#22877) Prometheus: Render missing labels in legend formats as an empty string (grafana#22355) BarGauge: Fix strict null that breaks Storybook build (grafana#22871) SQLStore: Add migration for adding index on annotation.alert_id (grafana#22876) Docs: Update export-pdf.md (grafana#22767) Variables: adds missing feature toggle in DashboardModel (grafana#22868) Devenv: adds grafana block with a customizeable version (grafana#22867) Alerting: support alerting on data.Frame (that can be time series) (grafana#22812) AlertNotifications: Translate notifications IDs to UIDs in Rule builder (grafana#19882) CircleCI: Don't build Storybook on PR (grafana#22865) Graphite: Rollup Indicator (grafana#22738) ...

AlertNotifications: Translate notifications IDs to UIDs in alert Rule…

6cd001f

… builder

marefr added the area/backend label Oct 30, 2019

papagian added the pr/external This PR is from external contributor label Oct 31, 2019

stale bot added the stale Issue with no recent activity label Dec 12, 2019

stale bot removed the stale Issue with no recent activity label Jan 6, 2020

stale bot added the stale Issue with no recent activity label Jan 20, 2020

marefr requested a review from papagian January 29, 2020 14:30

stale bot removed the stale Issue with no recent activity label Jan 29, 2020

papagian requested changes Feb 5, 2020

View reviewed changes

pkg/services/alerting/rule.go Outdated Show resolved Hide resolved

pkg/services/alerting/rule.go Outdated Show resolved Hide resolved

pkg/services/alerting/rule_test.go Show resolved Hide resolved

pkg/services/alerting/rule.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master'

d8b5d27

Avoid shadowing errors, raise validation error on non-existing notifi…

7ffe73d

…cation id

aSapien requested a review from papagian February 9, 2020 14:24

papagian requested changes Feb 10, 2020

View reviewed changes

pkg/services/alerting/rule.go Outdated Show resolved Hide resolved

inline notification uid extraction

1a3d084

aSapien requested a review from papagian February 10, 2020 14:50

lint fix

44e1409

papagian requested changes Feb 12, 2020

View reviewed changes

pkg/services/alerting/rule.go Outdated Show resolved Hide resolved

reduce log level, remove redundant formatting

48a3d07

Merge remote-tracking branch 'upstream/master'

7fb6d2c

papagian requested changes Feb 26, 2020

View reviewed changes

avoid caching empty notification Uids

a77c55e

aSapien requested a review from a team as a code owner March 11, 2020 11:40

Merge remote-tracking branch 'upstream/master'

46c145a

bergquist assigned papagian Mar 11, 2020

papagian reviewed Mar 13, 2020

View reviewed changes

isolate db in alert notificationUid caching tests

4e89ce3

papagian approved these changes Mar 17, 2020

View reviewed changes

papagian added this to the 7.0 milestone Mar 17, 2020

papagian added the add to changelog label Mar 17, 2020

aknuds1 closed this Mar 17, 2020

aknuds1 reopened this Mar 17, 2020

Merge remote-tracking branch 'upstream/master'

02660ed

papagian merged commit 44b7f3e into grafana:master Mar 18, 2020

marefr added the type/bug label Apr 28, 2020

kylebrandt mentioned this pull request May 21, 2020

Unable to translate notification id to uid #24969

Closed

This was referenced May 22, 2020

Cannot submit dashboard error kevchu3/openshift4-grafana#1

Closed

Reconciler error for Grafana CR grafana/grafana-operator#184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AlertNotifications: Translate notifications IDs to UIDs in Rule builder #19882

AlertNotifications: Translate notifications IDs to UIDs in Rule builder #19882

aSapien commented Oct 17, 2019 •

edited

Loading

CLAassistant commented Oct 17, 2019 •

edited

Loading

aSapien commented Oct 17, 2019

torkelo commented Oct 22, 2019

aSapien commented Oct 22, 2019

aSapien commented Oct 22, 2019

stale bot commented Dec 12, 2019

Tim-Schwalbe commented Jan 6, 2020 •

edited

Loading

stale bot commented Jan 20, 2020

papagian left a comment

aSapien commented Feb 6, 2020

papagian left a comment

papagian left a comment

aSapien commented Feb 13, 2020

papagian commented Feb 13, 2020

aSapien commented Feb 13, 2020

aSapien commented Feb 21, 2020

papagian commented Feb 24, 2020

aSapien commented Feb 24, 2020

papagian commented Feb 24, 2020 •

edited

Loading

papagian left a comment

papagian Feb 26, 2020

aSapien Mar 10, 2020

aSapien Mar 10, 2020

papagian Mar 10, 2020

papagian Mar 13, 2020

aSapien commented Feb 27, 2020

papagian left a comment

papagian Mar 13, 2020

aSapien commented Mar 15, 2020

papagian commented Mar 17, 2020

papagian commented Mar 17, 2020

aSapien commented Mar 18, 2020

	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()
	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}
	}
	model.Notifications = append(model.Notifications, uid)
	}

	func (n notificationService) getNeededNotifiers(orgID int64, notificationUids []string, evalContext EvalContext) (notifierStateSlice, error) {
	query := &models.GetAlertNotificationsWithUidToSendQuery{OrgId: orgID, Uids: notificationUids}

	Convey("Should enable all users", func() {
	ss = InitTestDB(t)
	createFiveTestUsers(func(i int) *models.CreateUserCommand {

AlertNotifications: Translate notifications IDs to UIDs in Rule builder #19882

AlertNotifications: Translate notifications IDs to UIDs in Rule builder #19882

Conversation

aSapien commented Oct 17, 2019 • edited Loading

CLAassistant commented Oct 17, 2019 • edited Loading

aSapien commented Oct 17, 2019

torkelo commented Oct 22, 2019

aSapien commented Oct 22, 2019

aSapien commented Oct 22, 2019

stale bot commented Dec 12, 2019

Tim-Schwalbe commented Jan 6, 2020 • edited Loading

stale bot commented Jan 20, 2020

papagian left a comment

Choose a reason for hiding this comment

aSapien commented Feb 6, 2020

papagian left a comment

Choose a reason for hiding this comment

papagian left a comment

Choose a reason for hiding this comment

aSapien commented Feb 13, 2020

papagian commented Feb 13, 2020

aSapien commented Feb 13, 2020

aSapien commented Feb 21, 2020

papagian commented Feb 24, 2020

aSapien commented Feb 24, 2020

papagian commented Feb 24, 2020 • edited Loading

papagian left a comment

Choose a reason for hiding this comment

papagian Feb 26, 2020

Choose a reason for hiding this comment

aSapien Mar 10, 2020

Choose a reason for hiding this comment

aSapien Mar 10, 2020

Choose a reason for hiding this comment

papagian Mar 10, 2020

Choose a reason for hiding this comment

papagian Mar 13, 2020

Choose a reason for hiding this comment

aSapien commented Feb 27, 2020

papagian left a comment

Choose a reason for hiding this comment

papagian Mar 13, 2020

Choose a reason for hiding this comment

aSapien commented Mar 15, 2020

papagian commented Mar 17, 2020

papagian commented Mar 17, 2020

aSapien commented Mar 18, 2020

aSapien commented Oct 17, 2019 •

edited

Loading

CLAassistant commented Oct 17, 2019 •

edited

Loading

Tim-Schwalbe commented Jan 6, 2020 •

edited

Loading

papagian commented Feb 24, 2020 •

edited

Loading