diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index 96542b5b64af58..8c0d79c41d7f9f 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -17,7 +17,7 @@ logger = logging.getLogger("sentry.digests") -Notification = namedtuple("Notification", "event rules") +Notification = namedtuple("Notification", "event rules notification_uuid") def split_key( @@ -57,13 +57,15 @@ def unsplit_key( return f"mail:p:{project.id}:{target_type.value}:{target_str}:{fallthrough}" -def event_to_record(event: Event, rules: Sequence[Rule]) -> Record: +def event_to_record( + event: Event, rules: Sequence[Rule], notification_uuid: str | None = None +) -> Record: if not rules: logger.warning(f"Creating record for {event} that does not contain any rules!") return Record( event.event_id, - Notification(event, [rule.id for rule in rules]), + Notification(event, [rule.id for rule in rules], notification_uuid), to_timestamp(event.datetime), ) @@ -145,7 +147,11 @@ def rewrite_record( return Record( record.key, - Notification(event, [_f for _f in [rules.get(id) for id in record.value.rules] if _f]), + Notification( + event, + [_f for _f in [rules.get(id) for id in record.value.rules] if _f], + record.value.notification_uuid, + ), record.timestamp, ) diff --git a/src/sentry/mail/adapter.py b/src/sentry/mail/adapter.py index 5363db16c43aeb..72febaa7c0b654 100644 --- a/src/sentry/mail/adapter.py +++ b/src/sentry/mail/adapter.py @@ -77,12 +77,12 @@ def get_digest_option(key): extra["digest_key"] = digest_key immediate_delivery = digests.add( digest_key, - event_to_record(event, rules), + event_to_record(event, rules, notification_uuid=notification_uuid), increment_delay=get_digest_option("increment_delay"), maximum_delay=get_digest_option("maximum_delay"), ) if immediate_delivery: - deliver_digest.delay(digest_key) + deliver_digest.delay(digest_key, notification_uuid=notification_uuid) else: log_event = "digested" diff --git a/src/sentry/notifications/notifications/digest.py b/src/sentry/notifications/notifications/digest.py index 5ab046288df190..ef8e5d83e7183f 100644 --- a/src/sentry/notifications/notifications/digest.py +++ b/src/sentry/notifications/notifications/digest.py @@ -102,7 +102,11 @@ def reference(self) -> Model | None: def get_context(self) -> MutableMapping[str, Any]: rule_details = get_rules(list(self.digest.keys()), self.project.organization, self.project) context = DigestNotification.build_context( - self.digest, self.project, self.project.organization, rule_details + self.digest, + self.project, + self.project.organization, + rule_details, + notification_uuid=self.notification_uuid, ) sentry_query_params = self.get_sentry_query_params(ExternalProviders.EMAIL) @@ -125,6 +129,7 @@ def build_context( organization: Organization, rule_details: Sequence[NotificationRuleDetails], alert_timestamp: int | None = None, + notification_uuid: str | None = None, ) -> MutableMapping[str, Any]: has_session_replay = features.has("organizations:session-replay", organization) show_replay_link = features.has("organizations:session-replay-issue-emails", organization) @@ -135,7 +140,11 @@ def build_context( "slack_link": get_integration_link(organization, "slack"), "rules_details": {rule.id: rule for rule in rule_details}, "link_params_for_rule": get_email_link_extra_params( - "digest_email", None, rule_details, alert_timestamp + "digest_email", + None, + rule_details, + alert_timestamp, + notification_uuid=notification_uuid, ), "show_replay_links": has_session_replay and show_replay_link, } diff --git a/src/sentry/tasks/digests.py b/src/sentry/tasks/digests.py index 3279c76104568d..4787db31d925d8 100644 --- a/src/sentry/tasks/digests.py +++ b/src/sentry/tasks/digests.py @@ -1,7 +1,8 @@ import logging import time +from typing import List, Optional -from sentry.digests import get_option_key +from sentry.digests import Record, get_option_key from sentry.digests.backends.base import InvalidState from sentry.digests.notifications import build_digest, split_key from sentry.models import Project, ProjectOption @@ -42,7 +43,7 @@ def schedule_digests(): queue="digests.delivery", silo_mode=SiloMode.REGION, ) -def deliver_digest(key, schedule_timestamp=None): +def deliver_digest(key, schedule_timestamp=None, notification_uuid: Optional[str] = None): from sentry import digests from sentry.mail import mail_adapter @@ -61,6 +62,9 @@ def deliver_digest(key, schedule_timestamp=None): try: with digests.digest(key, minimum_delay=minimum_delay) as records: digest, logs = build_digest(project, records) + + if not notification_uuid: + notification_uuid = get_notification_uuid_from_records(records) except InvalidState as error: logger.info(f"Skipped digest delivery: {error}", exc_info=True) return @@ -72,6 +76,7 @@ def deliver_digest(key, schedule_timestamp=None): target_type, target_identifier, fallthrough_choice=fallthrough_choice, + notification_uuid=notification_uuid, ) else: logger.info( @@ -84,3 +89,14 @@ def deliver_digest(key, schedule_timestamp=None): "fallthrough_choice": fallthrough_choice.value if fallthrough_choice else None, }, ) + + +def get_notification_uuid_from_records(records: List[Record]) -> Optional[str]: + for record in records: + try: + notification_uuid = record.value.notification_uuid + if notification_uuid: # Take the first existing notification_uuid + return notification_uuid + except Exception: + return None + return None diff --git a/src/sentry/web/frontend/debug/mail.py b/src/sentry/web/frontend/debug/mail.py index 8a908954bf7fcb..0ac73463104e57 100644 --- a/src/sentry/web/frontend/debug/mail.py +++ b/src/sentry/web/frontend/debug/mail.py @@ -479,7 +479,7 @@ def digest(request): } records = [] group_generator = make_group_generator(random, project) - + notification_uuid = str(uuid.uuid4()) for _ in range(random.randint(1, 30)): group = next(group_generator) state["groups"][group.id] = group @@ -511,6 +511,7 @@ def digest(request): random.sample( list(state["rules"].keys()), random.randint(1, len(state["rules"])) ), + notification_uuid, ), to_timestamp(event.datetime), ) @@ -534,6 +535,7 @@ def digest(request): random.sample( list(state["rules"].keys()), random.randint(1, len(state["rules"])) ), + notification_uuid, ), # this is required for acceptance tests to pass as the EventManager won't accept a timestamp in the past to_timestamp(datetime(2016, 6, 22, 16, 16, 0, tzinfo=timezone.utc)), @@ -557,6 +559,7 @@ def digest(request): random.sample( list(state["rules"].keys()), random.randint(1, len(state["rules"])) ), + notification_uuid, ), # this is required for acceptance tests to pass as the EventManager won't accept a timestamp in the past to_timestamp(datetime(2016, 6, 22, 16, 16, 0, tzinfo=timezone.utc)), diff --git a/tests/sentry/digests/test_notifications.py b/tests/sentry/digests/test_notifications.py index 3a00b2492c3c0a..2d8de50ade1ee7 100644 --- a/tests/sentry/digests/test_notifications.py +++ b/tests/sentry/digests/test_notifications.py @@ -1,5 +1,6 @@ from __future__ import annotations +import uuid from collections import defaultdict from functools import cached_property, reduce from typing import Mapping, MutableMapping, MutableSequence @@ -23,13 +24,16 @@ @region_silo_test(stable=True) class RewriteRecordTestCase(TestCase): + def setUp(self): + self.notification_uuid = str(uuid.uuid4()) + @cached_property def rule(self): return self.event.project.rule_set.all()[0] @cached_property def record(self): - return event_to_record(self.event, (self.rule,)) + return event_to_record(self.event, (self.rule,), self.notification_uuid) def test_success(self): assert rewrite_record( @@ -39,7 +43,7 @@ def test_success(self): rules={self.rule.id: self.rule}, ) == Record( self.record.key, - Notification(self.record.value.event, [self.rule]), + Notification(self.record.value.event, [self.rule], self.notification_uuid), self.record.timestamp, ) @@ -59,12 +63,17 @@ def test_filters_invalid_rules(self): groups={self.event.group.id: self.event.group}, rules={}, ) == Record( - self.record.key, Notification(self.record.value.event, []), self.record.timestamp + self.record.key, + Notification(self.record.value.event, [], self.notification_uuid), + self.record.timestamp, ) @region_silo_test(stable=True) class GroupRecordsTestCase(TestCase): + def setUp(self): + self.notification_uuid = str(uuid.uuid4()) + @cached_property def rule(self): return self.project.rule_set.all()[0] @@ -76,7 +85,11 @@ def test_success(self): ] group = events[0].group records = [ - Record(event.event_id, Notification(event, [self.rule]), event.datetime) + Record( + event.event_id, + Notification(event, [self.rule], self.notification_uuid), + event.datetime, + ) for event in events ] results: MutableMapping[str, Mapping[str, MutableSequence[Record]]] = defaultdict( diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py index 31034d69f62688..d6348ea591c9c8 100644 --- a/tests/sentry/mail/test_adapter.py +++ b/tests/sentry/mail/test_adapter.py @@ -1248,6 +1248,9 @@ def test_notify_digest(self, notify): message = mail.outbox[0] assert "List-ID" in message.message() + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] @mock.patch.object(mail_adapter, "notify", side_effect=mail_adapter.notify, autospec=True) def test_notify_digest_replay_id(self, notify): @@ -1297,6 +1300,9 @@ def test_notify_digest_replay_id(self, notify): message = mail.outbox[0] assert "View Replays" in message.message().as_string() + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] @mock.patch.object(mail_adapter, "notify", side_effect=mail_adapter.notify, autospec=True) def test_dont_notify_digest_snoozed(self, notify): diff --git a/tests/sentry/notifications/notifications/test_digests.py b/tests/sentry/notifications/notifications/test_digests.py index 4bb53a3b2ce87b..b3dec2ca6e4d84 100644 --- a/tests/sentry/notifications/notifications/test_digests.py +++ b/tests/sentry/notifications/notifications/test_digests.py @@ -5,6 +5,7 @@ import responses from django.core import mail +from django.core.mail.message import EmailMultiAlternatives import sentry from sentry.digests.backends.base import Backend @@ -103,6 +104,10 @@ def test_sends_digest_to_every_member(self, mock_record): 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 + message = mail.outbox[0] + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] mock_record.assert_any_call( "integrations.email.notification_sent", category="digest", @@ -135,6 +140,10 @@ def test_sends_alert_rule_notification_to_each_member(self): # It is an alert rule notification, not a digest assert "new alerts since" not in mail.outbox[0].subject + message = mail.outbox[0] + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] class DigestSlackNotification(SlackActivityNotificationTest): @@ -174,8 +183,19 @@ def test_slack_digest_notification(self, digests, mock_func): }, project_id=self.project.id, ) - backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0) - backend.add(key, event_to_record(event2, [rule]), increment_delay=0, maximum_delay=0) + notification_uuid = str(uuid.uuid4()) + backend.add( + key, + event_to_record(event, [rule], notification_uuid), + increment_delay=0, + maximum_delay=0, + ) + backend.add( + key, + event_to_record(event2, [rule], notification_uuid), + increment_delay=0, + maximum_delay=0, + ) with self.tasks(): deliver_digest(key) @@ -189,3 +209,5 @@ def test_slack_digest_notification(self, digests, mock_func): == f" " ) assert len(attachments) == 2 + assert "notification_uuid" in attachments[0]["title_link"] + assert "notification_uuid" in attachments[1]["title_link"] diff --git a/tests/sentry/tasks/test_digests.py b/tests/sentry/tasks/test_digests.py index f3deec29a4fc01..b66d4c6cd26a5d 100644 --- a/tests/sentry/tasks/test_digests.py +++ b/tests/sentry/tasks/test_digests.py @@ -1,6 +1,8 @@ +import uuid from unittest import mock from django.core import mail +from django.core.mail.message import EmailMultiAlternatives import sentry from sentry.digests.backends.redis import RedisBackend @@ -30,24 +32,51 @@ def run_test(self, key: str) -> None: data={"timestamp": iso_format(before_now(days=1)), "fingerprint": ["group-2"]}, project_id=self.project.id, ) - backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0) - backend.add(key, event_to_record(event_2, [rule]), increment_delay=0, maximum_delay=0) + notification_uuid = str(uuid.uuid4()) + backend.add( + key, + event_to_record(event, [rule], notification_uuid), + increment_delay=0, + maximum_delay=0, + ) + backend.add( + key, + event_to_record(event_2, [rule], notification_uuid), + increment_delay=0, + maximum_delay=0, + ) with self.tasks(): deliver_digest(key) assert "2 new alerts since" in mail.outbox[0].subject def test_old_key(self): self.run_test(f"mail:p:{self.project.id}") + message = mail.outbox[0] + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] def test_new_key(self): self.run_test(f"mail:p:{self.project.id}:IssueOwners:") + message = mail.outbox[0] + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] @with_feature("organizations:issue-alert-fallback-targeting") def test_fallthrough_choice_key(self): self.run_test(f"mail:p:{self.project.id}:IssueOwners::AllMembers") + message = mail.outbox[0] + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] def test_member_key(self): self.run_test(f"mail:p:{self.project.id}:Member:{self.user.id}") + message = mail.outbox[0] + assert isinstance(message, EmailMultiAlternatives) + assert isinstance(message.alternatives[0][0], str) + assert "notification_uuid" in message.alternatives[0][0] def test_no_records(self): # This shouldn't error if no records are present