Skip to content

feat(seer-activity): Implement activity action registry#117016

Merged
leeandher merged 3 commits into
masterfrom
leanderrodrigues/iswf-2728-add-notification-platform-path-for-supported-actions
Jun 9, 2026
Merged

feat(seer-activity): Implement activity action registry#117016
leeandher merged 3 commits into
masterfrom
leanderrodrigues/iswf-2728-add-notification-platform-path-for-supported-actions

Conversation

@leeandher

@leeandher leeandher commented Jun 5, 2026

Copy link
Copy Markdown
Member

The mapping is a bit messy but it goes like this:

ActionRegistry
> GroupTypeRegistry 
>> if (event is Activity) 
>>> if (activity is not Metric Resolution)
>>>> ActivityHandlerRegistry
>>>>> actual handlers (in this case, NotificationPlatformActivityHandler)

This is necessary since we don't want to modify the pathway for Metric Issue resolution just yet, but if we get those status changes migrated the activity registry, we can probably move all of this out of GroupActivityRegistry and just modify the ActionRegistry members directly, maybe even split out ActionInvocation into two concrete types 🤷

But for now, this new registry will let us create a pathway just for our new activity triggers. Added an option for safety, but it'll be removed when released.

@leeandher leeandher requested review from a team as code owners June 5, 2026 20:01
@linear-code

linear-code Bot commented Jun 5, 2026

Copy link
Copy Markdown

ISWF-2728

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 5, 2026
@leeandher leeandher requested a review from Christinarlong June 5, 2026 20:02

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f03d245. Configure here.

Comment thread src/sentry/notifications/notification_action/utils.py

@Christinarlong Christinarlong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is giving, notificationservice v2 lmao

Comment thread src/sentry/notifications/notification_action/utils.py Outdated
Comment thread src/sentry/notifications/notification_action/utils.py
Comment thread src/sentry/notifications/notification_action/utils.py Outdated
Comment thread src/sentry/notifications/notification_action/types.py Outdated
Comment on lines +25 to +30
@classmethod
def invoke_action(cls, invocation: ActionInvocation, activity: Activity) -> None:
# TODO(Leander): Implement this. The compatible_activity_types should be all activities that
# we have templates registered for in the notification platform. Here we'll fetch those
# templates, create a target from the ActionInvocation, and use the service to fire.
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think so far this
Activity -> ActivityHandlerRegistry -> get handler based on ActionType -> Handler Invoke makes sense.

Some half baked thoughts

  • This means actiontype is 1:1 with a handler, which is kinda rigid. Though, I'm not sure if we'd ever need some escape hatch for like email to throw to another handler.
  • But also registering everything that NP supports means that, all compatible activities will need to be supported. I wonder if it'd be better to seperate it out to like - EmailNotificationPlatformActivityHandler instead of 1 big np handler.
  • Or thoughts on instead going from activity -> handler (like key on activitytype vs actiontype)

@leeandher leeandher Jun 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thanks for the thoughts! I went back and forth on this stuff, but heres where im at:

actiontype is 1:1 with a handler, which is kinda rigid

  • I think I'm okay with this since at this stage the action is the thing we need to specifically be implementing. it also means that the total amount of handlers were expecting if everything is built out is less than a dozen (e.g. pagerduty, sentry apps, github tickets, messaging integrations etc)

I wonder if it'd be better to seperate it out to like - EmailNotificationPlatformActivityHandler instead of 1 big np handler

  • also fair! but for now, im going for the most coverage possible off the start. It's straightforward enough to separate out the handlers after by registering the EMAIL key to a new handler if we need, but we can take advantage of the defaults from notification platform this way 🙏

like key on activitytype vs actiontype

  • I thought about this too, the problem ends up being that we still need to have a key on actiontype at some point, since the same code for creating a gh ticket cant send a slack message. It just makes the amount of handlers much larger too, since for each activity (~35), there are separate actions we need (~10), so like 350 registrants that way. Even if we abstract the actions, that's still 35 registrants.

I figured itd be more likely that the action code would be shared, and added the compatible_activity_types to have some safety, so we end up with ~10 registrants, and even less if we keep this big notif platform handler

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, I'm still a bit worried about the scope of testing pt. 2 but as you said it's not that much work to separate out and I think the invoke implementation will also help inform

@leeandher leeandher requested a review from Christinarlong June 9, 2026 18:19
Comment on lines +25 to +30
@classmethod
def invoke_action(cls, invocation: ActionInvocation, activity: Activity) -> None:
# TODO(Leander): Implement this. The compatible_activity_types should be all activities that
# we have templates registered for in the notification platform. Here we'll fetch those
# templates, create a target from the ActionInvocation, and use the service to fire.
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, I'm still a bit worried about the scope of testing pt. 2 but as you said it's not that much work to separate out and I think the invoke implementation will also help inform

@leeandher leeandher merged commit 7001722 into master Jun 9, 2026
85 checks passed
@leeandher leeandher deleted the leanderrodrigues/iswf-2728-add-notification-platform-path-for-supported-actions branch June 9, 2026 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants