User notifications bundle #4457

Ferril · 2024-06-04T12:37:12Z

What this PR does

This PR adds two new models: UserNotificationBundle and BundledNotification (proposals for naming are welcome).

UserNotificationBundle manages the information about last notification time and scheduled notification task for bundled notifications. It is unique per user + notification_channel + notification importance.

BundledNotification contains notification policy and alert group, that triggered the notification. The BundledNotification instance is created in notify_user_task for every notification, that should be bundled, and is attached to UserNotificationBundle by ForeignKey connection.

How it works:
If the user was notified recently (within the last two minutes) by the current notification channel, and this channel is bundlable, BundledNotification instance will be created and attached to the UserNotificationBundle instance, and send_bundled_notification task will be scheduled to execute in 2 min.
In send_bundled_notification task we get all BundledNotification attached to the current UserNotificationBundle instance, check if alert groups are still active and if there is only one notification - perform regular notification by calling perform_notification task, otherwise call "notify_by__bundle" method for the current notification channel.

PR with method to send notification bundle by SMS - #4624

This feature is disabled by default by feature flag. Public docs will be added in a separate PR with enabling this feature.

Which issue(s) this PR closes

related to https://github.com/grafana/oncall-private/issues/2712

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
show up in the autogenerated release notes.

engine/apps/alerts/tasks/notify_user.py

Konstantinov-Innokentii · 2024-06-13T08:38:52Z

engine/apps/alerts/tasks/notify_user.py

-                        notification_channel=notification_policy.notify_by,
-                    )
+                    # todo: feature flag
+                    if notification_policy.notify_by in UserNotificationBundle.NOTIFICATION_CHANNELS_TO_BUNDLE:


nit: worth to make is_bundlable method for user_notification_policy

engine/apps/alerts/tasks/notify_user.py

joeyorlando · 2024-07-08T22:49:26Z

engine/apps/alerts/models/user_has_notification.py

@@ -16,3 +16,7 @@ class UserHasNotification(models.Model):

    class Meta:
        unique_together = ("user", "alert_group")
+
+    def update_active_task_id(self, task_id):
+        self.active_notification_policy_id = task_id


maybe out of scope for this PR but I think this model should be renamed to something like UserActiveNotificationPolicy (UserHasNotification isn't very informative imo 😬 )

joeyorlando · 2024-07-08T22:54:36Z

engine/apps/alerts/tasks/notify_user.py

-                    if next_notification_policy.notify_by not in collected_steps_ids:
-                        collected_steps_ids.append(next_notification_policy.notify_by)
-
+            reason = build_notification_reason_for_log_record(notification_policies, reason)


nice! slowly breaks apart this function 🙂

engine/apps/alerts/tasks/notify_user.py

joeyorlando · 2024-07-08T22:55:49Z

engine/apps/alerts/tasks/notify_user.py

-                        notification_step=notification_policy.step,
-                        notification_channel=notification_policy.notify_by,
-                    )
+                    if (


engine/apps/alerts/tasks/notify_user.py

engine/apps/metrics_exporter/tests/test_update_metrics_cache.py

joeyorlando

overall LGTM!

engine/apps/alerts/tasks/notify_user.py

engine/apps/alerts/models/user_notification_bundle.py

engine/apps/alerts/tasks/notify_user.py

engine/apps/alerts/models/user_notification_bundle.py

joeyorlando · 2024-07-09T00:21:20Z

engine/apps/alerts/models/user_notification_bundle.py

+        UserNotificationBundle, on_delete=models.CASCADE, related_name="notifications"
+    )
+    created_at = models.DateTimeField(auto_now_add=True)
+    bundle_uuid = models.CharField(max_length=100, null=True, default=None, db_index=True)


I see this is primarly used right now in as the second arg to PhoneBackend.notify_by_sms_bundle_async, but can we just use pk instead of needing also bundle_uuid, or is there a need for a UUID?

The idea is that bundle_uuid groups a bunch of BundledNotifications, that were processed in the send_bundled_notification task (if user got flooded, it can be a hundreds of them). While we are processing these notifications, new notifications can be created and attached to the current UserNotificationBundle and they will have their own bundle_uuid and will be processed and sent in a separate task.
That means, that we can't use UserNotificationBundle pk, because there could be processed and unprocessed notifications attached at the same time, and we want to keep them separately. We don't want to use BundledNotification pks, because there could be a long list of them

engine/apps/alerts/tasks/notify_user.py

# Conflicts: # engine/apps/alerts/tasks/notify_user.py # engine/apps/alerts/tests/test_notify_user.py

# What this PR does Adds method to render and send notification bundle by sms. Example of SMS message: ``` Grafana OnCall: Alert groups #1, #2, #3 and 2 more from stack: TestOrganization, integrations: Grafana Alerting and 1 more. ``` Should be merged with #4457 ## Which issue(s) this PR closes grafana/oncall-private#2713 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes.

Ferril added 3 commits June 4, 2024 14:19

Update user has notification

0dcad9f

Add user notification bundle model

872990c

Add bundling for user notifications

2ab8315

