From 509b7dca1ec4e6dad6b99166c57003f49ef76b5f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 10 Oct 2025 10:30:05 -0700 Subject: [PATCH 1/9] add sync notify --- src/sentry/notifications/platform/service.py | 44 +++++++++++++++----- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index adae1ff5cf7ca8..f6c9fcf42c308a 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -1,12 +1,17 @@ import logging +from collections import defaultdict from typing import Final +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__) @@ -19,12 +24,27 @@ 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 _get_and_validate_provider( + 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: """ - 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( @@ -32,15 +52,12 @@ def notify_target(self, *, target: NotificationTarget) -> None: ) # Step 1: Get the provider, and validate the target against it - provider = provider_registry.get(target.provider_key) - provider.validate_target(target=target) + 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) + renderable = self._render_template(template=template, provider=provider) # Step 3: Send the notification provider.send(target=target, renderable=renderable) @@ -50,7 +67,7 @@ def notify( *, strategy: NotificationStrategy | None = None, targets: list[NotificationTarget] | None = None, - ) -> None: + ) -> None | dict[NotificationProviderKey, list[ApiError]]: if not strategy and not targets: raise NotificationServiceError( "Must provide either a strategy or targets. Strategy is preferred." @@ -65,5 +82,12 @@ def notify( logger.info("Strategy '%s' did not yield targets", strategy.__class__.__name__) return + errors = defaultdict(list) for target in targets: - self.notify_target(target=target) + try: + self.notify_target(target=target) + except ApiError as e: + errors[target.provider_key].append(e.text) + + if errors: + return errors From 3a725a1e60ab8372ef5372330fa61dafc92ab32b Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 10 Oct 2025 11:03:17 -0700 Subject: [PATCH 2/9] add slos --- src/sentry/notifications/platform/metrics.py | 34 ++++++++++++++ src/sentry/notifications/platform/service.py | 47 +++++++++++++------- 2 files changed, 64 insertions(+), 17 deletions(-) create mode 100644 src/sentry/notifications/platform/metrics.py diff --git a/src/sentry/notifications/platform/metrics.py b/src/sentry/notifications/platform/metrics.py new file mode 100644 index 00000000000000..eb1b3c738de1a0 --- /dev/null +++ b/src/sentry/notifications/platform/metrics.py @@ -0,0 +1,34 @@ +from collections.abc import Mapping +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" + + +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 diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index f6c9fcf42c308a..e191803fb68300 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -2,6 +2,10 @@ from collections import defaultdict 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 ( @@ -51,23 +55,34 @@ def notify_target(self, *, target: NotificationTarget) -> None: "Notification service must be initialized with data before sending!" ) - # Step 1: Get the provider, and validate the target against it - provider = self._get_and_validate_provider(target=target) + with NotificationEventLifecycleMetric( + interaction_type=NotificationInteractionType.NOTIFY_TARGET_SYNC, + notification_source=self.data.source, + notification_provider=target.provider_key, + ) 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() - renderable = self._render_template(template=template, provider=provider) + # 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 - provider.send(target=target, renderable=renderable) + # Step 3: Send the notification + errors = defaultdict(list) + try: + provider.send(target=target, renderable=renderable) + except ApiError as e: + lifecycle.record_failure(failure_reason=e.text) + errors[target.provider_key].append(e.text) def notify( self, *, strategy: NotificationStrategy | None = None, targets: list[NotificationTarget] | None = None, - ) -> None | dict[NotificationProviderKey, list[ApiError]]: + sync_send: bool = False, + ) -> None | dict[NotificationProviderKey, list[str]]: if not strategy and not targets: raise NotificationServiceError( "Must provide either a strategy or targets. Strategy is preferred." @@ -82,12 +97,10 @@ def notify( logger.info("Strategy '%s' did not yield targets", strategy.__class__.__name__) return - errors = defaultdict(list) - for target in targets: - try: - self.notify_target(target=target) - except ApiError as e: - errors[target.provider_key].append(e.text) + if sync_send: + for target in targets: + errors = self.notify_target(target=target) + else: + pass - if errors: - return errors + return errors From 37c44263035498119b09538af6a0d2228d4351a2 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 10 Oct 2025 11:35:25 -0700 Subject: [PATCH 3/9] add slo and flag for synchronoous notify --- src/sentry/notifications/platform/metrics.py | 2 ++ src/sentry/notifications/platform/service.py | 27 ++++++++++++++----- .../notifications/platform/test_service.py | 2 +- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/sentry/notifications/platform/metrics.py b/src/sentry/notifications/platform/metrics.py index eb1b3c738de1a0..ea1657cde4b753 100644 --- a/src/sentry/notifications/platform/metrics.py +++ b/src/sentry/notifications/platform/metrics.py @@ -1,4 +1,5 @@ from collections.abc import Mapping +from dataclasses import dataclass from enum import StrEnum from sentry.integrations.types import EventLifecycleOutcome @@ -12,6 +13,7 @@ class NotificationInteractionType(StrEnum): NOTIFY_TARGET_SYNC = "notify_target_sync" +@dataclass class NotificationEventLifecycleMetric(EventLifecycleMetric): interaction_type: NotificationInteractionType # The template/source of the notification diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index e191803fb68300..2209c4e6ecccce 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -42,7 +42,9 @@ def _render_template[RenderableT]( 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: + def notify_target( + self, *, target: NotificationTarget + ) -> None | dict[NotificationProviderKey, list[str]]: """ Send a notification directly to a target synchronously. NOTE: This method ignores notification settings. When possible, consider using a strategy instead of @@ -59,7 +61,7 @@ def notify_target(self, *, target: NotificationTarget) -> None: interaction_type=NotificationInteractionType.NOTIFY_TARGET_SYNC, notification_source=self.data.source, notification_provider=target.provider_key, - ) as lifecycle: + ).capture() as lifecycle: # Step 1: Get the provider, and validate the target against it provider = self._get_and_validate_provider(target=target) @@ -73,8 +75,17 @@ def notify_target(self, *, target: NotificationTarget) -> None: try: provider.send(target=target, renderable=renderable) except ApiError as e: - lifecycle.record_failure(failure_reason=e.text) errors[target.provider_key].append(e.text) + lifecycle.record_failure(failure_reason=e) + return errors + + 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, @@ -95,12 +106,14 @@ 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: for target in targets: errors = self.notify_target(target=target) - else: - pass + return errors - return errors + else: + for target in targets: + self.notify_target_async(target=target) + return None diff --git a/tests/sentry/notifications/platform/test_service.py b/tests/sentry/notifications/platform/test_service.py index 2060fa013cdaaf..99963c826a758c 100644 --- a/tests/sentry/notifications/platform/test_service.py +++ b/tests/sentry/notifications/platform/test_service.py @@ -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: From 540a88668fd2d84222a791a179d31e459d3212a5 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 10 Oct 2025 11:57:52 -0700 Subject: [PATCH 4/9] revise the slo logic a bit --- src/sentry/notifications/platform/service.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index 2209c4e6ecccce..2e8bf102a41f5e 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -71,13 +71,12 @@ def notify_target( renderable = self._render_template(template=template, provider=provider) # Step 3: Send the notification - errors = defaultdict(list) try: provider.send(target=target, renderable=renderable) except ApiError as e: - errors[target.provider_key].append(e.text) - lifecycle.record_failure(failure_reason=e) - return errors + lifecycle.record_failure(failure_reason=e, capture_event=False) + raise + return None def notify_target_async(self, *, target: NotificationTarget) -> None: """ @@ -109,8 +108,12 @@ def notify( return None if sync_send: + errors = defaultdict(list) for target in targets: - errors = self.notify_target(target=target) + try: + self.notify_target(target=target) + except ApiError as e: + errors[target.provider_key].append(e) return errors else: From 88e9023faf0f1af0fc885aa93383b6c51e495996 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 10 Oct 2025 12:09:17 -0700 Subject: [PATCH 5/9] fix typing --- src/sentry/notifications/platform/service.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index 2e8bf102a41f5e..bcf1b732fdf6f9 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -1,5 +1,6 @@ import logging from collections import defaultdict +from collections.abc import Mapping from typing import Final from sentry.notifications.platform.metrics import ( @@ -74,7 +75,7 @@ def notify_target( try: provider.send(target=target, renderable=renderable) except ApiError as e: - lifecycle.record_failure(failure_reason=e, capture_event=False) + lifecycle.record_failure(failure_reason=e, create_issue=False) raise return None @@ -92,7 +93,7 @@ def notify( strategy: NotificationStrategy | None = None, targets: list[NotificationTarget] | None = None, sync_send: bool = False, - ) -> None | dict[NotificationProviderKey, list[str]]: + ) -> None | Mapping[NotificationProviderKey, list[ApiError]]: if not strategy and not targets: raise NotificationServiceError( "Must provide either a strategy or targets. Strategy is preferred." From 9482f160e8cd62bd81c7b4f06b43c7560ab14619 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Oct 2025 10:30:27 -0700 Subject: [PATCH 6/9] align a bit more with the async version --- src/sentry/notifications/platform/service.py | 27 ++++++++------------ 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index bcf1b732fdf6f9..ee81d0e9204fb9 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -25,24 +25,18 @@ class NotificationServiceError(Exception): pass +def _render_template[T: NotificationData, RenderableT]( + data: T, template: NotificationTemplate[T], provider: type[NotificationProvider[RenderableT]] +) -> RenderableT: + rendered_template = template.render(data=data) + renderer = provider.get_renderer(data=data, category=template.category) + return renderer.render(data=data, rendered_template=rendered_template) + + class NotificationService[T: NotificationData]: def __init__(self, *, data: T): self.data: Final[T] = data - def _get_and_validate_provider( - 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]]: @@ -64,12 +58,13 @@ def notify_target( 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) + provider = provider_registry.get(target.provider_key) + provider.validate_target(target=target) # Step 2: Render the template template_cls = template_registry.get(self.data.source) template = template_cls() - renderable = self._render_template(template=template, provider=provider) + renderable = _render_template(data=self.data, template=template, provider=provider) # Step 3: Send the notification try: From 86560b81f3cd4b8ece661fd87d58457d4a01bab4 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Oct 2025 10:40:02 -0700 Subject: [PATCH 7/9] fix more typing --- src/sentry/notifications/platform/service.py | 23 ++++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index ee81d0e9204fb9..454f313e46e3e4 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -37,9 +37,7 @@ class NotificationService[T: NotificationData]: def __init__(self, *, data: T): self.data: Final[T] = data - def notify_target( - self, *, target: NotificationTarget - ) -> None | dict[NotificationProviderKey, list[str]]: + def notify_target(self, *, target: NotificationTarget) -> None: """ Send a notification directly to a target synchronously. NOTE: This method ignores notification settings. When possible, consider using a strategy instead of @@ -74,14 +72,6 @@ def notify_target( raise return None - 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, *, @@ -114,5 +104,14 @@ def notify( else: for target in targets: - self.notify_target_async(target=target) + notify_target_async(target=target) return None + + +def notify_target_async(*, 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 From 9b56c141a9b6232446a68f82436343a7590846d0 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Oct 2025 17:01:29 -0700 Subject: [PATCH 8/9] address pr comments on structure and slos --- src/sentry/notifications/platform/metrics.py | 9 +- src/sentry/notifications/platform/service.py | 97 +++++++++++++------ src/sentry/runner/commands/notifications.py | 6 +- .../notifications/platform/test_service.py | 10 +- 4 files changed, 81 insertions(+), 41 deletions(-) diff --git a/src/sentry/notifications/platform/metrics.py b/src/sentry/notifications/platform/metrics.py index ea1657cde4b753..effd8a5827d2d6 100644 --- a/src/sentry/notifications/platform/metrics.py +++ b/src/sentry/notifications/platform/metrics.py @@ -4,7 +4,7 @@ from sentry.integrations.types import EventLifecycleOutcome from sentry.integrations.utils.metrics import EventLifecycleMetric -from sentry.notifications.platform.types import NotificationProviderKey +from sentry.notifications.platform.types import NotificationCategory, NotificationProviderKey class NotificationInteractionType(StrEnum): @@ -19,7 +19,9 @@ class NotificationEventLifecycleMetric(EventLifecycleMetric): # The template/source of the notification notification_source: str # The sender of the notification - notification_provider: NotificationProviderKey | None + notification_provider: NotificationProviderKey | None = None + # The category of the notification + notification_category: NotificationCategory | None = None def get_metric_key(self, outcome: EventLifecycleOutcome) -> str: tokens = ("notifications", "slo", str(outcome)) @@ -33,4 +35,7 @@ def get_metric_tags(self) -> Mapping[str, str]: if self.notification_provider: tags["notification_provider"] = self.notification_provider + if self.notification_category: + tags["category"] = self.notification_category + return tags diff --git a/src/sentry/notifications/platform/service.py b/src/sentry/notifications/platform/service.py index 454f313e46e3e4..521a1e4ce45bee 100644 --- a/src/sentry/notifications/platform/service.py +++ b/src/sentry/notifications/platform/service.py @@ -25,14 +25,6 @@ class NotificationServiceError(Exception): pass -def _render_template[T: NotificationData, RenderableT]( - data: T, template: NotificationTemplate[T], provider: type[NotificationProvider[RenderableT]] -) -> RenderableT: - rendered_template = template.render(data=data) - renderer = provider.get_renderer(data=data, category=template.category) - return renderer.render(data=data, rendered_template=rendered_template) - - class NotificationService[T: NotificationData]: def __init__(self, *, data: T): self.data: Final[T] = data @@ -50,11 +42,13 @@ def notify_target(self, *, target: NotificationTarget) -> None: "Notification service must be initialized with data before sending!" ) - with NotificationEventLifecycleMetric( + event_lifecycle = NotificationEventLifecycleMetric( interaction_type=NotificationInteractionType.NOTIFY_TARGET_SYNC, notification_source=self.data.source, notification_provider=target.provider_key, - ).capture() as lifecycle: + ) + + with event_lifecycle.capture() as lifecycle: # Step 1: Get the provider, and validate the target against it provider = provider_registry.get(target.provider_key) provider.validate_target(target=target) @@ -62,23 +56,70 @@ def notify_target(self, *, target: NotificationTarget) -> None: # Step 2: Render the template template_cls = template_registry.get(self.data.source) template = template_cls() - renderable = _render_template(data=self.data, template=template, provider=provider) + + # Update the lifecycle with the notification category now that we know it + event_lifecycle.notification_category = template.category + renderable = NotificationService.render_template( + data=self.data, template=template, provider=provider + ) # Step 3: Send the notification try: provider.send(target=target, renderable=renderable) - except ApiError as e: + except Exception as e: lifecycle.record_failure(failure_reason=e, create_issue=False) raise return None - def notify( + @classmethod + def render_template[RenderableT]( + cls, + data: T, + template: NotificationTemplate[T], + provider: type[NotificationProvider[RenderableT]], + ) -> RenderableT: + rendered_template = template.render(data=data) + renderer = provider.get_renderer(data=data, category=template.category) + return renderer.render(data=data, rendered_template=rendered_template) + + def notify_async( self, *, strategy: NotificationStrategy | None = None, targets: list[NotificationTarget] | None = None, - sync_send: bool = False, - ) -> None | Mapping[NotificationProviderKey, list[ApiError]]: + ) -> None: + """ + Send a notification directly to a target via task, if you care about using the result of the notification, use notify_sync instead. + """ + self._validate_strategy_and_targets(strategy=strategy, targets=targets) + targets = self._get_targets(strategy=strategy, targets=targets) + + for target in targets: + notify_target_async(target=target) + + def notify_sync( + self, + *, + strategy: NotificationStrategy | None = None, + targets: list[NotificationTarget] | None = None, + ) -> Mapping[NotificationProviderKey, list[str]]: + self._validate_strategy_and_targets(strategy=strategy, targets=targets) + targets = self._get_targets(strategy=strategy, targets=targets) + + errors = defaultdict(list) + for target in targets: + try: + self.notify_target(target=target) + except ApiError as e: + errors[target.provider_key].append(e.text) + return errors + + def _validate_strategy_and_targets( + self, + *, + strategy: NotificationStrategy | None = None, + targets: list[NotificationTarget] | None = None, + ) -> None: if not strategy and not targets: raise NotificationServiceError( "Must provide either a strategy or targets. Strategy is preferred." @@ -87,25 +128,19 @@ def notify( raise NotificationServiceError( "Cannot provide both strategy and targets, only one is permitted. Strategy is preferred." ) + + def _get_targets( + self, + *, + strategy: NotificationStrategy | None = None, + targets: list[NotificationTarget] | None = None, + ) -> list[NotificationTarget]: if strategy: targets = strategy.get_targets() if not targets: - logger.info("Strategy '%s' did not yield targets", strategy.__class__.__name__) - return None - - if sync_send: - errors = defaultdict(list) - for target in targets: - try: - self.notify_target(target=target) - except ApiError as e: - errors[target.provider_key].append(e) - return errors - - else: - for target in targets: - notify_target_async(target=target) - return None + logger.warning("Strategy '%s' did not yield targets", strategy.__class__.__name__) + return [] + return targets def notify_target_async(*, target: NotificationTarget) -> None: diff --git a/src/sentry/runner/commands/notifications.py b/src/sentry/runner/commands/notifications.py index bf2f3fbcea659e..0a57d03ffdeb21 100644 --- a/src/sentry/runner/commands/notifications.py +++ b/src/sentry/runner/commands/notifications.py @@ -62,7 +62,7 @@ def send_email(source: str, email: str) -> None: resource_id=email, ) template_cls = template_registry.get(source) - NotificationService(data=template_cls.example_data).notify(targets=[email_target]) + NotificationService(data=template_cls.example_data).notify_sync(targets=[email_target]) click.echo(f"Example '{source}' email sent to {email}.") @@ -152,7 +152,7 @@ def send_slack( ) template_cls = template_registry.get(source) - NotificationService(data=template_cls.example_data).notify(targets=[slack_target]) + NotificationService(data=template_cls.example_data).notify_sync(targets=[slack_target]) click.echo(f"Example '{source}' slack message sent to {integration.name}.") @@ -242,7 +242,7 @@ def send_discord( ) template_cls = template_registry.get(source) - NotificationService(data=template_cls.example_data).notify(targets=[discord_target]) + NotificationService(data=template_cls.example_data).notify_sync(targets=[discord_target]) click.echo(f"Example '{source}' discord message sent to channel with ID {channel_id}.") diff --git a/tests/sentry/notifications/platform/test_service.py b/tests/sentry/notifications/platform/test_service.py index 99963c826a758c..c993d02ae3298b 100644 --- a/tests/sentry/notifications/platform/test_service.py +++ b/tests/sentry/notifications/platform/test_service.py @@ -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], sync_send=True) + service.notify_sync(targets=[self.target]) @mock.patch("sentry.notifications.platform.service.logger") def test_validation_on_notify(self, mock_logger: mock.MagicMock) -> None: @@ -36,16 +36,16 @@ def test_validation_on_notify(self, mock_logger: mock.MagicMock) -> None: NotificationServiceError, match="Must provide either a strategy or targets. Strategy is preferred.", ): - service.notify() + service.notify_sync() strategy = MockStrategy(targets=[]) with pytest.raises( NotificationServiceError, match="Cannot provide both strategy and targets, only one is permitted. Strategy is preferred.", ): - service.notify(strategy=strategy, targets=[self.target]) + service.notify_sync(strategy=strategy, targets=[self.target]) - service.notify(strategy=strategy) - mock_logger.info.assert_called_once_with( + service.notify_sync(strategy=strategy) + mock_logger.warning.assert_called_once_with( "Strategy '%s' did not yield targets", strategy.__class__.__name__ ) From 9a2ffc43e2bb9862c482357c015dbdeaa524908c Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Oct 2025 17:17:08 -0700 Subject: [PATCH 9/9] add tests for the notifys --- .../notifications/platform/test_service.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/sentry/notifications/platform/test_service.py b/tests/sentry/notifications/platform/test_service.py index c993d02ae3298b..ea7d59cf73bc17 100644 --- a/tests/sentry/notifications/platform/test_service.py +++ b/tests/sentry/notifications/platform/test_service.py @@ -1,13 +1,18 @@ from unittest import mock import pytest +from django.core.mail import EmailMultiAlternatives +from sentry.integrations.types import EventLifecycleOutcome +from sentry.notifications.platform.email.provider import EmailNotificationProvider from sentry.notifications.platform.service import NotificationService, NotificationServiceError from sentry.notifications.platform.target import GenericNotificationTarget from sentry.notifications.platform.types import ( NotificationProviderKey, NotificationTargetResourceType, ) +from sentry.shared_integrations.exceptions import ApiError +from sentry.testutils.asserts import assert_count_of_metric from sentry.testutils.cases import TestCase from sentry.testutils.notifications.platform import ( MockNotification, @@ -49,3 +54,45 @@ def test_validation_on_notify(self, mock_logger: mock.MagicMock) -> None: mock_logger.warning.assert_called_once_with( "Strategy '%s' did not yield targets", strategy.__class__.__name__ ) + + @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @mock.patch("sentry.notifications.platform.email.provider.EmailNotificationProvider.send") + def test_notify_target_calls_provider_correctly( + self, mock_send: mock.MagicMock, mock_record: mock.MagicMock + ) -> None: + service = NotificationService(data=MockNotification(message="test")) + service.notify_target(target=self.target) + + mock_send.assert_called_once() + + # SLO assertions + assert_count_of_metric(mock_record, EventLifecycleOutcome.STARTED, outcome_count=1) + assert_count_of_metric(mock_record, EventLifecycleOutcome.SUCCESS, outcome_count=1) + + @mock.patch("sentry.notifications.platform.email.provider.EmailNotificationProvider.send") + def test_notify_sync_collects_errors(self, mock_send: mock.MagicMock) -> None: + mock_send.side_effect = ApiError("Provider error", 400) + + service = NotificationService(data=MockNotification(message="test")) + errors = service.notify_sync(targets=[self.target]) + + assert NotificationProviderKey.EMAIL in errors + assert len(errors[NotificationProviderKey.EMAIL]) == 1 + assert "Provider error" in errors[NotificationProviderKey.EMAIL][0] + + @mock.patch("sentry.notifications.platform.service.notify_target_async") + def test_notify_async_calls_task(self, mock_task: mock.MagicMock) -> None: + service = NotificationService(data=MockNotification(message="test")) + service.notify_async(targets=[self.target]) + + mock_task.assert_called_once_with(target=self.target) + + def test_render_template_classmethod(self) -> None: + data = MockNotification(message="test") + template = MockNotificationTemplate() + + result = NotificationService.render_template( + data=data, template=template, provider=EmailNotificationProvider + ) + + assert isinstance(result, EmailMultiAlternatives)