From 09787ed91753efb49274f5936b037de2a016c3c6 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 11:21:24 -0800 Subject: [PATCH 01/13] handle updating dynamic detector --- src/sentry/incidents/metric_issue_detector.py | 29 ++++++++- .../store_data_workflow_engine.py | 63 ++++++++++++++++++- 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index 1ee200557e86e0..bdeecd58bd9de6 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -9,7 +9,10 @@ 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.seer.anomaly_detection.store_data_workflow_engine import ( + send_new_detector_data, + update_detector_data, +) from sentry.snuba.dataset import Dataset from sentry.snuba.metrics.extraction import should_use_on_demand_metrics from sentry.snuba.models import ( @@ -308,6 +311,27 @@ def update(self, instance: Detector, validated_data: dict[str, Any]): if data_source is not None: self.update_data_source(instance, data_source) + # Handle anomaly detection changes + detection_type = instance.config.get("detection_type") + if ( + ( + not detection_type == AlertRuleDetectionType.DYNAMIC + and validated_data.get("config", {}).get("detection_type") + == AlertRuleDetectionType.DYNAMIC + ) + or ( + detection_type == AlertRuleDetectionType.DYNAMIC + and data_source.query != validated_data.get("dataSources", {}).get("query") + # and (data_source.query is not None or data_source.aggregate is not None) + ) + or ( + detection_type == AlertRuleDetectionType.DYNAMIC + and data_source.aggregate != validated_data.get("dataSources", {}).get("aggregate") + ) + ): + # detector has been changed to become a dynamic detector OR the snubaquery has changed on a dynamic detector + update_detector_data(instance, data_source) + instance.save() schedule_update_project_config(instance) @@ -325,10 +349,9 @@ def create(self, validated_data: dict[str, Any]): try: send_new_detector_data(detector) except Exception: - # Sending historical data failed; Detector won't be save, but we + # Sending historical data failed; Detector won't be saved, but we # need to clean up database state that has already been created. detector.workflow_condition_group.delete() - raise schedule_update_project_config(detector) diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index 9ef83411619d61..a6e6e5248f68af 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -30,7 +30,7 @@ from sentry.utils import json, metrics from sentry.utils.json import JSONDecodeError from sentry.workflow_engine.models import DataCondition, DataSource, DataSourceDetector, Detector -from sentry.workflow_engine.types import DetectorException +from sentry.workflow_engine.types import DetectorException, SnubaQueryDataSourceType logger = logging.getLogger(__name__) @@ -40,6 +40,67 @@ ) +def update_detector_data(detector: Detector, data_source: SnubaQueryDataSourceType) -> None: + try: + data_source = DataSourceDetector.objects.get(detector_id=detector.id).data_source + except DataSourceDetector.DoesNotExist: + raise Exception("Could not update detector, data source detector not found.") + try: + query_subscription = QuerySubscription.objects.get(id=data_source.source_id) + except QuerySubscription.DoesNotExist: + raise Exception("Could not update detector, query subscription not found.") + try: + snuba_query = SnubaQuery.objects.get(id=query_subscription.snuba_query_id) + except SnubaQuery.DoesNotExist: + raise Exception("Could not update detector, snuba query not found.") + try: + data_condition = DataCondition.objects.get( + condition_group=detector.workflow_condition_group + ) + except (DataCondition.DoesNotExist, DataCondition.MultipleObjectsReturned): + # there should only ever be one data condition for a dynamic metric detector, we dont actually expect a MultipleObjectsReturned + dcg_id = ( + detector.workflow_condition_group.id + if detector.workflow_condition_group is not None + else None + ) + raise DetectorException( + f"Could not create detector, data condition {dcg_id} not found or too many found." + ) + # use setattr to avoid saving the detector until the Seer call has successfully finished, + # otherwise the detector would be in a bad state + for k, v in data_source.items(): + setattr(data_source, k, v) + + for k, v in data_source.items(): + if k == "dataset": + v = v.value + elif k == "time_window": + time_window = data_source.get("time_window") + v = ( + int(time_window.total_seconds()) + if time_window is not None + else snuba_query.time_window + ) + elif k == "event_types": + continue + setattr(snuba_query, k, v) + + try: + handle_send_historical_data_to_seer( + detector, + data_source, + data_condition, + snuba_query, + detector.project, + SeerMethod.UPDATE, + data_source.event_types, + ) + except (TimeoutError, MaxRetryError, ParseError, ValidationError): + raise ValidationError("Couldn't send data to Seer, unable to update detector") + metrics.incr("anomaly_detection_monitor.updated") + + def send_new_detector_data(detector: Detector) -> None: """ Send historical data for a new Detector to Seer. From 4cb19c9092ede2b6caf60298f5c1b0ed6a44af04 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 15:47:40 -0800 Subject: [PATCH 02/13] Add tests, change update logic --- src/sentry/incidents/metric_issue_detector.py | 38 ++- .../store_data_workflow_engine.py | 37 ++- .../endpoints/validators/test_validators.py | 275 +++++++++++++++++- 3 files changed, 304 insertions(+), 46 deletions(-) diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index bdeecd58bd9de6..2924de4a7116e4 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -273,6 +273,12 @@ def update_data_source(self, instance: Detector, data_source: SnubaQueryDataSour raise serializers.ValidationError( "Invalid extrapolation mode for this detector type." ) + # Handle anomaly detection + if instance.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC: + if snuba_query.query != data_source.get( + "query" + ) or snuba_query.aggregate != data_source.get("aggregate"): + update_detector_data(instance, data_source) update_snuba_query( snuba_query=snuba_query, @@ -303,34 +309,24 @@ def update(self, instance: Detector, validated_data: dict[str, Any]): if query_subscriptions: enable_disable_subscriptions(query_subscriptions, enabled) - data_source: SnubaQueryDataSourceType | None = None + # Handle data sources + updated_data_source_data: SnubaQueryDataSourceType | None = None if "data_sources" in validated_data: - data_source = validated_data.pop("data_sources")[0] + updated_data_source_data = validated_data.pop("data_sources")[0] - if data_source is not None: - self.update_data_source(instance, data_source) + if updated_data_source_data is not None: + self.update_data_source(instance, updated_data_source_data) # Handle anomaly detection changes - detection_type = instance.config.get("detection_type") if ( - ( - not detection_type == AlertRuleDetectionType.DYNAMIC - and validated_data.get("config", {}).get("detection_type") - == AlertRuleDetectionType.DYNAMIC - ) - or ( - detection_type == AlertRuleDetectionType.DYNAMIC - and data_source.query != validated_data.get("dataSources", {}).get("query") - # and (data_source.query is not None or data_source.aggregate is not None) - ) - or ( - detection_type == AlertRuleDetectionType.DYNAMIC - and data_source.aggregate != validated_data.get("dataSources", {}).get("aggregate") - ) + not instance.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC + and validated_data.get("config", {}).get("detection_type") + == AlertRuleDetectionType.DYNAMIC ): - # detector has been changed to become a dynamic detector OR the snubaquery has changed on a dynamic detector - update_detector_data(instance, data_source) + # Detector has been changed to become a dynamic detector + send_new_detector_data(instance) + # update_detector_data(instance, updated_data_source_data) instance.save() diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index a6e6e5248f68af..a81886f77e769f 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -40,7 +40,9 @@ ) -def update_detector_data(detector: Detector, data_source: SnubaQueryDataSourceType) -> None: +def update_detector_data( + detector: Detector, updated_data_source_data: SnubaQueryDataSourceType +) -> None: try: data_source = DataSourceDetector.objects.get(detector_id=detector.id).data_source except DataSourceDetector.DoesNotExist: @@ -67,24 +69,21 @@ def update_detector_data(detector: Detector, data_source: SnubaQueryDataSourceTy raise DetectorException( f"Could not create detector, data condition {dcg_id} not found or too many found." ) - # use setattr to avoid saving the detector until the Seer call has successfully finished, - # otherwise the detector would be in a bad state - for k, v in data_source.items(): - setattr(data_source, k, v) + # use setattr to avoid saving the snuba query until the Seer call has successfully finished, + # otherwise it would be in a bad state + event_types = snuba_query.event_types + if updated_data_source_data: + event_types = updated_data_source_data.get("eventTypes") - for k, v in data_source.items(): - if k == "dataset": - v = v.value - elif k == "time_window": - time_window = data_source.get("time_window") - v = ( - int(time_window.total_seconds()) - if time_window is not None - else snuba_query.time_window - ) - elif k == "event_types": - continue - setattr(snuba_query, k, v) + for k, v in updated_data_source_data.items(): + if k == "dataset": + v = v.value + elif k == "time_window": + time_window = updated_data_source_data.get("time_window") + v = time_window if time_window is not None else snuba_query.time_window + elif k == "event_types": + continue + setattr(snuba_query, k, v) try: handle_send_historical_data_to_seer( @@ -94,7 +93,7 @@ def update_detector_data(detector: Detector, data_source: SnubaQueryDataSourceTy snuba_query, detector.project, SeerMethod.UPDATE, - data_source.event_types, + event_types, ) except (TimeoutError, MaxRetryError, ParseError, ValidationError): raise ValidationError("Couldn't send data to Seer, unable to update detector") diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index 82dd645e6b45cd..7a6c77e9d26295 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -2,6 +2,7 @@ import orjson import pytest +from django.utils import timezone from rest_framework.exceptions import ErrorDetail, ValidationError from urllib3.exceptions import MaxRetryError, TimeoutError from urllib3.response import HTTPResponse @@ -152,15 +153,15 @@ def setUp(self) -> None: } ], "conditionGroup": { - "id": self.data_condition_group.id, - "organizationId": self.organization.id, + # "id": self.data_condition_group.id, + # "organizationId": self.organization.id, "logicType": self.data_condition_group.logic_type, "conditions": [ { "type": Condition.GREATER, "comparison": 100, "conditionResult": DetectorPriorityLevel.HIGH, - "conditionGroupId": self.data_condition_group.id, + # "conditionGroupId": self.data_condition_group.id, }, { "type": Condition.LESS_OR_EQUAL, @@ -178,8 +179,8 @@ def setUp(self) -> None: self.valid_anomaly_detection_data = { **self.valid_data, "conditionGroup": { - "id": self.data_condition_group.id, - "organizationId": self.organization.id, + # "id": self.data_condition_group.id, + # "organizationId": self.organization.id, "logicType": self.data_condition_group.logic_type, "conditions": [ { @@ -190,7 +191,7 @@ def setUp(self) -> None: "threshold_type": AnomalyDetectionThresholdType.ABOVE_AND_BELOW, }, "conditionResult": DetectorPriorityLevel.HIGH, - "conditionGroupId": self.data_condition_group.id, + # "conditionGroupId": self.data_condition_group.id, }, ], }, @@ -228,6 +229,9 @@ def assert_validated(self, detector): assert snuba_query.environment == self.environment assert snuba_query.event_types == [SnubaQueryEventType.EventType.ERROR] + +class TestMetricAlertsCreateDetectorValidator(TestMetricAlertsDetectorValidator): + @mock.patch("sentry.incidents.metric_issue_detector.schedule_update_project_config") @mock.patch("sentry.workflow_engine.endpoints.validators.base.detector.create_audit_entry") def test_create_with_valid_data( @@ -591,6 +595,265 @@ def test_transaction_dataset_deprecation_multiple_data_sources(self) -> None: ): validator.save() + +class TestMetricAlertsUpdateDetectorValidator(TestMetricAlertsDetectorValidator): + def test_update_with_valid_data(self) -> None: + """ + Test a simple update + """ + validator = MetricIssueDetectorValidator(data=self.valid_data, context=self.context) + assert validator.is_valid(), validator.errors + detector = validator.save() + + # # the front end passes _all_ of the data, not just what changed + new_name = "Testing My Cool Detector" + update_data = { + **self.valid_data, + "id": detector.id, + "projectId": self.project.id, + "dateCreated": detector.date_added, + "dateUpdated": timezone.now(), + "conditionGroup": { + "logicType": self.data_condition_group.logic_type, + "conditions": [ + { + "type": Condition.GREATER, + "comparison": 100, + "conditionResult": DetectorPriorityLevel.HIGH, + }, + ], + }, + "name": new_name, # change the name + } + update_validator = MetricIssueDetectorValidator( + instance=detector, data=update_data, context=self.context, partial=True + ) + assert update_validator.is_valid(), update_validator.errors + updated_detector = update_validator.save() + assert updated_detector.name == new_name + + @mock.patch( + "sentry.seer.anomaly_detection.store_data_workflow_engine.seer_anomaly_detection_connection_pool.urlopen" + ) + @mock.patch("sentry.workflow_engine.endpoints.validators.base.detector.create_audit_entry") + def test_update_anomaly_detection_from_static( + self, mock_audit: mock.MagicMock, mock_seer_request: mock.MagicMock + ) -> None: + """ + Test that if a static detector is changed to become a dynamic one + we send the historical data to Seer for that detector + """ + validator = MetricIssueDetectorValidator( + data=self.valid_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + static_detector = validator.save() + + # Verify condition group in DB + condition_group = DataConditionGroup.objects.get( + id=static_detector.workflow_condition_group_id + ) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + # Verify conditions in DB + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 1 + condition = conditions[0] + assert condition.type == Condition.GREATER + assert condition.comparison == 100 + assert condition.condition_result == DetectorPriorityLevel.HIGH + + assert mock_seer_request.call_count == 1 + mock_seer_request.return_mock() + + mock_audit.assert_called() + mock_audit.reset_mock() + + # Change to become a dynamic detector + seer_return_value: StoreDataResponse = {"success": True} + mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + + update_validator = MetricIssueDetectorValidator( + instance=static_detector, + data=self.valid_anomaly_detection_data, + context=self.context, + partial=True, + ) + assert update_validator.is_valid(), update_validator.errors + dynamic_detector = update_validator.save() + + assert mock_seer_request.call_count == 1 + + # Verify detector in DB + self.assert_validated(dynamic_detector) + + # Verify condition group in DB + condition_group = DataConditionGroup.objects.get( + id=dynamic_detector.workflow_condition_group_id + ) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + # Verify conditions in DB + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 1 + + condition = conditions[0] + assert condition.type == Condition.ANOMALY_DETECTION + assert condition.comparison == { + "sensitivity": AnomalyDetectionSensitivity.HIGH, + "seasonality": AnomalyDetectionSeasonality.AUTO, + "threshold_type": AnomalyDetectionThresholdType.ABOVE_AND_BELOW, + } + assert condition.condition_result == DetectorPriorityLevel.HIGH + + mock_audit.assert_called_once_with( + request=self.context["request"], + organization=self.project.organization, + target_object=dynamic_detector.id, + event=audit_log.get_event_id("DETECTOR_EDIT"), + data=dynamic_detector.get_audit_log_data(), + ) + + @mock.patch( + "sentry.seer.anomaly_detection.store_data_workflow_engine.seer_anomaly_detection_connection_pool.urlopen" + ) + @mock.patch("sentry.workflow_engine.endpoints.validators.base.detector.create_audit_entry") + def test_update_anomaly_detection_snuba_query( + self, mock_audit: mock.MagicMock, mock_seer_request: mock.MagicMock + ) -> None: + """ + Test that when we update the snuba query for a dynamic detector + that we make a call to Seer with the changes + """ + seer_return_value: StoreDataResponse = {"success": True} + mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + + validator = MetricIssueDetectorValidator( + data=self.valid_anomaly_detection_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + detector = validator.save() + + # Verify detector in DB + self.assert_validated(detector) + + assert mock_seer_request.call_count == 1 + mock_seer_request.reset_mock() + + # Verify condition group in DB + condition_group = DataConditionGroup.objects.get(id=detector.workflow_condition_group_id) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + # Verify conditions in DB + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 1 + + condition = conditions[0] + assert condition.type == Condition.ANOMALY_DETECTION + assert condition.comparison == { + "sensitivity": AnomalyDetectionSensitivity.HIGH, + "seasonality": AnomalyDetectionSeasonality.AUTO, + "threshold_type": AnomalyDetectionThresholdType.ABOVE_AND_BELOW, + } + assert condition.condition_result == DetectorPriorityLevel.HIGH + + # Verify audit log + mock_audit.assert_called_once_with( + request=self.context["request"], + organization=self.project.organization, + target_object=detector.id, + event=audit_log.get_event_id("DETECTOR_ADD"), + data=detector.get_audit_log_data(), + ) + mock_audit.reset_mock() + + # Change the snuba query which should call Seer + updated_query = "different query" + update_data = { + **self.valid_anomaly_detection_data, + "dataSources": [ + { + "queryType": SnubaQuery.Type.ERROR.value, + "dataset": Dataset.Events.value, + "query": updated_query, # this is what's changing + "aggregate": "count()", + "timeWindow": 3600, + "environment": self.environment.name, + "eventTypes": [SnubaQueryEventType.EventType.ERROR.name.lower()], + } + ], + } + update_validator = MetricIssueDetectorValidator( + instance=detector, data=update_data, context=self.context, partial=True + ) + assert update_validator.is_valid(), update_validator.errors + dynamic_detector = update_validator.save() + + assert mock_seer_request.call_count == 1 + mock_seer_request.reset_mock() + + # Verify snuba query changes + data_source = DataSource.objects.get(detector=dynamic_detector) + query_subscription = QuerySubscription.objects.get(id=data_source.source_id) + snuba_query = SnubaQuery.objects.get(id=query_subscription.snuba_query_id) + assert snuba_query.query == updated_query + + mock_audit.assert_called_once_with( + request=self.context["request"], + organization=self.project.organization, + target_object=dynamic_detector.id, + event=audit_log.get_event_id("DETECTOR_EDIT"), + data=dynamic_detector.get_audit_log_data(), + ) + mock_audit.reset_mock() + + # Change the aggregate which should call Seer + updated_aggregate = "count_unique(user)" + update_data = { + **self.valid_anomaly_detection_data, + "dataSources": [ + { + "queryType": SnubaQuery.Type.ERROR.value, + "dataset": Dataset.Events.value, + "query": "updated_query", + "aggregate": updated_aggregate, # this is what's changing + "timeWindow": 3600, + "environment": self.environment.name, + "eventTypes": [SnubaQueryEventType.EventType.ERROR.name.lower()], + } + ], + } + update_validator = MetricIssueDetectorValidator( + instance=detector, data=update_data, context=self.context, partial=True + ) + assert update_validator.is_valid(), update_validator.errors + dynamic_detector = update_validator.save() + + assert mock_seer_request.call_count == 1 + + # Verify snuba query changes + data_source = DataSource.objects.get(detector=dynamic_detector) + query_subscription = QuerySubscription.objects.get(id=data_source.source_id) + snuba_query = SnubaQuery.objects.get(id=query_subscription.snuba_query_id) + assert snuba_query.aggregate == "count_unique(tags[sentry:user])" + + mock_audit.assert_called_once_with( + request=self.context["request"], + organization=self.project.organization, + target_object=dynamic_detector.id, + event=audit_log.get_event_id("DETECTOR_EDIT"), + data=dynamic_detector.get_audit_log_data(), + ) + @with_feature("organizations:discover-saved-queries-deprecation") def test_update_allowed_even_with_deprecated_dataset(self) -> None: # Updates should be allowed even when the feature flag is enabled From e57e1c7c87a2bab25facde567bd4e725d7845bb1 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 15:51:37 -0800 Subject: [PATCH 03/13] smol changes --- src/sentry/incidents/metric_issue_detector.py | 4 ++-- .../sentry/incidents/endpoints/validators/test_validators.py | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index 2924de4a7116e4..ef59285f286a56 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -273,7 +273,8 @@ def update_data_source(self, instance: Detector, data_source: SnubaQueryDataSour raise serializers.ValidationError( "Invalid extrapolation mode for this detector type." ) - # Handle anomaly detection + + # Handle a dynamic detector's snuba query changing if instance.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC: if snuba_query.query != data_source.get( "query" @@ -326,7 +327,6 @@ def update(self, instance: Detector, validated_data: dict[str, Any]): ): # Detector has been changed to become a dynamic detector send_new_detector_data(instance) - # update_detector_data(instance, updated_data_source_data) instance.save() diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index 7a6c77e9d26295..c89ea1f1807750 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -667,9 +667,6 @@ def test_update_anomaly_detection_from_static( assert condition.comparison == 100 assert condition.condition_result == DetectorPriorityLevel.HIGH - assert mock_seer_request.call_count == 1 - mock_seer_request.return_mock() - mock_audit.assert_called() mock_audit.reset_mock() From 5e8ce0f468089c21c15810635a164802d30cc891 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 17:14:35 -0800 Subject: [PATCH 04/13] update correctly --- src/sentry/incidents/metric_issue_detector.py | 32 +++-- .../store_data_workflow_engine.py | 23 +++- .../endpoints/validators/test_validators.py | 123 +++++++++++++++++- 3 files changed, 155 insertions(+), 23 deletions(-) diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index ef59285f286a56..9e7db40d00dcc4 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -279,7 +279,13 @@ def update_data_source(self, instance: Detector, data_source: SnubaQueryDataSour if snuba_query.query != data_source.get( "query" ) or snuba_query.aggregate != data_source.get("aggregate"): - update_detector_data(instance, data_source) + try: + update_detector_data(instance, data_source) + except Exception: + # don't update the snuba query if we failed to send data to Seer + raise serializers.ValidationError( + "Failed to send data to Seer, cannot update detector" + ) update_snuba_query( snuba_query=snuba_query, @@ -297,6 +303,21 @@ def update_data_source(self, instance: Detector, data_source: SnubaQueryDataSour ) def update(self, instance: Detector, validated_data: dict[str, Any]): + # Handle anomaly detection changes first in case we need to exit before saving + if ( + not instance.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC + and validated_data.get("config", {}).get("detection_type") + == AlertRuleDetectionType.DYNAMIC + ): + # Detector has been changed to become a dynamic detector + try: + update_detector_data(instance, validated_data) + except Exception: + # Don't update if we failed to send data to Seer + raise serializers.ValidationError( + "Failed to send data to Seer, cannot update detector" + ) + super().update(instance, validated_data) # Handle enable/disable query subscriptions @@ -319,15 +340,6 @@ def update(self, instance: Detector, validated_data: dict[str, Any]): if updated_data_source_data is not None: self.update_data_source(instance, updated_data_source_data) - # Handle anomaly detection changes - if ( - not instance.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC - and validated_data.get("config", {}).get("detection_type") - == AlertRuleDetectionType.DYNAMIC - ): - # Detector has been changed to become a dynamic detector - send_new_detector_data(instance) - instance.save() schedule_update_project_config(instance) diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index a81886f77e769f..5dd7d68d36a056 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -1,4 +1,5 @@ import logging +from typing import Any import sentry_sdk from django.conf import settings @@ -30,7 +31,7 @@ from sentry.utils import json, metrics from sentry.utils.json import JSONDecodeError from sentry.workflow_engine.models import DataCondition, DataSource, DataSourceDetector, Detector -from sentry.workflow_engine.types import DetectorException, SnubaQueryDataSourceType +from sentry.workflow_engine.types import DetectorException logger = logging.getLogger(__name__) @@ -41,7 +42,8 @@ def update_detector_data( - detector: Detector, updated_data_source_data: SnubaQueryDataSourceType + detector: Detector, + updated_fields: dict[str, Any], ) -> None: try: data_source = DataSourceDetector.objects.get(detector_id=detector.id).data_source @@ -69,17 +71,24 @@ def update_detector_data( raise DetectorException( f"Could not create detector, data condition {dcg_id} not found or too many found." ) - # use setattr to avoid saving the snuba query until the Seer call has successfully finished, - # otherwise it would be in a bad state + # use setattr to avoid saving the models until the Seer call has successfully finished, + # otherwise they would be in a bad state + updated_data_condition_data = updated_fields.get("condition_group", {}).get("conditions") + if updated_data_condition_data: + for k, v in updated_data_condition_data[0].items(): + setattr(data_condition, k, v) + event_types = snuba_query.event_types + updated_data_source_data = updated_fields.get("data_sources") if updated_data_source_data: - event_types = updated_data_source_data.get("eventTypes") + data_source_data = updated_data_source_data[0] + event_types = data_source_data.get("eventTypes") - for k, v in updated_data_source_data.items(): + for k, v in data_source_data.items(): if k == "dataset": v = v.value elif k == "time_window": - time_window = updated_data_source_data.get("time_window") + time_window = data_source_data.get("time_window") v = time_window if time_window is not None else snuba_query.time_window elif k == "event_types": continue diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index c89ea1f1807750..37d42a94493de1 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -153,15 +153,12 @@ def setUp(self) -> None: } ], "conditionGroup": { - # "id": self.data_condition_group.id, - # "organizationId": self.organization.id, "logicType": self.data_condition_group.logic_type, "conditions": [ { "type": Condition.GREATER, "comparison": 100, "conditionResult": DetectorPriorityLevel.HIGH, - # "conditionGroupId": self.data_condition_group.id, }, { "type": Condition.LESS_OR_EQUAL, @@ -179,8 +176,6 @@ def setUp(self) -> None: self.valid_anomaly_detection_data = { **self.valid_data, "conditionGroup": { - # "id": self.data_condition_group.id, - # "organizationId": self.organization.id, "logicType": self.data_condition_group.logic_type, "conditions": [ { @@ -191,7 +186,6 @@ def setUp(self) -> None: "threshold_type": AnomalyDetectionThresholdType.ABOVE_AND_BELOW, }, "conditionResult": DetectorPriorityLevel.HIGH, - # "conditionGroupId": self.data_condition_group.id, }, ], }, @@ -851,6 +845,123 @@ def test_update_anomaly_detection_snuba_query( data=dynamic_detector.get_audit_log_data(), ) + @mock.patch( + "sentry.seer.anomaly_detection.store_data_workflow_engine.seer_anomaly_detection_connection_pool.urlopen" + ) + def test_anomaly_detection__send_historical_data_update_fails( + self, mock_seer_request: mock.MagicMock + ) -> None: + """ + Test that if the call to Seer fails when we try to change a detector's type to dynamic from static that we do not update the detector or data condition + """ + + # Create static detector + validator = MetricIssueDetectorValidator( + data=self.valid_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + static_detector = validator.save() + + # Verify condition group in DB + condition_group = DataConditionGroup.objects.get( + id=static_detector.workflow_condition_group_id + ) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + # Verify conditions in DB + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 1 + condition = conditions[0] + assert condition.type == Condition.GREATER + assert condition.comparison == 100 + assert condition.condition_result == DetectorPriorityLevel.HIGH + + # Attempt to convert detector to dynamic type + mock_seer_request.side_effect = TimeoutError + + update_validator = MetricIssueDetectorValidator( + instance=static_detector, + data=self.valid_anomaly_detection_data, + context=self.context, + partial=True, + ) + assert update_validator.is_valid(), update_validator.errors + + with self.tasks(), pytest.raises(ValidationError): + update_validator.save() + + # Re-fetch the models and ensure they're not updated + detector = Detector.objects.get(id=static_detector.id) + assert detector.config.get("detection_type") == AlertRuleDetectionType.STATIC.value + + condition_group = DataConditionGroup.objects.get(id=detector.workflow_condition_group_id) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 1 + condition = conditions[0] + assert condition.type == Condition.GREATER + assert condition.comparison == 100 + assert condition.condition_result == DetectorPriorityLevel.HIGH + + @mock.patch( + "sentry.seer.anomaly_detection.store_data_workflow_engine.seer_anomaly_detection_connection_pool.urlopen" + ) + def test_anomaly_detection__send_historical_data_snuba_update_fails( + self, mock_seer_request: mock.MagicMock + ) -> None: + """ + Test that if the call to Seer fails when we try to change a dynamic detector's snuba query that we do not update the snuba query + """ + seer_return_value: StoreDataResponse = {"success": True} + mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + + validator = MetricIssueDetectorValidator( + data=self.valid_anomaly_detection_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + detector = validator.save() + + # Attempt to change the snuba query's query + mock_seer_request.side_effect = TimeoutError + + updated_query = "different query" + update_data = { + **self.valid_anomaly_detection_data, + "dataSources": [ + { + "queryType": SnubaQuery.Type.ERROR.value, + "dataset": Dataset.Events.value, + "query": updated_query, # this is what's changing + "aggregate": "count()", + "timeWindow": 3600, + "environment": self.environment.name, + "eventTypes": [SnubaQueryEventType.EventType.ERROR.name.lower()], + } + ], + } + update_validator = MetricIssueDetectorValidator( + instance=detector, data=update_data, context=self.context, partial=True + ) + assert update_validator.is_valid(), update_validator.errors + + with self.tasks(), pytest.raises(ValidationError): + update_validator.save() + + # Fetch data and ensure it hasn't changed + data_source = DataSource.objects.get(detector=detector) + query_sub = QuerySubscription.objects.get(id=data_source.source_id) + snuba_query = query_sub.snuba_query + assert snuba_query.query == "test query" + @with_feature("organizations:discover-saved-queries-deprecation") def test_update_allowed_even_with_deprecated_dataset(self) -> None: # Updates should be allowed even when the feature flag is enabled From cce55bf614f1082ad8772aeeaace54af74b506a0 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 17:28:31 -0800 Subject: [PATCH 05/13] dry up tests --- src/sentry/incidents/metric_issue_detector.py | 8 +- .../endpoints/validators/test_validators.py | 113 +++++++----------- 2 files changed, 49 insertions(+), 72 deletions(-) diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index 9e7db40d00dcc4..19eed2e1c4a763 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -332,13 +332,13 @@ def update(self, instance: Detector, validated_data: dict[str, Any]): enable_disable_subscriptions(query_subscriptions, enabled) # Handle data sources - updated_data_source_data: SnubaQueryDataSourceType | None = None + data_source: SnubaQueryDataSourceType | None = None if "data_sources" in validated_data: - updated_data_source_data = validated_data.pop("data_sources")[0] + data_source = validated_data.pop("data_sources")[0] - if updated_data_source_data is not None: - self.update_data_source(instance, updated_data_source_data) + if data_source is not None: + self.update_data_source(instance, data_source) instance.save() diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index 37d42a94493de1..25d2ba0085e0c9 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -591,15 +591,52 @@ def test_transaction_dataset_deprecation_multiple_data_sources(self) -> None: class TestMetricAlertsUpdateDetectorValidator(TestMetricAlertsDetectorValidator): + def create_static_detector(self) -> None: + validator = MetricIssueDetectorValidator( + data=self.valid_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + static_detector = validator.save() + + # Verify condition group in DB + condition_group = DataConditionGroup.objects.get( + id=static_detector.workflow_condition_group_id + ) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + # Verify conditions in DB + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 1 + condition = conditions[0] + assert condition.type == Condition.GREATER + assert condition.comparison == 100 + assert condition.condition_result == DetectorPriorityLevel.HIGH + + return static_detector + + def create_dynamic_detector(self) -> None: + validator = MetricIssueDetectorValidator( + data=self.valid_anomaly_detection_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + detector = validator.save() + + return detector + def test_update_with_valid_data(self) -> None: """ Test a simple update """ - validator = MetricIssueDetectorValidator(data=self.valid_data, context=self.context) - assert validator.is_valid(), validator.errors - detector = validator.save() + detector = self.create_static_detector() - # # the front end passes _all_ of the data, not just what changed + # the front end passes _all_ of the data, not just what changed new_name = "Testing My Cool Detector" update_data = { **self.valid_data, @@ -637,29 +674,7 @@ def test_update_anomaly_detection_from_static( Test that if a static detector is changed to become a dynamic one we send the historical data to Seer for that detector """ - validator = MetricIssueDetectorValidator( - data=self.valid_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - static_detector = validator.save() - - # Verify condition group in DB - condition_group = DataConditionGroup.objects.get( - id=static_detector.workflow_condition_group_id - ) - assert condition_group.logic_type == DataConditionGroup.Type.ANY - assert condition_group.organization_id == self.project.organization_id - - # Verify conditions in DB - conditions = list(DataCondition.objects.filter(condition_group=condition_group)) - assert len(conditions) == 1 - condition = conditions[0] - assert condition.type == Condition.GREATER - assert condition.comparison == 100 - assert condition.condition_result == DetectorPriorityLevel.HIGH + static_detector = self.create_static_detector() mock_audit.assert_called() mock_audit.reset_mock() @@ -724,14 +739,7 @@ def test_update_anomaly_detection_snuba_query( seer_return_value: StoreDataResponse = {"success": True} mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) - validator = MetricIssueDetectorValidator( - data=self.valid_anomaly_detection_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - detector = validator.save() + detector = self.self.create_dynamic_detector() # Verify detector in DB self.assert_validated(detector) @@ -854,31 +862,7 @@ def test_anomaly_detection__send_historical_data_update_fails( """ Test that if the call to Seer fails when we try to change a detector's type to dynamic from static that we do not update the detector or data condition """ - - # Create static detector - validator = MetricIssueDetectorValidator( - data=self.valid_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - static_detector = validator.save() - - # Verify condition group in DB - condition_group = DataConditionGroup.objects.get( - id=static_detector.workflow_condition_group_id - ) - assert condition_group.logic_type == DataConditionGroup.Type.ANY - assert condition_group.organization_id == self.project.organization_id - - # Verify conditions in DB - conditions = list(DataCondition.objects.filter(condition_group=condition_group)) - assert len(conditions) == 1 - condition = conditions[0] - assert condition.type == Condition.GREATER - assert condition.comparison == 100 - assert condition.condition_result == DetectorPriorityLevel.HIGH + static_detector = self.create_static_detector() # Attempt to convert detector to dynamic type mock_seer_request.side_effect = TimeoutError @@ -921,14 +905,7 @@ def test_anomaly_detection__send_historical_data_snuba_update_fails( seer_return_value: StoreDataResponse = {"success": True} mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) - validator = MetricIssueDetectorValidator( - data=self.valid_anomaly_detection_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - detector = validator.save() + detector = self.create_dynamic_detector() # Attempt to change the snuba query's query mock_seer_request.side_effect = TimeoutError From f26545b496b7f026d22a7382d91f5809b2e6c171 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 17:35:45 -0800 Subject: [PATCH 06/13] dry a little more --- .../endpoints/validators/test_validators.py | 91 +++++++------------ 1 file changed, 33 insertions(+), 58 deletions(-) diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index 25d2ba0085e0c9..16a6c4ed12c1f3 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -195,6 +195,36 @@ def setUp(self) -> None: }, } + def create_dynamic_detector(self) -> None: + validator = MetricIssueDetectorValidator( + data=self.valid_anomaly_detection_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + detector = validator.save() + + # Verify condition group in DB + condition_group = DataConditionGroup.objects.get(id=detector.workflow_condition_group_id) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + # Verify conditions in DB + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 1 + + condition = conditions[0] + assert condition.type == Condition.ANOMALY_DETECTION + assert condition.comparison == { + "sensitivity": AnomalyDetectionSensitivity.HIGH, + "seasonality": AnomalyDetectionSeasonality.AUTO, + "threshold_type": AnomalyDetectionThresholdType.ABOVE_AND_BELOW, + } + assert condition.condition_result == DetectorPriorityLevel.HIGH + + return detector + def assert_validated(self, detector): detector = Detector.objects.get(id=detector.id) assert detector.name == "Test Detector" @@ -275,38 +305,13 @@ def test_anomaly_detection( seer_return_value: StoreDataResponse = {"success": True} mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) - validator = MetricIssueDetectorValidator( - data=self.valid_anomaly_detection_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - detector = validator.save() + detector = self.create_dynamic_detector() # Verify detector in DB self.assert_validated(detector) assert mock_seer_request.call_count == 1 - # Verify condition group in DB - condition_group = DataConditionGroup.objects.get(id=detector.workflow_condition_group_id) - assert condition_group.logic_type == DataConditionGroup.Type.ANY - assert condition_group.organization_id == self.project.organization_id - - # Verify conditions in DB - conditions = list(DataCondition.objects.filter(condition_group=condition_group)) - assert len(conditions) == 1 - - condition = conditions[0] - assert condition.type == Condition.ANOMALY_DETECTION - assert condition.comparison == { - "sensitivity": AnomalyDetectionSensitivity.HIGH, - "seasonality": AnomalyDetectionSeasonality.AUTO, - "threshold_type": AnomalyDetectionThresholdType.ABOVE_AND_BELOW, - } - assert condition.condition_result == DetectorPriorityLevel.HIGH - # Verify audit log mock_audit.assert_called_once_with( request=self.context["request"], @@ -618,18 +623,6 @@ def create_static_detector(self) -> None: return static_detector - def create_dynamic_detector(self) -> None: - validator = MetricIssueDetectorValidator( - data=self.valid_anomaly_detection_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - detector = validator.save() - - return detector - def test_update_with_valid_data(self) -> None: """ Test a simple update @@ -734,12 +727,12 @@ def test_update_anomaly_detection_snuba_query( ) -> None: """ Test that when we update the snuba query for a dynamic detector - that we make a call to Seer with the changes + we make a call to Seer with the changes """ seer_return_value: StoreDataResponse = {"success": True} mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) - detector = self.self.create_dynamic_detector() + detector = self.create_dynamic_detector() # Verify detector in DB self.assert_validated(detector) @@ -747,24 +740,6 @@ def test_update_anomaly_detection_snuba_query( assert mock_seer_request.call_count == 1 mock_seer_request.reset_mock() - # Verify condition group in DB - condition_group = DataConditionGroup.objects.get(id=detector.workflow_condition_group_id) - assert condition_group.logic_type == DataConditionGroup.Type.ANY - assert condition_group.organization_id == self.project.organization_id - - # Verify conditions in DB - conditions = list(DataCondition.objects.filter(condition_group=condition_group)) - assert len(conditions) == 1 - - condition = conditions[0] - assert condition.type == Condition.ANOMALY_DETECTION - assert condition.comparison == { - "sensitivity": AnomalyDetectionSensitivity.HIGH, - "seasonality": AnomalyDetectionSeasonality.AUTO, - "threshold_type": AnomalyDetectionThresholdType.ABOVE_AND_BELOW, - } - assert condition.condition_result == DetectorPriorityLevel.HIGH - # Verify audit log mock_audit.assert_called_once_with( request=self.context["request"], From 5b82132cc24901b70ee5bcfa06071eb63f1226bf Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 17:39:15 -0800 Subject: [PATCH 07/13] dry slightly more --- .../endpoints/validators/test_validators.py | 81 +++++++------------ 1 file changed, 31 insertions(+), 50 deletions(-) diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index 16a6c4ed12c1f3..96c3d2f5bc053b 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -195,6 +195,36 @@ def setUp(self) -> None: }, } + def create_static_detector(self) -> None: + validator = MetricIssueDetectorValidator( + data=self.valid_data, + context=self.context, + ) + assert validator.is_valid(), validator.errors + + with self.tasks(): + static_detector = validator.save() + + # Verify detector in DB + self.assert_validated(static_detector) + + # Verify condition group in DB + condition_group = DataConditionGroup.objects.get( + id=static_detector.workflow_condition_group_id + ) + assert condition_group.logic_type == DataConditionGroup.Type.ANY + assert condition_group.organization_id == self.project.organization_id + + # Verify conditions in DB + conditions = list(DataCondition.objects.filter(condition_group=condition_group)) + assert len(conditions) == 2 + condition = conditions[0] + assert condition.type == Condition.GREATER + assert condition.comparison == 100 + assert condition.condition_result == DetectorPriorityLevel.HIGH + + return static_detector + def create_dynamic_detector(self) -> None: validator = MetricIssueDetectorValidator( data=self.valid_anomaly_detection_data, @@ -261,29 +291,7 @@ class TestMetricAlertsCreateDetectorValidator(TestMetricAlertsDetectorValidator) def test_create_with_valid_data( self, mock_audit: mock.MagicMock, mock_schedule_update_project_config ) -> None: - validator = MetricIssueDetectorValidator( - data=self.valid_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - detector = validator.save() - - # Verify detector in DB - self.assert_validated(detector) - # Verify condition group in DB - condition_group = DataConditionGroup.objects.get(id=detector.workflow_condition_group_id) - assert condition_group.logic_type == DataConditionGroup.Type.ANY - assert condition_group.organization_id == self.project.organization_id - - # Verify conditions in DB - conditions = list(DataCondition.objects.filter(condition_group=condition_group)) - assert len(conditions) == 2 - condition = conditions[0] - assert condition.type == Condition.GREATER - assert condition.comparison == 100 - assert condition.condition_result == DetectorPriorityLevel.HIGH + detector = self.create_static_detector() # Verify audit log mock_audit.assert_called_once_with( @@ -596,33 +604,6 @@ def test_transaction_dataset_deprecation_multiple_data_sources(self) -> None: class TestMetricAlertsUpdateDetectorValidator(TestMetricAlertsDetectorValidator): - def create_static_detector(self) -> None: - validator = MetricIssueDetectorValidator( - data=self.valid_data, - context=self.context, - ) - assert validator.is_valid(), validator.errors - - with self.tasks(): - static_detector = validator.save() - - # Verify condition group in DB - condition_group = DataConditionGroup.objects.get( - id=static_detector.workflow_condition_group_id - ) - assert condition_group.logic_type == DataConditionGroup.Type.ANY - assert condition_group.organization_id == self.project.organization_id - - # Verify conditions in DB - conditions = list(DataCondition.objects.filter(condition_group=condition_group)) - assert len(conditions) == 1 - condition = conditions[0] - assert condition.type == Condition.GREATER - assert condition.comparison == 100 - assert condition.condition_result == DetectorPriorityLevel.HIGH - - return static_detector - def test_update_with_valid_data(self) -> None: """ Test a simple update From 79206a92c0af840f74310f94e245ae78962acec8 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 6 Nov 2025 17:47:14 -0800 Subject: [PATCH 08/13] dry up fetching related models --- .../store_data_workflow_engine.py | 70 +++++++------------ 1 file changed, 26 insertions(+), 44 deletions(-) diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index 5dd7d68d36a056..49c35ceafebf39 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -41,22 +41,27 @@ ) -def update_detector_data( - detector: Detector, - updated_fields: dict[str, Any], -) -> None: - try: - data_source = DataSourceDetector.objects.get(detector_id=detector.id).data_source - except DataSourceDetector.DoesNotExist: - raise Exception("Could not update detector, data source detector not found.") +def _fetch_related_models( + detector: Detector, method: str +) -> tuple[DataSource, DataCondition, SnubaQuery] | None: + # XXX: it is technically possible (though not used today) that a detector could have multiple data sources + data_source_detector = DataSourceDetector.objects.filter(detector_id=detector.id).first() + if not data_source_detector: + raise DetectorException(f"Could not {method} detector, data source not found.") + data_source = data_source_detector.data_source + try: - query_subscription = QuerySubscription.objects.get(id=data_source.source_id) + query_subscription = QuerySubscription.objects.get(id=int(data_source.source_id)) except QuerySubscription.DoesNotExist: - raise Exception("Could not update detector, query subscription not found.") + raise DetectorException( + f"Could not {method} detector, query subscription {data_source.source_id} not found." + ) try: snuba_query = SnubaQuery.objects.get(id=query_subscription.snuba_query_id) except SnubaQuery.DoesNotExist: - raise Exception("Could not update detector, snuba query not found.") + raise DetectorException( + f"Could not {method} detector, snuba query {query_subscription.snuba_query_id} not found." + ) try: data_condition = DataCondition.objects.get( condition_group=detector.workflow_condition_group @@ -69,8 +74,16 @@ def update_detector_data( else None ) raise DetectorException( - f"Could not create detector, data condition {dcg_id} not found or too many found." + f"Could not {method} detector, data condition {dcg_id} not found or too many found." ) + return data_source, data_condition, snuba_query + + +def update_detector_data( + detector: Detector, + updated_fields: dict[str, Any], +) -> None: + data_source, data_condition, snuba_query = _fetch_related_models(detector, "update") # use setattr to avoid saving the models until the Seer call has successfully finished, # otherwise they would be in a bad state updated_data_condition_data = updated_fields.get("condition_group", {}).get("conditions") @@ -113,38 +126,7 @@ def send_new_detector_data(detector: Detector) -> None: """ Send historical data for a new Detector to Seer. """ - # XXX: it is technically possible (though not used today) that a detector could have multiple data sources - data_source_detector = DataSourceDetector.objects.filter(detector_id=detector.id).first() - if not data_source_detector: - raise DetectorException("Could not create detector, data source not found.") - data_source = data_source_detector.data_source - - try: - query_subscription = QuerySubscription.objects.get(id=int(data_source.source_id)) - except QuerySubscription.DoesNotExist: - raise DetectorException( - f"Could not create detector, query subscription {data_source.source_id} not found." - ) - try: - snuba_query = SnubaQuery.objects.get(id=query_subscription.snuba_query_id) - except SnubaQuery.DoesNotExist: - raise DetectorException( - f"Could not create detector, snuba query {query_subscription.snuba_query_id} not found." - ) - try: - data_condition = DataCondition.objects.get( - condition_group=detector.workflow_condition_group - ) - except (DataCondition.DoesNotExist, DataCondition.MultipleObjectsReturned): - # there should only ever be one data condition for a dynamic metric detector, we dont actually expect a MultipleObjectsReturned - dcg_id = ( - detector.workflow_condition_group.id - if detector.workflow_condition_group is not None - else None - ) - raise DetectorException( - f"Could not create detector, data condition {dcg_id} not found or too many found." - ) + data_source, data_condition, snuba_query = _fetch_related_models(detector, "create") try: handle_send_historical_data_to_seer( detector, data_source, data_condition, snuba_query, detector.project, SeerMethod.CREATE From 8c804a82e2596e8853d6044132abb40d31e829d2 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Nov 2025 10:23:42 -0800 Subject: [PATCH 09/13] typing --- src/sentry/incidents/metric_issue_detector.py | 5 +++-- .../seer/anomaly_detection/store_data_workflow_engine.py | 4 +++- .../sentry/incidents/endpoints/validators/test_validators.py | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index 19eed2e1c4a763..2fd1b7082469ed 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -1,5 +1,5 @@ from datetime import timedelta -from typing import Any +from typing import Any, cast from rest_framework import serializers @@ -280,7 +280,8 @@ def update_data_source(self, instance: Detector, data_source: SnubaQueryDataSour "query" ) or snuba_query.aggregate != data_source.get("aggregate"): try: - update_detector_data(instance, data_source) + validated_data_source = cast(dict[str, Any], data_source) + update_detector_data(instance, validated_data_source) except Exception: # don't update the snuba query if we failed to send data to Seer raise serializers.ValidationError( diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index 49c35ceafebf39..66256f9937794c 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -43,7 +43,7 @@ def _fetch_related_models( detector: Detector, method: str -) -> tuple[DataSource, DataCondition, SnubaQuery] | None: +) -> tuple[DataSource, DataCondition, SnubaQuery]: # XXX: it is technically possible (though not used today) that a detector could have multiple data sources data_source_detector = DataSourceDetector.objects.filter(detector_id=detector.id).first() if not data_source_detector: @@ -84,6 +84,7 @@ def update_detector_data( updated_fields: dict[str, Any], ) -> None: data_source, data_condition, snuba_query = _fetch_related_models(detector, "update") + # use setattr to avoid saving the models until the Seer call has successfully finished, # otherwise they would be in a bad state updated_data_condition_data = updated_fields.get("condition_group", {}).get("conditions") @@ -127,6 +128,7 @@ def send_new_detector_data(detector: Detector) -> None: Send historical data for a new Detector to Seer. """ data_source, data_condition, snuba_query = _fetch_related_models(detector, "create") + try: handle_send_historical_data_to_seer( detector, data_source, data_condition, snuba_query, detector.project, SeerMethod.CREATE diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index 96c3d2f5bc053b..df82bcd97b9d68 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -195,7 +195,7 @@ def setUp(self) -> None: }, } - def create_static_detector(self) -> None: + def create_static_detector(self) -> Detector: validator = MetricIssueDetectorValidator( data=self.valid_data, context=self.context, @@ -225,7 +225,7 @@ def create_static_detector(self) -> None: return static_detector - def create_dynamic_detector(self) -> None: + def create_dynamic_detector(self) -> Detector: validator = MetricIssueDetectorValidator( data=self.valid_anomaly_detection_data, context=self.context, From 63f70214c4fb5d7dd791c1736fbbc5502d4340a3 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 7 Nov 2025 11:52:23 -0800 Subject: [PATCH 10/13] update for resolution conditions --- .../anomaly_detection/store_data_workflow_engine.py | 11 ++++++++--- .../incidents/endpoints/validators/test_validators.py | 8 +++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index 66256f9937794c..f930e0dc881281 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -31,7 +31,7 @@ from sentry.utils import json, metrics from sentry.utils.json import JSONDecodeError from sentry.workflow_engine.models import DataCondition, DataSource, DataSourceDetector, Detector -from sentry.workflow_engine.types import DetectorException +from sentry.workflow_engine.types import DetectorException, DetectorPriorityLevel logger = logging.getLogger(__name__) @@ -64,10 +64,15 @@ def _fetch_related_models( ) try: data_condition = DataCondition.objects.get( - condition_group=detector.workflow_condition_group + condition_group=detector.workflow_condition_group, + condition_result__in=[ + DetectorPriorityLevel.HIGH, + DetectorPriorityLevel.MEDIUM, + DetectorPriorityLevel.LOW, + ], ) except (DataCondition.DoesNotExist, DataCondition.MultipleObjectsReturned): - # there should only ever be one data condition for a dynamic metric detector, we dont actually expect a MultipleObjectsReturned + # there should only ever be one non-resolution data condition for a dynamic metric detector, we dont actually expect a MultipleObjectsReturned dcg_id = ( detector.workflow_condition_group.id if detector.workflow_condition_group is not None diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index df82bcd97b9d68..f323594d7ab929 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -626,6 +626,12 @@ def test_update_with_valid_data(self) -> None: "comparison": 100, "conditionResult": DetectorPriorityLevel.HIGH, }, + { + "type": Condition.LESS_OR_EQUAL, + "comparison": 100, + "conditionResult": DetectorPriorityLevel.OK, + "conditionGroupId": self.data_condition_group.id, + }, ], }, "name": new_name, # change the name @@ -843,7 +849,7 @@ def test_anomaly_detection__send_historical_data_update_fails( assert condition_group.organization_id == self.project.organization_id conditions = list(DataCondition.objects.filter(condition_group=condition_group)) - assert len(conditions) == 1 + assert len(conditions) == 2 condition = conditions[0] assert condition.type == Condition.GREATER assert condition.comparison == 100 From c9efa9748dccd758003565afd6fbd48e6562d397 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 11 Nov 2025 16:56:27 -0800 Subject: [PATCH 11/13] rm unused priority level --- src/sentry/seer/anomaly_detection/store_data_workflow_engine.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index f930e0dc881281..ad91c72bce21e9 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -68,7 +68,6 @@ def _fetch_related_models( condition_result__in=[ DetectorPriorityLevel.HIGH, DetectorPriorityLevel.MEDIUM, - DetectorPriorityLevel.LOW, ], ) except (DataCondition.DoesNotExist, DataCondition.MultipleObjectsReturned): From d4afe115cfc37eb8058d42834b1fd3decd3236ff Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 12 Nov 2025 15:21:11 -0800 Subject: [PATCH 12/13] break out exceptions --- .../anomaly_detection/store_data_workflow_engine.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py index ad91c72bce21e9..eb5d08d4c79462 100644 --- a/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py +++ b/src/sentry/seer/anomaly_detection/store_data_workflow_engine.py @@ -122,8 +122,14 @@ def update_detector_data( SeerMethod.UPDATE, event_types, ) - except (TimeoutError, MaxRetryError, ParseError, ValidationError): - raise ValidationError("Couldn't send data to Seer, unable to update detector") + except TimeoutError: + raise ValidationError("Timed out sending data to Seer, unable to update detector") + except MaxRetryError: + raise ValidationError("Hit max retries sending data to Seer, unable to update detector") + except ParseError: + raise ValidationError("Couldn't parse response from Seer, unable to update detector") + except ValidationError: + raise ValidationError("Hit validation error, unable to update detector") metrics.incr("anomaly_detection_monitor.updated") From 2d5dec1c03d2c351b75d0da206a83b2afb3272a1 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 12 Nov 2025 16:26:19 -0800 Subject: [PATCH 13/13] comparison delta is only used for percent detectors - updating test --- .../endpoints/test_organization_detector_details.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 f01f1c1d21d151..a66669ac26d980 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py @@ -719,7 +719,7 @@ def test_update_config_valid(self) -> None: self.detector.save() # Update with valid new config - updated_config = {"detection_type": "dynamic", "comparison_delta": 3600} + updated_config = {"detection_type": "percent", "comparison_delta": 3600} data = { "config": updated_config, } @@ -737,7 +737,7 @@ def test_update_config_valid(self) -> None: assert self.detector.config == updated_config # API returns camelCase assert response.data["config"] == { - "detectionType": "dynamic", + "detectionType": "percent", "comparisonDelta": 3600, }