feat(aci milestone 3): delete data in Seer when AD detector is deleted #101843

mifu67 · 2025-10-20T23:53:14Z

When a detector is deleted, we need to delete the rule data in Seer. Similar implementation to the legacy alert rule code, but uses detector ID instead of alert rule ID.

Note that because we are no longer able to use the post delete signal (the QuerySubscription model is deleted by the time we get to post delete so we cannot access the source_id needed) we cannot ensure Seer knows to delete data on their end during a cascade deletion - however Seer will delete data that doesn't receive an update for 90 days so it will eventually be cleaned up.

codecov · 2025-10-21T00:15:34Z

Codecov Report

❌ Patch coverage is 81.57895% with 7 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/seer/anomaly_detection/delete_rule.py	77.27%	5 Missing ⚠️
src/sentry/incidents/logic.py	81.81%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #101843      +/-   ##
===========================================
+ Coverage   76.26%    80.67%   +4.40%     
===========================================
  Files        9200      9206       +6     
  Lines      393001    393451     +450     
  Branches    25000     25000              
===========================================
+ Hits       299740    317413   +17673     
+ Misses      92836     75613   -17223     
  Partials      425       425

src/sentry/workflow_engine/endpoints/organization_detector_details.py

saponifi3d

looks like there's a bit of code we can cleanup before merging, generally the setup for the hooks etc all looks good though!

src/sentry/seer/anomaly_detection/delete_rule.py

saponifi3d · 2025-10-31T23:32:59Z

src/sentry/seer/anomaly_detection/delete_rule.py

+    return True
+
+
+def delete_rule_in_seer_legacy(alert_rule: "AlertRule") -> bool:


could this method just invoke return delete_rule_in_seer(alert_rule.id)? (that way we can reduce code replication)

and

Suggested change

def delete_rule_in_seer_legacy(alert_rule: "AlertRule") -> bool:

def delete_rule_in_seer_legacy(alert_rule: AlertRule) -> bool:

The two methods are slightly different in what they pass to Seer. I think we've done this code duplication for all anomaly detection methods so we can easily delete the legacy code.

Since we need to maintain this code in the future, it might be a cool time to decompose the method and reuse what we can. That would also mean we don't need to maintain two code paths while we're waiting to remove the legacy code. Instead we cold take the opportunity to make this a bit easier to manage, and a lot easier for us to debug any issues in the mean time.

The way i tend to decompose things with two examples is to just look at those differences and try to determine where it would make the most sense to expose new methods. For example, maybe we could wrap the send to seer and handle errors as a method, then delete_rule_in_seer_legacy could compose those shared methods and make any tweaks it might need.

Actually, I think we can delete the legacy code outright if every alert rule has a detector. The call only needs to happen once.
I see your point about decomp, will look into it.

src/sentry/workflow_engine/endpoints/organization_detector_details.py

src/sentry/seer/anomaly_detection/delete_rule.py

saponifi3d

i think biggest feedback would be to make delete_rule_in_seer a little more debug friendly; we can either decompose the method and share it between alert rule / detector deletion or we can update the logs so we can easily differentiate (i think that'd just be changing rule to detector).

src/sentry/workflow_engine/endpoints/organization_detector_details.py

saponifi3d · 2025-11-05T19:34:22Z

src/sentry/seer/anomaly_detection/delete_rule.py

+    return True
+
+
+def delete_rule_in_seer_legacy(alert_rule: "AlertRule") -> bool:


Since we need to maintain this code in the future, it might be a cool time to decompose the method and reuse what we can. That would also mean we don't need to maintain two code paths while we're waiting to remove the legacy code. Instead we cold take the opportunity to make this a bit easier to manage, and a lot easier for us to debug any issues in the mean time.

The way i tend to decompose things with two examples is to just look at those differences and try to determine where it would make the most sense to expose new methods. For example, maybe we could wrap the send to seer and handle errors as a method, then delete_rule_in_seer_legacy could compose those shared methods and make any tweaks it might need.

src/sentry/seer/anomaly_detection/delete_rule.py

src/sentry/incidents/logic.py

mifu67 · 2025-11-06T02:05:49Z

@saponifi3d removed the legacy code. Let me know if you think we should still break down the error handling into a separate method.

ceorourke · 2025-11-06T18:35:56Z

src/sentry/incidents/logic.py

-                        extra={
-                            "rule_id": alert_rule.id,
-                        },
+                try:


When are we still hitting this code path? is it only for people using the legacy API? If so, don't we need to call this on delete too?

I put the delete code in the detector lifecycle hook, so any time a detector is deleted the code will be called. If all alert rules are dual written, then we don't need the call in multiple places.
I didn't hook deletion into updates, however, so this is for users of the legacy API. Good callout that I should create an update hook as well.

ceorourke · 2025-11-11T00:33:56Z

src/sentry/incidents/models/alert_rule.py

            )

