fix: improve Slack rate limiting logic when updating alert groups #5287

joeyorlando · 2024-11-21T15:56:39Z

What this PR does

https://www.loom.com/share/1ac33822301444748133ffe72638ddc4

The two asks in the original GH issue were:

Make the error message clearer. We can identify if it's delivering or updating and being rate-limited. This is possible because Slack sets limits per API method. Also, this limit is a per-slack channel while we are posting messages & applying ratelimit per on-call integration, which confuses customers.

Debounce update alert group message in Slack

Both of these have been addressed in this PR

Which issue(s) this PR closes

Closes https://github.com/grafana/oncall-private/issues/2947

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.

…:grafana/oncall into jorlando/integration-slack-rate-limiting

joeyorlando · 2024-11-21T18:33:37Z

engine/apps/alerts/models/alert_receive_channel.py

        return (
            self.rate_limited_in_slack_at is not None
            and self.rate_limited_in_slack_at + SLACK_RATE_LIMIT_TIMEOUT > timezone.now()
        )

-    def start_send_rate_limit_message_task(self, delay=SLACK_RATE_LIMIT_DELAY):
+    def start_send_rate_limit_message_task(self, error_message_verb: str, delay: int) -> None:


this is currently only invoked in two spots, both of which have been updated (additionally, both of these spots that were invoking this function were already passing in a delay, hence why I removed the default of SLACK_RATE_LIMIT_DELAY)

joeyorlando · 2024-11-21T18:35:39Z

engine/apps/slack/models/slack_message.py

+    def update_alert_groups_message(self) -> None:
+        """
+        Schedule an update task for the associated alert group's Slack message, respecting the debounce interval.
+
+        This method ensures that updates to the Slack message related to an alert group are not performed
+        too frequently, adhering to the `ALERT_GROUP_UPDATE_DEBOUNCE_INTERVAL_SECONDS` debounce interval.
+        It schedules a background task to update the message after the appropriate countdown.
+
+        The method performs the following steps:
+        - Checks if there's already an active update task (`active_update_task_id` is set). If so, exits to prevent
+        duplicate scheduling.
+        - Calculates the time since the last update (`last_updated` field) and determines the remaining time needed
+        to respect the debounce interval.
+        - Schedules the `update_alert_group_slack_message` task with the calculated countdown.
+        - Stores the task ID in `active_update_task_id` to prevent multiple tasks from being scheduled.
+        """
+
+        if not self.alert_group:
+            logger.warning(
+                f"skipping update_alert_groups_message as SlackMessage {self.pk} has no alert_group associated with it"
+            )
+            return
+        elif self.active_update_task_id:
+            logger.info(
+                f"skipping update_alert_groups_message as SlackMessage {self.pk} has an active update task {self.active_update_task_id}"
+            )
+            return
+
+        now = timezone.now()
+
+        # we previously weren't updating the last_updated field for messages, so there will be cases
+        # where the last_updated field is None
+        last_updated = self.last_updated or now
+
+        time_since_last_update = (now - last_updated).total_seconds()
+        remaining_time = self.ALERT_GROUP_UPDATE_DEBOUNCE_INTERVAL_SECONDS - int(time_since_last_update)
+        countdown = max(remaining_time, 10)
+
+        logger.info(
+            f"updating message for alert_group {self.alert_group.pk} in {countdown} seconds "
+            f"(debounce interval: {self.ALERT_GROUP_UPDATE_DEBOUNCE_INTERVAL_SECONDS})"
+        )
+
+        task_id = celery_uuid()
+        update_alert_group_slack_message.apply_async((self.pk,), countdown=countdown, task_id=task_id)
+        self.active_update_task_id = task_id
+        self.save(update_fields=["active_update_task_id"])


main change

joeyorlando · 2024-11-21T18:39:11Z

engine/apps/slack/tasks.py

