chore(slack): remove old slack client code for metric alerts (#72862)

src/sentry/features/temporary.py

@@ -389,8 +389,6 @@ def register_temporary_features(manager: FeatureManager):
     # Enable improvements to Slack notifications
     manager.add("organizations:slack-improvements", OrganizationFeature, FeatureHandlerStrategy.OPTIONS)
     # Feature flags for migrating to the Slack SDK WebClient
-    # Use new Slack SDK Client for sending metric alerts
-    manager.add("organizations:slack-sdk-metric-alert", OrganizationFeature, FeatureHandlerStrategy.OPTIONS)
     # Use new Slack SDK Client in _notify_recipient
     manager.add("organizations:slack-sdk-notify-recipient", OrganizationFeature, FeatureHandlerStrategy.OPTIONS)
     # Add regression chart as image to slack message

src/sentry/integrations/slack/metrics.py

@@ -4,3 +4,5 @@
 SLACK_ISSUE_ALERT_FAILURE_DATADOG_METRIC = "sentry.integrations.slack.issue_alert.failure"
 SLACK_ACTIVITY_THREAD_SUCCESS_DATADOG_METRIC = "sentry.integrations.slack.activity_thread.success"
 SLACK_ACTIVITY_THREAD_FAILURE_DATADOG_METRIC = "sentry.integrations.slack.activity_thread.failure"
+SLACK_METRIC_ALERT_SUCCESS_DATADOG_METRIC = "sentry.integrations.slack.metric_alert.success"
+SLACK_METRIC_ALERT_FAILURE_DATADOG_METRIC = "sentry.integrations.slack.metric_alert.failure"

src/sentry/integrations/slack/utils/notifications.py

@@ -19,12 +19,16 @@
 )
 from sentry.integrations.slack.client import SlackClient
 from sentry.integrations.slack.message_builder.incidents import SlackIncidentsMessageBuilder
+from sentry.integrations.slack.metrics import (
+    SLACK_METRIC_ALERT_FAILURE_DATADOG_METRIC,
+    SLACK_METRIC_ALERT_SUCCESS_DATADOG_METRIC,
+)
 from sentry.integrations.slack.sdk_client import SlackSdkClient
 from sentry.models.integrations.integration import Integration
 from sentry.models.options.organization_option import OrganizationOption
 from sentry.services.hybrid_cloud.integration import integration_service
 from sentry.shared_integrations.exceptions import ApiError
-from sentry.shared_integrations.response import BaseApiResponse, MappingApiResponse
+from sentry.utils import metrics
 
 from . import logger
 
