feat(alerts): Add notification_uuid to sent analytics (#55937)

scttcper · web-flow · commit 2b04cf5a2f1c · 2023-09-12T10:19:25.000-07:00
fixes #54422
diff --git a/src/sentry/integrations/discord/actions/notification.py b/src/sentry/integrations/discord/actions/notification.py
@@ -73,6 +73,7 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None:
             organization_id=event.organization.id,
             project_id=event.project_id,
             group_id=event.group_id,
+            notification_uuid=notification_uuid if notification_uuid else "",
         )
         metrics.incr("notifications.sent", instance="discord.notifications", skip_internal=False)
         yield self.future(send_notification, key=key)
diff --git a/src/sentry/integrations/discord/analytics.py b/src/sentry/integrations/discord/analytics.py
@@ -8,6 +8,8 @@ class DiscordIntegrationNotificationSent(analytics.Event):
         analytics.Attribute("organization_id"),
         analytics.Attribute("project_id"),
         analytics.Attribute("group_id"),
+        analytics.Attribute("notification_uuid"),
+        analytics.Attribute("alert_id", required=False),
     )
 
 
diff --git a/src/sentry/integrations/msteams/analytics.py b/src/sentry/integrations/msteams/analytics.py
@@ -10,6 +10,8 @@ class MSTeamsIntegrationNotificationSent(analytics.Event):
         analytics.Attribute("category"),
         analytics.Attribute("actor_id"),
         analytics.Attribute("user_id", required=False),
+        analytics.Attribute("notification_uuid"),
+        analytics.Attribute("alert_id", required=False),
     )
 
 
diff --git a/src/sentry/integrations/slack/analytics.py b/src/sentry/integrations/slack/analytics.py
@@ -29,6 +29,8 @@ class SlackIntegrationNotificationSent(analytics.Event):
         analytics.Attribute("actor_id"),
         analytics.Attribute("user_id", required=False),
         analytics.Attribute("group_id", required=False),
+        analytics.Attribute("notification_uuid"),
+        analytics.Attribute("alert_id", required=False),
     )
 
 
diff --git a/src/sentry/mail/analytics.py b/src/sentry/mail/analytics.py
@@ -14,8 +14,8 @@ class EmailNotificationSent(analytics.Event):
         analytics.Attribute("group_id", required=False),
         analytics.Attribute("id"),
         analytics.Attribute("actor_type"),
-        # Remove after IssueAlertFallbackExperiment
-        analytics.Attribute("fallback_experiment", required=False),
+        analytics.Attribute("notification_uuid"),
+        analytics.Attribute("alert_id", required=False),
     )
 
 
diff --git a/src/sentry/notifications/notifications/base.py b/src/sentry/notifications/notifications/base.py
@@ -158,6 +158,7 @@ def record_notification_sent(self, recipient: RpcActor, provider: ExternalProvid
             self.record_analytics(
                 f"integrations.{provider.name}.notification_sent",
                 category=self.metrics_key,
+                notification_uuid=self.notification_uuid if self.notification_uuid else "",
                 **self.get_log_params(recipient),
             )
             # record an optional second event
diff --git a/src/sentry/notifications/notifications/digest.py b/src/sentry/notifications/notifications/digest.py
@@ -216,3 +216,16 @@ def send(self) -> None:
                         participants_to_remove.add(participant)
                 participants -= participants_to_remove
             notify(provider, self, participants, shared_context, extra_context)
+
+    def get_log_params(self, recipient: RpcActor) -> Mapping[str, Any]:
+        try:
+            alert_id = list(self.digest.keys())[0].id
+        except Exception:
+            alert_id = None
+
+        return {
+            "target_type": self.target_type.value,
+            "target_identifier": self.target_identifier,
+            "alert_id": alert_id,
+            **super().get_log_params(recipient),
+        }
diff --git a/src/sentry/notifications/notifications/rules.py b/src/sentry/notifications/notifications/rules.py
@@ -291,5 +291,6 @@ def get_log_params(self, recipient: RpcActor) -> Mapping[str, Any]:
         return {
             "target_type": self.target_type,
             "target_identifier": self.target_identifier,
+            "alert_id": self.rules[0].id if self.rules else None,
             **super().get_log_params(recipient),
         }
diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py
@@ -4,6 +4,7 @@
 from functools import cached_property
 from typing import Mapping, Sequence
 from unittest import mock
+from unittest.mock import ANY
 
 import pytz
 from django.contrib.auth.models import AnonymousUser
@@ -170,7 +171,8 @@ def test_project_level_prefix(self):
 
 
 class MailAdapterNotifyTest(BaseMailAdapterTest):
-    def test_simple_notification(self):
+    @mock.patch("sentry.analytics.record")
+    def test_simple_notification(self, mock_record):
         event = self.store_event(
             data={"message": "Hello world", "level": "error"}, project_id=self.project.id
         )
@@ -189,6 +191,21 @@ def test_simple_notification(self):
         assert isinstance(msg.alternatives[0][0], str)
         assert "my rule" in msg.alternatives[0][0]
         assert "notification_uuid" in msg.body
+        mock_record.assert_called_with(
+            "integrations.email.notification_sent",
+            category="issue_alert",
+            target_type=ANY,
+            target_identifier=None,
+            project_id=self.project.id,
+            organization_id=self.organization.id,
+            group_id=event.group_id,
+            actor_id=ANY,
+            user_id=ANY,
+            id=ANY,
+            actor_type="User",
+            notification_uuid=ANY,
+            alert_id=rule.id,
+        )
 
     def test_simple_snooze(self):
         """Test that notification for alert snoozed by user is not send to that user."""
diff --git a/tests/sentry/notifications/notifications/test_digests.py b/tests/sentry/notifications/notifications/test_digests.py
@@ -1,6 +1,6 @@
 import uuid
 from unittest import mock
-from unittest.mock import patch
+from unittest.mock import ANY, patch
 from urllib.parse import parse_qs
 
 import responses
@@ -94,14 +94,30 @@ def setUp(self):
                 teams=[self.team],
             )
 
