feat(notif-platform): Prototype the service layer #93863

leeandher · 2025-06-18T21:36:00Z

Prototyping the service layer, with some added boilerplate for targets.

They'll need to have the integration data attached by the time we call .notify() so I've added a step in prepare_targets, which sidesteps the frozen data class to ensure they have all the data to send out to providers.

Also changed NotificationType to NotificationCategory since type is a keyword (and a bit overloaded)

src/sentry/notifications/platform/service.py

src/sentry/notifications/platform/renderer.py

…otificationplatform

GabeVillalobos

Changes look good for initial scaffolding, but I think we need to focus on Template & data design a bit more as a concept, focusing on the DX of it.

Generally: what should I as a dev have to define? Can I intuit this from another notification's configuration, and is this consolidated in some way?

GabeVillalobos · 2025-06-25T20:04:12Z

src/sentry/notifications/platform/service.py

+
+class NotificationService[T: NotificationData]:
+    def __init__(self, *, data: T):
+        self.data: Final[T] = data


What's the purpose of binding the data to the service instance? From a DX perspective, wouldn't it make more sense to bind the template & type, then take in different data chunks? (i.e. user data + targets)?

Wrong reply, I meant to respond that the reason I did it this way was so that the type safety would narrow the generic for the rest of the calls. It is what would allow the app code to error out if you did

notif = NotificationService(data=billing_data) notif.send(template=alert_template) # mypy raises notif.send(tempalte=billing_template) # all good

Out of curiosity, what's the concrete mypy error you get in the former case?

GabeVillalobos · 2025-06-25T20:05:00Z

src/sentry/notifications/platform/service.py

+        # Step 3: Get the provider, and validate the target against it
+        provider = provider_registry.get(target.provider_key)
+        provider.validate_target(target=target)
+
+        # Step 4: Render the template
+        renderer = provider.get_renderer(category=self.data.category)
+        renderable = renderer.render(data=self.data, template=template)
+
+        # Step 5: Send the notification
+        provider.send(target=target, renderable=renderable)


This is good scaffolding, but this will eventually be moved to a delivery task vs as part of the service.

Good point! I will incorporate this into some comments in a follow up

Meant to add this comment here!

GabeVillalobos · 2025-06-25T20:05:54Z

src/sentry/notifications/platform/service.py

+        if not strategy and not targets:
+            raise NotificationServiceError(
+                "Must provide either a strategy or targets. Strategy is preferred."
+            )
+        if strategy and targets:
+            raise NotificationServiceError(
+                "Cannot provide both strategy and targets, only one is permitted. Strategy is preferred."
+            )
+        if strategy:
+            targets = strategy.get_targets()
+        if not targets:
+            logger.info("Strategy '%s' did not yield targets", strategy.__class__.__name__)
+            return
+
+        # Prepare the targets for sending by fetching integration data, etc.
+        prepare_targets(targets=targets)
+
+        for target in targets:
+            self.notify_prepared_target(target=target, template=template)


Nice! I'm a big fan of having single target & batch targeting with strategies.

GabeVillalobos · 2025-06-25T20:08:29Z

src/sentry/notifications/platform/types.py

