From 9faa983d2ac860952ec5412e4545e4c3825da73e Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Thu, 9 Apr 2026 13:59:31 -0700 Subject: [PATCH 1/2] timedelta --- src/sentry/incidents/logic.py | 35 ++-- src/sentry/incidents/metric_issue_detector.py | 49 ++++- src/sentry/snuba/models.py | 3 + src/sentry/snuba/snuba_query_validator.py | 2 +- .../endpoints/validators/test_validators.py | 179 ++++++++++++++++++ tests/sentry/incidents/test_logic.py | 10 +- 6 files changed, 257 insertions(+), 21 deletions(-) diff --git a/src/sentry/incidents/logic.py b/src/sentry/incidents/logic.py index 6e0f99be42119c..d91bb06f463ec3 100644 --- a/src/sentry/incidents/logic.py +++ b/src/sentry/incidents/logic.py @@ -482,6 +482,8 @@ class AlertRuleNameAlreadyUsedError(Exception): # Default values for `SnubaQuery.resolution`, in minutes. DEFAULT_ALERT_RULE_RESOLUTION = 1 +# Comparison alerts query twice (current + comparison window), so we scale +# resolution down to compensate for the increased query load. DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER = 2 DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION = { 30: 2, @@ -505,13 +507,25 @@ class AlertRuleNameAlreadyUsedError(Exception): } -def get_alert_resolution(time_window: int, organization: Organization) -> int: +def get_alert_resolution(time_window: int, organization: Organization) -> timedelta: + """ + Return the Snuba subscription evaluation interval for a given alert time window. + + Larger time windows don't need fine-grained resolution, so we map them to + coarser buckets to reduce query load. See DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION. + + :param time_window: The alert's aggregation window, in minutes. + :param organization: The organization (reserved for future per-org overrides). + :return: The evaluation interval as a timedelta. + """ index = bisect.bisect_right(SORTED_TIMEWINDOWS, time_window) if index == 0: - return DEFAULT_ALERT_RULE_RESOLUTION + minutes = DEFAULT_ALERT_RULE_RESOLUTION + else: + minutes = DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION[SORTED_TIMEWINDOWS[index - 1]] - return DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION[SORTED_TIMEWINDOWS[index - 1]] + return timedelta(minutes=minutes) class _OwnerKwargs(TypedDict): @@ -585,7 +599,7 @@ def create_alert_rule( raise ResourceDoesNotExist("Your organization does not have access to this feature.") if detection_type == AlertRuleDetectionType.DYNAMIC: - resolution = time_window + resolution = timedelta(minutes=time_window) # NOTE: we hardcode seasonality for EA seasonality = AlertRuleSeasonality.AUTO if not sensitivity: @@ -617,8 +631,7 @@ def create_alert_rule( raise ValidationError("Comparison delta is not a valid field for this alert type") if comparison_delta is not None: - # Since comparison alerts make twice as many queries, run the queries less frequently. - resolution = resolution * DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER + resolution *= DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER comparison_delta = int(timedelta(minutes=comparison_delta).total_seconds()) with transaction.atomic(router.db_for_write(SnubaQuery)): @@ -629,7 +642,7 @@ def create_alert_rule( query=query, aggregate=aggregate, time_window=timedelta(minutes=time_window), - resolution=timedelta(minutes=resolution), + resolution=resolution, environment=environment, event_types=event_types, extrapolation_mode=extrapolation_mode, @@ -900,11 +913,9 @@ def update_alert_rule( ) if resolution_comparison_delta is not None: - updated_query_fields["resolution"] = timedelta( - minutes=(resolution * DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER) - ) - else: - updated_query_fields["resolution"] = timedelta(minutes=resolution) + resolution *= DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER + + updated_query_fields["resolution"] = resolution if detection_type: updated_fields["detection_type"] = detection_type diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index 4fe62e4c741cfe..1263aae3c1c2d3 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -8,7 +8,11 @@ from sentry import features, quotas from sentry.constants import ObjectStatus -from sentry.incidents.logic import enable_disable_subscriptions +from sentry.incidents.logic import ( + DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER, + enable_disable_subscriptions, + get_alert_resolution, +) from sentry.incidents.models.alert_rule import AlertRuleDetectionType from sentry.relay.config.metric_extraction import on_demand_metrics_feature_flags from sentry.search.eap.trace_metrics.validator import validate_trace_metrics_aggregate @@ -245,6 +249,24 @@ def _validate_extrapolation_mode(self, extrapolation_mode: ExtrapolationMode) -> f"{extrapolation_mode.name.lower()} extrapolation mode is not supported for new detectors. Allowed modes are: client_and_server_weighted, unknown." ) + def _get_resolution_for_window( + self, time_window_seconds: int, detection_type: str | None, comparison_delta: int | None + ) -> timedelta: + """ + Compute the appropriate SnubaQuery resolution for a given time window, + mirroring the logic in create_alert_rule / update_alert_rule. + """ + organization = self.context["organization"] + + if detection_type == AlertRuleDetectionType.DYNAMIC: + resolution = timedelta(seconds=time_window_seconds) + else: + resolution = get_alert_resolution(time_window_seconds // 60, organization) + if comparison_delta is not None: + resolution *= DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER + + return resolution + def get_quota(self) -> DetectorQuota: organization = self.context.get("organization") request = self.context.get("request") @@ -342,14 +364,21 @@ def update_data_source( data_source.get("extrapolation_mode", snuba_query.extrapolation_mode) ) + new_time_window = data_source.get("time_window", snuba_query.time_window) + resolution = self._get_resolution_for_window( + new_time_window, + instance.config.get("detection_type"), + instance.config.get("comparison_delta"), + ) + update_snuba_query( snuba_query=snuba_query, query_type=data_source.get("query_type", snuba_query.type), dataset=data_source.get("dataset", snuba_query.dataset), query=data_source.get("query", snuba_query.query), aggregate=data_source.get("aggregate", snuba_query.aggregate), - time_window=timedelta(seconds=data_source.get("time_window", snuba_query.time_window)), - resolution=timedelta(seconds=data_source.get("resolution", snuba_query.resolution)), + time_window=timedelta(seconds=new_time_window), + resolution=resolution, environment=data_source.get("environment", snuba_query.environment), event_types=data_source.get("event_types", [event_type for event_type in event_types]), extrapolation_mode=extrapolation_mode, @@ -413,6 +442,10 @@ def update(self, instance: Detector, validated_data: dict[str, Any]) -> Detector if data_source is not None: self.update_data_source(instance, data_source, seer_updated) + elif "config" in validated_data: + # Config changed (e.g. detection_type or comparison_delta) without a + # data_source update — recalculate resolution to match the new config. + self.update_data_source(instance, {}, seer_updated) instance.save() @@ -421,10 +454,20 @@ def update(self, instance: Detector, validated_data: dict[str, Any]) -> Detector def create(self, validated_data: dict[str, Any]) -> Detector: if "data_sources" in validated_data: + config = validated_data.get("config", {}) + detection_type = config.get("detection_type") + comparison_delta = config.get("comparison_delta") + for validated_data_source in validated_data["data_sources"]: self._validate_transaction_dataset_deprecation(validated_data_source.get("dataset")) self._validate_extrapolation_mode(validated_data_source.get("extrapolation_mode")) + time_window = validated_data_source.get("time_window") + if time_window is not None: + validated_data_source["resolution"] = self._get_resolution_for_window( + time_window, detection_type, comparison_delta + ) + detector = super().create(validated_data) if detector.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC.value: diff --git a/src/sentry/snuba/models.py b/src/sentry/snuba/models.py index 1ba1f9df0e96ce..d0b2c4cbaa0ca9 100644 --- a/src/sentry/snuba/models.py +++ b/src/sentry/snuba/models.py @@ -72,7 +72,10 @@ class Type(Enum): size=100, ) aggregate = models.TextField() + # The aggregation window for the query, in seconds. time_window = models.IntegerField() + # How often the subscription query is evaluated, in seconds. Scaled up for + # larger time windows and comparison alerts to reduce query load. resolution = models.IntegerField() extrapolation_mode = models.IntegerField( choices=ExtrapolationMode.as_choices(), diff --git a/src/sentry/snuba/snuba_query_validator.py b/src/sentry/snuba/snuba_query_validator.py index 8c9a66f3e1cdb4..12f6bfe5e55596 100644 --- a/src/sentry/snuba/snuba_query_validator.py +++ b/src/sentry/snuba/snuba_query_validator.py @@ -483,7 +483,7 @@ def create_source(self, validated_data: dict[str, Any]) -> QuerySubscription: query=validated_data["query"], aggregate=validated_data["aggregate"], time_window=timedelta(seconds=validated_data["time_window"]), - resolution=timedelta(minutes=1), + resolution=validated_data.get("resolution", timedelta(minutes=1)), environment=validated_data["environment"], event_types=validated_data["event_types"], group_by=validated_data.get("group_by"), diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index efb663845dfb49..0cb37998c8bbd8 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -11,6 +11,10 @@ from sentry.conf.server import SEER_ANOMALY_DETECTION_STORE_DATA_URL from sentry.constants import ObjectStatus from sentry.incidents.grouptype import MetricIssue +from sentry.incidents.logic import ( + DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER, + get_alert_resolution, +) from sentry.incidents.metric_issue_detector import ( MetricIssueComparisonConditionValidator, MetricIssueDetectorValidator, @@ -255,6 +259,11 @@ def create_dynamic_detector(self) -> Detector: return detector + def get_snuba_query(self, detector: Detector) -> SnubaQuery: + data_source = DataSource.objects.get(detector=detector) + query_sub = QuerySubscription.objects.get(id=data_source.source_id) + return query_sub.snuba_query + def assert_validated(self, detector): detector = Detector.objects.get(id=detector.id) assert detector.name == "Test Detector" @@ -747,6 +756,176 @@ def test_create_detector_trace_metrics_valid_aggregates(self) -> None: ) +class TestMetricAlertsResolution(TestMetricAlertsDetectorValidator): + """Tests that resolution is computed correctly via get_alert_resolution.""" + + @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_static_small_window_uses_default_resolution( + self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock + ) -> None: + """A 10-minute static detector should get 1-minute resolution (default).""" + data = { + **self.valid_data, + "dataSources": [ + { + **self.valid_data["dataSources"][0], + "timeWindow": 600, # 10 minutes in seconds + } + ], + } + validator = MetricIssueDetectorValidator(data=data, context=self.context) + assert validator.is_valid(), validator.errors + with self.tasks(): + detector = validator.save() + + snuba_query = self.get_snuba_query(detector) + expected = get_alert_resolution(10, self.project.organization).total_seconds() + assert snuba_query.resolution == expected + + @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_static_large_window_uses_scaled_resolution( + self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock + ) -> None: + """A 60-minute static detector should get 3-minute resolution.""" + data = { + **self.valid_data, + "dataSources": [ + { + **self.valid_data["dataSources"][0], + "timeWindow": 3600, # 60 minutes in seconds + } + ], + } + validator = MetricIssueDetectorValidator(data=data, context=self.context) + assert validator.is_valid(), validator.errors + with self.tasks(): + detector = validator.save() + + snuba_query = self.get_snuba_query(detector) + expected = get_alert_resolution(60, self.project.organization).total_seconds() + assert snuba_query.resolution == expected + assert snuba_query.resolution == 180 # 3 minutes in seconds + + @mock.patch("sentry.seer.anomaly_detection.store_data_workflow_engine.make_store_data_request") + @mock.patch("sentry.workflow_engine.endpoints.validators.base.detector.create_audit_entry") + def test_create_dynamic_resolution_equals_time_window( + self, mock_audit: mock.MagicMock, mock_seer_request: mock.MagicMock + ) -> None: + """Dynamic detectors should use resolution == time_window.""" + seer_return_value: StoreDataResponse = {"success": True} + mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + + detector = self.create_dynamic_detector() + + snuba_query = self.get_snuba_query(detector) + assert snuba_query.resolution == snuba_query.time_window + + @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_percent_resolution_doubled( + self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock + ) -> None: + """Percent (comparison) detectors should double the resolution.""" + data = { + **self.valid_data, + "dataSources": [ + { + **self.valid_data["dataSources"][0], + "timeWindow": 3600, # 60 minutes + } + ], + "config": { + "thresholdPeriod": 1, + "detectionType": AlertRuleDetectionType.PERCENT.value, + "comparisonDelta": 86400, + }, + } + validator = MetricIssueDetectorValidator(data=data, context=self.context) + assert validator.is_valid(), validator.errors + with self.tasks(): + detector = validator.save() + + snuba_query = self.get_snuba_query(detector) + base_resolution = get_alert_resolution(60, self.project.organization) + expected = (base_resolution * DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER).total_seconds() + assert snuba_query.resolution == expected + assert snuba_query.resolution == 360 # 3 min * 2 = 6 minutes in seconds + + @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_update_time_window_recalculates_resolution( + self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock + ) -> None: + """Changing time_window on update should recalculate resolution.""" + detector = self.create_static_detector() + snuba_query = self.get_snuba_query(detector) + old_resolution = snuba_query.resolution + + # Update to a larger time window (24 hours = 1440 minutes) + update_data = { + **self.valid_data, + "dataSources": [ + { + **self.valid_data["dataSources"][0], + "timeWindow": 86400, # 1440 minutes in seconds + } + ], + } + update_validator = MetricIssueDetectorValidator( + instance=detector, data=update_data, context=self.context, partial=True + ) + assert update_validator.is_valid(), update_validator.errors + update_validator.save() + + snuba_query.refresh_from_db() + expected = get_alert_resolution(1440, self.project.organization).total_seconds() + assert snuba_query.resolution == expected + assert snuba_query.resolution == 900 # 15 minutes in seconds + assert snuba_query.resolution != old_resolution + + @mock.patch("sentry.seer.anomaly_detection.store_data_workflow_engine.make_store_data_request") + @mock.patch("sentry.seer.anomaly_detection.delete_rule.delete_rule_in_seer") + @mock.patch("sentry.workflow_engine.endpoints.validators.base.detector.create_audit_entry") + def test_update_detection_type_change_recalculates_resolution( + self, + mock_audit: mock.MagicMock, + mock_seer_delete: mock.MagicMock, + mock_seer_store: mock.MagicMock, + ) -> None: + """Changing detection_type without data_sources should still recalculate resolution.""" + seer_return_value: StoreDataResponse = {"success": True} + mock_seer_store.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + mock_seer_delete.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + + detector = self.create_dynamic_detector() + snuba_query = self.get_snuba_query(detector) + # Dynamic: resolution == time_window + assert snuba_query.resolution == snuba_query.time_window + + # Change to static without changing data sources + update_data = { + **self.valid_data, + "config": { + "thresholdPeriod": 1, + "detectionType": AlertRuleDetectionType.STATIC.value, + }, + } + update_validator = MetricIssueDetectorValidator( + instance=detector, data=update_data, context=self.context, partial=True + ) + assert update_validator.is_valid(), update_validator.errors + update_validator.save() + + snuba_query.refresh_from_db() + time_window_minutes = snuba_query.time_window // 60 + expected = get_alert_resolution( + time_window_minutes, self.project.organization + ).total_seconds() + assert snuba_query.resolution == expected + + class TestMetricAlertsUpdateDetectorValidator(TestMetricAlertsDetectorValidator): def test_update_with_valid_data(self) -> None: """ diff --git a/tests/sentry/incidents/test_logic.py b/tests/sentry/incidents/test_logic.py index 3002a31d60d49f..dd451663491cc3 100644 --- a/tests/sentry/incidents/test_logic.py +++ b/tests/sentry/incidents/test_logic.py @@ -3691,28 +3691,28 @@ class TestGetAlertResolution(TestCase): def test_simple(self) -> None: time_window = 30 result = get_alert_resolution(time_window, self.organization) - assert result == DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION[time_window] + assert result == timedelta(minutes=DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION[time_window]) def test_low_range(self) -> None: time_window = 2 result = get_alert_resolution(time_window, self.organization) - assert result == DEFAULT_ALERT_RULE_RESOLUTION + assert result == timedelta(minutes=DEFAULT_ALERT_RULE_RESOLUTION) def test_high_range(self) -> None: last_window = list(DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION.keys())[-1] time_window = last_window + 1000 result = get_alert_resolution(time_window, self.organization) - assert result == DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION[last_window] + assert result == timedelta(minutes=DEFAULT_ALERT_RULE_WINDOW_TO_RESOLUTION[last_window]) def test_mid_range(self) -> None: time_window = 125 result = get_alert_resolution(time_window, self.organization) # 125 is not part of the dict, will round down to the lower window of 120 - assert result == 3 + assert result == timedelta(minutes=3) def test_crazy_low_range(self) -> None: time_window = -5 result = get_alert_resolution(time_window, self.organization) - assert result == DEFAULT_ALERT_RULE_RESOLUTION + assert result == timedelta(minutes=DEFAULT_ALERT_RULE_RESOLUTION) From 518a1a6adcd22182d64d0612dbc20d7a5a592515 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 10 Apr 2026 10:12:27 -0700 Subject: [PATCH 2/2] typing fix --- src/sentry/incidents/logic.py | 2 +- src/sentry/incidents/metric_issue_detector.py | 24 ++-- src/sentry/workflow_engine/types.py | 2 +- .../endpoints/validators/test_validators.py | 104 +++++------------- 4 files changed, 44 insertions(+), 88 deletions(-) diff --git a/src/sentry/incidents/logic.py b/src/sentry/incidents/logic.py index d91bb06f463ec3..c4dcee28e2a75f 100644 --- a/src/sentry/incidents/logic.py +++ b/src/sentry/incidents/logic.py @@ -928,12 +928,12 @@ def update_alert_rule( updated_fields["sensitivity"] = None updated_fields["seasonality"] = None elif detection_type == AlertRuleDetectionType.DYNAMIC: - # NOTE: we set seasonality for EA if time_window is not None: updated_query_fields["resolution"] = timedelta(minutes=time_window) else: # snuba_query.time_window is already in seconds updated_query_fields["resolution"] = timedelta(seconds=snuba_query.time_window) + # NOTE: we set seasonality for EA updated_fields["seasonality"] = AlertRuleSeasonality.AUTO updated_fields["comparison_delta"] = None if ( diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index 1263aae3c1c2d3..fb0ed29f0b605f 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -250,18 +250,21 @@ def _validate_extrapolation_mode(self, extrapolation_mode: ExtrapolationMode) -> ) def _get_resolution_for_window( - self, time_window_seconds: int, detection_type: str | None, comparison_delta: int | None + self, + time_window_seconds: int | float, + detection_type: str | None, + comparison_delta: int | float | None, ) -> timedelta: """ - Compute the appropriate SnubaQuery resolution for a given time window, - mirroring the logic in create_alert_rule / update_alert_rule. + Compute the appropriate SnubaQuery resolution for a given time window + (in seconds), mirroring the logic in create_alert_rule / update_alert_rule. """ organization = self.context["organization"] if detection_type == AlertRuleDetectionType.DYNAMIC: resolution = timedelta(seconds=time_window_seconds) else: - resolution = get_alert_resolution(time_window_seconds // 60, organization) + resolution = get_alert_resolution(int(time_window_seconds) // 60, organization) if comparison_delta is not None: resolution *= DEFAULT_CMP_ALERT_RULE_RESOLUTION_MULTIPLIER @@ -317,7 +320,10 @@ def is_editing_transaction_dataset( return False def update_data_source( - self, instance: Detector, data_source: SnubaQueryDataSourceType, seer_updated: bool = False + self, + instance: Detector, + data_source: SnubaQueryDataSourceType, + seer_updated: bool = False, ) -> None: try: source_instance = DataSource.objects.get(detector=instance) @@ -334,7 +340,7 @@ def update_data_source( except SnubaQuery.DoesNotExist: raise serializers.ValidationError("SnubaQuery not found, can't update") - event_types = SnubaQueryEventType.objects.filter(snuba_query_id=snuba_query.id) + event_types = snuba_query.event_types if self.is_editing_transaction_dataset(snuba_query, data_source): raise serializers.ValidationError( @@ -373,14 +379,14 @@ def update_data_source( update_snuba_query( snuba_query=snuba_query, - query_type=data_source.get("query_type", snuba_query.type), - dataset=data_source.get("dataset", snuba_query.dataset), + query_type=data_source.get("query_type", SnubaQuery.Type(snuba_query.type)), + dataset=data_source.get("dataset", Dataset(snuba_query.dataset)), query=data_source.get("query", snuba_query.query), aggregate=data_source.get("aggregate", snuba_query.aggregate), time_window=timedelta(seconds=new_time_window), resolution=resolution, environment=data_source.get("environment", snuba_query.environment), - event_types=data_source.get("event_types", [event_type for event_type in event_types]), + event_types=data_source.get("event_types", event_types), extrapolation_mode=extrapolation_mode, ) diff --git a/src/sentry/workflow_engine/types.py b/src/sentry/workflow_engine/types.py index 877347824fcda6..03e8e60543758e 100644 --- a/src/sentry/workflow_engine/types.py +++ b/src/sentry/workflow_engine/types.py @@ -392,7 +392,7 @@ class DataConditionType(TypedDict): # TODO - Move this to snuba module -class SnubaQueryDataSourceType(TypedDict): +class SnubaQueryDataSourceType(TypedDict, total=False): query_type: SnubaQuery.Type dataset: Dataset query: str diff --git a/tests/sentry/incidents/endpoints/validators/test_validators.py b/tests/sentry/incidents/endpoints/validators/test_validators.py index 0cb37998c8bbd8..f7233064e7f025 100644 --- a/tests/sentry/incidents/endpoints/validators/test_validators.py +++ b/tests/sentry/incidents/endpoints/validators/test_validators.py @@ -1,3 +1,4 @@ +from typing import Any from unittest import mock import orjson @@ -756,23 +757,28 @@ def test_create_detector_trace_metrics_valid_aggregates(self) -> None: ) +# No-op Seer and relay interactions — these tests only verify resolution values. +@mock.patch("sentry.incidents.metric_issue_detector.delete_data_in_seer_for_detector") +@mock.patch("sentry.incidents.metric_issue_detector.send_new_detector_data") +@mock.patch("sentry.incidents.metric_issue_detector.update_detector_data") +@mock.patch("sentry.incidents.metric_issue_detector.schedule_update_project_config") class TestMetricAlertsResolution(TestMetricAlertsDetectorValidator): - """Tests that resolution is computed correctly via get_alert_resolution.""" + def _make_data_source(self, **overrides: Any) -> dict[str, Any]: + return { + "queryType": SnubaQuery.Type.ERROR.value, + "dataset": Dataset.Events.value, + "query": "test query", + "aggregate": "count()", + "timeWindow": 3600, + "environment": self.environment.name, + "eventTypes": [SnubaQueryEventType.EventType.ERROR.name.lower()], + **overrides, + } - @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_static_small_window_uses_default_resolution( - self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock - ) -> None: - """A 10-minute static detector should get 1-minute resolution (default).""" + def test_create_static_small_window_uses_default_resolution(self, *mocks: Any) -> None: data = { **self.valid_data, - "dataSources": [ - { - **self.valid_data["dataSources"][0], - "timeWindow": 600, # 10 minutes in seconds - } - ], + "dataSources": [self._make_data_source(timeWindow=600)], } validator = MetricIssueDetectorValidator(data=data, context=self.context) assert validator.is_valid(), validator.errors @@ -783,20 +789,10 @@ def test_create_static_small_window_uses_default_resolution( expected = get_alert_resolution(10, self.project.organization).total_seconds() assert snuba_query.resolution == expected - @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_static_large_window_uses_scaled_resolution( - self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock - ) -> None: - """A 60-minute static detector should get 3-minute resolution.""" + def test_create_static_large_window_uses_scaled_resolution(self, *mocks: Any) -> None: data = { **self.valid_data, - "dataSources": [ - { - **self.valid_data["dataSources"][0], - "timeWindow": 3600, # 60 minutes in seconds - } - ], + "dataSources": [self._make_data_source(timeWindow=3600)], } validator = MetricIssueDetectorValidator(data=data, context=self.context) assert validator.is_valid(), validator.errors @@ -808,34 +804,16 @@ def test_create_static_large_window_uses_scaled_resolution( assert snuba_query.resolution == expected assert snuba_query.resolution == 180 # 3 minutes in seconds - @mock.patch("sentry.seer.anomaly_detection.store_data_workflow_engine.make_store_data_request") - @mock.patch("sentry.workflow_engine.endpoints.validators.base.detector.create_audit_entry") - def test_create_dynamic_resolution_equals_time_window( - self, mock_audit: mock.MagicMock, mock_seer_request: mock.MagicMock - ) -> None: - """Dynamic detectors should use resolution == time_window.""" - seer_return_value: StoreDataResponse = {"success": True} - mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) - + def test_create_dynamic_resolution_equals_time_window(self, *mocks: Any) -> None: detector = self.create_dynamic_detector() snuba_query = self.get_snuba_query(detector) assert snuba_query.resolution == snuba_query.time_window - @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_percent_resolution_doubled( - self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock - ) -> None: - """Percent (comparison) detectors should double the resolution.""" + def test_create_percent_resolution_doubled(self, *mocks: Any) -> None: data = { **self.valid_data, - "dataSources": [ - { - **self.valid_data["dataSources"][0], - "timeWindow": 3600, # 60 minutes - } - ], + "dataSources": [self._make_data_source(timeWindow=3600)], "config": { "thresholdPeriod": 1, "detectionType": AlertRuleDetectionType.PERCENT.value, @@ -853,25 +831,13 @@ def test_create_percent_resolution_doubled( assert snuba_query.resolution == expected assert snuba_query.resolution == 360 # 3 min * 2 = 6 minutes in seconds - @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_update_time_window_recalculates_resolution( - self, mock_audit: mock.MagicMock, mock_schedule: mock.MagicMock - ) -> None: - """Changing time_window on update should recalculate resolution.""" + def test_update_time_window_recalculates_resolution(self, *mocks: Any) -> None: detector = self.create_static_detector() snuba_query = self.get_snuba_query(detector) - old_resolution = snuba_query.resolution - # Update to a larger time window (24 hours = 1440 minutes) update_data = { **self.valid_data, - "dataSources": [ - { - **self.valid_data["dataSources"][0], - "timeWindow": 86400, # 1440 minutes in seconds - } - ], + "dataSources": [self._make_data_source(timeWindow=86400)], } update_validator = MetricIssueDetectorValidator( instance=detector, data=update_data, context=self.context, partial=True @@ -883,28 +849,12 @@ def test_update_time_window_recalculates_resolution( expected = get_alert_resolution(1440, self.project.organization).total_seconds() assert snuba_query.resolution == expected assert snuba_query.resolution == 900 # 15 minutes in seconds - assert snuba_query.resolution != old_resolution - - @mock.patch("sentry.seer.anomaly_detection.store_data_workflow_engine.make_store_data_request") - @mock.patch("sentry.seer.anomaly_detection.delete_rule.delete_rule_in_seer") - @mock.patch("sentry.workflow_engine.endpoints.validators.base.detector.create_audit_entry") - def test_update_detection_type_change_recalculates_resolution( - self, - mock_audit: mock.MagicMock, - mock_seer_delete: mock.MagicMock, - mock_seer_store: mock.MagicMock, - ) -> None: - """Changing detection_type without data_sources should still recalculate resolution.""" - seer_return_value: StoreDataResponse = {"success": True} - mock_seer_store.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) - mock_seer_delete.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) + def test_update_detection_type_change_recalculates_resolution(self, *mocks: Any) -> None: detector = self.create_dynamic_detector() snuba_query = self.get_snuba_query(detector) - # Dynamic: resolution == time_window assert snuba_query.resolution == snuba_query.time_window - # Change to static without changing data sources update_data = { **self.valid_data, "config": {