-    def test_sends_digest_to_every_member(self):
+    @patch("sentry.analytics.record")
+    def test_sends_digest_to_every_member(self, mock_record):
         """Test that each member of the project the events are created in receive a digest email notification"""
         event_count = 4
         self.run_test(event_count=event_count, performance_issues=True, generic_issues=True)
         assert f"{event_count + 2} new alerts since" in mail.outbox[0].subject
         assert "N+1 Query" in mail.outbox[0].body
         assert "oh no" in mail.outbox[0].body
         assert self.build_occurrence_data()["issue_title"] in mail.outbox[0].body
+        mock_record.assert_called_with(
+            "integrations.email.notification_sent",
+            category="digest",
+            notification_uuid=ANY,
+            target_type="IssueOwners",
+            target_identifier=None,
+            alert_id=self.rule.id,
+            project_id=self.project.id,
+            organization_id=self.organization.id,
+            actor_id=ANY,
+            id=ANY,
+            actor_type="User",
+            group_id=None,
+            user_id=ANY,
+        )
 
     def test_sends_alert_rule_notification_to_each_member(self):
         """Test that if there is only one event it is sent as a regular alert rule notification"""
diff --git a/tests/sentry/notifications/test_notifications.py b/tests/sentry/notifications/test_notifications.py
@@ -2,7 +2,7 @@
 import uuid
 from time import time
 from unittest.mock import patch
-from urllib.parse import parse_qs
+from urllib.parse import parse_qs, urlparse
 
 import responses
 from django.conf import settings
@@ -54,6 +54,13 @@ def get_attachment():
     return attachments[0], data["text"][0]
 
 
+def get_notification_uuid(url: str):
+    query_params = parse_qs(urlparse(url).query)
+    notification_uuid = query_params["notification_uuid"][0]
+    assert len(notification_uuid) > 1
+    return notification_uuid
+
+
 # The analytics event `name` was called with `kwargs` being a subset of its properties
 def analytics_called_with_args(fn, name, **kwargs):
     for call_args, call_kwargs in fn.call_args_list:
@@ -216,19 +223,22 @@ def test_sends_resolution_notification(self, record_analytics):
             attachment["footer"]
             == f"{self.project.slug} | <http://testserver/settings/account/notifications/workflow/?referrer=resolved_activity-slack-user|Notification Settings>"
         )
+        notification_uuid = get_notification_uuid(attachment["title_link"])
         assert analytics_called_with_args(
             record_analytics,
             "integrations.email.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=self.group.id,
+            notification_uuid=notification_uuid,
         )
         assert analytics_called_with_args(
             record_analytics,
             "integrations.slack.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=self.group.id,
+            notification_uuid=notification_uuid,
         )
 
     @responses.activate
@@ -336,19 +346,22 @@ def test_sends_regression_notification(self, record_analytics):
             attachment["footer"]
             == f"{self.project.slug} | <http://testserver/settings/account/notifications/workflow/?referrer=regression_activity-slack-user|Notification Settings>"
         )
+        notification_uuid = get_notification_uuid(attachment["title_link"])
         assert analytics_called_with_args(
             record_analytics,
             "integrations.email.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=group.id,
+            notification_uuid=notification_uuid,
         )
         assert analytics_called_with_args(
             record_analytics,
             "integrations.slack.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=group.id,
+            notification_uuid=notification_uuid,
         )
 
     @responses.activate
@@ -390,19 +403,22 @@ def test_sends_resolved_in_release_notification(self, record_analytics):
             attachment["footer"]
             == f"{self.project.slug} | <http://testserver/settings/account/notifications/workflow/?referrer=resolved_in_release_activity-slack-user|Notification Settings>"
         )