+    The exception is the `DEBUG` category, which is used for testing and development.
+    """
+
+    DEBUG = "debug"


I want to chat more about this in a sync, because this can have some broad implications. I like the idea of meta groups, but we may want to do something like our Outbox groupings, where we bind things to scopes in a larger config somewhere. Additionally, I think we should consider binding some of these enums/groupings to the template rather than the data.

leeandher · 2025-06-25T20:45:07Z

Changes look good for initial scaffolding, but I think we need to focus on Template & data design a bit more as a concept, focusing on the DX of it.

Generally: what should I as a dev have to define? Can I intuit this from another notification's configuration, and is this consolidated in some way?

Totally agree! this service layer was mostly focused on layout out the steps, but the next PR will focus on the templates and i'll touch on DX in more detail, ty for review!

Christinarlong · 2025-06-25T20:45:22Z

src/sentry/notifications/platform/service.py

+    def notify(
+        self,
+        *,
+        strategy: NotificationStrategy | None = None,
+        targets: list[NotificationTarget] | None = None,
+        template: NotificationTemplate[T],


curious q, why are we combining the handling of both strategy and target type inputs in the sample place ?

So strategies are preferred but there are certain notif types that might pick specific targets, so this allows both. It's possible that we don't allow targets if we have enough strategies to cover all bases, but this gives us some flexibilty.

Christinarlong · 2025-06-25T20:50:04Z

src/sentry/notifications/platform/slack/provider.py

@@ -17,10 +18,12 @@


 class SlackRenderer(NotificationRenderer[SlackRenderable]):
-    provider_key = NotificationProviderKey.SLACK
+    provider_key = NotificationProviderKey.DISCORD


should this be slack?

Christinarlong · 2025-06-25T21:21:57Z

src/sentry/notifications/platform/types.py

+    provider_key: NotificationProviderKey
+    resource_type: NotificationTargetResourceType
+    resource_id: str
+    specific_data: dict[str, Any] | None


q what would data for this field look like?

Christinarlong · 2025-06-25T21:34:17Z

src/sentry/notifications/platform/target.py

    """
    Adds necessary properties and methods to designate a target within an integration.
    Accepts the renderable object type that matches the connected provider.
    """

    integration_id: int
    organization_id: int
+    integration: RpcIntegration = field(init=False)


q im kinda confused on the flow of this target class & prepare_targets? Is the scope of a single IntegrationNotificationTarget a everything under a single OrganizationIntegration ? I guess I'm confused on how these properties on the target work if there's potentially multiple OrganizationIntegrations attached to an Integration

The GenericNotificationTargets have all they need to be used to send a notification immediately, but the IntegrationNotificationTargets are initialized with data but not all the meta stuff like API keys. For example, an alert might have the integration_id and an organization_id and a slack channel, but to actually notify that channel, we'll need integration.metadata.token or something along those lines.

So at some point we have to do a DB query for the credentials, and for performance reasons we wanna batch this (e.g. if we send 50 notifications, we dont want 50 DB/RPC calls), so thats where the prepare_targets comes from. The app code passes in targets, unprepared, and the prepare_targets function will do the DB query or RPC call in bulk to add the integration or org_integration RPC object to the targets, then each can have notify() called with no side-effects like RPC/DB calls.

kinda confusing, lmk if it doesnt make sense

leeandher added 2 commits June 18, 2025 15:37

✨ update type to category

b7b9089

✨ basic setup for targets

faed1f1

github-actions bot added the Scope: Backend label Jun 18, 2025

✅ tests and fixes

242fafd

vercel bot deployed to Preview June 19, 2025 18:03 View deployment

This comment was marked as outdated.

Sign in to view

leeandher force-pushed the leander/eco-722-spike-on-type-safety-for-notificationplatform branch from 4c991ff to 6eeb981 Compare June 19, 2025 18:55

vercel bot deployed to Preview June 19, 2025 18:57 View deployment

✅ fix tests

2e873b5

leeandher force-pushed the leander/eco-722-spike-on-type-safety-for-notificationplatform branch from 6eeb981 to 2e873b5 Compare June 19, 2025 18:58

vercel bot deployed to Preview June 19, 2025 18:59 View deployment

🏷️ more typing an structure

bdbd149

vercel bot deployed to Preview June 19, 2025 19:23 View deployment

🔥 remove unnecessary renderer changes

42af969

vercel bot deployed to Preview June 19, 2025 19:47 View deployment

🏷️ types

55f517e

vercel bot deployed to Preview June 19, 2025 20:08 View deployment

GabeVillalobos reviewed Jun 23, 2025

View reviewed changes

src/sentry/notifications/platform/service.py Outdated Show resolved Hide resolved

src/sentry/notifications/platform/service.py Outdated Show resolved Hide resolved

src/sentry/notifications/platform/renderer.py Outdated Show resolved Hide resolved

Merge branch 'master' into leander/eco-722-spike-on-type-safety-for-n…

bb21834

…otificationplatform

vercel bot deployed to Preview June 24, 2025 16:03 View deployment

vercel bot deployed to Preview June 24, 2025 16:15 View deployment

leeandher marked this pull request as ready for review June 24, 2025 16:37

leeandher requested review from a team as code owners June 24, 2025 16:37

leeandher requested a review from GabeVillalobos June 24, 2025 16:37

✏️ Address PR comments

2ec5eed

leeandher force-pushed the leander/eco-722-spike-on-type-safety-for-notificationplatform branch from be690a5 to 2ec5eed Compare June 24, 2025 19:13

vercel bot deployed to Preview June 24, 2025 19:15 View deployment

🏷️ typing fix

5d8caab

vercel bot deployed to Preview June 24, 2025 19:31 View deployment

GabeVillalobos approved these changes Jun 25, 2025

View reviewed changes

leeandher merged commit cc85440 into master Jun 25, 2025
64 checks passed

leeandher deleted the leander/eco-722-spike-on-type-safety-for-notificationplatform branch June 25, 2025 20:45

Christinarlong reviewed Jun 25, 2025

View reviewed changes

github-actions bot locked and limited conversation to collaborators Jul 11, 2025

Uh oh!

feat(notif-platform): Prototype the service layer #93863

feat(notif-platform): Prototype the service layer #93863

Conversation

leeandher commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

GabeVillalobos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeandher commented Jun 25, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leeandher commented Jun 18, 2025 •

edited

Loading