@@ -65,15 +69,6 @@ def send_incident_alert_notification(
     text = str(attachment["text"])
     blocks = {"blocks": attachment["blocks"], "color": attachment["color"]}
     attachments = orjson.dumps([blocks]).decode()
-    payload = {
-        "channel": channel,
-        "text": text,
-        "attachments": attachments,
-        # Prevent duplicate unfurl
-        # https://api.slack.com/reference/messaging/link-unfurling#no_unfurling_please
-        "unfurl_links": False,
-        "unfurl_media": False,
-    }
 
     repository: MetricAlertNotificationMessageRepository = get_default_metric_alert_repository()
     parent_notification_message = None
@@ -109,87 +104,51 @@ def send_incident_alert_notification(
         )
         # To reply to a thread, use the specific key in the payload as referenced by the docs
         # https://api.slack.com/methods/chat.postMessage#arg_thread_ts
-        payload["thread_ts"] = parent_notification_message.message_identifier
         thread_ts = parent_notification_message.message_identifier
 
         # If the incident is critical status, even if it's in a thread, send to main channel
         if incident.status == IncidentStatus.CRITICAL.value:
-            payload["reply_broadcast"] = True
             reply_broadcast = True
 
     success = False
-    if features.has("organizations:slack-sdk-metric-alert", organization):
-        try:
-            sdk_client = SlackSdkClient(integration_id=integration.id)
-            sdk_response = sdk_client.chat_postMessage(
-                attachments=attachments,
-                text=text,
-                channel=str(channel),
-                thread_ts=thread_ts,
-                reply_broadcast=reply_broadcast,
-                unfurl_links=False,
-                unfurl_media=False,
-            )
-        except SlackApiError as e:
-            # Record the error code and details from the exception
-            new_notification_message_object.error_code = e.response.status_code
-            new_notification_message_object.error_details = {
-                "msg": str(e),
-                "data": e.response.data,
-                "url": e.response.api_url,
-            }
-
-            log_params = {
-                "error": str(e),
-                "incident_id": incident.id,
-                "incident_status": new_status,
-                "attachments": attachments,
-            }
-            logger.info("slack.metric_alert.error", exc_info=True, extra=log_params)
-        else:
-            success = True
-            ts = sdk_response.get("ts")
-
-            logger.info(
-                "slack.metric_alert.ts", extra={"ts": ts, "attachments": attachments, "text": text}
-            )
-
-            new_notification_message_object.message_identifier = str(ts) if ts is not None else None
-
+    try:
+        client = SlackSdkClient(integration_id=integration.id)
+        response = client.chat_postMessage(
+            attachments=attachments,
+            text=text,
+            channel=str(channel),
+            thread_ts=thread_ts,
+            reply_broadcast=reply_broadcast,
+            unfurl_links=False,
+            unfurl_media=False,
+        )
+        metrics.incr(SLACK_METRIC_ALERT_SUCCESS_DATADOG_METRIC, sample_rate=1.0)
+    except SlackApiError as e:
+        # Record the error code and details from the exception
+        new_notification_message_object.error_code = e.response.status_code
+        new_notification_message_object.error_details = {
+            "msg": str(e),
+            "data": e.response.data,
+            "url": e.response.api_url,
+        }
+
+        log_params = {
+            "error": str(e),
+            "incident_id": incident.id,
+            "incident_status": new_status,
+            "attachments": attachments,
+        }
+        logger.info("slack.metric_alert.error", exc_info=True, extra=log_params)
+        metrics.incr(
+            SLACK_METRIC_ALERT_FAILURE_DATADOG_METRIC,
+            sample_rate=1.0,
+            tags={"ok": e.response.get("ok", False), "status": e.response.status_code},
+        )
     else:
-        try:
-            client = SlackClient(integration_id=integration.id)
-            response = client.post("/chat.postMessage", data=payload, timeout=5)
-            # response should include a "ts" key that represents the unique identifier for the message
-            # referenced at https://api.slack.com/methods/chat.postMessage#examples
-        except ApiError as e:
-            # Record the error code and details from the exception
-            new_notification_message_object.error_code = e.code
-            new_notification_message_object.error_details = {
-                "url": e.url,
-                "host": e.host,
-                "path": e.path,
-                "data": e.json if e.json else e.text,
-            }
-            logger.info("rule.fail.slack_post", exc_info=True)
-        else:
-            success = True
-            # Slack will always send back a ts identifier https://api.slack.com/methods/chat.postMessage#examples
-            # on a successful message
-            ts = None
-            # This is a workaround for typing, and the dynamic nature of the return value
-            if isinstance(response, BaseApiResponse):
-                ts = response.json.get("ts")
-            elif isinstance(response, MappingApiResponse):
-                ts = response.get("ts")
-            else:
-                logger.info(
-                    "failed to get ts from slack response",
-                    extra={
-                        "response_type": type(response).__name__,
-                    },
-                )
-            new_notification_message_object.message_identifier = str(ts) if ts is not None else None
+        success = True
+        ts = response.get("ts")
+
+        new_notification_message_object.message_identifier = str(ts) if ts is not None else None
 
     # Save the notification message we just sent with the response id or error we received
     try:

tests/sentry/incidents/action_handlers/__init__.py

@@ -23,4 +23,4 @@ def run_fire_test(self, method="fire", chart_url=None):
             update_incident_status(
                 incident, IncidentStatus.CLOSED, status_method=IncidentStatusMethod.MANUAL
             )
-        self.run_test(incident, method, **kwargs)
+        return self.run_test(incident, method, **kwargs)

tests/sentry/incidents/action_handlers/test_slack.py

@@ -1,5 +1,4 @@
 from unittest.mock import patch
-from urllib.parse import parse_qs
 
 import orjson
 import responses
@@ -9,10 +8,14 @@
 from sentry.incidents.logic import update_incident_status
 from sentry.incidents.models.alert_rule import AlertRuleTriggerAction
 from sentry.incidents.models.incident import IncidentStatus, IncidentStatusMethod
+from sentry.integrations.slack.message_builder.incidents import SlackIncidentsMessageBuilder
+from sentry.integrations.slack.metrics import (
+    SLACK_METRIC_ALERT_FAILURE_DATADOG_METRIC,
+    SLACK_METRIC_ALERT_SUCCESS_DATADOG_METRIC,
+)
 from sentry.models.notificationmessage import NotificationMessage
 from sentry.models.options.organization_option import OrganizationOption
 from sentry.testutils.helpers.datetime import freeze_time
-from sentry.testutils.helpers.features import with_feature
 from sentry.utils import json
 
 from . import FireTest
@@ -55,74 +58,59 @@ def setUp(self):
         )
         self.alert_rule = self.create_alert_rule()
 
