Skip to content

Commit

Permalink
feat: Add notification_uuid to digest URLs (#55951)
Browse files Browse the repository at this point in the history
  • Loading branch information
jangjodi authored Sep 13, 2023
1 parent db355fc commit 2a46053
Show file tree
Hide file tree
Showing 9 changed files with 123 additions and 19 deletions.
14 changes: 10 additions & 4 deletions src/sentry/digests/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

logger = logging.getLogger("sentry.digests")

Notification = namedtuple("Notification", "event rules")
Notification = namedtuple("Notification", "event rules notification_uuid")


def split_key(
Expand Down Expand Up @@ -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),
)

Expand Down Expand Up @@ -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,
)

Expand Down
4 changes: 2 additions & 2 deletions src/sentry/mail/adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
13 changes: 11 additions & 2 deletions src/sentry/notifications/notifications/digest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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,
}
Expand Down
20 changes: 18 additions & 2 deletions src/sentry/tasks/digests.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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(
Expand All @@ -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
5 changes: 4 additions & 1 deletion src/sentry/web/frontend/debug/mail.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
)
Expand All @@ -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)),
Expand All @@ -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)),
Expand Down
21 changes: 17 additions & 4 deletions tests/sentry/digests/test_notifications.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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(
Expand All @@ -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,
)

Expand All @@ -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]
Expand All @@ -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(
Expand Down
6 changes: 6 additions & 0 deletions tests/sentry/mail/test_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
26 changes: 24 additions & 2 deletions tests/sentry/notifications/notifications/test_digests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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)

Expand All @@ -189,3 +209,5 @@ def test_slack_digest_notification(self, digests, mock_func):
== f"<!date^{timestamp_secs}^2 issues detected {{date_pretty}} in| Digest Report for> <http://testserver/organizations/{self.organization.slug}/projects/{self.project.slug}/|{self.project.name}>"
)
assert len(attachments) == 2
assert "notification_uuid" in attachments[0]["title_link"]
assert "notification_uuid" in attachments[1]["title_link"]
33 changes: 31 additions & 2 deletions tests/sentry/tasks/test_digests.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 2a46053

Please sign in to comment.