From 921c286d1974b6cabc0d6fdeac6996ec6f6e0c5e Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Fri, 17 Jan 2025 09:26:51 -0800 Subject: [PATCH 1/4] :wrench: chore: change rate limit to halt --- .../actions/issue_alert/notification.py | 7 +- .../discord/actions/metric_alert.py | 7 +- .../incidents/action_handlers/test_discord.py | 106 ++++++++++++++++++ .../integrations/discord/test_issue_alert.py | 9 ++ 4 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 tests/sentry/incidents/action_handlers/test_discord.py diff --git a/src/sentry/integrations/discord/actions/issue_alert/notification.py b/src/sentry/integrations/discord/actions/issue_alert/notification.py index 0e98beae120e4e..943947ad171eec 100644 --- a/src/sentry/integrations/discord/actions/issue_alert/notification.py +++ b/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 @@ -59,7 +60,6 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: lifecycle.add_extras({"integration_id": integration.id, "channel": channel_id}) client.send_message(channel_id, message, notification_uuid=notification_uuid) except Exception as e: - # TODO(iamrajjoshi): Update some of these failures to halts lifecycle.add_extras( { "project_id": event.project_id, @@ -68,7 +68,10 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: "channel_id": channel_id, } ) - lifecycle.record_failure(e) + if isinstance(e, ApiRateLimitedError): + lifecycle.record_halt(e) + else: + lifecycle.record_failure(e) rule = rules[0] if rules else None self.record_notification_sent(event, channel_id, rule, notification_uuid) diff --git a/src/sentry/integrations/discord/actions/metric_alert.py b/src/sentry/integrations/discord/actions/metric_alert.py index 7edbc1d6ee2d2b..99f638c2fb03cf 100644 --- a/src/sentry/integrations/discord/actions/metric_alert.py +++ b/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 @@ -63,13 +64,15 @@ def send_incident_alert_notification( try: client.send_message(channel, message) 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) + if isinstance(error, ApiRateLimitedError): + lifecycle.record_halt(error) + else: + lifecycle.record_failure(error) return False return True diff --git a/tests/sentry/incidents/action_handlers/test_discord.py b/tests/sentry/incidents/action_handlers/test_discord.py new file mode 100644 index 00000000000000..2ca2615961754c --- /dev/null +++ b/tests/sentry/incidents/action_handlers/test_discord.py @@ -0,0 +1,106 @@ +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.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.value) + + assert_slo_metric(mock_record_event, EventLifecycleOutcome.FAILURE) diff --git a/tests/sentry/integrations/discord/test_issue_alert.py b/tests/sentry/integrations/discord/test_issue_alert.py index 15feb511e8d5ef..f4d346cc3c35ee 100644 --- a/tests/sentry/integrations/discord/test_issue_alert.py +++ b/tests/sentry/integrations/discord/test_issue_alert.py @@ -4,6 +4,7 @@ import orjson import responses from django.core.exceptions import ValidationError +from requests.exceptions import HTTPError from sentry.integrations.discord.actions.issue_alert.form import DiscordNotifyServiceForm from sentry.integrations.discord.actions.issue_alert.notification import DiscordNotifyServiceAction @@ -82,6 +83,14 @@ def assert_lifecycle_metrics(self, mock_record_event): def assert_lifecycle_metrics_failure(self, mock_record_event, mock_send_message): assert_slo_metric(mock_record_event, EventLifecycleOutcome.FAILURE) + @mock.patch( + "sentry.integrations.discord.client.DiscordClient.send_message", + side_effect=HTTPError(429), + ) + @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def assert_lifecycle_metrics_halt_for_rate_limit(self, mock_record_event, mock_send_message): + assert_slo_metric(mock_record_event, EventLifecycleOutcome.HALTED) + @responses.activate @mock.patch("sentry.analytics.record") def test_basic(self, mock_record): From 5d3335a2f944403c60e27b8274674736839c8a10 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Fri, 17 Jan 2025 11:39:58 -0800 Subject: [PATCH 2/4] :wrench: chore: fix tests and mpy --- tests/sentry/incidents/action_handlers/test_discord.py | 2 +- tests/sentry/integrations/discord/test_issue_alert.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/sentry/incidents/action_handlers/test_discord.py b/tests/sentry/incidents/action_handlers/test_discord.py index 2ca2615961754c..02c5bdac2ed2d2 100644 --- a/tests/sentry/incidents/action_handlers/test_discord.py +++ b/tests/sentry/incidents/action_handlers/test_discord.py @@ -101,6 +101,6 @@ def test_metric_alert_failure(self, mock_record_event, mock_send_message): handler = MessagingActionHandler(self.action, incident, self.project, self.spec) metric_value = 1000 with self.tasks(): - handler.fire(metric_value, IncidentStatus.OPEN.value) + handler.fire(metric_value, IncidentStatus.OPEN) assert_slo_metric(mock_record_event, EventLifecycleOutcome.FAILURE) diff --git a/tests/sentry/integrations/discord/test_issue_alert.py b/tests/sentry/integrations/discord/test_issue_alert.py index f4d346cc3c35ee..8d812059c390be 100644 --- a/tests/sentry/integrations/discord/test_issue_alert.py +++ b/tests/sentry/integrations/discord/test_issue_alert.py @@ -81,6 +81,7 @@ 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( @@ -89,6 +90,7 @@ def assert_lifecycle_metrics_failure(self, mock_record_event, mock_send_message) ) @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 From 96b96869a17f5760bd3c00ef236c2f3aa3df19f9 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Fri, 17 Jan 2025 11:52:23 -0800 Subject: [PATCH 3/4] :mag: nit: add comment --- .../integrations/discord/actions/issue_alert/notification.py | 1 + src/sentry/integrations/discord/actions/metric_alert.py | 1 + 2 files changed, 2 insertions(+) diff --git a/src/sentry/integrations/discord/actions/issue_alert/notification.py b/src/sentry/integrations/discord/actions/issue_alert/notification.py index 943947ad171eec..47be27362971c6 100644 --- a/src/sentry/integrations/discord/actions/issue_alert/notification.py +++ b/src/sentry/integrations/discord/actions/issue_alert/notification.py @@ -68,6 +68,7 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: "channel_id": channel_id, } ) + # TODO(ecosystem): We should batch this on a per-organization basis if isinstance(e, ApiRateLimitedError): lifecycle.record_halt(e) else: diff --git a/src/sentry/integrations/discord/actions/metric_alert.py b/src/sentry/integrations/discord/actions/metric_alert.py index 99f638c2fb03cf..a7592cfd3b8b39 100644 --- a/src/sentry/integrations/discord/actions/metric_alert.py +++ b/src/sentry/integrations/discord/actions/metric_alert.py @@ -70,6 +70,7 @@ def send_incident_alert_notification( "channel_id": channel, } ) + # TODO(ecosystem): We should batch this on a per-organization basis if isinstance(error, ApiRateLimitedError): lifecycle.record_halt(error) else: From 6a668e1b74c5caa7e412e40bf84209dae9489f2c Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Fri, 17 Jan 2025 16:46:56 -0800 Subject: [PATCH 4/4] :recycle: ref: fix up exception handling --- .../discord/actions/issue_alert/notification.py | 10 +++++----- .../integrations/discord/actions/metric_alert.py | 11 ++++++----- .../incidents/action_handlers/test_discord.py | 16 ++++++++++++++++ .../integrations/discord/test_issue_alert.py | 5 ++--- 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/sentry/integrations/discord/actions/issue_alert/notification.py b/src/sentry/integrations/discord/actions/issue_alert/notification.py index 47be27362971c6..a5d6507c4bcb71 100644 --- a/src/sentry/integrations/discord/actions/issue_alert/notification.py +++ b/src/sentry/integrations/discord/actions/issue_alert/notification.py @@ -59,6 +59,9 @@ 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: lifecycle.add_extras( { @@ -68,11 +71,8 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: "channel_id": channel_id, } ) - # TODO(ecosystem): We should batch this on a per-organization basis - if isinstance(e, ApiRateLimitedError): - lifecycle.record_halt(e) - else: - lifecycle.record_failure(e) + + lifecycle.record_failure(e) rule = rules[0] if rules else None self.record_notification_sent(event, channel_id, rule, notification_uuid) diff --git a/src/sentry/integrations/discord/actions/metric_alert.py b/src/sentry/integrations/discord/actions/metric_alert.py index a7592cfd3b8b39..7d90b0add66e36 100644 --- a/src/sentry/integrations/discord/actions/metric_alert.py +++ b/src/sentry/integrations/discord/actions/metric_alert.py @@ -63,6 +63,10 @@ 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: lifecycle.add_extras( { @@ -70,10 +74,7 @@ def send_incident_alert_notification( "channel_id": channel, } ) - # TODO(ecosystem): We should batch this on a per-organization basis - if isinstance(error, ApiRateLimitedError): - lifecycle.record_halt(error) - else: - lifecycle.record_failure(error) + + lifecycle.record_failure(error) return False return True diff --git a/tests/sentry/incidents/action_handlers/test_discord.py b/tests/sentry/incidents/action_handlers/test_discord.py index 02c5bdac2ed2d2..a77e27cac9a577 100644 --- a/tests/sentry/incidents/action_handlers/test_discord.py +++ b/tests/sentry/incidents/action_handlers/test_discord.py @@ -9,6 +9,7 @@ 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 @@ -104,3 +105,18 @@ def test_metric_alert_failure(self, mock_record_event, mock_send_message): 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) diff --git a/tests/sentry/integrations/discord/test_issue_alert.py b/tests/sentry/integrations/discord/test_issue_alert.py index 8d812059c390be..9d9a3ccd0dfa52 100644 --- a/tests/sentry/integrations/discord/test_issue_alert.py +++ b/tests/sentry/integrations/discord/test_issue_alert.py @@ -4,7 +4,6 @@ import orjson import responses from django.core.exceptions import ValidationError -from requests.exceptions import HTTPError from sentry.integrations.discord.actions.issue_alert.form import DiscordNotifyServiceForm from sentry.integrations.discord.actions.issue_alert.notification import DiscordNotifyServiceAction @@ -20,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 @@ -86,7 +85,7 @@ def assert_lifecycle_metrics_failure(self, mock_record_event, mock_send_message) @mock.patch( "sentry.integrations.discord.client.DiscordClient.send_message", - side_effect=HTTPError(429), + 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):