-    @responses.activate
     def run_test(self, incident, method, chart_url=None):
-        from sentry.integrations.slack.message_builder.incidents import SlackIncidentsMessageBuilder
-
-        responses.add(
-            method=responses.POST,
-            url="https://slack.com/api/chat.postMessage",
-            status=200,
-            content_type="application/json",
-            body='{"ok": true}',
-        )
         handler = SlackActionHandler(self.action, incident, self.project)
         metric_value = 1000
         status = IncidentStatus(incident.status)
         with self.tasks():
             getattr(handler, method)(metric_value, status)
-        data = parse_qs(responses.calls[0].request.body)
-        assert data["channel"] == [self.channel_id]
+
+        return incident, chart_url
+
+    def _assert_blocks(self, mock_post, incident, metric_value, chart_url):
         slack_body = SlackIncidentsMessageBuilder(
             self.action, incident, IncidentStatus(incident.status), metric_value, chart_url
         ).build()
         assert isinstance(slack_body, dict)
-        attachments = json.loads(data["attachments"][0])
+        attachments = orjson.loads(mock_post.call_args.kwargs["attachments"])
         assert attachments[0]["color"] == slack_body["color"]
         assert attachments[0]["blocks"][0] in slack_body["blocks"]
-        assert data["text"][0] == slack_body["text"]
-
-    def test_fire_metric_alert(self):
-        self.run_fire_test()
-
-        assert NotificationMessage.objects.all().count() == 1
+        assert mock_post.call_args.kwargs["text"] == slack_body["text"]
 
     @patch("slack_sdk.web.client.WebClient._perform_urllib_http_request")
-    @with_feature("organizations:slack-sdk-metric-alert")
-    def test_fire_metric_alert_sdk(self, mock_api_call):
+    @patch("sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage")
+    @patch("sentry.integrations.slack.utils.notifications.metrics")
+    def test_fire_metric_alert_sdk(self, mock_metrics, mock_post, mock_api_call):
         mock_api_call.return_value = {
             "body": orjson.dumps({"ok": True}).decode(),
             "headers": {},
             "status": 200,
         }
 
-        incident = self.create_incident(
-            alert_rule=self.alert_rule, status=IncidentStatus.CLOSED.value
-        )
-        handler = SlackActionHandler(self.action, incident, self.project)
-        metric_value = 1000
-        status = IncidentStatus(incident.status)
-        with self.tasks():
-            getattr(handler, "fire")(metric_value, status)
+        incident, chart_url = self.run_fire_test()
+        self._assert_blocks(mock_post, incident, 1000, chart_url)
 
         assert NotificationMessage.objects.all().count() == 1
