From 6dc489fc64b7d648119caad5ce2859a3f57303ff Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 16:27:47 -0700 Subject: [PATCH 1/5] feat(investigations): Apply update_detector requests as the user Part of a series that adds follow-up actions to Seer investigations: when a run completes, Seer suggests one next step, and the user can apply a typed action, such as a monitor change, with one click. This applies a suggested monitor change as the user who clicked, through the detector validator, or only checks it on a dry run. If someone changed the monitor after Seer made the suggestion, nothing is applied, and the user sees which settings moved. Depends on https://github.com/getsentry/sentry/pull/128681 --- src/sentry/incidents/metric_issue_detector.py | 7 +- .../services/action_kinds/update_detector.py | 142 ++++- .../serializers/test_investigation.py | 2 + ...tion_investigation_action_request_apply.py | 488 ++++++++++++++++++ 4 files changed, 635 insertions(+), 4 deletions(-) diff --git a/src/sentry/incidents/metric_issue_detector.py b/src/sentry/incidents/metric_issue_detector.py index e7fe8804c537..2cb4fed2e69e 100644 --- a/src/sentry/incidents/metric_issue_detector.py +++ b/src/sentry/incidents/metric_issue_detector.py @@ -91,6 +91,9 @@ def schedule_update_project_config(detector: Detector) -> None: ) +TRANSACTION_UPDATES_DISABLED = "Updates to transaction-based alerts is disabled, as we migrate to the span dataset. Create span-based alerts (dataset: events_analytics_platform) with the is_transaction:true filter instead." + + class MetricIssueComparisonConditionValidator(BaseDataConditionValidator): supported_conditions = frozenset( ( @@ -364,9 +367,7 @@ def update_data_source( event_types = snuba_query.event_types if self.is_editing_transaction_dataset(snuba_query, data_source): - raise serializers.ValidationError( - "Updates to transaction-based alerts is disabled, as we migrate to the span dataset. Create span-based alerts (dataset: events_analytics_platform) with the is_transaction:true filter instead." - ) + raise serializers.ValidationError(TRANSACTION_UPDATES_DISABLED) new_extrapolation_mode = data_source.get( "extrapolation_mode", snuba_query.extrapolation_mode diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index b254427b41b4..230fff21f3f3 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -6,14 +6,21 @@ from typing import Any, Literal, TypedDict, cast from django.contrib.auth.models import AnonymousUser -from rest_framework import serializers +from django.db import router, transaction +from rest_framework import serializers, status from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.serializers import serialize +from sentry.api.serializers.rest_framework.base import convert_dict_key_case, snake_to_camel_case from sentry.auth.access import Access from sentry.db.models.fields.bounded import I32_MAX, I64_MAX from sentry.incidents.endpoints.serializers.query_subscription import SnubaQuerySerializer from sentry.incidents.grouptype import MetricIssue +from sentry.incidents.metric_issue_detector import ( + TRANSACTION_UPDATES_DISABLED, + MetricIssueDetectorValidator, + fetch_snuba_query, +) from sentry.incidents.models.alert_rule import AlertRuleDetectionType from sentry.investigations.contracts import ( ActionPayloadSerializer, @@ -25,13 +32,19 @@ from sentry.investigations.models import ( Investigation, InvestigationActionRequest, + InvestigationActionRequestState, InvestigationOrchestrationRun, ) from sentry.investigations.services.action_kinds.base import ( ActionKind, + ApplyContext, + ApplyResult, CannotApplyReason, SettingValue, + apply_error, + mark_applied, ) +from sentry.models.environment import Environment from sentry.models.organization import Organization from sentry.snuba.models import SnubaQuery from sentry.users.models.user import User @@ -39,6 +52,7 @@ from sentry.workflow_engine.endpoints.organization_detector_details import ( check_metric_detector_allowed, ) +from sentry.workflow_engine.endpoints.organization_detector_index import get_detector_validator from sentry.workflow_engine.endpoints.serializers.data_condition_serializer import ( DataConditionSerializer, ) @@ -329,6 +343,40 @@ def _action_detector_id(action_request: InvestigationActionRequest) -> int: return int(action_request.action_payload["detectorId"]) +def _error_messages(errors: object) -> list[str]: + if isinstance(errors, dict): + return [message for value in errors.values() for message in _error_messages(value)] + if isinstance(errors, list): + return [message for value in errors for message in _error_messages(value)] + return [str(errors)] + + +def _validation_errors( + validator: MetricIssueDetectorValidator, + detector: Detector, + snuba_query: SnubaQuery, + environment: SettingValue, +) -> dict[str, Any] | list[Any] | None: + # The validator creates environments it doesn't know, like the monitor form does. A + # suggestion shouldn't add one, so an unknown name is just invalid. + if ( + environment is not None + and not Environment.objects.filter( + organization_id=detector.linked_project.organization_id, + name=environment, + projects=detector.linked_project, + ).exists() + ): + return {"environment": [f"The project has no {environment} environment."]} + if not validator.is_valid(): + return validator.errors + # The validator only checks this when it saves, so a dry run has to ask for it. + data_sources = validator.validated_data.get("data_sources") + if data_sources and validator.is_editing_transaction_dataset(snuba_query, data_sources[0]): + return {"dataSources": [TRANSACTION_UPDATES_DISABLED]} + return None + + class UpdateDetectorKind(ActionKind): kind = UPDATE_DETECTOR_ACTION payload_serializer = UpdateDetectorActionSerializer @@ -375,3 +423,95 @@ def cannot_apply_reasons( ) reasons[item.id] = detector_reasons[key] return reasons + + def check(self, context: ApplyContext) -> ApplyResult | None: + return self._apply(context, dry_run=True) + + def apply(self, context: ApplyContext) -> ApplyResult: + return self._apply(context, dry_run=False) or ApplyResult( + action_request=context.action_request + ) + + def _apply(self, context: ApplyContext, *, dry_run: bool) -> ApplyResult | None: + action_request = context.action_request + action = action_request.action_payload + assert action is not None + detector_id = int(action["detectorId"]) + detector = target_detectors(context.investigation.organization_id, [detector_id]).get( + detector_id + ) + snuba_query = fetch_snuba_query(detector) if detector is not None else None + if detector is None or snuba_query is None: + raise ResourceDoesNotExist + + state = DetectorState.load(detector, snuba_query) + current = state.values() + moved = { + key: current.get(key) + for key, value in action["before"].items() + if current.get(key) != value + } + if moved: + if not dry_run: + # Keep the error on the row, so Seer sending the same suggestion again can't + # bring it back. + action_request.update( + state=InvestigationActionRequestState.SUPERSEDED, + error={"code": "stale", "current": moved}, + ) + return apply_error( + action_request, + status.HTTP_409_CONFLICT, + "The monitor changed since this was suggested.", + "stale", + current=moved, + ) + + changes = action["changes"] + validator = get_detector_validator( + context.request, + detector.linked_project, + detector.group_type.slug, + detector, + partial=True, + data=state.update(changes), + ) + assert isinstance(validator, MetricIssueDetectorValidator) + errors = _validation_errors(validator, detector, snuba_query, changes.get("environment")) + if errors is None and not dry_run: + try: + # If the save fails partway, undo the monitor changes but keep the failure we + # record below. + with transaction.atomic(router.db_for_write(Detector)): + validator.save() + except serializers.ValidationError as error: + errors = convert_dict_key_case( + serializers.as_serializer_error(error), snake_to_camel_case + ) + if errors is not None: + if not dry_run: + action_request.update( + state=InvestigationActionRequestState.FAILED, + error={"code": "validation_failed", "messages": _error_messages(errors)}, + ) + return apply_error( + action_request, + status.HTTP_400_BAD_REQUEST, + "The monitor change was not valid.", + "validation_failed", + errors=errors, + messages=_error_messages(errors), + ) + if dry_run: + return None + + mark_applied( + action_request, + context.actor_id, + { + "detectorId": str(detector.id), + "detectorName": detector.name, + "fields": [key for key in DetectorChangesSerializer().fields if key in changes], + }, + ) + return None diff --git a/tests/sentry/investigations/endpoints/serializers/test_investigation.py b/tests/sentry/investigations/endpoints/serializers/test_investigation.py index 6460155ccd98..c030e38a1e6d 100644 --- a/tests/sentry/investigations/endpoints/serializers/test_investigation.py +++ b/tests/sentry/investigations/endpoints/serializers/test_investigation.py @@ -458,6 +458,7 @@ def test_query_count_does_not_grow_with_action_requests(self) -> None: state=InvestigationActionRequestState.APPLIED, applied_by_id=self.user.id, date_applied=timezone.now(), + applied_details={"fields": ["highThreshold"]}, ) self.add_request("connect", action={"kind": "connect_telemetry", "provider": "datadog"}) with CaptureQueriesContext(connection) as one: @@ -470,6 +471,7 @@ def test_query_count_does_not_grow_with_action_requests(self) -> None: state=InvestigationActionRequestState.APPLIED, applied_by_id=other.id, date_applied=timezone.now(), + applied_details={"fields": ["highThreshold"]}, ) self.add_request("connect-gcp", action={"kind": "connect_telemetry", "provider": "gcp"}) for index in range(3): diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index 8344cd8e3e0b..edd82b687e96 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -4,24 +4,36 @@ from datetime import timedelta from typing import Any +from django.test import override_settings from django.urls import reverse +from rest_framework.test import APIClient +from sentry import audit_log +from sentry.constants import ObjectStatus from sentry.incidents.grouptype import MetricIssue +from sentry.incidents.metric_issue_detector import TRANSACTION_UPDATES_DISABLED from sentry.incidents.utils.constants import INCIDENTS_SNUBA_SUBSCRIPTION_TYPE from sentry.incidents.utils.subscription_limits import METRIC_SUBSCRIPTION_FEATURE_FLAGS from sentry.investigations.models import ( InvestigationActionRequest, InvestigationActionRequestState, + InvestigationBlock, InvestigationStatus, ) from sentry.investigations.services.action_kinds import ACTION_KINDS from sentry.investigations.services.action_kinds.base import ActionInvalid, ActionStale +from sentry.models.auditlogentry import AuditLogEntry +from sentry.models.environment import Environment +from sentry.seer import agent_token from sentry.seer.models.run import SeerRunMirrorStatus, SeerRunType +from sentry.silo.base import SiloMode from sentry.snuba.dataset import Dataset from sentry.snuba.models import ExtrapolationMode, SnubaQuery, SnubaQueryEventType from sentry.snuba.subscriptions import create_snuba_query, create_snuba_subscription from sentry.testutils.cases import APITestCase from sentry.testutils.helpers.features import with_feature +from sentry.testutils.outbox import outbox_runner +from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_kafka, requires_snuba from sentry.workflow_engine.models import DataConditionGroup, Detector from sentry.workflow_engine.models.data_condition import Condition @@ -153,6 +165,98 @@ def snuba_query(self, detector: Detector | None = None) -> SnubaQuery: data_source = (detector or self.detector).data_sources.get() return SnubaQuery.objects.get(subscriptions__id=int(data_source.source_id)) + def conditions(self) -> dict[int, tuple[int, float]]: + assert self.detector.workflow_condition_group is not None + return { + condition.id: (condition.condition_result, condition.comparison) + for condition in self.detector.workflow_condition_group.conditions.all() + } + + def test_applies_a_threshold_change(self) -> None: + action_request = self.suggest( + before={"highThreshold": 500}, changes={"highThreshold": 800.0} + ) + conditions = self.conditions() + + response = self.apply(action_request) + + assert response.status_code == 200, response.data + assert set(response.data) == {"actionRequest"} + serialized = response.data["actionRequest"] + assert serialized["id"] == str(action_request.id) + assert serialized["state"] == "applied" + assert serialized["appliedBy"]["id"] == str(self.user.id) + assert serialized["cannotApplyReason"] == "not_suggested" + updated = self.conditions() + assert set(updated) == set(conditions) + assert sorted(updated.values()) == [ + (DetectorPriorityLevel.OK, 500.0), + (DetectorPriorityLevel.HIGH, 800.0), + ] + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.APPLIED + assert action_request.applied_by_id == self.user.id + assert action_request.date_applied is not None + assert self.snuba_query().time_window == 300 + + def test_applies_a_time_window_change_to_an_errors_detector(self) -> None: + action_request = self.suggest() + conditions = self.conditions() + + response = self.apply(action_request) + + assert response.status_code == 200, response.data + snuba_query = self.snuba_query() + assert snuba_query.time_window == 900 + assert snuba_query.query == "level:error" + assert snuba_query.environment == self.environment + assert self.conditions() == conditions + + def test_applies_a_time_window_change_to_an_eap_detector(self) -> None: + self.detector = self.create_metric_detector(dataset=Dataset.EventsAnalyticsPlatform) + self.orchestration_run.update( + source={"type": "breached_metric", "detectorId": str(self.detector.id)} + ) + action_request = self.suggest() + + response = self.apply(action_request) + + assert response.status_code == 200, response.data + snuba_query = self.snuba_query() + assert snuba_query.time_window == 900 + assert snuba_query.dataset == Dataset.EventsAnalyticsPlatform.value + assert snuba_query.event_types == [SnubaQueryEventType.EventType.TRACE_ITEM_SPAN] + assert snuba_query.extrapolation_mode == ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED.value + + def test_applies_query_and_environment_changes(self) -> None: + staging = self.create_environment(project=self.project, name="staging") + action_request = self.suggest( + before={"query": "level:error", "environment": "production"}, + changes={"query": "level:error !transaction:/health", "environment": "staging"}, + ) + conditions = self.conditions() + + response = self.apply(action_request) + + assert response.status_code == 200, response.data + snuba_query = self.snuba_query() + assert snuba_query.query == "level:error !transaction:/health" + assert snuba_query.environment == staging + assert snuba_query.time_window == 300 + assert self.conditions() == conditions + + def test_a_changed_query_is_stale(self) -> None: + action_request = self.suggest( + before={"query": "level:warning"}, changes={"query": "level:warning !logger:health"} + ) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["code"] == "stale" + assert response.data["current"] == {"query": "level:error"} + assert self.snuba_query().query == "level:error" + def details_request(self) -> dict[str, Any]: response = self.client.get( reverse( @@ -199,6 +303,18 @@ def test_requests_that_are_no_longer_suggested_conflict(self) -> None: assert response.status_code == 409 assert response.data["code"] == "not_suggested" + def test_a_second_apply_conflicts(self) -> None: + action_request = self.suggest() + assert self.apply(action_request).status_code == 200 + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data == { + "detail": "This action was already applied.", + "code": "already_applied", + } + def test_a_dry_run_runs_the_same_checks(self) -> None: action_request = self.suggest(state=InvestigationActionRequestState.SUPERSEDED) @@ -213,6 +329,146 @@ def test_rejects_an_unknown_field(self) -> None: assert response.status_code == 400 assert response.data == {"dryRn": "Unknown field."} + def test_a_stale_monitor_conflicts_and_supersedes_the_suggestion(self) -> None: + action_request = self.suggest(before={"timeWindowSeconds": 600}) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data == { + "detail": "The monitor changed since this was suggested.", + "code": "stale", + "current": {"timeWindowSeconds": 300}, + } + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUPERSEDED + assert action_request.error == {"code": "stale", "current": {"timeWindowSeconds": 300}} + assert self.snuba_query().time_window == 300 + assert self.apply(action_request).data["code"] == "not_suggested" + + def test_a_missing_threshold_is_stale(self) -> None: + action_request = self.suggest( + before={"mediumThreshold": 400}, changes={"mediumThreshold": 450} + ) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["code"] == "stale" + assert response.data["current"] == {"mediumThreshold": None} + + def test_only_the_source_monitor_can_change(self) -> None: + other = self.create_metric_detector() + action_request = self.suggest(detector_id=other.id) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data == { + "detail": "This action can't be applied.", + "code": "not_target_detector", + } + assert self.snuba_query(other).time_window == 300 + + def test_a_deleted_monitor_is_not_found(self) -> None: + action_request = self.suggest() + self.detector.update(status=ObjectStatus.PENDING_DELETION) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["code"] == "detector_not_found" + + def test_another_organizations_monitor_is_not_found(self) -> None: + foreign = self.create_detector( + project=self.create_project(organization=self.create_organization()), + type=MetricIssue.slug, + ) + self.orchestration_run.update( + source={"type": "breached_metric", "detectorId": str(foreign.id)} + ) + action_request = self.suggest(detector_id=foreign.id) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["code"] == "detector_not_found" + + def test_members_without_alerts_write_cannot_apply(self) -> None: + self.organization.update_option("sentry:alerts_member_write", False) + member = self.create_user() + self.create_member(user=member, organization=self.organization, role="member") + self.login_as(member) + action_request = self.suggest() + + response = self.apply(action_request) + + assert response.status_code == 403 + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + assert self.snuba_query().time_window == 300 + + def test_an_invalid_change_fails_the_request(self) -> None: + self.detector = self.create_metric_detector(dataset=Dataset.EventsAnalyticsPlatform) + self.orchestration_run.update( + source={"type": "breached_metric", "detectorId": str(self.detector.id)} + ) + action_request = self.suggest(changes={"timeWindowSeconds": 450}) + version = self.investigation.version + + response = self.apply(action_request) + + assert response.status_code == 400 + assert response.data["detail"] == "The monitor change was not valid." + assert response.data["code"] == "validation_failed" + assert "Invalid Time Window" in str(response.data["errors"]["dataSources"]) + action_request.refresh_from_db() + assert action_request.error is not None + assert response.data["messages"] == action_request.error["messages"] + assert action_request.state == InvestigationActionRequestState.FAILED + assert action_request.error is not None + assert set(action_request.error) == {"code", "messages"} + assert action_request.error["code"] == "validation_failed" + assert any("Invalid Time Window" in message for message in action_request.error["messages"]) + assert self.snuba_query().time_window == 300 + self.investigation.refresh_from_db() + assert self.investigation.version == version + + def test_a_dry_run_of_a_valid_change_leaves_the_monitor_alone(self) -> None: + action_request = self.suggest() + + response = self.apply(action_request, dryRun=True) + + assert response.status_code == 200, response.data + assert response.data["actionRequest"]["state"] == "suggested" + assert self.snuba_query().time_window == 300 + + def test_a_dry_run_reports_an_invalid_change_without_failing_it(self) -> None: + self.detector = self.create_metric_detector(dataset=Dataset.EventsAnalyticsPlatform) + self.orchestration_run.update( + source={"type": "breached_metric", "detectorId": str(self.detector.id)} + ) + action_request = self.suggest(changes={"timeWindowSeconds": 450}) + + response = self.apply(action_request, dryRun=True) + + assert response.status_code == 400 + assert response.data["code"] == "validation_failed" + assert "Invalid Time Window" in str(response.data["errors"]["dataSources"]) + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + assert action_request.error is None + + def test_a_dry_run_reports_a_stale_monitor_without_superseding_it(self) -> None: + action_request = self.suggest(before={"timeWindowSeconds": 600}) + + response = self.apply(action_request, dryRun=True) + + assert response.status_code == 409 + assert response.data["code"] == "stale" + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + def test_archived_investigations_are_read_only(self) -> None: action_request = self.suggest() self.investigation.update(status=InvestigationStatus.ARCHIVED) @@ -290,6 +546,238 @@ def test_requires_the_feature(self) -> None: assert response.status_code == 404 + def set_detector(self, **kwargs: Any) -> None: + self.detector = self.create_metric_detector(**kwargs) + self.orchestration_run.update( + source={"type": "breached_metric", "detectorId": str(self.detector.id)} + ) + + def test_applies_a_high_and_resolution_change_together(self) -> None: + action_request = self.suggest( + before={"highThreshold": 500, "resolutionThreshold": 500}, + changes={"highThreshold": 400, "resolutionThreshold": 400}, + ) + + response = self.apply(action_request) + + assert response.status_code == 200, response.data + assert sorted(self.conditions().values()) == [ + (DetectorPriorityLevel.OK, 400.0), + (DetectorPriorityLevel.HIGH, 400.0), + ] + + def test_applies_a_medium_threshold_change_and_keeps_every_condition(self) -> None: + self.set_detector( + conditions=[ + (Condition.GREATER, 800.0, DetectorPriorityLevel.HIGH), + (Condition.GREATER, 500.0, DetectorPriorityLevel.MEDIUM), + (Condition.LESS_OR_EQUAL, 500.0, DetectorPriorityLevel.OK), + ] + ) + conditions = self.conditions() + action_request = self.suggest( + before={"mediumThreshold": 500, "resolutionThreshold": 500}, + changes={"mediumThreshold": 600, "resolutionThreshold": 600}, + ) + + response = self.apply(action_request) + + assert response.status_code == 200, response.data + updated = self.conditions() + assert set(updated) == set(conditions) + assert sorted(updated.values()) == [ + (DetectorPriorityLevel.OK, 600.0), + (DetectorPriorityLevel.MEDIUM, 600.0), + (DetectorPriorityLevel.HIGH, 800.0), + ] + + def test_applies_a_raw_threshold_to_a_percent_monitor(self) -> None: + self.set_detector(high=110.0, config={"detection_type": "static", "comparison_delta": 3600}) + action_request = self.suggest( + before={"highThreshold": 110, "resolutionThreshold": 110}, + changes={"highThreshold": 130, "resolutionThreshold": 130}, + ) + + response = self.apply(action_request) + + assert response.status_code == 200, response.data + assert sorted(self.conditions().values()) == [ + (DetectorPriorityLevel.OK, 130.0), + (DetectorPriorityLevel.HIGH, 130.0), + ] + self.detector.refresh_from_db() + assert self.detector.config["comparison_delta"] == 3600 + + def test_two_conditions_for_one_level_are_stale(self) -> None: + self.set_detector( + conditions=[ + (Condition.GREATER, 500.0, DetectorPriorityLevel.HIGH), + (Condition.LESS, 10.0, DetectorPriorityLevel.HIGH), + (Condition.LESS_OR_EQUAL, 500.0, DetectorPriorityLevel.OK), + ] + ) + conditions = self.conditions() + action_request = self.suggest(before={"highThreshold": 500}, changes={"highThreshold": 800}) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["code"] == "stale" + assert response.data["current"] == {"highThreshold": None} + assert self.conditions() == conditions + + def test_dynamic_monitors_are_not_changed(self) -> None: + self.set_detector(config={"detection_type": "dynamic"}) + action_request = self.suggest() + + with mock.patch( + "sentry.incidents.metric_issue_detector.update_detector_data" + ) as update_detector_data: + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data == { + "detail": "This action can't be applied.", + "code": "unsupported_action", + } + update_detector_data.assert_not_called() + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + assert self.snuba_query().time_window == 300 + + def test_a_dataset_the_plan_no_longer_allows_is_not_found(self) -> None: + self.set_detector(dataset=Dataset.EventsAnalyticsPlatform) + action_request = self.suggest() + + with self.feature({"organizations:visibility-explore-view": False}): + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["code"] == "detector_not_found" + assert self.snuba_query().time_window == 300 + + def test_an_unknown_environment_is_invalid_and_not_created(self) -> None: + action_request = self.suggest( + before={"environment": "production"}, changes={"environment": "made-up"} + ) + + dry_run = self.apply(action_request, dryRun=True) + + assert dry_run.status_code == 400 + assert dry_run.data["messages"] == ["The project has no made-up environment."] + assert not Environment.objects.filter(name="made-up").exists() + + def test_a_transaction_monitor_change_fails_before_saving(self) -> None: + snuba_query = self.snuba_query() + snuba_query.update( + type=SnubaQuery.Type.PERFORMANCE.value, dataset=Dataset.Transactions.value, query="" + ) + snuba_query.snubaqueryeventtype_set.update( + type=SnubaQueryEventType.EventType.TRANSACTION.value + ) + action_request = self.suggest( + before={"timeWindowSeconds": 300, "highThreshold": 500}, + changes={"timeWindowSeconds": 900, "highThreshold": 800}, + ) + conditions = self.conditions() + + with self.feature("organizations:discover-saved-queries-deprecation"): + dry_run = self.apply(action_request, dryRun=True) + response = self.apply(action_request) + + assert dry_run.status_code == 400 + assert dry_run.data["code"] == "validation_failed" + + assert response.status_code == 400 + assert response.data["code"] == "validation_failed" + [error] = response.data["errors"]["dataSources"] + assert error == TRANSACTION_UPDATES_DISABLED + assert self.conditions() == conditions + assert self.snuba_query().time_window == 300 + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.FAILED + assert action_request.error == {"code": "validation_failed", "messages": [error]} + + @override_settings(SEER_API_SHARED_SECRET="test-seer-api-shared-secret-thirty-two-bytes!") + def test_an_agent_token_without_alerts_write_cannot_apply(self) -> None: + action_request = self.suggest() + token, _ = agent_token.encode_agent_token( + user_id=self.user.id, + organization_id=self.organization.id, + scopes=["org:read"], + session_id="investigation-apply", + ) + client = APIClient() + client.credentials(HTTP_AUTHORIZATION=f"Bearer {token}") + + with self.feature(agent_token.FEATURE_FLAG): + response = client.post(self.apply_url(action_request.id), data={}, format="json") + + assert response.status_code == 403, response.content + assert response.data["code"] == "permission_denied" + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + + def test_applying_records_activity_and_an_audit_entry(self) -> None: + self.detector.update(name="Checkout [p95]") + action_request = self.suggest( + title="Widen the window", + before={"timeWindowSeconds": 300, "highThreshold": 500}, + changes={"timeWindowSeconds": 3600, "highThreshold": 800.5}, + report_revision=2, + ) + blocks = InvestigationBlock.objects.filter(investigation=self.investigation).count() + version = self.investigation.version + + with outbox_runner(): + response = self.apply(action_request) + + assert response.status_code == 200, response.data + assert "blockId" not in response.data + serialized = response.data["actionRequest"] + assert serialized["appliedBy"]["id"] == str(self.user.id) + # Reads look up the applier, so renaming them afterwards shows the new name. + with assume_test_silo_mode(SiloMode.CONTROL): + self.user.update(name="Renamed after the apply") + details = self.client.get( + reverse( + "sentry-api-0-organization-investigation-details", + kwargs={ + "organization_id_or_slug": self.organization.slug, + "investigation_id": self.investigation.id, + }, + ) + ) + [read] = details.data["actionRequests"] + assert read["appliedBy"]["id"] == str(self.user.id) + assert read["appliedBy"]["name"] == "Renamed after the apply" + assert serialized["appliedDetails"] == { + "detectorId": str(self.detector.id), + "detectorName": "Checkout [p95]", + "fields": ["timeWindowSeconds", "highThreshold"], + } + assert InvestigationBlock.objects.filter(investigation=self.investigation).count() == blocks + self.investigation.refresh_from_db() + assert self.investigation.version == version + 1 + + with assume_test_silo_mode(SiloMode.CONTROL): + entry = AuditLogEntry.objects.get(event=audit_log.get_event_id("DETECTOR_EDIT")) + assert entry.actor_id == self.user.id + assert entry.data["name"] == "Checkout [p95]" + + def test_activity_shows_query_and_environment_changes(self) -> None: + self.create_environment(project=self.project, name="staging") + action_request = self.suggest( + before={"query": "level:error", "environment": "production"}, + changes={"query": "", "environment": "staging"}, + ) + + with outbox_runner(): + response = self.apply(action_request) + + assert response.status_code == 200, response.data + details = response.data["actionRequest"]["appliedDetails"] + assert details["fields"] == ["query", "environment"] @with_feature(FEATURE) class SharedApplyPathTest(APITestCase): From b59b3b816a795069672855aa2a9dfd5553d7b5c0 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 18:00:03 -0700 Subject: [PATCH 2/5] ref(investigations): Apply update_detector changes through the shared path The kind only makes the change now. It raises when the monitor is stale or the change is invalid, locks the monitor until it saves, and checks every setting Seer relied on, not just the ones it changes. A rejected change stays suggested so the user can try again, and applying no longer bumps the notebook version. --- .../services/action_kinds/update_detector.py | 117 +++++++----------- ...tion_investigation_action_request_apply.py | 43 ++++--- .../services/test_update_detector_contract.py | 21 +++- 3 files changed, 86 insertions(+), 95 deletions(-) diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index 230fff21f3f3..4fa5ea9891a4 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -7,7 +7,7 @@ from django.contrib.auth.models import AnonymousUser from django.db import router, transaction -from rest_framework import serializers, status +from rest_framework import serializers from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.serializers import serialize @@ -32,17 +32,15 @@ from sentry.investigations.models import ( Investigation, InvestigationActionRequest, - InvestigationActionRequestState, InvestigationOrchestrationRun, ) from sentry.investigations.services.action_kinds.base import ( + ActionInvalid, ActionKind, + ActionStale, ApplyContext, - ApplyResult, CannotApplyReason, SettingValue, - apply_error, - mark_applied, ) from sentry.models.environment import Environment from sentry.models.organization import Organization @@ -97,8 +95,10 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: changes = attrs["changes"] if not changes: raise serializers.ValidationError({"changes": "At least one change is required."}) - if set(before) != set(changes): - raise serializers.ValidationError("before and changes must have the same keys.") + # before can also hold settings Seer relied on but didn't change, so apply can tell + # if any of them moved. + if not set(changes) <= set(before): + raise serializers.ValidationError("Every change needs its before value.") if any(changes[key] == before[key] for key in changes): raise serializers.ValidationError({"changes": "Every change must differ from before."}) return attrs @@ -116,7 +116,7 @@ def trim(cls, action: dict[str, Any]) -> dict[str, Any] | None: if ( not isinstance(before, dict) or not isinstance(changes, dict) - or set(before) != set(changes) + or not set(changes) <= set(before) ): return action dropped = { @@ -137,7 +137,7 @@ def trim(cls, action: dict[str, Any]) -> dict[str, Any] | None: return None return { **action, - "before": {key: before[key] for key in kept}, + "before": {key: value for key, value in before.items() if key not in dropped}, "changes": {key: changes[key] for key in kept}, } @@ -343,14 +343,6 @@ def _action_detector_id(action_request: InvestigationActionRequest) -> int: return int(action_request.action_payload["detectorId"]) -def _error_messages(errors: object) -> list[str]: - if isinstance(errors, dict): - return [message for value in errors.values() for message in _error_messages(value)] - if isinstance(errors, list): - return [message for value in errors for message in _error_messages(value)] - return [str(errors)] - - def _validation_errors( validator: MetricIssueDetectorValidator, detector: Detector, @@ -424,25 +416,34 @@ def cannot_apply_reasons( reasons[item.id] = detector_reasons[key] return reasons - def check(self, context: ApplyContext) -> ApplyResult | None: - return self._apply(context, dry_run=True) + def check(self, context: ApplyContext) -> None: + self._apply(context, dry_run=True) - def apply(self, context: ApplyContext) -> ApplyResult: - return self._apply(context, dry_run=False) or ApplyResult( - action_request=context.action_request - ) + def apply(self, context: ApplyContext) -> dict[str, object]: + return self._apply(context, dry_run=False) - def _apply(self, context: ApplyContext, *, dry_run: bool) -> ApplyResult | None: - action_request = context.action_request - action = action_request.action_payload + def _apply(self, context: ApplyContext, *, dry_run: bool) -> dict[str, object]: + action = context.action_request.action_payload assert action is not None detector_id = int(action["detectorId"]) detector = target_detectors(context.investigation.organization_id, [detector_id]).get( detector_id ) - snuba_query = fetch_snuba_query(detector) if detector is not None else None - if detector is None or snuba_query is None: + if detector is None: + raise ResourceDoesNotExist + if not dry_run: + # Lock the monitor until we save, so an edit between the stale check and the save + # isn't lost. + detector = ( + Detector.objects.select_for_update(of=("self",)) + .select_related("project", "workflow_condition_group") + .get(id=detector.id) + ) + snuba_query = fetch_snuba_query(detector) + if snuba_query is None: raise ResourceDoesNotExist + if not dry_run: + snuba_query = SnubaQuery.objects.select_for_update().get(id=snuba_query.id) state = DetectorState.load(detector, snuba_query) current = state.values() @@ -452,20 +453,7 @@ def _apply(self, context: ApplyContext, *, dry_run: bool) -> ApplyResult | None: if current.get(key) != value } if moved: - if not dry_run: - # Keep the error on the row, so Seer sending the same suggestion again can't - # bring it back. - action_request.update( - state=InvestigationActionRequestState.SUPERSEDED, - error={"code": "stale", "current": moved}, - ) - return apply_error( - action_request, - status.HTTP_409_CONFLICT, - "The monitor changed since this was suggested.", - "stale", - current=moved, - ) + raise ActionStale(dict(moved)) changes = action["changes"] validator = get_detector_validator( @@ -478,40 +466,21 @@ def _apply(self, context: ApplyContext, *, dry_run: bool) -> ApplyResult | None: ) assert isinstance(validator, MetricIssueDetectorValidator) errors = _validation_errors(validator, detector, snuba_query, changes.get("environment")) - if errors is None and not dry_run: + if errors is not None: + raise ActionInvalid(errors) + if not dry_run: try: - # If the save fails partway, undo the monitor changes but keep the failure we - # record below. + # If the save fails partway, undo the monitor changes it made. with transaction.atomic(router.db_for_write(Detector)): validator.save() except serializers.ValidationError as error: - errors = convert_dict_key_case( - serializers.as_serializer_error(error), snake_to_camel_case + raise ActionInvalid( + convert_dict_key_case( + serializers.as_serializer_error(error), snake_to_camel_case + ) ) - if errors is not None: - if not dry_run: - action_request.update( - state=InvestigationActionRequestState.FAILED, - error={"code": "validation_failed", "messages": _error_messages(errors)}, - ) - return apply_error( - action_request, - status.HTTP_400_BAD_REQUEST, - "The monitor change was not valid.", - "validation_failed", - errors=errors, - messages=_error_messages(errors), - ) - if dry_run: - return None - - mark_applied( - action_request, - context.actor_id, - { - "detectorId": str(detector.id), - "detectorName": detector.name, - "fields": [key for key in DetectorChangesSerializer().fields if key in changes], - }, - ) - return None + return { + "detectorId": str(detector.id), + "detectorName": detector.name, + "fields": [key for key in DetectorChangesSerializer().fields if key in changes], + } diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index edd82b687e96..3804a8e55451 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -3,6 +3,7 @@ from collections.abc import Sequence from datetime import timedelta from typing import Any +from unittest import mock from django.test import override_settings from django.urls import reverse @@ -329,6 +330,18 @@ def test_rejects_an_unknown_field(self) -> None: assert response.status_code == 400 assert response.data == {"dryRn": "Unknown field."} + def test_a_setting_seer_relied_on_but_did_not_change_counts_as_stale(self) -> None: + action_request = self.suggest( + before={"timeWindowSeconds": 300, "aggregate": "p95(transaction.duration)"}, + changes={"timeWindowSeconds": 900}, + ) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data["current"] == {"aggregate": "count()"} + assert self.snuba_query().time_window == 300 + def test_a_stale_monitor_conflicts_and_supersedes_the_suggestion(self) -> None: action_request = self.suggest(before={"timeWindowSeconds": 600}) @@ -336,7 +349,7 @@ def test_a_stale_monitor_conflicts_and_supersedes_the_suggestion(self) -> None: assert response.status_code == 409 assert response.data == { - "detail": "The monitor changed since this was suggested.", + "detail": "This changed since Seer suggested it.", "code": "stale", "current": {"timeWindowSeconds": 300}, } @@ -408,7 +421,7 @@ def test_members_without_alerts_write_cannot_apply(self) -> None: assert action_request.state == InvestigationActionRequestState.SUGGESTED assert self.snuba_query().time_window == 300 - def test_an_invalid_change_fails_the_request(self) -> None: + def test_an_invalid_change_is_rejected_and_can_be_tried_again(self) -> None: self.detector = self.create_metric_detector(dataset=Dataset.EventsAnalyticsPlatform) self.orchestration_run.update( source={"type": "breached_metric", "detectorId": str(self.detector.id)} @@ -419,17 +432,12 @@ def test_an_invalid_change_fails_the_request(self) -> None: response = self.apply(action_request) assert response.status_code == 400 - assert response.data["detail"] == "The monitor change was not valid." + assert response.data["detail"] == "The change wasn't valid." assert response.data["code"] == "validation_failed" - assert "Invalid Time Window" in str(response.data["errors"]["dataSources"]) + assert any("Invalid Time Window" in message for message in response.data["messages"]) action_request.refresh_from_db() - assert action_request.error is not None - assert response.data["messages"] == action_request.error["messages"] - assert action_request.state == InvestigationActionRequestState.FAILED - assert action_request.error is not None - assert set(action_request.error) == {"code", "messages"} - assert action_request.error["code"] == "validation_failed" - assert any("Invalid Time Window" in message for message in action_request.error["messages"]) + assert action_request.state == InvestigationActionRequestState.SUGGESTED + assert action_request.error is None assert self.snuba_query().time_window == 300 self.investigation.refresh_from_db() assert self.investigation.version == version @@ -454,7 +462,7 @@ def test_a_dry_run_reports_an_invalid_change_without_failing_it(self) -> None: assert response.status_code == 400 assert response.data["code"] == "validation_failed" - assert "Invalid Time Window" in str(response.data["errors"]["dataSources"]) + assert any("Invalid Time Window" in message for message in response.data["messages"]) action_request.refresh_from_db() assert action_request.state == InvestigationActionRequestState.SUGGESTED assert action_request.error is None @@ -638,7 +646,7 @@ def test_dynamic_monitors_are_not_changed(self) -> None: assert response.status_code == 409 assert response.data == { "detail": "This action can't be applied.", - "code": "unsupported_action", + "code": "dynamic_detector", } update_detector_data.assert_not_called() action_request.refresh_from_db() @@ -690,13 +698,11 @@ def test_a_transaction_monitor_change_fails_before_saving(self) -> None: assert response.status_code == 400 assert response.data["code"] == "validation_failed" - [error] = response.data["errors"]["dataSources"] - assert error == TRANSACTION_UPDATES_DISABLED + assert response.data["messages"] == [TRANSACTION_UPDATES_DISABLED] assert self.conditions() == conditions assert self.snuba_query().time_window == 300 action_request.refresh_from_db() - assert action_request.state == InvestigationActionRequestState.FAILED - assert action_request.error == {"code": "validation_failed", "messages": [error]} + assert action_request.state == InvestigationActionRequestState.SUGGESTED @override_settings(SEER_API_SHARED_SECRET="test-seer-api-shared-secret-thirty-two-bytes!") def test_an_agent_token_without_alerts_write_cannot_apply(self) -> None: @@ -758,7 +764,7 @@ def test_applying_records_activity_and_an_audit_entry(self) -> None: } assert InvestigationBlock.objects.filter(investigation=self.investigation).count() == blocks self.investigation.refresh_from_db() - assert self.investigation.version == version + 1 + assert self.investigation.version == version with assume_test_silo_mode(SiloMode.CONTROL): entry = AuditLogEntry.objects.get(event=audit_log.get_event_id("DETECTOR_EDIT")) @@ -779,6 +785,7 @@ def test_activity_shows_query_and_environment_changes(self) -> None: details = response.data["actionRequest"]["appliedDetails"] assert details["fields"] == ["query", "environment"] + @with_feature(FEATURE) class SharedApplyPathTest(APITestCase): def setUp(self) -> None: diff --git a/tests/sentry/investigations/services/test_update_detector_contract.py b/tests/sentry/investigations/services/test_update_detector_contract.py index 36483e671277..9154412d9809 100644 --- a/tests/sentry/investigations/services/test_update_detector_contract.py +++ b/tests/sentry/investigations/services/test_update_detector_contract.py @@ -100,12 +100,27 @@ def test_trim_drops_changes_sentry_cannot_store() -> None: assert UpdateDetectorActionSerializer.trim(ACTION) == ACTION -def test_trim_leaves_mismatched_keys_for_validation_to_reject() -> None: +def test_accepts_settings_seer_relied_on_but_did_not_change() -> None: + action = { + **ACTION, + "before": {"timeWindowSeconds": 300, "aggregate": "p95(transaction.duration)"}, + "changes": {"timeWindowSeconds": 900}, + } + + assert validated(action) is not None + + +def test_trim_drops_long_text_from_unchanged_settings_too() -> None: action = { **ACTION, "before": {"query": "x" * 2_001, "timeWindowSeconds": 300}, "changes": {"timeWindowSeconds": 900}, } - assert UpdateDetectorActionSerializer.trim(action) == action - assert validated(action) is None + trimmed = UpdateDetectorActionSerializer.trim(action) + + assert trimmed is not None + assert (trimmed["before"], trimmed["changes"]) == ( + {"timeWindowSeconds": 300}, + {"timeWindowSeconds": 900}, + ) From 13f3e5ac9e9430c2581d7bd644321d23e95de9ef Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 20:01:12 -0700 Subject: [PATCH 3/5] fix(investigations): Type update_detector's applied details and error messages --- .../endpoints/serializers/action_request.py | 7 +- .../services/action_kinds/__init__.py | 7 +- .../services/action_kinds/update_detector.py | 71 ++++++++++--------- ...tion_investigation_action_request_apply.py | 7 +- 4 files changed, 49 insertions(+), 43 deletions(-) diff --git a/src/sentry/investigations/endpoints/serializers/action_request.py b/src/sentry/investigations/endpoints/serializers/action_request.py index 722344cd5c5a..e536f633492f 100644 --- a/src/sentry/investigations/endpoints/serializers/action_request.py +++ b/src/sentry/investigations/endpoints/serializers/action_request.py @@ -16,6 +16,7 @@ InvestigationBlock, ) from sentry.investigations.services.action_kinds import ( + AppliedDetails, get_action_kind, shared_cannot_apply_reason, ) @@ -60,7 +61,7 @@ class InvestigationActionRequestSerializerResponse(TypedDict): canApply: bool cannotApplyReason: CannotApplyReason | None appliedBy: UserSerializerResponse | None - appliedDetails: dict[str, Any] | None + appliedDetails: AppliedDetails | None dateApplied: datetime | None error: ActionRequestError | None dateCreated: datetime @@ -199,9 +200,9 @@ def serialize( "canApply": attrs["cannot_apply_reason"] is None, "cannotApplyReason": attrs["cannot_apply_reason"], "appliedBy": attrs["applied_by"], - "appliedDetails": obj.applied_details, + # Only the shared apply path writes these columns. + "appliedDetails": cast(AppliedDetails | None, obj.applied_details), "dateApplied": obj.date_applied, - # Only the shared apply path writes this column. "error": cast(ActionRequestError | None, obj.error), "dateCreated": obj.date_added, } diff --git a/src/sentry/investigations/services/action_kinds/__init__.py b/src/sentry/investigations/services/action_kinds/__init__.py index c081123fa7e5..cd0da4968d10 100644 --- a/src/sentry/investigations/services/action_kinds/__init__.py +++ b/src/sentry/investigations/services/action_kinds/__init__.py @@ -8,7 +8,12 @@ ) from sentry.investigations.services.action_kinds.base import ActionKind, CannotApplyReason from sentry.investigations.services.action_kinds.connect_telemetry import ConnectTelemetryKind -from sentry.investigations.services.action_kinds.update_detector import UpdateDetectorKind +from sentry.investigations.services.action_kinds.update_detector import ( + UpdateDetectorAppliedDetails, + UpdateDetectorKind, +) + +AppliedDetails = UpdateDetectorAppliedDetails ACTION_KINDS: dict[str, ActionKind] = { kind.kind: kind for kind in (UpdateDetectorKind(), ConnectTelemetryKind()) diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index 4fa5ea9891a4..d16ff34c557e 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -11,7 +11,6 @@ from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.serializers import serialize -from sentry.api.serializers.rest_framework.base import convert_dict_key_case, snake_to_camel_case from sentry.auth.access import Access from sentry.db.models.fields.bounded import I32_MAX, I64_MAX from sentry.incidents.endpoints.serializers.query_subscription import SnubaQuerySerializer @@ -343,12 +342,27 @@ def _action_detector_id(action_request: InvestigationActionRequest) -> int: return int(action_request.action_payload["detectorId"]) -def _validation_errors( +class UpdateDetectorAppliedDetails(TypedDict): + detectorId: str + detectorName: str + fields: list[str] + + +def _error_messages(errors: Mapping[str, Any] | Sequence[Any] | str) -> list[str]: + """Flatten the detector validator's nested errors.""" + + if isinstance(errors, str): + return [errors] + values = errors.values() if isinstance(errors, Mapping) else errors + return [message for value in values for message in _error_messages(value)] + + +def _validation_messages( validator: MetricIssueDetectorValidator, detector: Detector, snuba_query: SnubaQuery, environment: SettingValue, -) -> dict[str, Any] | list[Any] | None: +) -> list[str]: # The validator creates environments it doesn't know, like the monitor form does. A # suggestion shouldn't add one, so an unknown name is just invalid. if ( @@ -359,14 +373,14 @@ def _validation_errors( projects=detector.linked_project, ).exists() ): - return {"environment": [f"The project has no {environment} environment."]} + return [f"The project has no {environment} environment."] if not validator.is_valid(): - return validator.errors + return _error_messages(validator.errors) # The validator only checks this when it saves, so a dry run has to ask for it. data_sources = validator.validated_data.get("data_sources") if data_sources and validator.is_editing_transaction_dataset(snuba_query, data_sources[0]): - return {"dataSources": [TRANSACTION_UPDATES_DISABLED]} - return None + return [TRANSACTION_UPDATES_DISABLED] + return [] class UpdateDetectorKind(ActionKind): @@ -419,31 +433,24 @@ def cannot_apply_reasons( def check(self, context: ApplyContext) -> None: self._apply(context, dry_run=True) - def apply(self, context: ApplyContext) -> dict[str, object]: + def apply(self, context: ApplyContext) -> UpdateDetectorAppliedDetails: return self._apply(context, dry_run=False) - def _apply(self, context: ApplyContext, *, dry_run: bool) -> dict[str, object]: + def _apply(self, context: ApplyContext, *, dry_run: bool) -> UpdateDetectorAppliedDetails: action = context.action_request.action_payload assert action is not None - detector_id = int(action["detectorId"]) - detector = target_detectors(context.investigation.organization_id, [detector_id]).get( - detector_id - ) - if detector is None: - raise ResourceDoesNotExist + detectors = Detector.objects.select_related("project", "workflow_condition_group") + snuba_queries = SnubaQuery.objects.all() if not dry_run: # Lock the monitor until we save, so an edit between the stale check and the save # isn't lost. - detector = ( - Detector.objects.select_for_update(of=("self",)) - .select_related("project", "workflow_condition_group") - .get(id=detector.id) - ) + detectors = detectors.select_for_update(of=("self",)) + snuba_queries = snuba_queries.select_for_update() + detector = detectors.get(id=int(action["detectorId"])) snuba_query = fetch_snuba_query(detector) - if snuba_query is None: - raise ResourceDoesNotExist + assert snuba_query is not None if not dry_run: - snuba_query = SnubaQuery.objects.select_for_update().get(id=snuba_query.id) + snuba_query = snuba_queries.get(id=snuba_query.id) state = DetectorState.load(detector, snuba_query) current = state.values() @@ -453,7 +460,7 @@ def _apply(self, context: ApplyContext, *, dry_run: bool) -> dict[str, object]: if current.get(key) != value } if moved: - raise ActionStale(dict(moved)) + raise ActionStale(moved) changes = action["changes"] validator = get_detector_validator( @@ -465,22 +472,20 @@ def _apply(self, context: ApplyContext, *, dry_run: bool) -> dict[str, object]: data=state.update(changes), ) assert isinstance(validator, MetricIssueDetectorValidator) - errors = _validation_errors(validator, detector, snuba_query, changes.get("environment")) - if errors is not None: - raise ActionInvalid(errors) + messages = _validation_messages( + validator, detector, snuba_query, changes.get("environment") + ) + if messages: + raise ActionInvalid(messages) if not dry_run: try: # If the save fails partway, undo the monitor changes it made. with transaction.atomic(router.db_for_write(Detector)): validator.save() except serializers.ValidationError as error: - raise ActionInvalid( - convert_dict_key_case( - serializers.as_serializer_error(error), snake_to_camel_case - ) - ) + raise ActionInvalid(_error_messages(error.detail)) return { "detectorId": str(detector.id), "detectorName": detector.name, - "fields": [key for key in DetectorChangesSerializer().fields if key in changes], + "fields": [key for key in DetectorChangesSerializer().fields.keys() if key in changes], } diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index 3804a8e55451..a0d8b7c13380 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -3,7 +3,6 @@ from collections.abc import Sequence from datetime import timedelta from typing import Any -from unittest import mock from django.test import override_settings from django.urls import reverse @@ -638,17 +637,13 @@ def test_dynamic_monitors_are_not_changed(self) -> None: self.set_detector(config={"detection_type": "dynamic"}) action_request = self.suggest() - with mock.patch( - "sentry.incidents.metric_issue_detector.update_detector_data" - ) as update_detector_data: - response = self.apply(action_request) + response = self.apply(action_request) assert response.status_code == 409 assert response.data == { "detail": "This action can't be applied.", "code": "dynamic_detector", } - update_detector_data.assert_not_called() action_request.refresh_from_db() assert action_request.state == InvestigationActionRequestState.SUGGESTED assert self.snuba_query().time_window == 300 From 927362ad988330b64e417f0eedb2e60629b0f326 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 22:16:47 -0700 Subject: [PATCH 4/5] fix(investigations): Scope the detector fetch to the organization Look up the monitor in the investigation's organization and return 404 when it's gone or has no query, instead of a 500. --- .../services/action_kinds/update_detector.py | 23 ++++++++++++++----- ...tion_investigation_action_request_apply.py | 10 ++++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index d16ff34c557e..6019d8ea6ebb 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -55,6 +55,7 @@ ) from sentry.workflow_engine.endpoints.validators.utils import can_edit_detector_with_access from sentry.workflow_engine.models import Detector +from sentry.workflow_engine.models.detector import DetectorQuerySet from sentry.workflow_engine.types import DetectorPriorityLevel UPDATE_DETECTOR_ACTION = "update_detector" @@ -296,11 +297,17 @@ def target_detectors(organization_id: int, detector_ids: Collection[int]) -> dic return { detector.id: detector - for detector in Detector.objects.by_organization(organization_id) + for detector in _metric_detectors(organization_id).filter(id__in=detector_ids) + } + + +def _metric_detectors(organization_id: int) -> DetectorQuerySet: + return ( + Detector.objects.by_organization(organization_id) .with_type_filters() - .filter(id__in=detector_ids, type=MetricIssue.slug) + .filter(type=MetricIssue.slug) .select_related("project", "workflow_condition_group") - } + ) def source_detector_id(source: Mapping[str, Any]) -> str | None: @@ -439,16 +446,20 @@ def apply(self, context: ApplyContext) -> UpdateDetectorAppliedDetails: def _apply(self, context: ApplyContext, *, dry_run: bool) -> UpdateDetectorAppliedDetails: action = context.action_request.action_payload assert action is not None - detectors = Detector.objects.select_related("project", "workflow_condition_group") + detectors = _metric_detectors(context.investigation.organization_id) snuba_queries = SnubaQuery.objects.all() if not dry_run: # Lock the monitor until we save, so an edit between the stale check and the save # isn't lost. detectors = detectors.select_for_update(of=("self",)) snuba_queries = snuba_queries.select_for_update() - detector = detectors.get(id=int(action["detectorId"])) + try: + detector = detectors.get(id=int(action["detectorId"])) + except Detector.DoesNotExist: + raise ResourceDoesNotExist snuba_query = fetch_snuba_query(detector) - assert snuba_query is not None + if snuba_query is None: + raise ResourceDoesNotExist if not dry_run: snuba_query = snuba_queries.get(id=snuba_query.id) diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index a0d8b7c13380..8b4f17d83ee3 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -391,6 +391,16 @@ def test_a_deleted_monitor_is_not_found(self) -> None: assert response.status_code == 409 assert response.data["code"] == "detector_not_found" + def test_a_monitor_without_a_query_is_not_found(self) -> None: + action_request = self.suggest() + self.detector.data_sources.get().delete() + + response = self.apply(action_request) + + assert response.status_code == 404 + action_request.refresh_from_db() + assert action_request.state == InvestigationActionRequestState.SUGGESTED + def test_another_organizations_monitor_is_not_found(self) -> None: foreign = self.create_detector( project=self.create_project(organization=self.create_organization()), From 22c9a534a66ae111f8eef6963af36a06c06639a3 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Mon, 26 Oct 2026 07:38:28 -0700 Subject: [PATCH 5/5] fix(investigations): Return detector_not_found when the monitor or query is gone Apply returned a plain 404 here, while every other refusal has a code the UI switches on. --- .../services/action_kinds/update_detector.py | 11 +++++++++-- ...organization_investigation_action_request_apply.py | 6 +++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index 6019d8ea6ebb..a9cebb56f1b2 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -38,6 +38,7 @@ ActionKind, ActionStale, ApplyContext, + ApplyFailed, CannotApplyReason, SettingValue, ) @@ -343,6 +344,12 @@ def detector_cannot_apply_reason( return None +def _detector_not_found() -> ApplyFailed: + return ApplyFailed( + {"detail": "This monitor wasn't found.", "code": CannotApplyReason.DETECTOR_NOT_FOUND} + ) + + def _action_detector_id(action_request: InvestigationActionRequest) -> int: # The payload was validated when it arrived, and advice never reaches the kind. assert action_request.action_payload is not None @@ -456,10 +463,10 @@ def _apply(self, context: ApplyContext, *, dry_run: bool) -> UpdateDetectorAppli try: detector = detectors.get(id=int(action["detectorId"])) except Detector.DoesNotExist: - raise ResourceDoesNotExist + raise _detector_not_found() snuba_query = fetch_snuba_query(detector) if snuba_query is None: - raise ResourceDoesNotExist + raise _detector_not_found() if not dry_run: snuba_query = snuba_queries.get(id=snuba_query.id) diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py index 8b4f17d83ee3..eda046591cef 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -397,7 +397,11 @@ def test_a_monitor_without_a_query_is_not_found(self) -> None: response = self.apply(action_request) - assert response.status_code == 404 + assert response.status_code == 409 + assert response.data == { + "detail": "This monitor wasn't found.", + "code": "detector_not_found", + } action_request.refresh_from_db() assert action_request.state == InvestigationActionRequestState.SUGGESTED