feat(ACI): Delete endpoint for a detector #84279
Conversation
github-actions
bot
added
the
Scope: Backend
src/sentry/workflow_engine/endpoints/project_detector_details.py
Outdated
Show resolved
Hide resolved
ceorourke
force-pushed
the
ceorourke/delete-detector-endpoint
branch
from
8920d82 to
57af780
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| @@ -69,3 +77,51 @@ def get(self, request: Request, project, detector): | |||
| DetectorSerializer(), | |||
| ) | |||
| return Response(serialized_detector) | |||
|
|
|||
| @extend_schema( | |||
There was a problem hiding this comment.
🥔 for adding documentation to the endpoint from the beginning. People make experimental endpoints left and right and the endpoint sits there for long before publishing so everyone forgets the context 😅
src/sentry/workflow_engine/endpoints/project_detector_details.py
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| class DetectorDeletionTask(ModelDeletionTask[Detector]): | ||
| def get_child_relations(self, instance: Detector) -> list[BaseRelation]: |
There was a problem hiding this comment.
Just note that with cascade delete if there are a large number of related rows the delete can fail. That's probably unlikely here, but jfyi
| def setUp(self): | ||
| self.data_condition_group = self.create_data_condition_group() | ||
| self.data_condition = self.create_data_condition(condition_group=self.data_condition_group) | ||
| self.snuba_query = self.create_snuba_query() |
There was a problem hiding this comment.
Yeah, we could do that. In the past we didn't because potentially one snuba query could be shared, but in practice we haven't really used that.
Could make sense to just check that there aren't other links to the snuba query and then delete it if orphaned.
| model_relations: list[BaseRelation] = [ | ||
| ModelRelation(QuerySubscription, {"id": instance.query_id}) | ||
| ] |
There was a problem hiding this comment.
Do we need to check the type here? query_id will point to different datasources in the future.
Looks like you can do instance.type_handler.bulk_get_query_object([instance]) to get all related objects.
I think that probably, you should build the SnubaQuery deletion stuff into a delete module for subscriptions rather than here. That way, you can just add the QuerySubscription as the model relation, and then it will automatically clean itself up.
There was a problem hiding this comment.
Today we only appear to have a single type for a QuerySubscription (incident). Since I moved deleting the snuba query to the query subscription deletion task do I still need to filter?
I moved deleting it into querysubscription.py but ended up having to check for an AlertRule first - after everything is migrated that won't be a problem and I'm not sure if in practice it'll ever happen that we're deleting a QuerySubscription that still has an active AlertRule.
There was a problem hiding this comment.
I think you still need to use bulk_get_query_object here or the QuerySubscription rows won't be deleted when the DataSource is deleted. Probably you'd need to schedule deletions for them, since we don't have an explicit relationship that we can follow here...
| if not AlertRule.objects.filter(snuba_query_id=instance.snuba_query_id).exists(): | ||
| return [ModelRelation(SnubaQuery, {"id": instance.snuba_query_id})] |
There was a problem hiding this comment.
Should we also check that the SnubaQuery only has this one subscription?
Create a DELETE endpoint for a given detector. This deletes the related
DataSource,QuerySubscription, andSnubaQuery.It assumes that a data source is only connected to a single detector. The db model supports multiple detectors but the UI does not.