♻️ ref(slo): Add SLOs for Slack Service (#81866)

Slack Service makes some external API calls, so adding SLOs here.

Author: iamrajjoshi

src/sentry/integrations/messaging/metrics.py

@@ -46,6 +46,9 @@ class MessagingInteractionType(StrEnum):
     SEND_INCIDENT_ALERT_NOTIFICATION = "SEND_INCIDENT_ALERT_NOTIFICATION"
     SEND_ISSUE_ALERT_NOTIFICATION = "SEND_ISSUE_ALERT_NOTIFICATION"
 
+    SEND_ACTIVITY_NOTIFICATION = "SEND_ACTIVITY_NOTIFICATION"
+    SEND_GENERIC_NOTIFICATION = "SEND_GENERIC_NOTIFICATION"
+
 
 @dataclass
 class MessagingInteractionEvent(IntegrationEventLifecycleMetric):

src/sentry/integrations/slack/metrics.py

@@ -1,5 +1,13 @@
 # metrics constants
 
+from slack_sdk.errors import SlackApiError
+
+from sentry.integrations.slack.utils.errors import (
+    SLACK_SDK_HALT_ERROR_CATEGORIES,
+    unpack_slack_api_error,
+)
+from sentry.integrations.utils.metrics import EventLifecycle
+
 SLACK_ISSUE_ALERT_SUCCESS_DATADOG_METRIC = "sentry.integrations.slack.issue_alert.success"
 SLACK_ISSUE_ALERT_FAILURE_DATADOG_METRIC = "sentry.integrations.slack.issue_alert.failure"
 SLACK_ACTIVITY_THREAD_SUCCESS_DATADOG_METRIC = "sentry.integrations.slack.activity_thread.success"
@@ -79,3 +87,14 @@
 # Middleware Parsers
 SLACK_MIDDLE_PARSERS_SUCCESS_DATADOG_METRIC = "sentry.middleware.integrations.slack.parsers.success"
 SLACK_MIDDLE_PARSERS_FAILURE_DATADOG_METRIC = "sentry.middleware.integrations.slack.parsers.failure"
+
+
+def record_lifecycle_termination_level(lifecycle: EventLifecycle, error: SlackApiError) -> None:
+    if (
+        (reason := unpack_slack_api_error(error))
+        and reason is not None
+        and reason in SLACK_SDK_HALT_ERROR_CATEGORIES
+    ):
+        lifecycle.record_halt(reason.message)
+    else:
+        lifecycle.record_failure(error)

src/sentry/integrations/slack/service.py

@@ -10,6 +10,10 @@
 from slack_sdk.errors import SlackApiError
 
 from sentry.constants import ISSUE_ALERTS_THREAD_DEFAULT
+from sentry.integrations.messaging.metrics import (
+    MessagingInteractionEvent,
+    MessagingInteractionType,
+)
 from sentry.integrations.models.integration import Integration
 from sentry.integrations.notifications import get_context
 from sentry.integrations.repository import get_default_issue_alert_repository
@@ -25,8 +29,10 @@
     SLACK_ACTIVITY_THREAD_SUCCESS_DATADOG_METRIC,
     SLACK_NOTIFY_RECIPIENT_FAILURE_DATADOG_METRIC,
     SLACK_NOTIFY_RECIPIENT_SUCCESS_DATADOG_METRIC,
+    record_lifecycle_termination_level,
 )
 from sentry.integrations.slack.sdk_client import SlackSdkClient
