improve unfurl slos

Author: cathteng

src/sentry/integrations/messaging/metrics.py

@@ -37,6 +37,7 @@ class MessagingInteractionType(StrEnum):
     VIEW_SUBMISSION = "VIEW_SUBMISSION"
 
     # Automatic behaviors
+    PROCESS_SHARED_LINK = "PROCESS_SHARED_LINK"
     UNFURL_LINK = "UNFURL_LINK"
     UNFURL_ISSUES = "UNFURL_ISSUES"
     UNFURL_METRIC_ALERTS = "UNFURL_METRIC_ALERTS"

src/sentry/integrations/slack/unfurl/discover.py

@@ -123,10 +123,10 @@ def unfurl_discover(
     links: list[UnfurlableUrl],
     user: User | RpcUser | None = None,
 ) -> UnfurledUrl:
-    event = MessagingInteractionEvent(
+    with MessagingInteractionEvent(
         MessagingInteractionType.UNFURL_DISCOVER, SlackMessagingSpec(), user=user
-    )
-    with event.capture():
+    ).capture() as lifecycle:
+        lifecycle.add_extras({"integration_id": integration.id})
         return _unfurl_discover(integration, links, user)
 
 

src/sentry/integrations/slack/unfurl/issues.py

@@ -43,10 +43,10 @@ def unfurl_issues(
     for a particular issue by the URL of the yet-unfurled links a user included
     in their Slack message.
     """
-    event = MessagingInteractionEvent(
+    with MessagingInteractionEvent(
         MessagingInteractionType.UNFURL_ISSUES, SlackMessagingSpec(), user=user
-    )
-    with event.capture():
+    ).capture() as lifecycle:
+        lifecycle.add_extras({"integration_id": integration.id})
         return _unfurl_issues(integration, links)
 
 

src/sentry/integrations/slack/unfurl/metric_alerts.py

@@ -57,10 +57,10 @@ def unfurl_metric_alerts(
     links: list[UnfurlableUrl],
     user: User | RpcUser | None = None,
 ) -> UnfurledUrl:
-    event = MessagingInteractionEvent(
+    with MessagingInteractionEvent(
         MessagingInteractionType.UNFURL_METRIC_ALERTS, SlackMessagingSpec(), user=user
-    )
-    with event.capture():
+    ).capture() as lifecycle:
+        lifecycle.add_extras({"integration_id": integration.id})
         return _unfurl_metric_alerts(integration, links, user)
 
 

src/sentry/integrations/slack/webhooks/event.py

@@ -30,7 +30,6 @@
 from sentry.integrations.slack.unfurl.handlers import link_handlers, match_link
 from sentry.integrations.slack.unfurl.types import LinkType, UnfurlableUrl
 from sentry.integrations.slack.views.link_identity import build_linking_url
-from sentry.integrations.utils.metrics import IntegrationEventLifecycle
 from sentry.organizations.services.organization import organization_service
 
 from .base import SlackDMEndpoint
@@ -160,82 +159,89 @@ def on_message(self, request: Request, slack_request: SlackDMRequest) -> Respons
 
         return self.respond()
 
-    def on_link_shared(
-        self, request: Request, slack_request: SlackDMRequest, lifecycle: IntegrationEventLifecycle
-    ) -> bool:
+    def on_link_shared(self, request: Request, slack_request: SlackDMRequest) -> bool:
         """Returns true on success"""
         matches: MutableMapping[LinkType, list[UnfurlableUrl]] = defaultdict(list)
         links_seen = set()
 
         data = slack_request.data.get("event", {})
 
+        ois = integration_service.get_organization_integrations(
+            integration_id=slack_request.integration.id, limit=1
+        )
+        organization_id = ois[0].organization_id if len(ois) > 0 else None
+        organization_context = (
+            organization_service.get_organization_by_id(
+                id=organization_id,
+                user_id=None,
+                include_projects=False,
+                include_teams=False,
+            )
+            if organization_id
+            else None
+        )
+        organization = organization_context.organization if organization_context else None
+
         logger_params = {
             "integration_id": slack_request.integration.id,
             "team_id": slack_request.team_id,
             "channel_id": slack_request.channel_id,
             "user_id": slack_request.user_id,
             "channel": slack_request.channel_id,
+            "organization_id": organization_id,
             **data,
         }
 
         # An unfurl may have multiple links to unfurl
         for item in data.get("links", []):
-            try:
-                url = item["url"]
-            except Exception:
-                _logger.exception("parse-link-error", extra={**logger_params, "url": url})
-                continue
-
-            link_type, args = match_link(url)
-
-            # Link can't be unfurled
-            if link_type is None or args is None:
-                continue
-
-            ois = integration_service.get_organization_integrations(
-                integration_id=slack_request.integration.id, limit=1
-            )
-            organization_id = ois[0].organization_id if len(ois) > 0 else None
-            organization_context = (
-                organization_service.get_organization_by_id(
-                    id=organization_id, user_id=None, include_projects=False, include_teams=False
-                )
-                if organization_id
-                else None
-            )
-            organization = organization_context.organization if organization_context else None
-            logger_params["organization_id"] = organization_id
-
-            if (
-                organization
-                and link_type == LinkType.DISCOVER
-                and not slack_request.has_identity
-                and features.has("organizations:discover-basic", organization, actor=request.user)
-            ):
+            with MessagingInteractionEvent(
+                interaction_type=MessagingInteractionType.PROCESS_SHARED_LINK,
+                spec=SlackMessagingSpec(),
+            ).capture() as lifecycle:
                 try:
-                    analytics.record(
-                        SlackIntegrationChartUnfurl(
-                            organization_id=organization.id,
-                            unfurls_count=0,
-                        )
+                    url = item["url"]
+                except Exception:
+                    lifecycle.record_failure("Failed to parse link", extra={**logger_params})
+                    continue
+
+                link_type, args = match_link(url)
+
+                # Link can't be unfurled
+                if link_type is None or args is None:
+                    lifecycle.record_halt("Unfurlable link", extra={"url": url})
+                    continue
+
+                if (
+                    organization
+                    and link_type == LinkType.DISCOVER
+                    and not slack_request.has_identity
+                    and features.has(
+                        "organizations:discover-basic", organization, actor=request.user
                     )
-                except Exception as e:
-                    sentry_sdk.capture_exception(e)
+                ):
+                    try:
+                        analytics.record(
+                            SlackIntegrationChartUnfurl(
+                                organization_id=organization.id,
+                                unfurls_count=0,
+                            )
+                        )
+                    except Exception as e:
+                        sentry_sdk.capture_exception(e)
 
-                self.prompt_link(slack_request)
-                return True
+                    self.prompt_link(slack_request)
+                    lifecycle.record_halt("Discover link requires identity", extra={"url": url})
+                    return True
 
-            # Don't unfurl the same thing multiple times
-            seen_marker = hash(orjson.dumps((link_type, list(args))).decode())
-            if seen_marker in links_seen:
-                continue
+                # Don't unfurl the same thing multiple times
+                seen_marker = hash(orjson.dumps((link_type, list(args))).decode())
+                if seen_marker in links_seen:
+                    continue
 
-            links_seen.add(seen_marker)
-            matches[link_type].append(UnfurlableUrl(url=url, args=args))
+                links_seen.add(seen_marker)
+                matches[link_type].append(UnfurlableUrl(url=url, args=args))
 
         if not matches:
-            lifecycle.add_extras(logger_params)
-            lifecycle.record_failure("No matches found")
             return False
 
         # Unfurl each link type
@@ -251,8 +257,6 @@ def on_link_shared(
             )
 
         if not results:
-            lifecycle.add_extras(logger_params)
-            lifecycle.record_failure("No unfurls generated")
             return False
 
         # XXX(isabella): we use our message builders to create the blocks for each link to be
@@ -262,21 +266,24 @@ def on_link_shared(
             if "text" in link_info:
                 del link_info["text"]
 
-        payload = {"channel": data["channel"], "ts": data["message_ts"], "unfurls": results}
-
-        client = SlackSdkClient(integration_id=slack_request.integration.id)
-        try:
-            client.chat_unfurl(
-                channel=data["channel"],
-                ts=data["message_ts"],
-                unfurls=payload["unfurls"],
-            )
-        except SlackApiError as e:
-            lifecycle.add_extras(logger_params)
-            if options.get("slack.log-unfurl-payload", False):
-                lifecycle.add_extra("unfurls", payload["unfurls"])
-            lifecycle.record_failure(e)
-            return False
+        with MessagingInteractionEvent(
+            interaction_type=MessagingInteractionType.UNFURL_LINK,
+            spec=SlackMessagingSpec(),
+        ).capture() as lifecycle:
+            payload = {"channel": data["channel"], "ts": data["message_ts"], "unfurls": results}
+            client = SlackSdkClient(integration_id=slack_request.integration.id)
+            try:
+                client.chat_unfurl(
+                    channel=data["channel"],
+                    ts=data["message_ts"],
+                    unfurls=payload["unfurls"],
+                )
+            except SlackApiError as e:
+                lifecycle.add_extras(logger_params)
+                if options.get("slack.log-unfurl-payload", False):
+                    lifecycle.add_extra("unfurls", payload["unfurls"])
+                lifecycle.record_failure(e)
+                return False
 
         return True
 
@@ -291,12 +298,8 @@ def post(self, request: Request) -> Response:
         if slack_request.is_challenge():
             return self.on_url_verification(request, slack_request.data)
         if slack_request.type == "link_shared":
-            with MessagingInteractionEvent(
-                interaction_type=MessagingInteractionType.UNFURL_LINK,
-                spec=SlackMessagingSpec(),
-            ).capture() as lifecycle:
-                if self.on_link_shared(request, slack_request, lifecycle):
-                    return self.respond()
+            if self.on_link_shared(request, slack_request):
+                return self.respond()
 
         if slack_request.type == "message":
             if slack_request.is_bot():

tests/sentry/integrations/slack/webhooks/events/test_link_shared.py

@@ -9,7 +9,7 @@
 
 from sentry.integrations.slack.unfurl.types import Handler, make_type_coercer
 from sentry.integrations.types import EventLifecycleOutcome
-from sentry.testutils.asserts import assert_slo_metric
+from sentry.testutils.asserts import assert_slo_metric_calls
 
 from . import LINK_SHARED_EVENT, BaseEventTest, build_test_block
 
@@ -108,7 +108,7 @@ def test_share_links_block_kit_sdk(self, mock_match_link, mock_record) -> None:
         assert unfurls["link1"] == result1
         assert unfurls["link2"] == result2
 
-        assert_slo_metric(mock_record, EventLifecycleOutcome.SUCCESS)
+        assert_slo_metric_calls(mock_record.mock_calls[-2:], EventLifecycleOutcome.SUCCESS)
 
     @patch(
         "sentry.integrations.slack.webhooks.event.match_link",
@@ -155,4 +155,12 @@ def test_share_links_failure(self, mock_match_link, mock_record) -> None:
 
         assert resp.status_code == 200, resp.content
 
-        assert_slo_metric(mock_record, EventLifecycleOutcome.FAILURE)
+        assert_slo_metric_calls(mock_record.mock_calls[-2:], EventLifecycleOutcome.FAILURE)
+
+    def test_share_links_no_matches(self, mock_record) -> None:
+        event_data = orjson.loads(LINK_SHARED_EVENT)
+        event_data["links"] = [{}]
+        resp = self.post_webhook(event_data=event_data)
+        assert resp.status_code == 200, resp.content
+
+        assert_slo_metric_calls(mock_record.mock_calls[-2:], EventLifecycleOutcome.FAILURE)