+def update_alert_group_slack_message(slack_message_pk: int) -> None:
+    """
+    Background task to update the Slack message for an alert group.
+
+    This function is intended to be executed as a Celery task. It performs the following:
+    - Compares the current task ID with the `active_update_task_id` stored in the `SlackMessage`.
+      - If they do not match, it means a newer task has been scheduled, so the current task exits to prevent outdated updates.
+    - Uses the `AlertGroupSlackService` to perform the actual update of the Slack message.
+    - Upon successful completion, clears the `active_update_task_id` to allow future updates.
+
+    Args:
+        slack_message_pk (int): The primary key of the `SlackMessage` instance to update.
+    """
+
+    from apps.slack.models import SlackMessage
+
+    current_task_id = update_alert_group_slack_message.request.id

-        return (
-            f"update_incident_slack_message rescheduled because of current task_id ({current_task_id})"
-            f" for alert_group {alert_group_pk} doesn't exist in cache"
+    logger.info(
+        f"update_alert_group_slack_message for slack message {slack_message_pk} started with task_id {current_task_id}"
+    )
+
+    try:
+        slack_message = SlackMessage.objects.get(pk=slack_message_pk)
+    except SlackMessage.DoesNotExist:
+        logger.warning(f"SlackMessage {slack_message_pk} doesn't exist")
+        return
+
+    if not current_task_id == slack_message.active_update_task_id:
+        logger.warning(
+            f"update_alert_group_slack_message skipped, because current_task_id ({current_task_id}) "
+            f"does not equal to active_update_task_id ({slack_message.active_update_task_id}) "
        )
-    if not current_task_id == cached_task_id:
-        return (
-            f"update_incident_slack_message skipped, because of current task_id ({current_task_id})"
-            f" doesn't equal to cached task_id ({cached_task_id}) for alert_group {alert_group_pk}"
+        return
+
+    alert_group = slack_message.alert_group
+    if not alert_group:
+        logger.warning(
+            f"skipping update_alert_group_slack_message as SlackMessage {slack_message_pk} "
+            "doesn't have an alert group associated with it"
        )
+        return
+
+    AlertGroupSlackService(slack_message.slack_team_identity).update_alert_group_slack_message(alert_group)

+    slack_message.active_update_task_id = None
+    slack_message.last_updated = timezone.now()
+    slack_message.save(update_fields=["active_update_task_id", "last_updated"])


main change

joeyorlando · 2024-11-21T18:39:51Z

engine/apps/slack/tasks.py

+
+
+@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True)
+def update_incident_slack_message(slack_team_identity_pk: int, alert_group_pk: int) -> None:


this has been renamed to update_alert_group_slack_message.. leaving this around to allow the celery workers to process any remaining update_incident_slack_message tasks (before removing this in a subsequent PR)

joeyorlando · 2024-12-02T17:28:55Z

engine/apps/slack/migrations/0007_migrate_slackmessage_channel_id.py

@vadimkerr to what we discussed earlier, the relocation of this file will trigger the SlackChannel._channel_id -> SlackChannel.channel migration for OSS (I will skip this in Cloud (ie. mark this migration as "completed"), as doing the migration this way is not performant enough (see https://github.com/grafana/oncall/blob/dev/engine/engine/management/commands/batch_migrate_slack_message_channel.py)

joeyorlando · 2024-12-02T17:29:57Z

engine/apps/slack/models/slack_message.py

@@ -85,7 +91,7 @@ class SlackMessage(models.Model):

    active_update_task_id = models.CharField(max_length=100, null=True, default=None)


this field is not used anywhere. At first I considered using, but went a different route. Marking it to be dropped in a subsequent PR/release.

vstpme

LGTM

engine/apps/slack/models/slack_message.py

joeyorlando added 2 commits November 21, 2024 05:37

fix: improve Slack rate limiting logic when updating alert groups

2e7e46f

remove django-deprecate-fields

ee013d8

joeyorlando mentioned this pull request Nov 21, 2024

wip: optimize slack alert group updates #5185

Closed

3 tasks

joeyorlando had a problem deploying to github-pages November 21, 2024 15:56 — with GitHub Actions Error

joeyorlando had a problem deploying to github-pages November 21, 2024 15:57 — with GitHub Actions Error

joeyorlando and others added 3 commits November 21, 2024 10:59

Merge branch 'dev' into jorlando/integration-slack-rate-limiting

202f8b6

wip

6229ff3

Merge branch 'jorlando/integration-slack-rate-limiting' of github.com…

26df057

…:grafana/oncall into jorlando/integration-slack-rate-limiting

joeyorlando had a problem deploying to github-pages November 21, 2024 15:59 — with GitHub Actions Error

linting

522a699

joeyorlando temporarily deployed to github-pages November 21, 2024 16:04 — with GitHub Actions Inactive

wip

4cafa21

joeyorlando temporarily deployed to github-pages November 21, 2024 18:28 — with GitHub Actions Inactive

wip

2d55fb5

joeyorlando had a problem deploying to github-pages November 21, 2024 18:43 — with GitHub Actions Error

wip

5b9b4a8

joeyorlando temporarily deployed to github-pages November 21, 2024 18:44 — with GitHub Actions Inactive

joeyorlando commented Nov 21, 2024

View reviewed changes

joeyorlando added the pr:no public docs Added to a PR that does not require public documentation updates label Nov 21, 2024

joeyorlando added 2 commits December 2, 2024 12:25

Merge branch 'dev' into jorlando/integration-slack-rate-limiting

221db90

linting

97c67d4

joeyorlando had a problem deploying to github-pages December 2, 2024 17:26 — with GitHub Actions Error

joeyorlando commented Dec 2, 2024

View reviewed changes

wip

10cbeac

joeyorlando temporarily deployed to github-pages December 2, 2024 17:32 — with GitHub Actions Inactive

PR comment

82db6a5

joeyorlando temporarily deployed to github-pages December 2, 2024 17:56 — with GitHub Actions Inactive

joeyorlando requested review from Ferril and vstpme December 2, 2024 17:58

vstpme approved these changes Dec 2, 2024

View reviewed changes

engine/apps/slack/models/slack_message.py Outdated Show resolved Hide resolved

pr comment

d175490

joeyorlando enabled auto-merge December 2, 2024 18:53

joeyorlando had a problem deploying to github-pages December 2, 2024 18:53 — with GitHub Actions Error

linting

6ad5957

joeyorlando temporarily deployed to github-pages December 2, 2024 18:57 — with GitHub Actions Inactive

joeyorlando disabled auto-merge December 2, 2024 19:40

joeyorlando merged commit 26946f0 into dev Dec 2, 2024
25 of 26 checks passed

joeyorlando deleted the jorlando/integration-slack-rate-limiting branch December 2, 2024 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve Slack rate limiting logic when updating alert groups #5287

fix: improve Slack rate limiting logic when updating alert groups #5287

joeyorlando commented Nov 21, 2024 •

edited

Loading

joeyorlando Nov 21, 2024

joeyorlando Nov 21, 2024

joeyorlando Nov 21, 2024

joeyorlando Nov 21, 2024

joeyorlando Dec 2, 2024 •

edited

Loading

joeyorlando Dec 2, 2024

vstpme left a comment



		@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True)
		def update_incident_slack_message(slack_team_identity_pk: int, alert_group_pk: int) -> None:

		@@ -85,7 +91,7 @@ class SlackMessage(models.Model):

		active_update_task_id = models.CharField(max_length=100, null=True, default=None)

fix: improve Slack rate limiting logic when updating alert groups #5287

fix: improve Slack rate limiting logic when updating alert groups #5287

Conversation

joeyorlando commented Nov 21, 2024 • edited Loading

What this PR does

Which issue(s) this PR closes

Checklist

joeyorlando Nov 21, 2024

Choose a reason for hiding this comment

joeyorlando Nov 21, 2024

Choose a reason for hiding this comment

joeyorlando Nov 21, 2024

Choose a reason for hiding this comment

joeyorlando Nov 21, 2024

Choose a reason for hiding this comment

joeyorlando Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

joeyorlando Dec 2, 2024

Choose a reason for hiding this comment

vstpme left a comment

Choose a reason for hiding this comment

joeyorlando commented Nov 21, 2024 •

edited

Loading

joeyorlando Dec 2, 2024 •

edited

Loading