diff --git a/src/sentry/integrations/slack/actions/notification.py b/src/sentry/integrations/slack/actions/notification.py index 17d3e46d2806e..4b92768f02387 100644 --- a/src/sentry/integrations/slack/actions/notification.py +++ b/src/sentry/integrations/slack/actions/notification.py @@ -5,6 +5,7 @@ from typing import Any import orjson +from slack_sdk.errors import SlackApiError from sentry import features from sentry.api.serializers.rest_framework.rule import ACTION_UUID_KEY @@ -22,7 +23,9 @@ from sentry.integrations.slack.message_builder.notifications.rule_save_edit import ( SlackRuleSaveEditMessageBuilder, ) +from sentry.integrations.slack.sdk_client import SlackSdkClient from sentry.integrations.slack.utils import get_channel_id +from sentry.integrations.slack.utils.options import has_slack_sdk_flag from sentry.models.integrations.integration import Integration from sentry.models.options.organization_option import OrganizationOption from sentry.models.rule import Rule @@ -103,8 +106,10 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: "unfurl_links": False, "unfurl_media": False, } + json_blocks = None if payload_blocks := blocks.get("blocks"): - payload["blocks"] = orjson.dumps(payload_blocks).decode() + json_blocks = orjson.dumps(payload_blocks).decode() + payload["blocks"] = json_blocks rule = rules[0] if rules else None rule_to_use = self.rule if self.rule else rule @@ -128,6 +133,8 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: # Only try to get the parent notification message if the organization is in the FF # We need to search by rule action uuid and rule id, so only search if they exist + reply_broadcast = False + thread_ts = None if ( features.has( "organizations:slack-thread-issue-alert", event.group.project.organization @@ -161,56 +168,108 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: # 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 this flow is triggered again for the same issue, we want it to be seen in the main channel payload["reply_broadcast"] = True + reply_broadcast = True + + organization = event.group.project.organization + + if has_slack_sdk_flag(organization.id): + sdk_client = SlackSdkClient(integration_id=integration.id) + text = str(payload["text"]) if payload["text"] is not None else None + try: + sdk_response = sdk_client.chat_postMessage( + blocks=json_blocks, + text=text, + channel=channel, + unfurl_links=False, + unfurl_media=False, + thread_ts=thread_ts, + reply_broadcast=reply_broadcast, + ) + 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), + "project_id": event.project_id, + "event_id": event.event_id, + "channel_name": self.get_option("channel"), + } + # temporarily log the payload so we can debug message failures + log_params["payload"] = orjson.dumps(payload).decode() + + self.logger.info( + "slack.issue-alert.error", + extra=log_params, + ) + else: + ts = None + response_data = sdk_response.data + if isinstance(response_data, dict): + ts = response_data.get("ts") + + self.logger.info("slack.issue-alert.ts", extra={"ts": ts}) + + new_notification_message_object.message_identifier = ( + str(ts) if ts is not None else None + ) - client = SlackClient(integration_id=integration.id) - try: - response = client.post( - "/chat.postMessage", data=payload, timeout=5, log_response_with_error=True - ) - 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, - } - - log_params = { - "error": str(e), - "project_id": event.project_id, - "event_id": event.event_id, - "channel_name": self.get_option("channel"), - } - # temporarily log the payload so we can debug message failures - log_params["payload"] = orjson.dumps(payload).decode() - - self.logger.info( - "rule.fail.slack_post", - extra=log_params, - ) else: - # 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") + client = SlackClient(integration_id=integration.id) + try: + response = client.post( + "/chat.postMessage", data=payload, timeout=5, log_response_with_error=True + ) + 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, + } + + log_params = { + "error": str(e), + "project_id": event.project_id, + "event_id": event.event_id, + "channel_name": self.get_option("channel"), + } + # temporarily log the payload so we can debug message failures + log_params["payload"] = orjson.dumps(payload).decode() + + self.logger.info( + "rule.fail.slack_post", + extra=log_params, + ) else: - _default_logger.info( - "failed to get ts from slack response", - extra={ - "response_type": type(response).__name__, - }, + # 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: + _default_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 ) - new_notification_message_object.message_identifier = ( - str(ts) if ts is not None else None - ) if ( features.has( diff --git a/src/sentry/integrations/slack/sdk_client.py b/src/sentry/integrations/slack/sdk_client.py new file mode 100644 index 0000000000000..43fb9fc4df85e --- /dev/null +++ b/src/sentry/integrations/slack/sdk_client.py @@ -0,0 +1,91 @@ +import logging +from functools import wraps +from types import FunctionType + +from slack_sdk import WebClient +from slack_sdk.errors import SlackApiError +from slack_sdk.web import SlackResponse + +from sentry.models.integrations import Integration +from sentry.services.hybrid_cloud.integration import integration_service +from sentry.silo.base import SiloMode +from sentry.utils import metrics + +SLACK_DATADOG_METRIC = "integrations.slack.http_response" + +logger = logging.getLogger(__name__) + + +def track_response_data(response: SlackResponse, error: str | None = None) -> None: + is_ok = response.get("ok", False) + code = response.status_code + + metrics.incr( + SLACK_DATADOG_METRIC, + sample_rate=1.0, + tags={"ok": is_ok, "status": code}, + ) + + extra = { + "integration": "slack", + "status_string": str(code), + "error": error, + } + logger.info("integration.http_response", extra=extra) + + +def wrapper(method): + @wraps(method) + def wrapped(*args, **kwargs): + try: + response = method(*args, **kwargs) + if isinstance(response, SlackResponse): + track_response_data(response=response) + except SlackApiError as e: + if e.response: + track_response_data(response=e.response, error=str(e)) + else: + logger.info("slack_sdk.missing_error_response", extra={"error": str(e)}) + raise + + return response + + return wrapped + + +def wrap_methods_in_class(cls): + for name, attribute in vars(cls).items(): + if isinstance(attribute, FunctionType): + setattr(cls, name, wrapper(attribute)) + + for base in cls.__bases__: + wrap_methods_in_class(base) + + +class MetaClass(type): + def __new__(meta, name, bases, dct): + cls = super().__new__(meta, name, bases, dct) + wrap_methods_in_class(cls) + return cls + + def __init__(cls, name, bases, dct): + super().__init__(name, bases, dct) + + +class SlackSdkClient(WebClient, metaclass=MetaClass): + def __init__(self, integration_id: int): + integration = None + if SiloMode.get_current_mode() == SiloMode.REGION: + integration = integration_service.get_integration(integration_id=integration_id) + else: # control or monolith (local) + integration = Integration.objects.filter(id=integration_id).first() + + if integration is None: + raise ValueError(f"Integration with id {integration_id} not found") + + access_token = integration.metadata.get("access_token") + if not access_token: + raise ValueError(f"Missing token for integration with id {integration_id}") + + # TODO: missing from old SlackClient: verify_ssl, logging_context + super().__init__(token=access_token) diff --git a/src/sentry/integrations/slack/utils/options.py b/src/sentry/integrations/slack/utils/options.py new file mode 100644 index 0000000000000..1404706b515c7 --- /dev/null +++ b/src/sentry/integrations/slack/utils/options.py @@ -0,0 +1,5 @@ +from sentry import options + + +def has_slack_sdk_flag(organization_id: int) -> bool: + return organization_id in options.get("slack.sdk-web-client") diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 9bcae92d2512a..fb5a0ee6c0e63 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -499,6 +499,12 @@ # signing-secret is preferred, but need to keep verification-token for apps that use it register("slack.verification-token", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) register("slack.signing-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) +register( + "slack.sdk-web-client", + type=Sequence, + default=[], + flags=FLAG_ALLOW_EMPTY | FLAG_AUTOMATOR_MODIFIABLE, +) # Codecov Integration register("codecov.client-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) diff --git a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py index d34ade8deace1..1a024faeae4ab 100644 --- a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py +++ b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py @@ -1,3 +1,4 @@ +from unittest.mock import patch from urllib.parse import parse_qs from uuid import uuid4 @@ -5,10 +6,12 @@ import responses from sentry.integrations.slack import SlackNotifyServiceAction +from sentry.integrations.slack.sdk_client import SLACK_DATADOG_METRIC from sentry.models.notificationmessage import NotificationMessage from sentry.models.rulefirehistory import RuleFireHistory from sentry.silo.base import SiloMode from sentry.testutils.cases import RuleTestCase +from sentry.testutils.helpers import override_options from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import assume_test_silo_mode from sentry.types.rules import RuleFuture @@ -18,6 +21,10 @@ class TestInit(RuleTestCase): rule_cls = SlackNotifyServiceAction def setUp(self) -> None: + with assume_test_silo_mode(SiloMode.REGION): + self.organization = self.create_organization(id=1, owner=self.user) + self.project = self.create_project(organization=self.organization) + with assume_test_silo_mode(SiloMode.CONTROL): self.integration = self.create_integration( organization=self.organization, @@ -94,6 +101,23 @@ def test_after(self): assert NotificationMessage.objects.all().count() == 0 + @override_options({"slack.sdk-web-client": [1]}) + @patch("sentry.integrations.slack.sdk_client.metrics") + def test_after_using_sdk(self, mock_metrics): + # tests error flow because we're actually trying to POST + + rule = self.get_rule(data=self.action_data) + results = list(rule.after(event=self.event)) + assert len(results) == 1 + + results[0].callback(self.event, futures=[]) + + mock_metrics.incr.assert_called_with( + SLACK_DATADOG_METRIC, sample_rate=1.0, tags={"ok": False, "status": 200} + ) + + assert NotificationMessage.objects.all().count() == 0 + @with_feature("organizations:slack-thread-issue-alert") @responses.activate def test_after_with_threads(self): diff --git a/tests/sentry/integrations/slack/test_sdk_client.py b/tests/sentry/integrations/slack/test_sdk_client.py new file mode 100644 index 0000000000000..190fdb9495157 --- /dev/null +++ b/tests/sentry/integrations/slack/test_sdk_client.py @@ -0,0 +1,71 @@ +from unittest.mock import patch + +import orjson +import pytest +from slack_sdk.errors import SlackApiError + +from sentry.integrations.slack.sdk_client import SLACK_DATADOG_METRIC, SlackSdkClient +from sentry.silo.base import SiloMode +from sentry.testutils.cases import TestCase +from sentry.testutils.silo import assume_test_silo_mode + + +class SlackClientTest(TestCase): + def setUp(self): + self.access_token = "xoxb-access-token" + self.integration, self.organization_integration = self.create_provider_integration_for( + organization=self.organization, + user=self.user, + external_id="slack:1", + provider="slack", + metadata={"access_token": self.access_token}, + ) + + def test_no_integration_found_error(self): + with pytest.raises(ValueError): + SlackSdkClient(integration_id=2) + + def test_no_access_token_error(self): + with assume_test_silo_mode(SiloMode.CONTROL): + self.integration.update(metadata={}) + + with pytest.raises(ValueError): + SlackSdkClient(integration_id=self.integration.id) + + @patch("sentry.integrations.slack.sdk_client.metrics") + @patch("slack_sdk.web.client.WebClient._perform_urllib_http_request") + def test_api_call_success(self, mock_api_call, mock_metrics): + mock_api_call.return_value = { + "body": orjson.dumps({"ok": True}).decode(), + "headers": {}, + "status": 200, + } + + client = SlackSdkClient(integration_id=self.integration.id) + + client.chat_postMessage(channel="#announcements", text="hello") + mock_metrics.incr.assert_called_with( + SLACK_DATADOG_METRIC, + sample_rate=1.0, + tags={"ok": True, "status": 200}, + ) + + @patch("sentry.integrations.slack.sdk_client.metrics") + @patch("slack_sdk.web.client.WebClient._perform_urllib_http_request") + def test_api_call_error(self, mock_api_call, mock_metrics): + mock_api_call.return_value = { + "body": orjson.dumps({"ok": False}).decode() + "'", + "headers": {}, + "status": 500, + } + + client = SlackSdkClient(integration_id=self.integration.id) + + with pytest.raises(SlackApiError): + client.chat_postMessage(channel="#announcements", text="hello") + + mock_metrics.incr.assert_called_with( + SLACK_DATADOG_METRIC, + sample_rate=1.0, + tags={"ok": False, "status": 500}, + )