-    @classmethod
-    def delete_data_in_seer(cls, instance: AlertRule, **kwargs: Any) -> None:


Since we now require the source id (the query subscription id) instead of the rule id we can't execute this in post delete because the query subscription is deleted by this point. Instead we call it in logic.py's delete_alert_rule method.

The post_delete is used for cascade deletions (like when a project gets deleted), during which the code in logic.py will not be executed 😭 I don't think we can take this out of here

~~I guess it doesn't matter if Seer has a cleanup task, though~~ EDIT: since we're overriding the detector delete method, we don't need anything for the alert rule at all, so this is good to remove.

ceorourke · 2025-11-11T00:47:21Z

src/sentry/incidents/logic.py


-        incidents = Incident.objects.filter(alert_rule=alert_rule)
-        if incidents.exists():
+        if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC:


We're now deleting the rule in Seer regardless of whether an incident exists or not - previously we had it behind that condition because we were deleting it in the post delete signal and due to the order of deletions there would still be incidents after the rule was deleted so this was the only way we could make sure a dynamic alert rule with incidents sent the data to Seer.

If we're overriding the delete() method on the detector, I don't think we need any logic to do it for the alert rule (given that every alert rule has a corresponding detector). Let's take this out.

I have it here for the legacy API use case - if they hit this directly, it won't hit the detector validator code path right? Worst case Seer gets 2 requests to delete the same data, but the 2nd one would just fail because it's already gone.

Okay, I see that we changed the validator and not the model itself, in which case this should stay.

sentry · 2025-11-11T19:35:38Z

src/sentry/seer/anomaly_detection/delete_rule.py

+            },
+        )
+        return
+


Bug: The delete_data_in_seer_for_detector() function incompletely deletes data for detectors with multiple data sources.
_{Severity: CRITICAL | Confidence: 1.00}

🔍 Detailed Analysis

If a detector has multiple associated data sources, the delete_data_in_seer_for_detector() function will only clean up the first one from Seer due to its use of .first(). The remaining data sources will have orphaned data in Seer, causing data bloat and inconsistent state where some detector data remains after deletion, despite the operation appearing to succeed.

💡 Suggested Fix

Modify delete_data_in_seer_for_detector() to iterate over all DataSourceDetector objects associated with the detector, rather than using .first(), to ensure all related data in Seer is properly cleaned up.

🤖 Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: src/sentry/seer/anomaly_detection/delete_rule.py#L40 Potential issue: If a detector has multiple associated data sources, the `delete_data_in_seer_for_detector()` function will only clean up the first one from Seer due to its use of `.first()`. The remaining data sources will have orphaned data in Seer, causing data bloat and inconsistent state where some detector data remains after deletion, despite the operation appearing to succeed.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

There are no detectors w/ multiple data sources

cursor · 2025-11-11T19:37:15Z

src/sentry/incidents/logic.py

