Browse Source

adds secondary analytics event optionally (#30645)

This PR is part of a greater effort to increase standardization of notifications by adding support for two different analytics events for sending a notification. Now we will always record the integrations.{provider.name}.notification_sent event for every notification in addition to an optional one based on analytics_event. The purpose of this is to make it easier to do different types of analysis (e.g: looking at a specific notification funnel vs tracking how many times we send any type of Slack notification).
Stephen Cefali 3 years ago
parent
commit
1f4a80fa4c

+ 19 - 12
src/sentry/notifications/notifications/base.py

@@ -16,6 +16,7 @@ class BaseNotification(abc.ABC):
     message_builder = "SlackNotificationsMessageBuilder"
     fine_tuning_key: str | None = None
     metrics_key: str = ""
+    analytics_event: str = ""
 
     def __init__(self, organization: Organization):
         self.organization = organization
@@ -88,9 +89,6 @@ class BaseNotification(abc.ABC):
     def get_unsubscribe_key(self) -> tuple[str, int, str | None] | None:
         return None
 
-    def record_notification_sent(self, recipient: Team | User, provider: ExternalProviders) -> None:
-        raise NotImplementedError
-
     def get_log_params(self, recipient: Team | User) -> Mapping[str, Any]:
         return {
             "organization_id": self.organization.id,
@@ -103,6 +101,24 @@ class BaseNotification(abc.ABC):
     def get_callback_data(self) -> Mapping[str, Any] | None:
         return None
 
+    def record_analytics(self, event_name: str, **kwargs: Any) -> None:
+        analytics.record(event_name, **kwargs)
+
+    def record_notification_sent(self, recipient: Team | User, provider: ExternalProviders) -> None:
+        # may want to explicitly pass in the parameters for this event
+        self.record_analytics(
+            f"integrations.{provider.name}.notification_sent",
+            category=self.get_category(),
+            **self.get_log_params(recipient),
+        )
+        # record an optional second event
+        if self.analytics_event:
+            self.record_analytics(
+                self.analytics_event,
+                providers=provider.name.lower(),
+                **self.get_log_params(recipient),
+            )
+
 
 class ProjectNotification(BaseNotification, abc.ABC):
     def __init__(self, project: Project) -> None:
@@ -114,14 +130,5 @@ class ProjectNotification(BaseNotification, abc.ABC):
         project_link: str = absolute_uri(f"/{self.organization.slug}/{self.project.slug}/")
         return project_link
 
-    def record_notification_sent(self, recipient: Team | User, provider: ExternalProviders) -> None:
-        analytics.record(
-            f"integrations.{provider.name.lower()}.notification_sent",
-            actor_id=recipient.id,
-            category=self.get_category(),
-            organization_id=self.organization.id,
-            project_id=self.project.id,
-        )
-
     def get_log_params(self, recipient: Team | User) -> Mapping[str, Any]:
         return {"project_id": self.project.id, **super().get_log_params(recipient)}

+ 7 - 10
src/sentry/notifications/notifications/organization_request/abstract_invite_request.py

@@ -4,7 +4,6 @@ from typing import TYPE_CHECKING, Any, Mapping, MutableMapping, Sequence
 
 from django.urls import reverse
 
-from sentry import analytics
 from sentry.models import OrganizationMember
 from sentry.notifications.notifications.organization_request import OrganizationRequestNotification
 from sentry.notifications.notifications.strategies.member_write_role_recipient_strategy import (
@@ -80,12 +79,10 @@ class AbstractInviteRequestNotification(OrganizationRequestNotification):
     def get_callback_data(self) -> Mapping[str, Any]:
         return {"member_id": self.pending_member.id, "member_email": self.pending_member.email}
 
-    def record_notification_sent(self, recipient: Team | User, provider: ExternalProviders) -> None:
-        analytics.record(
-            self.analytics_event,
-            organization_id=self.organization.id,
-            user_id=self.pending_member.inviter.id if self.pending_member.inviter else None,
-            target_user_id=recipient.id,
-            invited_member_id=self.pending_member.id,
-            providers=provider.name.lower(),
-        )
+    def get_log_params(self, recipient: Team | User) -> MutableMapping[str, Any]:
+        # TODO: figure out who the user should be when pending_member.inviter is None
+        return {
+            **super().get_log_params(recipient),
+            "user_id": self.pending_member.inviter.id if self.pending_member.inviter else None,
+            "invited_member_id": self.pending_member.id,
+        }

+ 10 - 12
src/sentry/notifications/notifications/organization_request/base.py

@@ -4,7 +4,7 @@ import abc
 import logging
 from typing import TYPE_CHECKING, Any, Iterable, Mapping, MutableMapping, Sequence, Type
 
-from sentry import analytics, roles
+from sentry import roles
 from sentry.models import NotificationSetting, OrganizationMember, Team
 from sentry.notifications.notifications.base import BaseNotification
 from sentry.notifications.notifications.strategies.role_based_recipient_strategy import (
@@ -22,7 +22,6 @@ logger = logging.getLogger(__name__)
 
 
 class OrganizationRequestNotification(BaseNotification, abc.ABC):
-    analytics_event: str = ""
     referrer_base: str = ""
     member_by_user_id: MutableMapping[int, OrganizationMember] = {}
     fine_tuning_key = "approval"
@@ -113,13 +112,12 @@ class OrganizationRequestNotification(BaseNotification, abc.ABC):
     def get_title_link(self) -> str | None:
         return None
 
-    def record_notification_sent(self, recipient: Team | User, provider: ExternalProviders) -> None:
-        # this event is meant to work for multiple providers but architecture
-        # limitations mean we will fire individual for each provider
-        analytics.record(
-            self.analytics_event,
-            organization_id=self.organization.id,
-            user_id=self.requester.id,
-            target_user_id=recipient.id,
-            providers=provider.name.lower(),
-        )
+    def get_log_params(self, recipient: Team | User) -> MutableMapping[str, Any]:
+        if isinstance(recipient, Team):
+            raise NotImplementedError
+
+        return {
+            **super().get_log_params(recipient),
+            "user_id": self.requester.id,
+            "target_user_id": recipient.id,
+        }