-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
♻️ ref(metric alerts): refactor slack metric alert logic #87261
Conversation
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
25ae920
to
0b48e1d
Compare
0b48e1d
to
c0c2e4f
Compare
id=group.id, | ||
# TODO(iamrajjoshi): This should probably be the id of the latest open period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ty for the context comments on these! It makes it much easier to review these changes.
def send_incident_alert_notification( | ||
action: AlertRuleTriggerAction, | ||
incident: Incident, | ||
metric_value: float | int | None, | ||
new_status: IncidentStatus, | ||
notification_uuid: str | None = None, | ||
) -> bool: | ||
# Make sure organization integration is still active: | ||
result = integration_service.organization_context( | ||
organization_id=incident.organization_id, integration_id=action.integration_id | ||
) | ||
integration = result.integration | ||
org_integration = result.organization_integration | ||
if org_integration is None or integration is None or integration.status != ObjectStatus.ACTIVE: | ||
# Integration removed, but rule is still active. | ||
return False | ||
|
||
organization = incident.organization | ||
chart_url = None | ||
if features.has("organizations:metric-alert-chartcuterie", incident.organization): | ||
try: | ||
alert_rule_serialized_response: AlertRuleSerializerResponse = serialize( | ||
incident.alert_rule, None, AlertRuleSerializer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, could you please break down refactors like these into their own isolated PRs? Having a combination of changes like this makes it harder to review thoroughly when the diffs are bulky.
74088fe
to
f6a1174
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Slack metric alert notification logic by decomposing the incident notification handling into several helper functions and updating the underlying context types.
- Extracts helper functions (_fetch_parent_notification_message_for_incident, _build_notification_payload, _send_notification, _save_notification_message) to better modularize the notification logic.
- Updates the SlackIncidentsMessageBuilder and corresponding tests to use MetricIssueContext instead of separately passing parameters like open_period_identifier, snuba_query, new_status, and metric_value.
- Modifies the typings in metric_detector to include additional fields (e.g., id, subscription) for better traceability.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/sentry/integrations/slack/utils/notifications.py | Refactors notification logic, extracting several helper functions and updating context usage. |
tests/sentry/integrations/slack/test_message_builder.py | Adjusts tests to use MetricIssueContext and ensures consistency with the new builder interface. |
src/sentry/incidents/typings/metric_detector.py | Updates context classes by adding new fields and modifying factory methods accordingly. |
tests/sentry/incidents/action_handlers/test_slack.py | Updates test setup to use MetricIssueContext in place of direct parameters. |
src/sentry/integrations/slack/message_builder/incidents.py | Updates builder to accept MetricIssueContext and assigns values accordingly. |
this pr doesn't do anything new, it decomposes the slack metric alert action handler util so it is ready for me to make the open period refactors
it might be easier to review by cloning locally and taking a look
pr to add open period stuff to follow