🔧 chore(discord): change rate limits to halt for SLOs (#83656)

src/sentry/integrations/discord/actions/issue_alert/notification.py

@@ -12,6 +12,7 @@
 )
 from sentry.rules.actions import IntegrationEventAction
 from sentry.rules.base import CallbackFuture
+from sentry.shared_integrations.exceptions import ApiRateLimitedError
 from sentry.types.rules import RuleFuture
 from sentry.utils import metrics
 
@@ -58,8 +59,10 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None:
                 try:
                     lifecycle.add_extras({"integration_id": integration.id, "channel": channel_id})
                     client.send_message(channel_id, message, notification_uuid=notification_uuid)
+                except ApiRateLimitedError as e:
+                    # TODO(ecosystem): We should batch this on a per-organization basis
+                    lifecycle.record_halt(e)
                 except Exception as e:
-                    # TODO(iamrajjoshi): Update some of these failures to halts
                     lifecycle.add_extras(
                         {
                             "project_id": event.project_id,
@@ -68,6 +71,7 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None:
                             "channel_id": channel_id,
                         }
                     )
+
                     lifecycle.record_failure(e)
 
             rule = rules[0] if rules else None

src/sentry/integrations/discord/actions/metric_alert.py

@@ -15,6 +15,7 @@
     MessagingInteractionEvent,
     MessagingInteractionType,
 )
+from sentry.shared_integrations.exceptions import ApiRateLimitedError
 
 from ..utils import logger
 
@@ -62,14 +63,18 @@ def send_incident_alert_notification(
     ).capture() as lifecycle:
         try:
             client.send_message(channel, message)
+        except ApiRateLimitedError as error:
+            # TODO(ecosystem): We should batch this on a per-organization basis
+            lifecycle.record_halt(error)
+            return False
         except Exception as error:
-            # TODO(iamrajjoshi): Update some of these failures to halts
             lifecycle.add_extras(
                 {
                     "incident_id": incident.id,
                     "channel_id": channel,
                 }
             )
+
             lifecycle.record_failure(error)
             return False
         return True

tests/sentry/incidents/action_handlers/test_discord.py

@@ -0,0 +1,122 @@
+from unittest.mock import patch
+
+import orjson
+import responses
+
+from sentry.incidents.models.alert_rule import AlertRuleTriggerAction
+from sentry.incidents.models.incident import IncidentStatus
+from sentry.integrations.discord.client import CHANNEL_URL, DISCORD_BASE_URL, MESSAGE_URL
+from sentry.integrations.discord.spec import DiscordMessagingSpec
+from sentry.integrations.messaging.spec import MessagingActionHandler
+from sentry.integrations.types import EventLifecycleOutcome
+from sentry.shared_integrations.exceptions import ApiRateLimitedError
+from sentry.testutils.asserts import assert_slo_metric
+from sentry.testutils.helpers.datetime import freeze_time
+
+from . import FireTest
+
+
+@freeze_time()
+class DiscordActionHandlerTest(FireTest):
+    @responses.activate
+    def setUp(self):
+        self.spec = DiscordMessagingSpec()
+
+        self.guild_id = "guild-id"
+        self.channel_id = "12345678910"
+        self.discord_user_id = "user1234"
+
+        responses.add(
+            method=responses.GET,
+            url=f"{DISCORD_BASE_URL}{CHANNEL_URL.format(channel_id=self.channel_id)}",
+            json={"type": 0, "guild_id": self.guild_id},
+            status=200,
+        )
+        self.discord_integration = self.create_integration(
+            provider="discord",
+            name="Cool server",
+            external_id=self.guild_id,
+            organization=self.organization,
+        )
+        self.provider = self.create_identity_provider(integration=self.discord_integration)
+        self.identity = self.create_identity(
+            user=self.user, identity_provider=self.provider, external_id=self.discord_user_id
+        )
+
+        self.action = self.create_alert_rule_trigger_action(
+            target_identifier=self.channel_id,
+            type=AlertRuleTriggerAction.Type.DISCORD,
+            target_type=AlertRuleTriggerAction.TargetType.SPECIFIC,
+            integration=self.discord_integration,
+        )
+
+    @responses.activate
+    def run_test(self, incident, method):
+        responses.add(
+            method=responses.POST,
+            url=f"{DISCORD_BASE_URL}{MESSAGE_URL.format(channel_id=self.channel_id)}",
+            json={},
+            status=200,
+        )
+
+        handler = MessagingActionHandler(self.action, incident, self.project, self.spec)
+        metric_value = 1000
+        with self.tasks():
+            getattr(handler, method)(metric_value, IncidentStatus(incident.status))
+
+        data = orjson.loads(responses.calls[0].request.body)
+        return data
+
+    def test_fire_metric_alert(self):
+        self.run_fire_test()
+
+    def test_resolve_metric_alert(self):
+        self.run_fire_test("resolve")
+
+    @responses.activate
+    def test_rule_snoozed(self):
+        alert_rule = self.create_alert_rule()
+        incident = self.create_incident(alert_rule=alert_rule, status=IncidentStatus.CLOSED.value)
+        self.snooze_rule(alert_rule=alert_rule)
+
+        responses.add(
+            method=responses.POST,
+            url=f"{DISCORD_BASE_URL}{MESSAGE_URL.format(channel_id=self.channel_id)}",
+            json={},
+            status=200,
+        )
+
+        handler = MessagingActionHandler(self.action, incident, self.project, self.spec)
+        metric_value = 1000
+        with self.tasks():
+            handler.fire(metric_value, IncidentStatus(incident.status))
+
+        assert len(responses.calls) == 0
+
+    @responses.activate
+    @patch("sentry.integrations.discord.client.DiscordClient.send_message", side_effect=Exception)
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def test_metric_alert_failure(self, mock_record_event, mock_send_message):
+        alert_rule = self.create_alert_rule()
+        incident = self.create_incident(alert_rule=alert_rule, status=IncidentStatus.CLOSED.value)
+        handler = MessagingActionHandler(self.action, incident, self.project, self.spec)
+        metric_value = 1000
+        with self.tasks():
+            handler.fire(metric_value, IncidentStatus.OPEN)
+
+        assert_slo_metric(mock_record_event, EventLifecycleOutcome.FAILURE)
+
+    @patch(
+        "sentry.integrations.discord.client.DiscordClient.send_message",
+        side_effect=ApiRateLimitedError(text="Rate limited"),
+    )
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def test_metric_alert_halt_for_rate_limited(self, mock_record_event, mock_send_message):
+        alert_rule = self.create_alert_rule()
+        incident = self.create_incident(alert_rule=alert_rule, status=IncidentStatus.CLOSED.value)
+        handler = MessagingActionHandler(self.action, incident, self.project, self.spec)
+        metric_value = 1000
+        with self.tasks():
+            handler.fire(metric_value, IncidentStatus.OPEN)
+
+        assert_slo_metric(mock_record_event, EventLifecycleOutcome.HALTED)

tests/sentry/integrations/discord/test_issue_alert.py

@@ -19,7 +19,7 @@
 from sentry.integrations.types import EventLifecycleOutcome, ExternalProviders
 from sentry.models.group import GroupStatus
 from sentry.models.release import Release
-from sentry.shared_integrations.exceptions import ApiTimeoutError
+from sentry.shared_integrations.exceptions import ApiRateLimitedError, ApiTimeoutError
 from sentry.testutils.asserts import assert_slo_metric
 from sentry.testutils.cases import RuleTestCase, TestCase
 from sentry.testutils.helpers.datetime import before_now
@@ -80,8 +80,18 @@ def assert_lifecycle_metrics(self, mock_record_event):
     )
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     def assert_lifecycle_metrics_failure(self, mock_record_event, mock_send_message):
+        self.rule.after(self.event)
         assert_slo_metric(mock_record_event, EventLifecycleOutcome.FAILURE)
 
+    @mock.patch(
+        "sentry.integrations.discord.client.DiscordClient.send_message",
+        side_effect=ApiRateLimitedError(text="Rate limited"),
+    )
+    @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def assert_lifecycle_metrics_halt_for_rate_limit(self, mock_record_event, mock_send_message):
+        self.rule.after(self.event)
+        assert_slo_metric(mock_record_event, EventLifecycleOutcome.HALTED)
+
     @responses.activate
     @mock.patch("sentry.analytics.record")
     def test_basic(self, mock_record):