Skip to content

chore(aci milestone 3): migrate edge case alerts #93749

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

Merged
merged 1 commit into from
Jun 18, 2025

Conversation

mifu67
Copy link
Contributor

@mifu67 mifu67 commented Jun 17, 2025

This migration creates ACI objects for the following alerts:

  • Alerts that were disabled during the initial migration but are now enabled
  • Alerts that were anomaly detection alerts and created before anomaly detection dual write was enabled, but became static/percent alerts before the anomaly detection alert migration
  • All disabled alert rules with projects to prevent future instances of case 1. These detectors will be set to disabled.

@mifu67 mifu67 requested a review from ceorourke June 17, 2025 20:28
@mifu67 mifu67 requested review from a team as code owners June 17, 2025 20:28
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 17, 2025
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/workflow_engine/migrations/0071_migrate_remaining_metric_alerts.py

for 0071_migrate_remaining_metric_alerts in workflow_engine

--
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL

AlertRuleTriggerActionType.SLACK.value: "Slack",
AlertRuleTriggerActionType.MSTEAMS.value: "Microsoft Teams",
AlertRuleTriggerActionType.OPSGENIE.value: "Opsgenie",
AlertRuleTriggerActionType.DISCORD.value: "Discord",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made sure I didn't miss this this time 😅

Comment on lines +532 to +538
# resolve action filter
DataCondition.objects.create(
comparison=PRIORITY_MAP.get(trigger.label, DetectorPriorityLevel.HIGH),
condition_result=True,
type=Condition.ISSUE_PRIORITY_DEESCALATING,
condition_group=data_condition_group,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was introduced after the initial migration, so added it here to avoid the need for a backfill

Comment on lines +778 to +787
alert_rules = (
AlertRule.objects_with_snapshots.filter(
status__in=[
AlertRuleStatus.PENDING.value,
AlertRuleStatus.DISABLED.value, # we don't need to worry about NOT_ENOUGH_DATA here, because all anomaly detection alerts have been migrated
]
)
.filter(~Exists(AlertRuleDetector.objects.filter(alert_rule_id=OuterRef("id"))))
.filter(Exists(AlertRuleProjects.objects.filter(alert_rule_id=OuterRef("id"))))
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're fetching all non-deleted alert rules that 1) have not been migrated and 2) have associated projects

Comment on lines +806 to +810
enabled = (
True
if snoozed is None and alert_rule.status == AlertRuleStatus.PENDING.value
else False
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disable the detector if the alert rule is disabled

Copy link

codecov bot commented Jun 17, 2025

Codecov Report

Attention: Patch coverage is 91.48148% with 46 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...migrations/0071_migrate_remaining_metric_alerts.py 86.74% 46 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #93749       +/-   ##
===========================================
+ Coverage   46.09%   88.04%   +41.95%     
===========================================
  Files       10309    10330       +21     
  Lines      594907   596553     +1646     
  Branches    23134    23134               
===========================================
+ Hits       274198   525230   +251032     
+ Misses     320215    70829   -249386     
  Partials      494      494               

@mifu67 mifu67 merged commit 4c71e6a into master Jun 18, 2025
66 checks passed
@mifu67 mifu67 deleted the mifu67/aci/migrate-metric-alert-stragglers branch June 18, 2025 17:58
andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025
This migration creates ACI objects for the following alerts:
- Alerts that were disabled during the initial migration but are now
enabled
- Alerts that were anomaly detection alerts and created before anomaly
detection dual write was enabled, but became static/percent alerts
before the anomaly detection alert migration
- All disabled alert rules with projects to prevent future instances of
case 1. These detectors will be set to disabled.
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