diff --git a/src/sentry/incidents/logic.py b/src/sentry/incidents/logic.py index 3bf3bbb1a54ef7..93338dc51ff04a 100644 --- a/src/sentry/incidents/logic.py +++ b/src/sentry/incidents/logic.py @@ -755,6 +755,30 @@ def nullify_id(model: Model) -> None: ) +def delete_anomaly_detection_rule(snuba_query: SnubaQuery, alert_rule: AlertRule) -> None: + """ + Delete accompanying data in Seer for anomaly detection rules + """ + try: + source_id = QuerySubscription.objects.get(snuba_query_id=snuba_query.id).id + success = delete_rule_in_seer( + organization=alert_rule.organization, + source_id=source_id, + ) + if not success: + logger.error( + "Call to delete rule data in Seer failed", + extra={ + "source_id": source_id, + }, + ) + except QuerySubscription.DoesNotExist: + logger.exception( + "Snuba query missing query subscription", + extra={"snuba_query_id": snuba_query.id}, + ) + + def update_alert_rule( alert_rule: AlertRule, query_type: SnubaQuery.Type | None = None, @@ -921,18 +945,8 @@ def update_alert_rule( alert_rule, project, snuba_query, updated_fields, updated_query_fields ) else: - # if this was a dynamic rule, delete the data in Seer if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC: - success = delete_rule_in_seer( - alert_rule=alert_rule, - ) - if not success: - logger.error( - "Call to delete rule data in Seer failed", - extra={ - "rule_id": alert_rule.id, - }, - ) + delete_anomaly_detection_rule(snuba_query, alert_rule) # if this alert was previously a dynamic alert, then we should update the rule to be ready if alert_rule.status == AlertRuleStatus.NOT_ENOUGH_DATA.value: alert_rule.update(status=AlertRuleStatus.PENDING.value) @@ -1088,20 +1102,11 @@ def delete_alert_rule( ) subscriptions = _unpack_snuba_query(alert_rule).subscriptions.all() + if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC: + delete_anomaly_detection_rule(alert_rule.snuba_query, alert_rule) + incidents = Incident.objects.filter(alert_rule=alert_rule) if incidents.exists(): - # if this was a dynamic rule, delete the data in Seer - if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC: - success = delete_rule_in_seer( - alert_rule=alert_rule, - ) - if not success: - logger.error( - "Call to delete rule data in Seer failed", - extra={ - "rule_id": alert_rule.id, - }, - ) AlertRuleActivity.objects.create( alert_rule=alert_rule, user_id=user.id if user else None, diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index 841ec889b8f6e0..0383b1ba97df36 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -8,6 +8,7 @@ from sentry.incidents.logic import enable_disable_subscriptions from sentry.incidents.models.alert_rule import AlertRuleDetectionType from sentry.relay.config.metric_extraction import on_demand_metrics_feature_flags +from sentry.seer.anomaly_detection.delete_rule import delete_data_in_seer_for_detector from sentry.seer.anomaly_detection.store_data_workflow_engine import send_new_detector_data from sentry.snuba.dataset import Dataset from sentry.snuba.metrics.extraction import should_use_on_demand_metrics @@ -294,3 +295,11 @@ def create(self, validated_data: dict[str, Any]): schedule_update_project_config(detector) return detector + + def delete(self): + # Let Seer know we're deleting a dynamic detector so the data can be deleted there too + assert self.instance is not None + detector: Detector = self.instance + delete_data_in_seer_for_detector(detector) + + super().delete() diff --git a/src/sentry/incidents/models/alert_rule.py b/src/sentry/incidents/models/alert_rule.py index e47881a8a94e8b..67c44ab54b236d 100644 --- a/src/sentry/incidents/models/alert_rule.py +++ b/src/sentry/incidents/models/alert_rule.py @@ -36,7 +36,6 @@ ActionService, ActionTarget, ) -from sentry.seer.anomaly_detection.delete_rule import delete_rule_in_seer from sentry.snuba.models import QuerySubscription from sentry.types.actor import Actor from sentry.utils import metrics @@ -140,18 +139,6 @@ def clear_alert_rule_subscription_caches(cls, instance: AlertRule, **kwargs: Any for sub_id in subscription_ids ) - @classmethod - def delete_data_in_seer(cls, instance: AlertRule, **kwargs: Any) -> None: - if instance.detection_type == AlertRuleDetectionType.DYNAMIC: - success = delete_rule_in_seer(alert_rule=instance) - if not success: - logger.error( - "Call to delete rule data in Seer failed", - extra={ - "rule_id": instance.id, - }, - ) - @region_silo_model class AlertRuleProjects(Model): @@ -624,7 +611,6 @@ class Meta: post_delete.connect(AlertRuleManager.clear_subscription_cache, sender=QuerySubscription) -post_delete.connect(AlertRuleManager.delete_data_in_seer, sender=AlertRule) post_save.connect(AlertRuleManager.clear_subscription_cache, sender=QuerySubscription) post_save.connect(AlertRuleManager.clear_alert_rule_subscription_caches, sender=AlertRule) post_delete.connect(AlertRuleManager.clear_alert_rule_subscription_caches, sender=AlertRule) diff --git a/src/sentry/seer/anomaly_detection/delete_rule.py b/src/sentry/seer/anomaly_detection/delete_rule.py index 9655165e39a853..1c9d23d794849b 100644 --- a/src/sentry/seer/anomaly_detection/delete_rule.py +++ b/src/sentry/seer/anomaly_detection/delete_rule.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import logging from typing import TYPE_CHECKING @@ -5,8 +7,9 @@ from urllib3.exceptions import MaxRetryError, TimeoutError from sentry.conf.server import SEER_ALERT_DELETION_URL +from sentry.models.organization import Organization from sentry.net.http import connection_from_url -from sentry.seer.anomaly_detection.types import DeleteAlertDataRequest +from sentry.seer.anomaly_detection.types import AlertInSeer, DataSourceType, DeleteAlertDataRequest from sentry.seer.signed_seer_api import make_signed_seer_api_request from sentry.utils import json from sentry.utils.json import JSONDecodeError @@ -18,21 +21,51 @@ ) if TYPE_CHECKING: - from sentry.incidents.models.alert_rule import AlertRule + from sentry.workflow_engine.models.detector import Detector + +def delete_data_in_seer_for_detector(detector: Detector): + from sentry.incidents.models.alert_rule import AlertRuleDetectionType + from sentry.workflow_engine.models import DataSourceDetector -def delete_rule_in_seer(alert_rule: "AlertRule") -> bool: + data_source_detector = DataSourceDetector.objects.filter(detector_id=detector.id).first() + if not data_source_detector: + logger.error( + "No data source found for detector", + extra={ + "detector_id": detector.id, + }, + ) + return + + organization = detector.project.organization + + if detector.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC: + success = delete_rule_in_seer( + source_id=int(data_source_detector.data_source.source_id), organization=organization + ) + if not success: + logger.error( + "Call to delete rule data in Seer failed", + extra={ + "detector_id": detector.id, + }, + ) + + +def delete_rule_in_seer(source_id: int, organization: Organization) -> bool: """ Send a request to delete an alert rule from Seer. Returns True if the request was successful. """ body = DeleteAlertDataRequest( - organization_id=alert_rule.organization.id, - alert={"id": alert_rule.id}, + organization_id=organization.id, + alert=AlertInSeer( + id=None, source_id=source_id, source_type=DataSourceType.SNUBA_QUERY_SUBSCRIPTION + ), ) extra_data = { - "rule_id": alert_rule.id, + "source_id": source_id, } - try: response = make_signed_seer_api_request( connection_pool=seer_anomaly_detection_connection_pool, @@ -46,7 +79,7 @@ def delete_rule_in_seer(alert_rule: "AlertRule") -> bool: ) return False - if response.status > 400: + if response.status >= 400: logger.error( "Error when hitting Seer delete rule data endpoint", extra={ @@ -75,7 +108,14 @@ def delete_rule_in_seer(alert_rule: "AlertRule") -> bool: return False status = results.get("success") - if status is None or status is not True: + if status is None: + logger.error( + "Request to delete alert rule from Seer was unsuccessful", + extra=extra_data, + ) + return False + elif status is not True: + extra_data["message"] = results.get("message") logger.error( "Request to delete alert rule from Seer was unsuccessful", extra=extra_data, diff --git a/tests/sentry/deletions/test_alert_rule.py b/tests/sentry/deletions/test_alert_rule.py index 14f4dfc5758f19..5089b8c5fd1ffe 100644 --- a/tests/sentry/deletions/test_alert_rule.py +++ b/tests/sentry/deletions/test_alert_rule.py @@ -77,20 +77,14 @@ def test_simple(self) -> None: assert not AlertRuleWorkflow.objects.filter(alert_rule_id=alert_rule.id).exists() @with_feature("organizations:anomaly-detection-alerts") - @patch( - "sentry.seer.anomaly_detection.delete_rule.seer_anomaly_detection_connection_pool.urlopen" - ) @patch( "sentry.seer.anomaly_detection.store_data.seer_anomaly_detection_connection_pool.urlopen" ) - def test_dynamic_alert_rule( - self, mock_store_request: MagicMock, mock_delete_request: MagicMock - ) -> None: + def test_dynamic_alert_rule(self, mock_store_request: MagicMock) -> None: organization = self.create_organization() seer_return_value = {"success": True} mock_store_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) - mock_delete_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) alert_rule = self.create_alert_rule( sensitivity=AlertRuleSensitivity.HIGH, @@ -120,5 +114,3 @@ def test_dynamic_alert_rule( assert not AlertRule.objects.filter(id=alert_rule.id).exists() assert not AlertRuleTrigger.objects.filter(id=alert_rule_trigger.id).exists() assert not Incident.objects.filter(id=incident.id).exists() - - assert mock_delete_request.call_count == 1 diff --git a/tests/sentry/incidents/test_logic.py b/tests/sentry/incidents/test_logic.py index 5c13a6553b2436..e2ade373fc06ce 100644 --- a/tests/sentry/incidents/test_logic.py +++ b/tests/sentry/incidents/test_logic.py @@ -2008,6 +2008,7 @@ def test_with_incident_anomaly_detection_rule_error( alert_rule_id = alert_rule.id incident = self.create_incident() incident.update(alert_rule=alert_rule) + query_sub = QuerySubscription.objects.get(snuba_query_id=alert_rule.snuba_query.id) mock_seer_request.return_value = HTTPResponse("Bad request", status=500) with self.tasks(): @@ -2020,11 +2021,11 @@ def test_with_incident_anomaly_detection_rule_error( mock_seer_logger.error.assert_called_with( "Error when hitting Seer delete rule data endpoint", - extra={"response_data": "Bad request", "rule_id": alert_rule_id}, + extra={"response_data": "Bad request", "source_id": query_sub.id}, ) mock_model_logger.error.assert_called_with( "Call to delete rule data in Seer failed", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id}, ) assert mock_seer_request.call_count == 1 @@ -2045,13 +2046,13 @@ def test_delete_anomaly_detection_rule(self, mock_seer_request: MagicMock) -> No alert_rule = self.dynamic_alert_rule alert_rule_id = alert_rule.id + seer_return_value: StoreDataResponse = {"success": True} + mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + with self.tasks(): delete_alert_rule(alert_rule) - assert not AlertRule.objects.filter(id=alert_rule_id).exists() assert AlertRule.objects_with_snapshots.filter(id=alert_rule_id).exists() - seer_return_value: StoreDataResponse = {"success": True} - mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) with self.tasks(): run_scheduled_deletions() @@ -2066,19 +2067,18 @@ def test_delete_anomaly_detection_rule(self, mock_seer_request: MagicMock) -> No "sentry.seer.anomaly_detection.delete_rule.seer_anomaly_detection_connection_pool.urlopen" ) @patch("sentry.seer.anomaly_detection.delete_rule.logger") - @patch("sentry.incidents.models.alert_rule.logger") + @patch("sentry.incidents.logic.logger") def test_delete_anomaly_detection_rule_timeout( self, mock_model_logger, mock_seer_logger, mock_seer_request ): alert_rule = self.dynamic_alert_rule alert_rule_id = alert_rule.id - - with self.tasks(): - delete_alert_rule(alert_rule) + query_sub = QuerySubscription.objects.get(snuba_query_id=alert_rule.snuba_query.id) mock_seer_request.side_effect = TimeoutError with self.tasks(): + delete_alert_rule(alert_rule) run_scheduled_deletions() assert not AlertRule.objects.filter(id=alert_rule_id).exists() @@ -2086,11 +2086,11 @@ def test_delete_anomaly_detection_rule_timeout( mock_seer_logger.warning.assert_called_with( "Timeout error when hitting Seer delete rule data endpoint", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id}, ) mock_model_logger.error.assert_called_with( "Call to delete rule data in Seer failed", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id}, ) assert mock_seer_request.call_count == 1 @@ -2099,19 +2099,17 @@ def test_delete_anomaly_detection_rule_timeout( "sentry.seer.anomaly_detection.delete_rule.seer_anomaly_detection_connection_pool.urlopen" ) @patch("sentry.seer.anomaly_detection.delete_rule.logger") - @patch("sentry.incidents.models.alert_rule.logger") + @patch("sentry.incidents.logic.logger") def test_delete_anomaly_detection_rule_error( self, mock_model_logger, mock_seer_logger, mock_seer_request ): alert_rule = self.dynamic_alert_rule alert_rule_id = alert_rule.id - - with self.tasks(): - delete_alert_rule(alert_rule) - + query_sub = QuerySubscription.objects.get(snuba_query_id=alert_rule.snuba_query.id) mock_seer_request.return_value = HTTPResponse("Bad request", status=500) with self.tasks(): + delete_alert_rule(alert_rule) run_scheduled_deletions() assert not AlertRule.objects.filter(id=alert_rule_id).exists() @@ -2119,11 +2117,11 @@ def test_delete_anomaly_detection_rule_error( mock_seer_logger.error.assert_called_with( "Error when hitting Seer delete rule data endpoint", - extra={"response_data": "Bad request", "rule_id": alert_rule_id}, + extra={"response_data": "Bad request", "source_id": query_sub.id}, ) mock_model_logger.error.assert_called_with( "Call to delete rule data in Seer failed", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id}, ) assert mock_seer_request.call_count == 1 @@ -2132,19 +2130,17 @@ def test_delete_anomaly_detection_rule_error( "sentry.seer.anomaly_detection.delete_rule.seer_anomaly_detection_connection_pool.urlopen" ) @patch("sentry.seer.anomaly_detection.delete_rule.logger") - @patch("sentry.incidents.models.alert_rule.logger") + @patch("sentry.incidents.logic.logger") def test_delete_anomaly_detection_rule_attribute_error( self, mock_model_logger, mock_seer_logger, mock_seer_request ): alert_rule = self.dynamic_alert_rule alert_rule_id = alert_rule.id - - with self.tasks(): - delete_alert_rule(alert_rule) - + query_sub = QuerySubscription.objects.get(snuba_query_id=alert_rule.snuba_query.id) mock_seer_request.return_value = HTTPResponse(None, status=200) # type:ignore[arg-type] with self.tasks(): + delete_alert_rule(alert_rule) run_scheduled_deletions() assert not AlertRule.objects.filter(id=alert_rule_id).exists() @@ -2152,11 +2148,11 @@ def test_delete_anomaly_detection_rule_attribute_error( mock_seer_logger.exception.assert_called_with( "Failed to parse Seer delete rule data response", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id}, ) mock_model_logger.error.assert_called_with( "Call to delete rule data in Seer failed", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id}, ) assert mock_seer_request.call_count == 1 @@ -2165,20 +2161,18 @@ def test_delete_anomaly_detection_rule_attribute_error( "sentry.seer.anomaly_detection.delete_rule.seer_anomaly_detection_connection_pool.urlopen" ) @patch("sentry.seer.anomaly_detection.delete_rule.logger") - @patch("sentry.incidents.models.alert_rule.logger") + @patch("sentry.incidents.logic.logger") def test_delete_anomaly_detection_rule_failure( self, mock_model_logger, mock_seer_logger, mock_seer_request ): alert_rule = self.dynamic_alert_rule alert_rule_id = alert_rule.id - - with self.tasks(): - delete_alert_rule(alert_rule) - + query_sub = QuerySubscription.objects.get(snuba_query_id=alert_rule.snuba_query.id) seer_return_value: StoreDataResponse = {"success": False} mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) with self.tasks(): + delete_alert_rule(alert_rule) run_scheduled_deletions() assert not AlertRule.objects.filter(id=alert_rule_id).exists() @@ -2186,11 +2180,11 @@ def test_delete_anomaly_detection_rule_failure( mock_seer_logger.error.assert_called_with( "Request to delete alert rule from Seer was unsuccessful", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id, "message": None}, ) mock_model_logger.error.assert_called_with( "Call to delete rule data in Seer failed", - extra={"rule_id": alert_rule_id}, + extra={"source_id": query_sub.id}, ) assert mock_seer_request.call_count == 1 diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py index fc9d6c740f5364..245a4034a6ea1c 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py @@ -10,6 +10,7 @@ from sentry.deletions.models.scheduleddeletion import RegionScheduledDeletion from sentry.grouping.grouptype import ErrorGroupType from sentry.incidents.grouptype import MetricIssue +from sentry.incidents.models.alert_rule import AlertRuleDetectionType from sentry.incidents.utils.constants import INCIDENTS_SNUBA_SUBSCRIPTION_TYPE from sentry.models.auditlogentry import AuditLogEntry from sentry.silo.base import SiloMode @@ -18,6 +19,7 @@ from sentry.snuba.subscriptions import create_snuba_query, create_snuba_subscription from sentry.testutils.asserts import assert_status_code from sentry.testutils.cases import APITestCase +from sentry.testutils.helpers import with_feature from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import assume_test_silo_mode, region_silo_test from sentry.testutils.skips import requires_kafka, requires_snuba @@ -757,3 +759,34 @@ def test_error_group_type(self) -> None: event=audit_log.get_event_id("DETECTOR_REMOVE"), actor=self.user, ).exists() + + @with_feature("organizations:anomaly-detection-alerts") + @mock.patch("sentry.seer.anomaly_detection.delete_rule.delete_rule_in_seer") + @mock.patch( + "sentry.workflow_engine.endpoints.organization_detector_details.schedule_update_project_config" + ) + def test_anomaly_detection( + self, mock_schedule_update_project_config: mock.MagicMock, mock_seer_request: mock.MagicMock + ) -> None: + self.detector.config = {"detection_type": AlertRuleDetectionType.DYNAMIC} + self.detector.save() + + mock_seer_request.return_value = True + + with outbox_runner(): + self.get_success_response(self.organization.slug, self.detector.id) + + assert RegionScheduledDeletion.objects.filter( + model_name="Detector", object_id=self.detector.id + ).exists() + with assume_test_silo_mode(SiloMode.CONTROL): + assert AuditLogEntry.objects.filter( + target_object=self.detector.id, + event=audit_log.get_event_id("DETECTOR_REMOVE"), + actor=self.user, + ).exists() + self.detector.refresh_from_db() + assert self.detector.status == ObjectStatus.PENDING_DELETION + mock_seer_request.assert_called_once_with( + source_id=int(self.data_source.source_id), organization=self.organization + )