feat(notifications): Pre compute notification fields prior to calling into the platform#112160
Conversation
…lert using different event types - So we're going to hook in later and do more pre computation of the metric alert notif fields. Pass those into the renderer and the renderer has to do less work to make the data usable for slack.
There was a problem hiding this comment.
Detector.objects.get() can raise DoesNotExist without handler (src/sentry/notifications/notification_action/metric_alert_registry/handlers/slack_metric_alert_handler.py:142)
Line 142 uses Detector.objects.get(id=alert_context.action_identifier_id) without a try/except for Detector.DoesNotExist. If the detector is deleted between when the invocation is created and when send_alert runs, this will raise an unhandled exception. The check on line 143-144 if not detector is dead code since .get() either returns an object or throws - it never returns None.
Identified by Warden sentry-backend-bugs
| # METRIC_ALERT | ||
| METRIC_ALERT = "metric-alert" | ||
| ACTIVITY_METRIC_ALERT = "activity-metric-alert" | ||
|
|
There was a problem hiding this comment.
Bug: Removing ACTIVITY_METRIC_ALERT from NotificationSource will cause deserialization to fail for existing NotificationThread records with that key_type, raising an error.
Severity: MEDIUM
Suggested Fix
To prevent deserialization failures, either add a data migration to update or remove existing NotificationThread records with key_type="activity-metric-alert", or implement a fallback mechanism to handle the legacy source type gracefully during deserialization, perhaps by mapping it to the new MetricAlertNotificationData path or ignoring it.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/notifications/platform/types.py#L61
Potential issue: The pull request removes `ACTIVITY_METRIC_ALERT` from the
`NotificationSource` enum and its corresponding template from the registry. If any
`NotificationThread` records with `key_type="activity-metric-alert"` exist in the
database, any operation that requires deserializing them (such as replying to an old
notification) will fail. The `deserialize_notification_data` function will attempt to
look up the template for the source `"activity-metric-alert"`, which no longer exists in
the registry, causing a `NoRegistrationExistsError`. This will lead to a crash when
interacting with older metric alert notification threads.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
ACTIVITY_METRIC_ALERT shouldnt be used anywehre anymore
GabeVillalobos
left a comment
There was a problem hiding this comment.
Changes look good! Thanks for overhauling this.
| metric_issue_context = _build_metric_issue_context_from_group_event(data) | ||
| elif isinstance(data, ActivityMetricAlertNotificationData): | ||
| metric_issue_context = _build_metric_issue_context_from_activity(data) | ||
| incident_text = f"{data.text}\n{get_started_at(data.open_period_context.date_started)}" |
There was a problem hiding this comment.
Are we still planning to hoist this into the NOA layer? Seems like we don't need the full open_period_context anymore at the very least.
There was a problem hiding this comment.
sadly we can't hoist this up as get_started_at turns the text into a slack sepcific format
|
PR reverted: 36ffdae |
Summary
Note: