feat(np): Specify synchronous notification send w/ slos #101358
Conversation
github-actions
bot
added
the
Scope: Backend
| strategy: NotificationStrategy | None = None, | ||
| targets: list[NotificationTarget] | None = None, | ||
| ) -> None: | ||
| sync_send: bool = False, |
There was a problem hiding this comment.
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
| try: | ||
| provider.send(target=target, renderable=renderable) | ||
| except ApiError as e: | ||
| lifecycle.record_failure(failure_reason=e, capture_event=False) | ||
| raise | ||
| return None |
There was a problem hiding this comment.
goal here is that we want the failure metrics but not the sentry errors since those should be reported at the provider level so that we don't have to pollute the notifications code with provider specific exception handling
| return None | ||
|
|
||
| if sync_send: | ||
| errors = defaultdict(list) |
There was a problem hiding this comment.
result would be something like
{
"slack": ["too many blocks"],
"discord": ["something something"]...
}
|
|
||
| # 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( |
There was a problem hiding this comment.
separating out since we'll reuse for the async version
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| else: | ||
| for target in targets: | ||
| self.notify_target_async(target=target) | ||
| return None |
There was a problem hiding this comment.
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.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this comment.
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
notifymethod's signature was changed to default to asynchronous execution (sync_send=False). However, the asynchronous path callsnotify_target_async, which is not implemented and raises aNotImplementedError. Existing callers insrc/sentry/runner/commands/notifications.pydo not specify thesync_sendparameter and will therefore use the new default. This will cause thesentry notifications send email/slack/discordCLI commands to crash deterministically whenever they are run. -
Suggested fix: To fix this, either change the default parameter in the
notifymethod tosync_send=Trueto maintain backward compatibility, or update all existing callers insrc/sentry/runner/commands/notifications.pyto explicitly passsync_send=True.
severity: 0.85, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #101358 +/- ##
===========================================
+ Coverage 79.98% 81.04% +1.06%
===========================================
Files 8683 8688 +5
Lines 385208 385506 +298
Branches 24337 24337
===========================================
+ Hits 308096 312440 +4344
+ Misses 76751 72705 -4046
Partials 361 361 |
1 participant
see comments, async will be in seperate pr to keep this one uh slimmer