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.

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

❌ 8 Tests Failed:

Tests completed	Failed	Passed	Skipped
29497	8	29489	243

View the top 3 failed test(s) by shortest run time

tests.sentry.incidents.test_logic.DeleteAlertRuleTest::test_delete_anomaly_detection_rule

Stack Traces | 3.11s run time

#x1B[1m#x1B[.../sentry/incidents/test_logic.py#x1B[0m:2062: in test_delete_anomaly_detection_rule
    assert mock_seer_request.call_count == 1
#x1B[1m#x1B[31mE   AssertionError: assert 0 == 1#x1B[0m
#x1B[1m#x1B[31mE    +  where 0 = <MagicMock name='urlopen' id='139862062123392'>.call_count#x1B[0m

tests.sentry.incidents.test_logic.DeleteAlertRuleTest::test_delete_anomaly_detection_rule_failure

Stack Traces | 3.12s run time

#x1B[1m#x1B[.../sentry/incidents/test_logic.py#x1B[0m:2187: in test_delete_anomaly_detection_rule_failure
    mock_seer_logger.error.assert_called_with(
#x1B[1m#x1B[.../hostedtoolcache/Python/3.13.1.../x64/lib/python3.13/unittest/mock.py#x1B[0m:968: in assert_called_with
    raise AssertionError(error_message)
#x1B[1m#x1B[31mE   AssertionError: expected call not found.#x1B[0m
#x1B[1m#x1B[31mE   Expected: error('Request to delete alert rule from Seer was unsuccessful', extra={'rule_id': 36})#x1B[0m
#x1B[1m#x1B[31mE     Actual: not called.#x1B[0m

tests.sentry.incidents.test_logic.DeleteAlertRuleTest::test_delete_anomaly_detection_rule_error

Stack Traces | 3.15s run time

#x1B[1m#x1B[.../sentry/incidents/test_logic.py#x1B[0m:2120: in test_delete_anomaly_detection_rule_error
    mock_seer_logger.error.assert_called_with(
#x1B[1m#x1B[.../hostedtoolcache/Python/3.13.1.../x64/lib/python3.13/unittest/mock.py#x1B[0m:968: in assert_called_with
    raise AssertionError(error_message)
#x1B[1m#x1B[31mE   AssertionError: expected call not found.#x1B[0m
#x1B[1m#x1B[31mE   Expected: error('Error when hitting Seer delete rule data endpoint', extra={'response_data': 'Bad request', 'rule_id': 24})#x1B[0m
#x1B[1m#x1B[31mE     Actual: not called.#x1B[0m

tests.sentry.incidents.test_logic.DeleteAlertRuleTest::test_delete_anomaly_detection_rule_timeout

Stack Traces | 3.26s run time

#x1B[1m#x1B[.../sentry/incidents/test_logic.py#x1B[0m:2087: in test_delete_anomaly_detection_rule_timeout
    mock_seer_logger.warning.assert_called_with(
#x1B[1m#x1B[.../hostedtoolcache/Python/3.13.1.../x64/lib/python3.13/unittest/mock.py#x1B[0m:968: in assert_called_with
    raise AssertionError(error_message)
#x1B[1m#x1B[31mE   AssertionError: expected call not found.#x1B[0m
#x1B[1m#x1B[31mE   Expected: warning('Timeout error when hitting Seer delete rule data endpoint', extra={'rule_id': 32})#x1B[0m
#x1B[1m#x1B[31mE     Actual: not called.#x1B[0m

tests.sentry.incidents.test_logic.DeleteAlertRuleTest::test_with_incident_anomaly_detection_rule

Stack Traces | 3.38s run time

#x1B[1m#x1B[.../sentry/incidents/test_logic.py#x1B[0m:1996: in test_with_incident_anomaly_detection_rule
    assert mock_seer_request.call_count == 1
#x1B[1m#x1B[31mE   AssertionError: assert 0 == 1#x1B[0m
#x1B[1m#x1B[31mE    +  where 0 = <MagicMock name='urlopen' id='139839358436416'>.call_count#x1B[0m

tests.sentry.incidents.test_logic.DeleteAlertRuleTest::test_with_incident_anomaly_detection_rule_error

Stack Traces | 3.57s run time

#x1B[1m#x1B[.../sentry/incidents/test_logic.py#x1B[0m:2021: in test_with_incident_anomaly_detection_rule_error
    mock_seer_logger.error.assert_called_with(
#x1B[1m#x1B[.../hostedtoolcache/Python/3.13.1.../x64/lib/python3.13/unittest/mock.py#x1B[0m:968: in assert_called_with
    raise AssertionError(error_message)
#x1B[1m#x1B[31mE   AssertionError: expected call not found.#x1B[0m
#x1B[1m#x1B[31mE   Expected: error('Error when hitting Seer delete rule data endpoint', extra={'response_data': 'Bad request', 'rule_id': 104})#x1B[0m
#x1B[1m#x1B[31mE     Actual: not called.#x1B[0m

tests.sentry.deletions.test_alert_rule.DeleteAlertRuleTest::test_dynamic_alert_rule

Stack Traces | 3.72s run time

#x1B[1m#x1B[.../sentry/deletions/test_alert_rule.py#x1B[0m:124: in test_dynamic_alert_rule
    assert mock_delete_request.call_count == 1
#x1B[1m#x1B[31mE   AssertionError: assert 0 == 1#x1B[0m
#x1B[1m#x1B[31mE    +  where 0 = <MagicMock name='urlopen' id='140290219011312'>.call_count#x1B[0m

tests.sentry.incidents.test_logic.DeleteAlertRuleTest::test_delete_anomaly_detection_rule_attribute_error

Stack Traces | 3.77s run time

#x1B[1m#x1B[.../sentry/incidents/test_logic.py#x1B[0m:2153: in test_delete_anomaly_detection_rule_attribute_error
    mock_seer_logger.exception.assert_called_with(
#x1B[1m#x1B[.../hostedtoolcache/Python/3.13.1.../x64/lib/python3.13/unittest/mock.py#x1B[0m:968: in assert_called_with
    raise AssertionError(error_message)
#x1B[1m#x1B[31mE   AssertionError: expected call not found.#x1B[0m
#x1B[1m#x1B[31mE   Expected: exception('Failed to parse Seer delete rule data response', extra={'rule_id': 25})#x1B[0m
#x1B[1m#x1B[31mE     Actual: not called.#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

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

cursor · 2025-11-03T19:47:51Z

src/sentry/seer/anomaly_detection/delete_rule.py

+        )
+        return False
+
+    if response.status >= 400:


Bug: Inconsistent HTTP Error Handling Between Functions

There's an inconsistency in HTTP error handling between delete_rule_in_seer and delete_rule_in_seer_legacy. The new function treats response.status >= 400 as an error, but the legacy one uses > 400. This means the legacy function won't flag HTTP 400 responses from Seer as errors, leading to different behavior for new detectors and legacy alert rules.

Fine, I don't think Seer returns any 400s

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.

saponifi3d · 2025-11-05T20:07:36Z

src/sentry/seer/anomaly_detection/delete_rule.py

+        results = json.loads(decoded_data)
+    except JSONDecodeError:
+        logger.exception(
+            "Failed to parse Seer delete rule data response",


if we want to keep these methods separate, let's update the exception text so we can determine if it was triggered by delete_rule_in_seer_legacy or this method.

cursor · 2025-11-06T02:05:40Z

src/sentry/incidents/logic.py

                success = delete_rule_in_seer(
-                    alert_rule=alert_rule,
+                    organization=alert_rule.organization,
+                    source_id=source_id,


Bug: Unhandled exception leads to undefined variable usage crash

If QuerySubscription.DoesNotExist exception is raised, the variable source_id is never assigned, but the code continues execution and tries to use source_id in the delete_rule_in_seer() call on line 933, which will raise a NameError. The code should either return early after logging the exception or assign a default value to source_id before the try block.

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.

implement

c5cbb39

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

mifu67 and others added 2 commits October 31, 2025 15:29

Merge branch 'master' into mifu67/aci/delete-data-in-seer

513aa9c

🛠️ apply pre-commit fixes

356ab03

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

fix test

62a7dec

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

wrong ID

7f8a712

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

fixes

2e40979

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

mifu67 added 2 commits November 3, 2025 11:43

correct ID

968f2e3

sure

f3ec8a1

cursor bot reviewed Nov 3, 2025

View reviewed changes

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

saponifi3d reviewed Nov 5, 2025

View reviewed changes

remove unneededed legacy code

cec4a2e

cursor bot reviewed Nov 6, 2025

View reviewed changes

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

bug

96d1254

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

		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

Are you sure you want to change the base?

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

Conversation

mifu67 commented Oct 20, 2025 • edited 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!

❌ 8 Tests Failed:

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!

cursor bot Nov 3, 2025

Choose a reason for hiding this comment

Bug: Inconsistent HTTP Error Handling Between Functions

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

cursor bot Nov 6, 2025

Choose a reason for hiding this comment

Bug: Unhandled exception leads to undefined variable usage crash

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mifu67 commented Oct 20, 2025 •

edited

Loading

codecov bot commented Oct 21, 2025 •

edited

Loading

saponifi3d Oct 31, 2025 •

edited

Loading