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/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 b254427b41b4..a9cebb56f1b2 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -6,6 +6,7 @@ from typing import Any, Literal, TypedDict, cast from django.contrib.auth.models import AnonymousUser +from django.db import router, transaction from rest_framework import serializers from sentry.api.exceptions import ResourceDoesNotExist @@ -14,6 +15,11 @@ 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, @@ -28,10 +34,15 @@ InvestigationOrchestrationRun, ) from sentry.investigations.services.action_kinds.base import ( + ActionInvalid, ActionKind, + ActionStale, + ApplyContext, + ApplyFailed, CannotApplyReason, SettingValue, ) +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,11 +50,13 @@ 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, ) 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" @@ -83,8 +96,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 @@ -102,7 +117,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 = { @@ -123,7 +138,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}, } @@ -283,11 +298,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: @@ -323,12 +344,59 @@ 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 return int(action_request.action_payload["detectorId"]) +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, +) -> 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 ( + environment is not None + and not Environment.objects.filter( + organization_id=detector.linked_project.organization_id, + name=environment, + projects=detector.linked_project, + ).exists() + ): + return [f"The project has no {environment} environment."] + if not validator.is_valid(): + 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 [TRANSACTION_UPDATES_DISABLED] + return [] + + class UpdateDetectorKind(ActionKind): kind = UPDATE_DETECTOR_ACTION payload_serializer = UpdateDetectorActionSerializer @@ -375,3 +443,67 @@ def cannot_apply_reasons( ) reasons[item.id] = detector_reasons[key] return reasons + + def check(self, context: ApplyContext) -> None: + self._apply(context, dry_run=True) + + def apply(self, context: ApplyContext) -> UpdateDetectorAppliedDetails: + return self._apply(context, dry_run=False) + + def _apply(self, context: ApplyContext, *, dry_run: bool) -> UpdateDetectorAppliedDetails: + action = context.action_request.action_payload + assert action is not None + 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() + try: + detector = detectors.get(id=int(action["detectorId"])) + except Detector.DoesNotExist: + raise _detector_not_found() + snuba_query = fetch_snuba_query(detector) + if snuba_query is None: + raise _detector_not_found() + if not dry_run: + snuba_query = snuba_queries.get(id=snuba_query.id) + + 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: + raise ActionStale(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) + 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(_error_messages(error.detail)) + return { + "detectorId": str(detector.id), + "detectorName": detector.name, + "fields": [key for key in DetectorChangesSerializer().fields.keys() if key in changes], + } 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..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 @@ -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,167 @@ 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}) + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data == { + "detail": "This changed since Seer suggested it.", + "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_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 == 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 + + 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_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)} + ) + 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 change wasn't valid." + assert response.data["code"] == "validation_failed" + 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 + 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 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 + + 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 +567,233 @@ 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() + + response = self.apply(action_request) + + assert response.status_code == 409 + assert response.data == { + "detail": "This action can't be applied.", + "code": "dynamic_detector", + } + 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" + 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.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: + 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 + + 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): 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}, + )