feat(aci milestone 3): anomaly detection dual write helpers #92693
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #92693 +/- ##
==========================================
+ Coverage 87.89% 87.94% +0.04%
==========================================
Files 10298 10266 -32
Lines 591309 590606 -703
Branches 23008 22814 -194
==========================================
- Hits 519733 519400 -333
+ Misses 71140 70764 -376
- Partials 436 442 +6 |
ceorourke
left a comment
ceorourke
left a comment
There was a problem hiding this comment.
mostly LGTM just some questions
| # changing detector priority level for anomaly detection alerts is handled by the data condition | ||
| # so we should delete the explicit resolution condition if it exists |
There was a problem hiding this comment.
does this mean this was a regular alert that was changed to become an anomaly detection alert?
There was a problem hiding this comment.
Yes, that's what is happening here
| if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC: | ||
| if resolve_condition is not None: |
There was a problem hiding this comment.
| if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC: | |
| if resolve_condition is not None: | |
| if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC and resolve_condition is not None: |
There was a problem hiding this comment.
I wrote it this way because we should always return before the resolve detector trigger logic for anomaly detection alerts!
| updated_detector_trigger_fields["comparison"] = alert_rule_trigger.alert_threshold | ||
| alert_rule = alert_rule_trigger.alert_rule | ||
|
|
||
| # if we're changing to anomaly detection, replace the detector trigger with the new type |
There was a problem hiding this comment.
how do we know it wasn't already one? I guess it doesn't hurt but we could check if comparison already has this stuff
There was a problem hiding this comment.
We don't have a record of what was changed on the alert, so anomaly detection trigger changes should always set the comparison field. I think the comment is a bit misleading.
| resolve_data_condition = DataCondition.objects.get( | ||
| comparison=DetectorPriorityLevel.HIGH, | ||
| condition_result=True, | ||
| condition_group__in=workflow_dcgs, | ||
| type=Condition.ISSUE_PRIORITY_DEESCALATING, | ||
| ) | ||
| assert resolve_data_condition.condition_group == data_condition.condition_group |
There was a problem hiding this comment.
I thought we wouldn't have the resolve data condition for a dynamic rule?
There was a problem hiding this comment.
This is the issue de-escalation action filter 😅 sorry, bit confusing.
tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py
Show resolved
Hide resolved
Update the dual write and update helpers to handle anomaly detection alerts. Anomaly detection alerts have a different kind of detector trigger and do not have explicit resolution detector triggers. Additionally, add support to the dual update helpers for switching between the two alert types.
Update the dual write and update helpers to handle anomaly detection alerts. Anomaly detection alerts have a different kind of detector trigger and do not have explicit resolution detector triggers. Additionally, add support to the dual update helpers for switching between the two alert types.