feat(np): Adds rough notification renderer for issue alerts, as a thin wrapper"#110929
feat(np): Adds rough notification renderer for issue alerts, as a thin wrapper"#110929GabeVillalobos wants to merge 3 commits intomasterfrom
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| action = invocation.action | ||
| detector = invocation.detector | ||
| event_data = invocation.event_data | ||
|
|
||
| blob = SlackDataBlob(**action.data) | ||
| tags_set = {t.strip() for t in blob.tags.split(",") if t.strip()} if blob.tags else set() | ||
|
|
||
| rule = BaseIssueAlertHandler.create_rule_instance_from_action( | ||
| action=action, | ||
| detector=detector, | ||
| event_data=event_data, | ||
| ) |
There was a problem hiding this comment.
This entire data creation piece is busted since rule data can't be effectively JSON serializable atm. I need to implement ISWF-2257 before it makes sense to fix this.
There was a problem hiding this comment.
I wouldn't try to handle this using rules, instead focus on the workflow side of things and how those would interact today.
That would basically be data manipulation in the Notification action to mutate the data in a way that's consumable in the notification platform.
There was a problem hiding this comment.
That's the future goal. We'll be dropping this entire renderer once we've nailed down the default renderer's core logic. This is just meant to be a stepping stone to that by using the old render logic, while we roll out notification platform as is to get a feel for how well threading and task handling are going.
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
High level, this makes a lot of sense.
digging into some details...
I'd really recommend us to avoid use IssueAlert nomenclature here if possible.
Instead, focusing on the template and the data required.
My thought is that in the future, seer may want to send a notification about the issue and they could reuse aspect of these templates to compose a notification. In that world, it'd be really cool to compose a notification using the issue notification + a seer notification. (That way we wouldnt need to do stuff like copy paste that template, and rebuild parts if the renderer)
|
|
||
| if category == NotificationCategory.SEER: | ||
| return SeerSlackRenderer | ||
| if category == NotificationCategory.ISSUE_ALERT: |
There was a problem hiding this comment.
It might be nice to make this a registry by the category, so you can just apply a decorator rather than updating this if/else tree.
| ) | ||
|
|
||
|
|
||
| class IssueAlertSlackRenderer(NotificationRenderer[SlackRenderable]): |
There was a problem hiding this comment.
I'm a bit nervous about us calling this a IssueAlertSlackRenderer. It creates this connotation that we would need a Rule model to be able to send the notification, and that we'd need a different renderer for each monitor (like a metric alert slack renderer). To me, this would be more of an IssueNotificationSlackRenderer.
The hope is to create a stronger link to the data for the template and because slack, we want to render it different than other integrations.
Is there anything in code that would create a strong coupling to issue alerts?
There was a problem hiding this comment.
Kinda related to my comment above, this is only IssueAlertSlackRenderer because rewriting/reworking the entire notification to fit the standard default renderer would be a pretty significant change. I want to break these into incremental steps to give us some time and space to fully shift to using our default render logic if possible.
|
|
||
| from sentry.integrations.slack.message_builder.issues import SlackIssuesMessageBuilder | ||
|
|
||
| group = Group.objects.get_from_cache(id=data.group_id) |
There was a problem hiding this comment.
I'm a bit nervous that we're creating a strong coupling from the data to the notification here.
Instead, could we make this be input data and use types to enforce the shape of the data?
I would expect us to have the Group already, if we've determined we need to send a notification about it.
| group=group, | ||
| event=event, | ||
| tags=data.tags or None, | ||
| rules=[data.rule] if data.rule else None, | ||
| notes=data.notes or None, |
There was a problem hiding this comment.
Could this be a typed dict for the input data?
| action = invocation.action | ||
| detector = invocation.detector | ||
| event_data = invocation.event_data | ||
|
|
||
| blob = SlackDataBlob(**action.data) | ||
| tags_set = {t.strip() for t in blob.tags.split(",") if t.strip()} if blob.tags else set() | ||
|
|
||
| rule = BaseIssueAlertHandler.create_rule_instance_from_action( | ||
| action=action, | ||
| detector=detector, | ||
| event_data=event_data, | ||
| ) |
There was a problem hiding this comment.
I wouldn't try to handle this using rules, instead focus on the workflow side of things and how those would interact today.
That would basically be data manipulation in the Notification action to mutate the data in a way that's consumable in the notification platform.
| @template_registry.register(IssueAlertNotificationData.source) | ||
| class IssueAlertNotificationTemplate(NotificationTemplate[IssueAlertNotificationData]): | ||
| category = NotificationCategory.ISSUE_ALERT | ||
| example_data = IssueAlertNotificationData( |
There was a problem hiding this comment.
IssueNotificationData? That way we can signify that there's not a dependency on Rule models.
GabeVillalobos
force-pushed
the
gv/slack-issue-alert-renderer
branch
from
b58b6cf to
1b1025a
Compare
2 participants
Adds a new renderer that wraps the existing Slack render logic, translating its response into the correct SlackRenderer block format. The rollout for this will happen in 2 phases: