Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
36 changes: 36 additions & 0 deletions src/sentry/notifications/platform/metrics.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from collections.abc import Mapping
from dataclasses import dataclass
from enum import StrEnum

from sentry.integrations.types import EventLifecycleOutcome
from sentry.integrations.utils.metrics import EventLifecycleMetric
from sentry.notifications.platform.types import NotificationProviderKey


class NotificationInteractionType(StrEnum):
"""Actions involved in notifications"""

NOTIFY_TARGET_SYNC = "notify_target_sync"


@dataclass
class NotificationEventLifecycleMetric(EventLifecycleMetric):
interaction_type: NotificationInteractionType
# The template/source of the notification
notification_source: str
# The sender of the notification
notification_provider: NotificationProviderKey | None

def get_metric_key(self, outcome: EventLifecycleOutcome) -> str:
tokens = ("notifications", "slo", str(outcome))
return ".".join(tokens)

def get_metric_tags(self) -> Mapping[str, str]:
tags = {
"interaction_type": self.interaction_type,
"notification_source": self.notification_source,
}

if self.notification_provider:
tags["notification_provider"] = self.notification_provider
return tags
92 changes: 73 additions & 19 deletions src/sentry/notifications/platform/service.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
import logging
from collections import defaultdict
from collections.abc import Mapping
from typing import Final

from sentry.notifications.platform.metrics import (
NotificationEventLifecycleMetric,
NotificationInteractionType,
)
from sentry.notifications.platform.provider import NotificationProvider
from sentry.notifications.platform.registry import provider_registry, template_registry
from sentry.notifications.platform.types import (
NotificationData,
NotificationProviderKey,
NotificationStrategy,
NotificationTarget,
NotificationTemplate,
)
from sentry.shared_integrations.exceptions import ApiError

logger = logging.getLogger(__name__)

Expand All @@ -19,38 +29,71 @@ class NotificationService[T: NotificationData]:
def __init__(self, *, data: T):
self.data: Final[T] = data

# TODO(ecosystem): Eventually this should be converted to spawn a task with the business logic below
def notify_target(self, *, target: NotificationTarget) -> None:
def _get_and_validate_provider(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

separating out since we'll reuse for the async version

self, target: NotificationTarget
) -> type[NotificationProvider[T]]:
provider = provider_registry.get(target.provider_key)
provider.validate_target(target=target)
return provider

def _render_template[RenderableT](
self, template: NotificationTemplate[T], provider: type[NotificationProvider[RenderableT]]
) -> RenderableT:
rendered_template = template.render(data=self.data)
renderer = provider.get_renderer(data=self.data, category=template.category)
return renderer.render(data=self.data, rendered_template=rendered_template)

def notify_target(
self, *, target: NotificationTarget
) -> None | dict[NotificationProviderKey, list[str]]:
"""
Send a notification directly to a target.
Send a notification directly to a target synchronously.
NOTE: This method ignores notification settings. When possible, consider using a strategy instead of
using this method directly to prevent unwanted noise associated with your notifications.
using this method directly to prevent unwanted noise associated with your notifications.
NOTE: Use this method when you care about the notification sending result and delivering that back to the user.
Otherwise, we generally reccomend using the async version.
"""
if not self.data:
raise NotificationServiceError(
"Notification service must be initialized with data before sending!"
)

# Step 1: Get the provider, and validate the target against it
provider = provider_registry.get(target.provider_key)
provider.validate_target(target=target)
with NotificationEventLifecycleMetric(
interaction_type=NotificationInteractionType.NOTIFY_TARGET_SYNC,
notification_source=self.data.source,
notification_provider=target.provider_key,
).capture() as lifecycle:
# Step 1: Get the provider, and validate the target against it
provider = self._get_and_validate_provider(target=target)

# Step 2: Render the template
template_cls = template_registry.get(self.data.source)
template = template_cls()
rendered_template = template.render(data=self.data)
renderer = provider.get_renderer(data=self.data, category=template.category)
renderable = renderer.render(data=self.data, rendered_template=rendered_template)
# Step 2: Render the template
template_cls = template_registry.get(self.data.source)
template = template_cls()
renderable = self._render_template(template=template, provider=provider)

# Step 3: Send the notification
try:
provider.send(target=target, renderable=renderable)
except ApiError as e:
lifecycle.record_failure(failure_reason=e, create_issue=False)
raise
return None

# Step 3: Send the notification
provider.send(target=target, renderable=renderable)
def notify_target_async(self, *, target: NotificationTarget) -> None:
"""
Send a notification directly to a target asynchronously.
NOTE: This method ignores notification settings. When possible, consider using a strategy instead of
using this method directly to prevent unwanted noise associated with your notifications.
"""
raise NotImplementedError

def notify(
self,
*,
strategy: NotificationStrategy | None = None,
targets: list[NotificationTarget] | None = None,
) -> None:
sync_send: bool = False,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so it made the most sense to me to have the sync/async sending be decided on a per notification source/type basis (i.e all link_team alerts are sync and all issue alerts are async etc.). So I put a flag on the notify func level but also this could be on the template or notification data level as well

) -> None | Mapping[NotificationProviderKey, list[ApiError]]:
if not strategy and not targets:
raise NotificationServiceError(
"Must provide either a strategy or targets. Strategy is preferred."
Expand All @@ -63,7 +106,18 @@ def notify(
targets = strategy.get_targets()
if not targets:
logger.info("Strategy '%s' did not yield targets", strategy.__class__.__name__)
return
return None

if sync_send:
errors = defaultdict(list)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

result would be something like

{
   "slack": ["too many blocks"],
   "discord": ["something something"]...
 }

for target in targets:
try:
self.notify_target(target=target)
except ApiError as e:
errors[target.provider_key].append(e)
return errors

for target in targets:
self.notify_target(target=target)
else:
for target in targets:
self.notify_target_async(target=target)
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Notify Method Breaks Synchronous Calls

The notify method's default sync_send=False now calls notify_target_async, which raises NotImplementedError, breaking existing synchronous calls. Also, when targets are provided directly and are empty, a logging statement accesses strategy.__class__.__name__ while strategy is None, causing an AttributeError.

Fix in Cursor Fix in Web

Comment on lines +113 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: The notify method defaults to an async path via notify_target_async, which is not implemented and raises a NotImplementedError, causing callers to fail.
  • Description: The notify method's signature was changed to default to asynchronous execution (sync_send=False). However, the asynchronous path calls notify_target_async, which is not implemented and raises a NotImplementedError. Existing callers in src/sentry/runner/commands/notifications.py do not specify the sync_send parameter and will therefore use the new default. This will cause the sentry notifications send email/slack/discord CLI commands to crash deterministically whenever they are run.

  • Suggested fix: To fix this, either change the default parameter in the notify method to sync_send=True to maintain backward compatibility, or update all existing callers in src/sentry/runner/commands/notifications.py to explicitly pass sync_send=True.
    severity: 0.85, confidence: 0.98

Did we get this right? 👍 / 👎 to inform future reviews.

2 changes: 1 addition & 1 deletion tests/sentry/notifications/platform/test_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def setUp(self) -> None:

def test_basic_notify(self) -> None:
service = NotificationService(data=MockNotification(message="this is a test notification"))
service.notify(targets=[self.target])
service.notify(targets=[self.target], sync_send=True)

@mock.patch("sentry.notifications.platform.service.logger")
def test_validation_on_notify(self, mock_logger: mock.MagicMock) -> None:
Expand Down
Loading