Browse Source

ref(slack): Formalize SlackApiError message categories (#74040)

Refine the way we unpack and parse `SlackApiError` objects. Formalize
the set of expected error conditions.
Ryan Skonnord 7 months ago
parent
commit
0f423349d6

+ 7 - 4
src/sentry/integrations/slack/notifications.py

@@ -14,6 +14,11 @@ from sentry.integrations.slack.metrics import (
     SLACK_NOTIFY_MIXIN_SUCCESS_DATADOG_METRIC,
 )
 from sentry.integrations.slack.service import SlackService
+from sentry.integrations.slack.utils.errors import (
+    CHANNEL_NOT_FOUND,
+    EXPIRED_URL,
+    unpack_slack_api_error,
+)
 from sentry.integrations.types import ExternalProviders
 from sentry.notifications.notifications.base import BaseNotification
 from sentry.notifications.notify import register_notification_provider
@@ -34,13 +39,11 @@ class SlackNotifyBasicMixin(NotifyBasicMixin):
         except SlackApiError as e:
             metrics.incr(SLACK_NOTIFY_MIXIN_FAILURE_DATADOG_METRIC, sample_rate=1.0)
 
-            error = str(e)
-            message = error.split("\n")[0]
             # TODO: remove this
-            if "expired_url" not in message and "channel_not_found" not in message:
+            if unpack_slack_api_error(e) not in (EXPIRED_URL, CHANNEL_NOT_FOUND):
                 logger.exception(
                     "slack.slash-response.error",
-                    extra={"error": error},
+                    extra={"error": str(e)},
                 )
 
 

+ 8 - 3
src/sentry/integrations/slack/utils/channel.py

@@ -14,6 +14,11 @@ from sentry.integrations.slack.metrics import (
     SLACK_UTILS_CHANNEL_SUCCESS_DATADOG_METRIC,
 )
 from sentry.integrations.slack.sdk_client import SlackSdkClient
+from sentry.integrations.slack.utils.errors import (
+    CHANNEL_NOT_FOUND,
+    RATE_LIMITED,
+    unpack_slack_api_error,
+)
 from sentry.integrations.slack.utils.users import get_slack_user_list
 from sentry.models.organization import Organization
 from sentry.shared_integrations.exceptions import (
@@ -119,7 +124,7 @@ def validate_channel_id(name: str, integration_id: int | None, input_channel_id:
             },
         )
 
-        if "channel_not_found" in str(e):
+        if unpack_slack_api_error(e) == CHANNEL_NOT_FOUND:
             raise ValidationError("Channel not found. Invalid ID provided.") from e
 
         raise ValidationError("Could not retrieve Slack channel information.") from e
@@ -238,7 +243,7 @@ def check_user_with_timeout(
                 return SlackChannelIdData(prefix=_prefix, channel_id=None, timed_out=True)
     except SlackApiError as e:
         _logger.exception("rule.slack.user_check_error", extra=logger_params)
-        if "ratelimited" in str(e):
+        if unpack_slack_api_error(e) == RATE_LIMITED:
             metrics.incr(
                 SLACK_UTILS_CHANNEL_FAILURE_DATADOG_METRIC,
                 sample_rate=1.0,
@@ -285,7 +290,7 @@ def check_for_channel(
             "post_at": int(time.time() + 500),
         }
         _logger.exception("slack.chat_scheduleMessage.error", extra=logger_params)
-        if "channel_not_found" in str(e):
+        if unpack_slack_api_error(e) == CHANNEL_NOT_FOUND:
             return None
         else:
             raise

+ 51 - 0
src/sentry/integrations/slack/utils/errors.py

@@ -0,0 +1,51 @@
+import logging
+from dataclasses import dataclass
+
+from slack_sdk.errors import SlackApiError, SlackRequestError
+
+_logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True, eq=True)
+class SlackSdkErrorCategory:
+    message: str
+    check_body: bool = False
+
+
+SLACK_SDK_ERROR_CATEGORIES = (
+    RATE_LIMITED := SlackSdkErrorCategory("ratelimited"),
+    CHANNEL_NOT_FOUND := SlackSdkErrorCategory("channel_not_found"),
+    EXPIRED_URL := SlackSdkErrorCategory("Expired url", check_body=True),
+)
+
+_CATEGORIES_BY_MESSAGE = {c.message: c for c in SLACK_SDK_ERROR_CATEGORIES}
+
+
+def unpack_slack_api_error(exc: SlackApiError | SlackRequestError) -> SlackSdkErrorCategory | None:
+    """Retrieve the Slack API error category from an exception object.
+
+    Check three places in priority order:
+    1. the error field of the server response;
+    2. the first line of the message body; and,
+    3. for categories with the `check_body` flag, the rest of the message.
+    """
+
+    if isinstance(exc, SlackApiError):
+        try:
+            error_attr = exc.response["error"]
+            return _CATEGORIES_BY_MESSAGE[error_attr]
+        except KeyError:
+            pass
+
+    dump = str(exc)
+    category = _CATEGORIES_BY_MESSAGE.get(dump.split("\n")[0])
+    if category:
+        return category
+
+    for category in SLACK_SDK_ERROR_CATEGORIES:
+        if category.check_body and category.message in dump:
+            return category
+
+    # Indicate that a new value needs to be added to SLACK_SDK_ERROR_CATEGORIES
+    _logger.warning("unrecognized_slack_api_message", extra={"slack_api_message": dump})
+    return None

+ 3 - 2
src/sentry/integrations/slack/utils/notifications.py

@@ -26,6 +26,7 @@ from sentry.integrations.slack.metrics import (
     SLACK_METRIC_ALERT_SUCCESS_DATADOG_METRIC,
 )
 from sentry.integrations.slack.sdk_client import SlackSdkClient
+from sentry.integrations.slack.utils.errors import EXPIRED_URL, unpack_slack_api_error
 from sentry.integrations.slack.views.types import IdentityParams
 from sentry.models.options.organization_option import OrganizationOption
 from sentry.utils import metrics
@@ -180,7 +181,7 @@ def respond_to_slack_command(
                 tags={"type": "webhook", "command": command},
             )
         except (SlackApiError, SlackRequestError) as e:
-            if "Expired url" not in str(e):
+            if unpack_slack_api_error(e) != EXPIRED_URL:
                 metrics.incr(
                     SLACK_LINK_IDENTITY_MSG_FAILURE_DATADOG_METRIC,
                     sample_rate=1.0,
@@ -203,7 +204,7 @@ def respond_to_slack_command(
                 tags={"type": "ephemeral", "command": command},
             )
         except SlackApiError as e:
-            if "Expired url" not in str(e):
+            if unpack_slack_api_error(e) != EXPIRED_URL:
                 metrics.incr(
                     SLACK_LINK_IDENTITY_MSG_FAILURE_DATADOG_METRIC,
                     sample_rate=1.0,