ref(uptime): Remove usage of ProjectUptimeSubscription from endpoints #98794
Conversation
github-actions
bot
added
the
Scope: Backend
src/sentry/uptime/endpoints/bases.py
Outdated
There was a problem hiding this comment.
Removes querying for ProejctUptimeSubscriptions, now only passes in uptime detectors.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #98794 +/- ##
=======================================
Coverage 81.22% 81.22%
=======================================
Files 8534 8534
Lines 376905 376920 +15
Branches 23890 23890
=======================================
+ Hits 306148 306165 +17
+ Misses 70389 70387 -2
Partials 368 368 |
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-uptime-remove-usage-of-projectuptimesubscription-from-endpoints
branch
from
dabe5d0 to
545fb75
Compare
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-uptime-remove-usage-of-projectuptimesubscription-from-endpoints
branch
from
545fb75 to
0ecd18a
Compare
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-uptime-remove-usage-of-projectuptimesubscription-from-endpoints
branch
from
0ecd18a to
4792899
Compare
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-uptime-remove-usage-of-projectuptimesubscription-from-endpoints
branch
from
4792899 to
38ced18
Compare
| "id": item_id, | ||
| "issue_rule": isinstance(item, Rule), | ||
| "metric_rule": isinstance(item, AlertRule), | ||
| "uptime_rule": isinstance(item, ProjectUptimeSubscription), | ||
| "uptime_rule": ( | ||
| isinstance(item, Detector) | ||
| and item.type == GROUP_TYPE_UPTIME_DOMAIN_CHECK_FAILURE | ||
| ), | ||
| "crons_rule": isinstance(item, Monitor), |
There was a problem hiding this comment.
This is kind of a weird structure... shouldn't this just be item_type: type(item)?
I know this isn't your code, but maybe rather than adding to this pattern, just add both type(item) and item.type here, so that others don't have to keep adding this weird logic
There was a problem hiding this comment.
What do you mean exactly?
| @@ -455,7 +467,7 @@ def serialize( | |||
| updated_attrs["type"] = "alert_rule" | |||
| elif isinstance(obj, Rule): | |||
| updated_attrs["type"] = "rule" | |||
| elif isinstance(obj, ProjectUptimeSubscription): | |||
| elif isinstance(obj, Detector) and obj.type == GROUP_TYPE_UPTIME_DOMAIN_CHECK_FAILURE: | |||
There was a problem hiding this comment.
This is also not a great pattern... do you know if this is a long term pattern or just transitional? People shouldn't need to add things like this in a bunch of places, it should just be on the grouptype
There was a problem hiding this comment.
I think this is just because of what we're doing here in this combined alert serializer, which would go away in the future right?
| # Create DetectorState based on the uptime_status from the uptime_subscription | ||
| if project_sub.uptime_subscription.uptime_status == UptimeStatus.FAILED: | ||
| DetectorState.objects.create( | ||
| detector=detector, | ||
| state=DetectorPriorityLevel.HIGH, | ||
| is_triggered=True, | ||
| ) | ||
| else: | ||
| DetectorState.objects.create( | ||
| detector=detector, | ||
| state=DetectorPriorityLevel.OK, | ||
| is_triggered=False, | ||
| ) |
There was a problem hiding this comment.
Why do we create the state here? Isn't this just happening on detector creation? So presumably the state is not failed?
There was a problem hiding this comment.
One of the tests for the alert rules combined index that was testing sorting for rules that are in a failing state vs rules that are in an OK state needed this.
Previously it was reading the uptime status on the uptime_subscription relationship of the ProjectUptimeSubscription. When I switched that endpoint over to reading from detectors I had it subquery the DetectorState table (instead of looking at uptime_status, since that will go away in the future). Something that was missing previously from this test fixture was setting the detector state to the correct state when the uptime_status parameter was passed as failing.
This systemically removes ProjectUptimeSubscription usages from uptime endpoints. - Remove legacy dual ID handling logic and query parameter dependencies. The useDetectorId query parameter no longer has any affect, all IDs are treated as detector Ids. - Update all endpoints to use Detector model instead of ProjectUptimeSubscription. - Refactor serializers to work with Detector objects and associated UptimeSubscription data. - Update validators to create and update Detector instances directly - Update audit logging to use detector IDs and consolidated audit log data functions. Previously the get_audit_log_data method was part of the ProjectUptimeSubscription, which is no longer used. - Remove unused ProjectUptimeSubscription references throughout codebase IMPORTANTLY the tests have very little changes to them, this is because this change does NOT change how data is returned. Requires #98527
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-uptime-remove-usage-of-projectuptimesubscription-from-endpoints
branch
from
38ced18 to
2cec51e
Compare
This systemically removes ProjectUptimeSubscription usages from uptime endpoints.
Remove legacy dual ID handling logic and query parameter dependencies. The useDetectorId query parameter no longer has any affect, all IDs are treated as detector Ids.
Update all endpoints to use Detector model instead of ProjectUptimeSubscription.
Refactor serializers to work with Detector objects and associated UptimeSubscription data.
Update validators to create and update Detector instances directly
Update audit logging to use detector IDs and consolidated audit log data functions. Previously the get_audit_log_data method was part of the ProjectUptimeSubscription, which is no longer used.
Remove unused ProjectUptimeSubscription references throughout codebase
IMPORTANTLY the tests have very little changes to them, this is because this change does NOT change how data is returned.
Requires #98527