fix(aci): Ensure QuerySubscription changes to AlertRule are reflected on Detector DataSource#102334
Conversation
… on Detector DataSource
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #102334 +/- ##
=========================================
Coverage 80.95% 80.96%
=========================================
Files 8769 8770 +1
Lines 389622 389754 +132
Branches 24777 24777
=========================================
+ Hits 315431 315561 +130
- Misses 73813 73815 +2
Partials 378 378 |
|
FYI, all the uncovered lines are for the "if for some weird reason the snuba/subscription/datasource id doesn't actually exist, log and exit" cases. I find that adding test cases for every control flow branch even the ones that just log a thing isn't super useful, but just assuming things exist and allowing the exception when they don't to be caught and reported up the stack (as is the design here) tends to make cursor bot and seer bot freak out. |
saponifi3d
left a comment
There was a problem hiding this comment.
lgtm to fix the legacy api, but not sure if this is a problem in the new api as well -- we should probably double check that as well.
| ) | ||
|
|
||
|
|
||
| def update_data_source_for_detector(alert_rule: AlertRule, detector: Detector) -> None: |
There was a problem hiding this comment.
since this code is meant to be temporary (the directory is meant to be deletable once we want to start removing the dual write code). should we have this live somewhere else? it seems like this only affects metric alerts, so maybe somewhere in the incidents app?
There was a problem hiding this comment.
I think we want this to be deleted to, pending confirmation that we are actually updating QuerySubscriptions properly on AlertRule-less DataSources.
… on Detector DataSource (#102334) Fixes ACI-505.
… on Detector DataSource (#102334) Fixes ACI-505.
… on Detector DataSource (#102334) Fixes ACI-505.
Fixes ACI-505.