+from sentry.integrations.slack.spec import SlackMessagingSpec
 from sentry.integrations.slack.threads.activity_notifications import (
     AssignedActivityNotification,
     ExternalIssueCreatedActivityNotification,
@@ -182,12 +188,23 @@ def notify_all_threads_for_activity(self, activity: Activity) -> None:
         slack_client = SlackSdkClient(integration_id=integration.id)
 
         # Get all parent notifications, which will have the message identifier to use to reply in a thread
-        parent_notifications = (
-            self._notification_message_repository.get_all_parent_notification_messages_by_filters(
+        with MessagingInteractionEvent(
+            interaction_type=MessagingInteractionType.GET_PARENT_NOTIFICATION,
+            spec=SlackMessagingSpec(),
+        ).capture() as lifecycle:
+            lifecycle.add_extras(
+                {
+                    "activity_id": activity.id,
+                    "group_id": activity.group.id,
+                    "project_id": activity.project.id,
+                }
+            )
+            parent_notifications = self._notification_message_repository.get_all_parent_notification_messages_by_filters(
                 group_ids=[activity.group.id],
                 project_ids=[activity.project.id],
             )
-        )
+
+        # We don't wrap this in a lifecycle because _handle_parent_notification is already wrapped in a lifecycle
         for parent_notification in parent_notifications:
             try:
                 self._handle_parent_notification(
@@ -196,6 +213,7 @@ def notify_all_threads_for_activity(self, activity: Activity) -> None:
                     client=slack_client,
                 )
             except Exception as err:
+                # TODO(iamrajjoshi): We can probably swallow this error once we audit the lifecycle
                 self._logger.info(
                     "failed to send notification",
                     exc_info=err,
@@ -254,25 +272,33 @@ def _handle_parent_notification(
             "rule_action_uuid": parent_notification.rule_action_uuid,
         }
 
-        try:
-            client.chat_postMessage(
-                channel=channel_id,
-                thread_ts=parent_notification.message_identifier,
-                text=notification_to_send,
-                blocks=json_blocks,
-            )
-            metrics.incr(SLACK_ACTIVITY_THREAD_SUCCESS_DATADOG_METRIC, sample_rate=1.0)
-        except SlackApiError as e:
-            self._logger.info(
-                "failed to post message to slack",
-                extra={"error": str(e), "blocks": json_blocks, **extra},
-            )
-            metrics.incr(
-                SLACK_ACTIVITY_THREAD_FAILURE_DATADOG_METRIC,
-                sample_rate=1.0,
-                tags={"ok": e.response.get("ok", False), "status": e.response.status_code},
-            )
-            raise
+        with MessagingInteractionEvent(
+            interaction_type=MessagingInteractionType.SEND_ACTIVITY_NOTIFICATION,
+            spec=SlackMessagingSpec(),
+        ).capture() as lifecycle:
+            try:
+                client.chat_postMessage(
+                    channel=channel_id,
+                    thread_ts=parent_notification.message_identifier,
+                    text=notification_to_send,
+                    blocks=json_blocks,
+                )
+                # TODO(iamrajjoshi): Remove this after we validate lifecycle
+                metrics.incr(SLACK_ACTIVITY_THREAD_SUCCESS_DATADOG_METRIC, sample_rate=1.0)
+            except SlackApiError as e:
+                # TODO(iamrajjoshi): Remove this after we validate lifecycle
+                self._logger.info(
+                    "failed to post message to slack",
+                    extra={"error": str(e), "blocks": json_blocks, **extra},
+                )
+                metrics.incr(
+                    SLACK_ACTIVITY_THREAD_FAILURE_DATADOG_METRIC,
+                    sample_rate=1.0,
+                    tags={"ok": e.response.get("ok", False), "status": e.response.status_code},
+                )
+                lifecycle.add_extras({"rule_action_uuid": parent_notification.rule_action_uuid})
+                record_lifecycle_termination_level(lifecycle, e)
+                raise
 
     def _get_notification_message_to_send(self, activity: Activity) -> str | None:
         """
@@ -427,21 +453,32 @@ def send_message_to_slack_channel(
         """Execution of send_notification_as_slack."""
 
         client = SlackSdkClient(integration_id=integration_id)
-        try:
-            client.chat_postMessage(
-                blocks=str(payload.get("blocks", "")),
-                text=str(payload.get("text", "")),
-                channel=str(payload.get("channel", "")),
-                unfurl_links=False,
-                unfurl_media=False,
-                callback_id=str(payload.get("callback_id", "")),
-            )
-            metrics.incr(SLACK_NOTIFY_RECIPIENT_SUCCESS_DATADOG_METRIC, sample_rate=1.0)
-        except SlackApiError as e:
-            extra = {"error": str(e), **log_params}
-            self._logger.info(log_error_message, extra=extra)
-            metrics.incr(
-                SLACK_NOTIFY_RECIPIENT_FAILURE_DATADOG_METRIC,
-                sample_rate=1.0,
-                tags={"ok": e.response.get("ok", False), "status": e.response.status_code},
-            )
+        with MessagingInteractionEvent(
+            interaction_type=MessagingInteractionType.SEND_GENERIC_NOTIFICATION,
+            spec=SlackMessagingSpec(),
+        ).capture() as lifecycle:
+            try:
+                lifecycle.add_extras({"integration_id": integration_id})
+                client.chat_postMessage(
+                    blocks=str(payload.get("blocks", "")),
+                    text=str(payload.get("text", "")),
+                    channel=str(payload.get("channel", "")),
+                    unfurl_links=False,
+                    unfurl_media=False,
+                    callback_id=str(payload.get("callback_id", "")),
+                )
+                # TODO(iamrajjoshi): Remove this after we validate lifecycle
+                metrics.incr(SLACK_NOTIFY_RECIPIENT_SUCCESS_DATADOG_METRIC, sample_rate=1.0)
+            except SlackApiError as e:
+                # TODO(iamrajjoshi): Remove this after we validate lifecycle
+                extra = {"error": str(e), **log_params}
+                self._logger.info(log_error_message, extra=extra)
+                metrics.incr(
+                    SLACK_NOTIFY_RECIPIENT_FAILURE_DATADOG_METRIC,
+                    sample_rate=1.0,
+                    tags={"ok": e.response.get("ok", False), "status": e.response.status_code},
+                )
+                lifecycle.add_extras(
+                    {k: str(v) for k, v in log_params.items() if isinstance(v, (int, str))}
+                )
+                record_lifecycle_termination_level(lifecycle, e)

tests/sentry/integrations/slack/service/test_slack_service.py

@@ -9,11 +9,13 @@
 from sentry.integrations.repository.issue_alert import IssueAlertNotificationMessage
 from sentry.integrations.slack.sdk_client import SlackSdkClient
 from sentry.integrations.slack.service import RuleDataError, SlackService
+from sentry.integrations.types import EventLifecycleOutcome
 from sentry.models.activity import Activity
 from sentry.models.options.organization_option import OrganizationOption
 from sentry.models.rulefirehistory import RuleFireHistory
 from sentry.notifications.models.notificationmessage import NotificationMessage
 from sentry.silo.base import SiloMode
+from sentry.testutils.asserts import assert_slo_metric
 from sentry.testutils.cases import TestCase
 from sentry.testutils.silo import assume_test_silo_mode
 from sentry.types.activity import ActivityType
@@ -157,8 +159,9 @@ def test_no_parent_notification(self, mock_handle):
         self.service.notify_all_threads_for_activity(activity=self.activity)
         assert not mock_handle.called
 
+    @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     @mock.patch("sentry.integrations.slack.service.SlackService._handle_parent_notification")
-    def test_calls_handle_parent_notification_sdk_client(self, mock_handle):
+    def test_calls_handle_parent_notification_sdk_client(self, mock_handle, mock_record):
         parent_notification = IssueAlertNotificationMessage.from_model(
             instance=self.parent_notification
         )
@@ -170,6 +173,8 @@ def test_calls_handle_parent_notification_sdk_client(self, mock_handle):
         # check client type
         assert isinstance(mock_handle.call_args.kwargs["client"], SlackSdkClient)
 
+        assert_slo_metric(mock_record, EventLifecycleOutcome.SUCCESS)
+
 
 class TestHandleParentNotification(TestCase):
     def setUp(self) -> None:
@@ -233,8 +238,9 @@ def setUp(self) -> None:
             rule_fire_history=self.slack_rule_fire_history,
         )
 
+    @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     @mock.patch("slack_sdk.web.client.WebClient._perform_urllib_http_request")
-    def test_handles_parent_notification_sdk(self, mock_api_call):
+    def test_handles_parent_notification_sdk(self, mock_api_call, mock_record):
         mock_api_call.return_value = {
             "body": orjson.dumps({"ok": True}).decode(),
             "headers": {},
@@ -246,8 +252,12 @@ def test_handles_parent_notification_sdk(self, mock_api_call):
             client=SlackSdkClient(integration_id=self.integration.id),
         )
 
+        assert_slo_metric(mock_record, EventLifecycleOutcome.SUCCESS)
+
+    @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     def test_handles_parent_notification_sdk_error(
         self,
+        mock_record,
     ) -> None:
         with pytest.raises(SlackApiError):
             self.service._handle_parent_notification(
@@ -256,6 +266,8 @@ def test_handles_parent_notification_sdk_error(
                 client=SlackSdkClient(integration_id=self.integration.id),
             )
 
+        assert_slo_metric(mock_record, EventLifecycleOutcome.FAILURE)
+
     def test_raises_exception_when_parent_notification_does_not_have_rule_fire_history_data(
         self,
     ) -> None: