feat(alerts): Migrate metric alert rules #80505
Conversation
github-actions
bot
added
the
Scope: Backend
|
This PR has a migration; here is the generated SQL for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #80505 +/- ##
==========================================
- Coverage 80.34% 80.34% -0.01%
==========================================
Files 7220 7221 +1
Lines 319570 319699 +129
Branches 20782 20782
==========================================
+ Hits 256771 256864 +93
- Misses 62405 62441 +36
Partials 394 394 |
| WorkflowDataConditionGroup = apps.get_model("sentry", "WorkflowDataConditionGroup") | ||
|
|
||
| for rule in RangeQuerySetWrapperWithProgressBarApprox(AlertRule.objects.all()): | ||
| # should I skip migrating snapshotted and disabled rules? |
There was a problem hiding this comment.
I think so, they're just history, and we're not migrating history from what I remember?
|
|
||
| for rule in RangeQuerySetWrapperWithProgressBarApprox(AlertRule.objects.all()): | ||
| # should I skip migrating snapshotted and disabled rules? | ||
| # what about the date_added and date_updated fields? can I write to those to preserve history? |
There was a problem hiding this comment.
We might need to change the definitions or something, I think that auto_add and so on make the cols read only. You'd need to experiment
There was a problem hiding this comment.
Yeah according to the docs it's uneditable, we'll have to temporarily change the DefaultFieldsModels
| # what about the date_added and date_updated fields? can I write to those to preserve history? | ||
| # description column will be added to the Detector model, but at the time of writing it isn't there yet. will need to update this | ||
| # same for threshold_period, will likely be added to the Detector model but isn't there yet | ||
| # AlertRuleActivity creation rows need to be backfilled into the Detector model, needs new created_by and created_at columns |
There was a problem hiding this comment.
Not sure if we need this if we're not keeping history?
We probably do need to migrate the audit logs over though...
There was a problem hiding this comment.
If we want to maintain the data about which user created the rule and when we'll need to put this somewhere. The audit log table has a lot of this but since it was added more recently there's a ~2 year gap of rule data that's missing. I wouldn't be opposed to trying to backfill that table with this instead though.
| condition=trigger.threshold_type, | ||
| comparison=trigger.alert_threshold, | ||
| condition_result=state, # ??? is that right? | ||
| type="idk", # talk to josh about this |
There was a problem hiding this comment.
in issue alerts this is https://github.com/getsentry/sentry/blob/master/src/sentry/constants.py#L264, I'm not sure if we need more than 1 for metric alerts
| ) | ||
|
|
||
| # state is based on incident status | ||
| project = AlertRuleProjects.objects.get(alert_rule_id=rule.id) |
There was a problem hiding this comment.
are there any performance concerns with us fetching all the alert rules, then fetching these individually?
There was a problem hiding this comment.
It'll be slower, but we have < 200k metric alerts, so I don't think it's a huge deal overall
| DetectorWorkflow = apps.get_model("sentry", "DetectorWorkflow") | ||
| WorkflowDataConditionGroup = apps.get_model("sentry", "WorkflowDataConditionGroup") | ||
|
|
||
| for rule in RangeQuerySetWrapperWithProgressBarApprox(AlertRule.objects.all()): |
There was a problem hiding this comment.
if so, i wonder how we could decompose the loop body into a method that's reusable for the migration and the API 🤔
There was a problem hiding this comment.
We don't typically share code between migrations and real code
ceorourke
force-pushed
the
ceorourke/metric-alert-migration-draft
branch
from
3ce2b4d to
d4d626c
Compare
|
This PR has a migration; here is the generated SQL for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
| condition=trigger.threshold_type, | ||
| comparison=trigger.alert_threshold, | ||
| condition_result=condition_result, | ||
| type="metric_alert", # this will probably change by the time we actually do the migration |
There was a problem hiding this comment.
needs condition_group column (DataConditionGroup id)
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Draft of metric alert migration to use as reference (and at some point, as an actual migration)