+        notification_uuid = get_notification_uuid(attachment["title_link"])
         assert analytics_called_with_args(
             record_analytics,
             "integrations.email.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=self.group.id,
+            notification_uuid=notification_uuid,
         )
         assert analytics_called_with_args(
             record_analytics,
             "integrations.slack.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=self.group.id,
+            notification_uuid=notification_uuid,
         )
 
     @responses.activate
@@ -468,17 +484,21 @@ def test_sends_issue_notification(self, record_analytics):
             attachment["footer"]
             == f"{self.project.slug} | <http://testserver/settings/account/notifications/alerts/?referrer=issue_alert-slack-user|Notification Settings>"
         )
+        notification_uuid = get_notification_uuid(attachment["title_link"])
+        assert len(notification_uuid) > 1
         assert analytics_called_with_args(
             record_analytics,
             "integrations.email.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=event.group_id,
+            notification_uuid=notification_uuid,
         )
         assert analytics_called_with_args(
             record_analytics,
             "integrations.slack.notification_sent",
             user_id=self.user.id,
             organization_id=self.organization.id,
             group_id=event.group_id,
+            notification_uuid=notification_uuid,
         )

Original file line number	Diff line number	Diff line change
`@@ -73,6 +73,7 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None:`
`73`	`73`	`organization_id=event.organization.id,`
`74`	`74`	`project_id=event.project_id,`
`75`	`75`	`group_id=event.group_id,`
	`76`	`+ notification_uuid=notification_uuid if notification_uuid else "",`
`76`	`77`	`)`
`77`	`78`	`metrics.incr("notifications.sent", instance="discord.notifications", skip_internal=False)`
`78`	`79`	`yield self.future(send_notification, key=key)`
Original file line number	Diff line number	Diff line change
`@@ -8,6 +8,8 @@ class DiscordIntegrationNotificationSent(analytics.Event):`
`8`	`8`	`analytics.Attribute("organization_id"),`
`9`	`9`	`analytics.Attribute("project_id"),`
`10`	`10`	`analytics.Attribute("group_id"),`
	`11`	`+ analytics.Attribute("notification_uuid"),`
	`12`	`+ analytics.Attribute("alert_id", required=False),`
`11`	`13`	`)`
`12`	`14`
`13`	`15`
Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,8 @@ class MSTeamsIntegrationNotificationSent(analytics.Event):`
`10`	`10`	`analytics.Attribute("category"),`
`11`	`11`	`analytics.Attribute("actor_id"),`
`12`	`12`	`analytics.Attribute("user_id", required=False),`
	`13`	`+ analytics.Attribute("notification_uuid"),`
	`14`	`+ analytics.Attribute("alert_id", required=False),`
`13`	`15`	`)`
`14`	`16`
`15`	`17`
Original file line number	Diff line number	Diff line change
`@@ -29,6 +29,8 @@ class SlackIntegrationNotificationSent(analytics.Event):`
`29`	`29`	`analytics.Attribute("actor_id"),`
`30`	`30`	`analytics.Attribute("user_id", required=False),`
`31`	`31`	`analytics.Attribute("group_id", required=False),`
	`32`	`+ analytics.Attribute("notification_uuid"),`
	`33`	`+ analytics.Attribute("alert_id", required=False),`
`32`	`34`	`)`
`33`	`35`
`34`	`36`
Original file line number	Diff line number	Diff line change
`@@ -14,8 +14,8 @@ class EmailNotificationSent(analytics.Event):`
`14`	`14`	`analytics.Attribute("group_id", required=False),`
`15`	`15`	`analytics.Attribute("id"),`
`16`	`16`	`analytics.Attribute("actor_type"),`
`17`		`- # Remove after IssueAlertFallbackExperiment`
`18`		`- analytics.Attribute("fallback_experiment", required=False),`
	`17`	`+ analytics.Attribute("notification_uuid"),`
	`18`	`+ analytics.Attribute("alert_id", required=False),`
`19`	`19`	`)`
`20`	`20`
`21`	`21`
Original file line number	Diff line number	Diff line change
`@@ -158,6 +158,7 @@ def record_notification_sent(self, recipient: RpcActor, provider: ExternalProvid`
`158`	`158`	`self.record_analytics(`
`159`	`159`	`f"integrations.{provider.name}.notification_sent",`
`160`	`160`	`category=self.metrics_key,`
	`161`	`+ notification_uuid=self.notification_uuid if self.notification_uuid else "",`
`161`	`162`	`**self.get_log_params(recipient),`
`162`	`163`	`)`
`163`	`164`	`# record an optional second event`
Original file line number	Diff line number	Diff line change
`@@ -291,5 +291,6 @@ def get_log_params(self, recipient: RpcActor) -> Mapping[str, Any]:`
`291`	`291`	`return {`
`292`	`292`	`"target_type": self.target_type,`
`293`	`293`	`"target_identifier": self.target_identifier,`
	`294`	`+ "alert_id": self.rules[0].id if self.rules else None,`
`294`	`295`	`**super().get_log_params(recipient),`
`295`	`296`	`}`