diff --git a/src/sentry/integrations/discord/actions/issue_alert/notification.py b/src/sentry/integrations/discord/actions/issue_alert/notification.py index 0e98beae120e4e..a5d6507c4bcb71 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 @@ -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 diff --git a/src/sentry/integrations/discord/actions/metric_alert.py b/src/sentry/integrations/discord/actions/metric_alert.py index 7edbc1d6ee2d2b..7d90b0add66e36 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 @@ -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 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..a77e27cac9a577 --- /dev/null +++ b/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) diff --git a/tests/sentry/integrations/discord/test_issue_alert.py b/tests/sentry/integrations/discord/test_issue_alert.py index 15feb511e8d5ef..9d9a3ccd0dfa52 100644 --- a/tests/sentry/integrations/discord/test_issue_alert.py +++ b/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):