+    Delete accompanying data in Seer for anomaly detection rules
+    """
+    try:
+        source_id = QuerySubscription.objects.get(snuba_query_id=snuba_query.id).id


Bug: Handle Multiple Subscriptions Gracefully

QuerySubscription.objects.get(snuba_query_id=snuba_query.id) will raise MultipleObjectsReturned when an alert rule has multiple projects, since each project has its own subscription pointing to the same SnubaQuery. The function should either use .first() instead of .get(), or iterate through all subscriptions if multiple Seer deletions are needed.

no multi-project alert rules/detectors exist

mifu67 · 2025-11-12T18:13:54Z

Note for approver (I cannot approve since I started the PR): without the post-delete, we now have no way to clean up Seer data when a dynamic detector/alert rule is cascade deleted (such as during project or organization deletion). Seer has a cleanup task to delete data on their end for all alerts that haven't sent updates in 90 days, so it's not the end of the world, but I wanted to call out that we necessarily must lose some functionality here due to the switch to data source for the Seer key.

ram-senth · 2025-11-12T20:11:36Z

src/sentry/seer/anomaly_detection/delete_rule.py

    if status is None or status is not True:
        logger.error(
            "Request to delete alert rule from Seer was unsuccessful",
            extra=extra_data,


When status is not True, Seer includes a message indicating what failed. It will be worth logging that here.

ram-senth

Reviewed the seer interaction part and have one comment about logging failure. I do not have full context to review the other aspects like retrieving the alert details.

vercel bot deployed to Preview October 20, 2025 23:53 View deployment

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 20, 2025

kcons reviewed Oct 29, 2025

View reviewed changes

src/sentry/workflow_engine/endpoints/organization_detector_details.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview October 31, 2025 22:34 View deployment

mifu67 requested review from kcons and saponifi3d October 31, 2025 23:04

mifu67 marked this pull request as ready for review October 31, 2025 23:04

mifu67 requested review from a team as code owners October 31, 2025 23:04

vercel bot deployed to Preview October 31, 2025 23:06 View deployment

This comment was marked as outdated.

Sign in to view

vercel bot deployed to Preview October 31, 2025 23:17 View deployment

This comment was marked as outdated.

Sign in to view

saponifi3d reviewed Oct 31, 2025

View reviewed changes

mifu67 requested review from a team and saponifi3d November 3, 2025 18:11

vercel bot deployed to Preview November 3, 2025 18:14 View deployment

This comment was marked as outdated.

Sign in to view

cursor bot reviewed Nov 3, 2025

View reviewed changes

src/sentry/seer/anomaly_detection/delete_rule.py Show resolved Hide resolved

vercel bot deployed to Preview November 3, 2025 19:49 View deployment

saponifi3d reviewed Nov 5, 2025

View reviewed changes

cursor bot reviewed Nov 6, 2025

View reviewed changes

src/sentry/incidents/logic.py Outdated Show resolved Hide resolved

mifu67 requested a review from saponifi3d November 6, 2025 02:05

vercel bot deployed to Preview November 6, 2025 02:07 View deployment

vercel bot deployed to Preview November 6, 2025 02:10 View deployment

ceorourke reviewed Nov 6, 2025

View reviewed changes

move imports, don't check for dynamic twice

da0082b

vercel bot deployed to Preview November 10, 2025 22:46 View deployment

appease typing

8f7c691

vercel bot deployed to Preview November 10, 2025 22:56 View deployment

update tests, put back imports to avoid circular, fix logic.py

f78d262

ceorourke reviewed Nov 11, 2025

View reviewed changes

vercel bot deployed to Preview November 11, 2025 00:35 View deployment

delete regardless of incidents

1b19f1d

ceorourke reviewed Nov 11, 2025

View reviewed changes

vercel bot deployed to Preview November 11, 2025 00:47 View deployment

dry up

0592a42

vercel bot deployed to Preview November 11, 2025 01:03 View deployment

typing

1859b62

vercel bot deployed to Preview November 11, 2025 01:14 View deployment

update test

df56f01

vercel bot deployed to Preview November 11, 2025 18:25 View deployment

ceorourke marked this pull request as ready for review November 11, 2025 19:33

sentry bot reviewed Nov 11, 2025

View reviewed changes

cursor bot reviewed Nov 11, 2025

View reviewed changes

ram-senth reviewed Nov 12, 2025

View reviewed changes

include failure message in logs

b52d7e0

vercel bot deployed to Preview November 12, 2025 23:12 View deployment

update test

360e1bb

vercel bot deployed to Preview November 12, 2025 23:40 View deployment

ram-senth approved these changes Nov 13, 2025

View reviewed changes

ceorourke merged commit 62b1f73 into master Nov 13, 2025
66 checks passed

ceorourke deleted the mifu67/aci/delete-data-in-seer branch November 13, 2025 17:48

ceorourke mentioned this pull request Nov 13, 2025

feat(ACI): Delete data in Seer when dynamic detector type is changed #103323

Open

		return True


		def delete_rule_in_seer_legacy(alert_rule: "AlertRule") -> bool:

Uh oh!

feat(aci milestone 3): delete data in Seer when AD detector is deleted #101843

feat(aci milestone 3): delete data in Seer when AD detector is deleted #101843

Uh oh!

Conversation

mifu67 commented Oct 20, 2025 • edited by ceorourke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

saponifi3d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

saponifi3d Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

saponifi3d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mifu67 commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mifu67 Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sentry bot Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor bot Nov 11, 2025

Choose a reason for hiding this comment

Bug: Handle Multiple Subscriptions Gracefully

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mifu67 commented Nov 12, 2025

mifu67 commented Oct 20, 2025 •

edited by ceorourke

Loading

codecov bot commented Oct 21, 2025 •

edited

Loading

saponifi3d Oct 31, 2025 •

edited

Loading

mifu67 Nov 11, 2025 •

edited

Loading