Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add notification_uuid to digest URLs #55951

Merged
merged 7 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
9 changes: 8 additions & 1 deletion src/sentry/tasks/digests.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import logging
import time
from typing import Optional

from sentry.digests import get_option_key
from sentry.digests.backends.base import InvalidState
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want an interim PR to use kwargs and then swap to a explicit variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm told that this may be a 3 way rollout:
(1) add new optional parameter
(2) start sending new parameter
(3) remove optional-ness

from sentry import digests
from sentry.mail import mail_adapter

Expand All @@ -61,6 +62,11 @@ def deliver_digest(key, schedule_timestamp=None):
try:
with digests.digest(key, minimum_delay=minimum_delay) as records:
digest, logs = build_digest(project, records)

for record in records:
notification_uuid = record.value.notification_uuid
if notification_uuid: # Take the first existing notification_uuid
break
except InvalidState as error:
logger.info(f"Skipped digest delivery: {error}", exc_info=True)
return
Expand All @@ -72,6 +78,7 @@ def deliver_digest(key, schedule_timestamp=None):
target_type,
target_identifier,
fallthrough_choice=fallthrough_choice,
notification_uuid=notification_uuid,
)
else:
logger.info(
Expand Down
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 @@ -1221,6 +1221,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 @@ -1270,6 +1273,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 @@ -102,13 +103,21 @@ def test_sends_digest_to_every_member(self):
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]

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"""
self.run_test(event_count=1)

# 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 @@ -148,8 +157,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 @@ -163,3 +183,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
Loading