-
-    @with_feature("organizations:slack-sdk-metric-alert")
-    def test_fire_metric_alert_sdk_error(self):
-        incident = self.create_incident(
-            alert_rule=self.alert_rule, status=IncidentStatus.CLOSED.value
+        mock_metrics.incr.assert_called_with(
+            SLACK_METRIC_ALERT_SUCCESS_DATADOG_METRIC,
+            sample_rate=1.0,
         )
-        handler = SlackActionHandler(self.action, incident, self.project)
-        metric_value = 1000
-        status = IncidentStatus(incident.status)
-        with self.tasks():
-            getattr(handler, "fire")(metric_value, status)
+
+    @patch("sentry.integrations.slack.utils.notifications.metrics")
+    def test_fire_metric_alert_sdk_error(self, mock_metrics):
+        self.run_fire_test()
 
         assert NotificationMessage.objects.all().count() == 1
         msg = NotificationMessage.objects.all()[0]
         assert msg.error_code == 200
         assert msg.error_details["data"] == {"ok": False, "error": "invalid_auth"}
 
+        mock_metrics.incr.assert_called_with(
+            SLACK_METRIC_ALERT_FAILURE_DATADOG_METRIC,
+            sample_rate=1.0,
+            tags={"ok": False, "status": 200},
+        )
+
     def test_resolve_metric_alert_no_threading(self):
         OrganizationOption.objects.set_value(
             self.organization, "sentry:metric_alerts_thread_flag", False
@@ -179,51 +167,43 @@ def test_fire_metric_alert_with_missing_integration(self):
         with self.tasks():
             handler.fire(metric_value, IncidentStatus(incident.status))
 
-    @responses.activate
-    def test_rule_snoozed(self):
+    @patch("sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage")
+    def test_rule_snoozed(self, mock_post):
         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="https://slack.com/api/chat.postMessage",
-            status=200,
-            content_type="application/json",
-            body='{"ok": true}',
-        )
         handler = SlackActionHandler(self.action, incident, self.project)
         metric_value = 1000
         with self.tasks():
             handler.fire(metric_value, IncidentStatus(incident.status))
 
-        assert len(responses.calls) == 0
+        assert not mock_post.called
 
-    @responses.activate
-    def test_rule_snoozed_by_user_still_sends(self):
+    @patch("sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage")
+    def test_rule_snoozed_by_user_still_sends(self, mock_post):
         """We shouldn't be able to get into this state from the UI, but this test ensures that if an alert whose action
         is to notify an integration is muted for a specific user, that the alert still fires because it should only NOT
         fire if it's muted for everyone"""
         alert_rule = self.create_alert_rule()
         incident = self.create_incident(alert_rule=alert_rule, status=IncidentStatus.CLOSED.value)
         self.snooze_rule(user_id=self.user.id, alert_rule=alert_rule)
 
-        responses.add(
-            method=responses.POST,
-            url="https://slack.com/api/chat.postMessage",
-            status=200,
-            content_type="application/json",
-            body='{"ok": true}',
-        )
         handler = SlackActionHandler(self.action, incident, self.project)
         metric_value = 1000
         with self.tasks():
             handler.fire(metric_value, IncidentStatus(incident.status))
 
-        assert len(responses.calls) == 1
+        mock_post.assert_called
 
     @patch("sentry.analytics.record")
-    def test_alert_sent_recorded(self, mock_record):
+    @patch("slack_sdk.web.client.WebClient._perform_urllib_http_request")
+    def test_alert_sent_recorded(self, mock_api_call, mock_record):
+        mock_api_call.return_value = {
+            "body": orjson.dumps({"ok": True}).decode(),
+            "headers": {},
+            "status": 200,
+        }
         self.run_fire_test()
         mock_record.assert_called_with(
             "alert.sent",

tests/sentry/incidents/test_subscription_processor.py

@@ -183,7 +183,9 @@ def assert_incident_is_latest_for_rule(self, incident):
 class ProcessUpdateTest(ProcessUpdateBaseClass):
     @pytest.fixture(autouse=True)
     def _setup_slack_client(self):
-        with mock.patch("sentry.integrations.slack.SlackClient.post") as self.slack_client:
+        with mock.patch(
+            "sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage"
+        ) as self.slack_client:
             yield
 
     @cached_property
@@ -292,7 +294,7 @@ def send_update(self, rule, value, time_delta=None, subscription=None):
     def assert_slack_calls(self, trigger_labels):
         expected_result = [f"{label}: some rule 2" for label in trigger_labels]
         actual = [
-            (call_kwargs["data"]["text"], json.loads(call_kwargs["data"]["attachments"]))
+            (call_kwargs["text"], json.loads(call_kwargs["attachments"]))
             for (_, call_kwargs) in self.slack_client.call_args_list
         ]