-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): Drop metric_alert_fire detectors #89924
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
Conversation
|
This PR has a migration; here is the generated SQL for --
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQL |
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 #89924 +/- ##
===========================================
+ Coverage 74.82% 87.74% +12.91%
===========================================
Files 10201 10216 +15
Lines 575312 576344 +1032
Branches 22655 22655
===========================================
+ Hits 430500 505713 +75213
+ Misses 144378 70197 -74181
Partials 434 434 |
|
If we're dropping the detectors, we probably also want to drop the dual written metric alert workflows. |
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.
lgtm - cc @mifu67 to make sure the rollout flag is at 0 (and that we'll want to keep it at 0 until we can update to metric_issue)
also +1 to michelle's callout abuot dropping the workflows -- probably easier to do before we drop the detectors to use the DetectorWorkflow lookups.
wedamija
left a comment
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.
lgtm, is there any way that more of these will get created in between running this and merging your other pr?
|
@wedamija dual write is currently turned off (rollout set to 0%), so there shouldn't be any new detectors created with the old slug. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
We're renaming `MetricAlertFire` to `MetricIssue` ([PR](#89896)). To do so, we need to drop existing detectors that use the `metric_alert_fire` slug. There's ~230 rows that match that filter and the table has ~3k rows. All the rows in the table were written during a test of the dual write and are safe to drop as they'll be rewritten again once the flag is on. I've marked this as post-deploy but it probably doesn't need to be.
We're renaming `MetricAlertFire` to `MetricIssue` and changing the group type slug from `metric_alert_fire` to `metric_issue`. This change is safe to land once [this migration](#89924) drops all the existing detector rows that use `metric_alert_fire`. I've left a skeleton class def with the slug to keep getsentry tests passing. The [corresponding getsentry PR](https://github.com/getsentry/getsentry/pull/17255) can land after this one, and then I'll remove the class in a followup.
We're renaming
MetricAlertFiretoMetricIssue(PR). To do so, we need to drop existing detectors that use themetric_alert_fireslug. There's ~230 rows that match that filter and the table has ~3k rows. All the rows in the table were written during a test of the dual write and are safe to drop as they'll be rewritten again once the flag is on. I've marked this as post-deploy but it probably doesn't need to be.