Ferril temporarily deployed to github-pages June 4, 2024 12:37 — with GitHub Actions Inactive

Fix update metrics

63bc4d0

Ferril temporarily deployed to github-pages June 4, 2024 13:03 — with GitHub Actions Inactive

mderynck reviewed Jun 4, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Outdated Show resolved Hide resolved

mderynck reviewed Jun 4, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Outdated Show resolved Hide resolved

mderynck reviewed Jun 4, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Outdated Show resolved Hide resolved

mderynck reviewed Jun 4, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Outdated Show resolved Hide resolved

Update sending and tracking bundled notifications

7e889fe

Ferril had a problem deploying to github-pages June 7, 2024 13:12 — with GitHub Actions Error

remove commented code

59a9664

Ferril temporarily deployed to github-pages June 7, 2024 13:14 — with GitHub Actions Inactive

Update comment

94c66ee

Ferril temporarily deployed to github-pages June 7, 2024 13:27 — with GitHub Actions Inactive

Fix updating metrics on user notification

271c99f

Ferril temporarily deployed to github-pages June 10, 2024 11:09 — with GitHub Actions Inactive

Konstantinov-Innokentii reviewed Jun 13, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Outdated Show resolved Hide resolved

Konstantinov-Innokentii reviewed Jun 13, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Outdated Show resolved Hide resolved

Fix updating metrics on user notification

ea06498

Konstantinov-Innokentii reviewed Jun 14, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Outdated Show resolved Hide resolved

Konstantinov-Innokentii reviewed Jun 14, 2024

View reviewed changes

engine/apps/alerts/tasks/notify_user.py Show resolved Hide resolved

Ferril added 6 commits June 18, 2024 10:31

Update user notification bundle

cb71560

Add test for updating metrics for bundled notifications

b0db509

Add tests for bundling notifications

c943841

Add test for notification bundle

0b036e6

Update send_bundled_notification task

1145755

update test

4ff84fe

Ferril had a problem deploying to github-pages July 8, 2024 16:01 — with GitHub Actions Error

Ferril added release:patch PR will be added to "Other Changes" section of release notes pr:no public docs Added to a PR that does not require public documentation updates labels Jul 8, 2024

remove comment

e0e237d

Ferril temporarily deployed to github-pages July 8, 2024 16:08 — with GitHub Actions Inactive

Ferril marked this pull request as ready for review July 8, 2024 16:12

Ferril requested a review from a team July 8, 2024 16:12

joeyorlando reviewed Jul 8, 2024

View reviewed changes

joeyorlando approved these changes Jul 9, 2024

View reviewed changes

Ferril added 4 commits July 9, 2024 14:50

Update typing

e2f2727

remove todo comment

cc1ede0

Merge branch 'dev' into user-notification-bundle

d6b9242

# Conflicts: # engine/apps/alerts/tasks/notify_user.py # engine/apps/alerts/tests/test_notify_user.py

Fix notify_user_task

0394ea1

Ferril temporarily deployed to github-pages July 10, 2024 11:12 — with GitHub Actions Inactive

Fix mypy

4eed7d8

Ferril temporarily deployed to github-pages July 11, 2024 08:43 — with GitHub Actions Inactive

Ferril temporarily deployed to github-pages July 11, 2024 09:00 — with GitHub Actions Inactive

mderynck approved these changes Jul 11, 2024

View reviewed changes

Ferril added this pull request to the merge queue Jul 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 16, 2024

Ferril added 2 commits July 16, 2024 12:54

Merge branch 'dev' into user-notification-bundle

58cf5ef

Fix migration

1431674

Ferril temporarily deployed to github-pages July 16, 2024 10:56 — with GitHub Actions Inactive

Ferril added this pull request to the merge queue Jul 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 16, 2024

Ferril added this pull request to the merge queue Jul 16, 2024

Merged via the queue into dev with commit 191814b Jul 16, 2024
21 checks passed

Ferril deleted the user-notification-bundle branch July 16, 2024 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User notifications bundle #4457

User notifications bundle #4457

Ferril commented Jun 4, 2024 •

edited

Loading

Konstantinov-Innokentii Jun 13, 2024

joeyorlando Jul 8, 2024

joeyorlando Jul 8, 2024

joeyorlando Jul 8, 2024

joeyorlando left a comment

joeyorlando Jul 9, 2024 •

edited

Loading

Ferril Jul 9, 2024

User notifications bundle #4457

User notifications bundle #4457

Conversation

Ferril commented Jun 4, 2024 • edited Loading

What this PR does

Which issue(s) this PR closes

Checklist

Konstantinov-Innokentii Jun 13, 2024

Choose a reason for hiding this comment

joeyorlando Jul 8, 2024

Choose a reason for hiding this comment

joeyorlando Jul 8, 2024

Choose a reason for hiding this comment

joeyorlando Jul 8, 2024

Choose a reason for hiding this comment

joeyorlando left a comment

Choose a reason for hiding this comment

joeyorlando Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Ferril Jul 9, 2024

Choose a reason for hiding this comment

Ferril commented Jun 4, 2024 •

edited

Loading

joeyorlando Jul 9, 2024 •

edited

Loading