diff --git a/src/sentry/testutils/silo.py b/src/sentry/testutils/silo.py index 9746ab11a9c61a..49d96572c09bbd 100644 --- a/src/sentry/testutils/silo.py +++ b/src/sentry/testutils/silo.py @@ -574,12 +574,14 @@ def validate_no_cross_silo_deletions( ) -> None: from sentry import deletions from sentry.deletions.base import BaseDeletionTask + from sentry.incidents.grouptype import MetricIssue from sentry.incidents.utils.types import DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION - from sentry.workflow_engine.models.data_source import DataSource + from sentry.workflow_engine.models import DataSource, Detector # hack for datasource registry, needs type instantiation_params: dict[type[Model], dict[str, str]] = { - DataSource: {"type": DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION} + DataSource: {"type": DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION}, + Detector: {"type": MetricIssue.slug}, } for model_class in iter_models(app_name): diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_details.py b/src/sentry/workflow_engine/endpoints/organization_detector_details.py index ac70636f101029..fb62860cf6f7bd 100644 --- a/src/sentry/workflow_engine/endpoints/organization_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/organization_detector_details.py @@ -37,6 +37,7 @@ ) from sentry.workflow_engine.endpoints.validators.utils import get_unknown_detector_type_error from sentry.workflow_engine.models import Detector +from sentry.workflow_engine.types import DetectorLifeCycleHooks def get_detector_validator( @@ -204,6 +205,8 @@ def delete(self, request: Request, organization: Organization, detector: Detecto RegionScheduledDeletion.schedule(detector, days=0, actor=request.user) detector.update(status=ObjectStatus.PENDING_DELETION) + DetectorLifeCycleHooks.on_pending_delete(detector) + if detector.type == MetricIssue.slug: schedule_update_project_config(detector) diff --git a/src/sentry/workflow_engine/endpoints/validators/base/detector.py b/src/sentry/workflow_engine/endpoints/validators/base/detector.py index 41ad262d86883b..5711552d301f97 100644 --- a/src/sentry/workflow_engine/endpoints/validators/base/detector.py +++ b/src/sentry/workflow_engine/endpoints/validators/base/detector.py @@ -28,7 +28,7 @@ ) from sentry.workflow_engine.models.data_condition import DataCondition from sentry.workflow_engine.models.detector import enforce_config_schema -from sentry.workflow_engine.types import DataConditionType +from sentry.workflow_engine.types import DataConditionType, DetectorLifeCycleHooks @dataclass(frozen=True) @@ -135,6 +135,8 @@ def update(self, instance: Detector, validated_data: dict[str, Any]): event=audit_log.get_event_id("DETECTOR_EDIT"), data=instance.get_audit_log_data(), ) + + DetectorLifeCycleHooks.on_after_update(instance) return instance def _create_data_source(self, validated_data_source, detector: Detector): @@ -207,4 +209,7 @@ def create(self, validated_data): event=audit_log.get_event_id("DETECTOR_ADD"), data=detector.get_audit_log_data(), ) + + DetectorLifeCycleHooks.on_after_create(detector) + return detector diff --git a/src/sentry/workflow_engine/models/detector.py b/src/sentry/workflow_engine/models/detector.py index 2a67214fc7c0d2..3ffefe92f3a033 100644 --- a/src/sentry/workflow_engine/models/detector.py +++ b/src/sentry/workflow_engine/models/detector.py @@ -23,6 +23,7 @@ from sentry.models.owner_base import OwnerModel from sentry.utils.cache import cache from sentry.workflow_engine.models import DataCondition +from sentry.workflow_engine.types import DetectorSettings from .json_config import JSONConfigBase @@ -111,23 +112,15 @@ def get_error_detector_for_project(cls, project_id: int) -> Detector: def group_type(self) -> builtins.type[GroupType]: group_type = grouptype.registry.get_by_slug(self.type) if not group_type: - raise ValueError(f"Group type {self.type} not registered") + raise ValueError(f"Group type '{self.type}' not registered") + return group_type @property def detector_handler(self) -> DetectorHandler | None: group_type = self.group_type - if not group_type: - logger.error( - "No registered grouptype for detector", - extra={ - "detector_id": self.id, - "detector_type": self.type, - }, - ) - return None - if not group_type.detector_settings or not group_type.detector_settings.handler: + if self.settings.handler is None: logger.error( "Registered grouptype for detector has no detector_handler", extra={ @@ -137,7 +130,16 @@ def detector_handler(self) -> DetectorHandler | None: }, ) return None - return group_type.detector_settings.handler(self) + return self.settings.handler(self) + + @property + def settings(self) -> DetectorSettings: + settings = self.group_type.detector_settings + + if settings is None: + raise ValueError("Registered grouptype has no detector settings") + + return settings def get_audit_log_data(self) -> dict[str, Any]: return {"name": self.name} diff --git a/src/sentry/workflow_engine/types.py b/src/sentry/workflow_engine/types.py index 74ad05008dbd55..a5967b6530e84f 100644 --- a/src/sentry/workflow_engine/types.py +++ b/src/sentry/workflow_engine/types.py @@ -1,6 +1,7 @@ from __future__ import annotations from abc import ABC, abstractmethod +from collections.abc import Callable from dataclasses import dataclass, field from enum import IntEnum, StrEnum from typing import TYPE_CHECKING, Any, ClassVar, Generic, TypedDict, TypeVar @@ -182,8 +183,54 @@ class SnubaQueryDataSourceType(TypedDict): event_types: list[SnubaQueryEventType] +type DetectorLifeCycleHook = Callable[[Detector], None] + + +@dataclass(frozen=True) +class DetectorLifeCycleHooks: + """ + The DetectorLifeCycleHooks allow for hooks to be added into different areas of the API. + These hooks are only invoked through the API, for changes that might be happening outside of the API, + it's recommended to use a signal to capture the model updates. + + Args: + after_create: + This method is used as a callback after a detector is created. The first argument is the detector that was created. + + before_delete: + This method is used in the deletion API for a detector, the callback will be invoked with the id for the detector + that is deleted. + + after_update: + This method is used to access when a detector is updated in the API. + """ + + after_create: DetectorLifeCycleHook | None = None + pending_delete: DetectorLifeCycleHook | None = None + after_update: DetectorLifeCycleHook | None = None + + @staticmethod + def on_after_create(detector: Detector): + hooks = detector.settings.hooks + if hooks and hooks.after_create: + hooks.after_create(detector) + + @staticmethod + def on_after_update(detector: Detector): + hooks = detector.settings.hooks + if hooks and hooks.after_update: + hooks.after_update(detector) + + @staticmethod + def on_pending_delete(detector: Detector): + hooks = detector.settings.hooks + if hooks and hooks.pending_delete: + hooks.pending_delete(detector) + + @dataclass(frozen=True) class DetectorSettings: + hooks: DetectorLifeCycleHooks | None = None handler: type[DetectorHandler] | None = None validator: type[BaseDetectorTypeValidator] | None = None config_schema: dict[str, Any] = field(default_factory=dict) diff --git a/tests/sentry/workflow_engine/detectors/test_error_detector.py b/tests/sentry/workflow_engine/detectors/test_error_detector.py index 36bc1b2a318bd8..1a42e688ed6d42 100644 --- a/tests/sentry/workflow_engine/detectors/test_error_detector.py +++ b/tests/sentry/workflow_engine/detectors/test_error_detector.py @@ -9,6 +9,7 @@ from sentry.workflow_engine.models.detector import Detector +# TODO - This should probably live in the same module the detector does class TestErrorDetectorValidator(TestCase): def setUp(self) -> None: super().setUp() diff --git a/tests/sentry/workflow_engine/detectors/test_life_cycle_hooks.py b/tests/sentry/workflow_engine/detectors/test_life_cycle_hooks.py new file mode 100644 index 00000000000000..b3795479dcc3c8 --- /dev/null +++ b/tests/sentry/workflow_engine/detectors/test_life_cycle_hooks.py @@ -0,0 +1,77 @@ +from unittest.mock import Mock, PropertyMock, patch + +from django.test.client import RequestFactory + +from sentry.issues.grouptype import PerformanceSlowDBQueryGroupType +from sentry.testutils.cases import TestCase +from sentry.workflow_engine.endpoints.validators.base.detector import BaseDetectorTypeValidator +from sentry.workflow_engine.models import Detector +from sentry.workflow_engine.types import DetectorLifeCycleHooks, DetectorSettings + + +class TestDetectorLifeCycleValidatorHooks(TestCase): + def setUp(self) -> None: + super().setUp() + + self.login_as(user=self.user) + self.url = f"/api/0/organizations/{self.organization.slug}/monitors" + self.request = RequestFactory().post(self.url) + self.request.user = self.user + + self.context = { + "organization": self.organization, + "project": self.project, + "request": self.request, + } + + self.valid_data = { + "name": "LifeCycle Test Detector", + # Just using a random type, this will have mocked info + "type": PerformanceSlowDBQueryGroupType, + } + + self.detector_settings = DetectorSettings( + hooks=DetectorLifeCycleHooks( + after_create=Mock(), + after_update=Mock(), + ) + ) + + def test_create(self) -> None: + validator = BaseDetectorTypeValidator(self.valid_data, context=self.context) + + with patch.object(Detector, "settings", new_callable=PropertyMock) as mock_settings: + mock_settings.return_value = self.detector_settings + detector = validator.create(self.valid_data) + detector.settings.hooks.after_create.assert_called_with(detector) + + def test_create__no_hooks(self) -> None: + validator = BaseDetectorTypeValidator(self.valid_data, context=self.context) + self.detector_settings = DetectorSettings() + + with patch.object(Detector, "settings", new_callable=PropertyMock) as mock_settings: + mock_settings.return_value = self.detector_settings + detector = validator.create(self.valid_data) + assert detector + + def test_update(self) -> None: + validator = BaseDetectorTypeValidator(self.valid_data, context=self.context) + detector = self.create_detector(name="Example") + + with patch.object(Detector, "settings", new_callable=PropertyMock) as mock_settings: + mock_settings.return_value = self.detector_settings + detector = validator.update(detector, self.valid_data) + + # Ensure update happened, and hook was invoked + assert detector.name == self.valid_data["name"] + detector.settings.hooks.after_update.assert_called_with(detector) + + def test_update__no_hooks(self) -> None: + validator = BaseDetectorTypeValidator(self.valid_data, context=self.context) + self.detector = self.create_detector(name="Example") + self.detector_settings = DetectorSettings() + + with patch.object(Detector, "settings", new_callable=PropertyMock) as mock_settings: + mock_settings.return_value = self.detector_settings + detector = validator.update(self.detector, self.valid_data) + assert detector.name == self.valid_data["name"] diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py index 8675b13cb7e497..44aabba5b12ee4 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py @@ -31,7 +31,11 @@ ) from sentry.workflow_engine.models.data_condition import Condition from sentry.workflow_engine.models.detector_workflow import DetectorWorkflow -from sentry.workflow_engine.types import DetectorPriorityLevel +from sentry.workflow_engine.types import ( + DetectorLifeCycleHooks, + DetectorPriorityLevel, + DetectorSettings, +) pytestmark = [pytest.mark.sentry_metrics, requires_snuba, requires_kafka] @@ -726,3 +730,29 @@ def test_error_group_type(self) -> None: event=audit_log.get_event_id("DETECTOR_REMOVE"), actor=self.user, ).exists() + + def test_detector_life_cycle_delete_hook(self) -> None: + detector_settings = DetectorSettings( + hooks=DetectorLifeCycleHooks(pending_delete=mock.Mock()) + ) + + with mock.patch.object( + Detector, "settings", new_callable=mock.PropertyMock + ) as mock_settings: + mock_settings.return_value = detector_settings + + with outbox_runner(): + self.get_success_response(self.organization.slug, self.detector.id) + + detector_settings.hooks.pending_delete.assert_called_with(self.detector) # type: ignore[union-attr] + + def test_detector_life_cycle__no_delete_hook(self) -> None: + detector_settings = DetectorSettings(hooks=DetectorLifeCycleHooks()) + + with mock.patch.object( + Detector, "settings", new_callable=mock.PropertyMock + ) as mock_settings: + mock_settings.return_value = detector_settings + + with outbox_runner(): + self.get_success_response(self.organization.slug, self.detector.id) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py index cdc0965432d980..da5817414620b2 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py @@ -72,6 +72,7 @@ def create_occurrence( {}, ) + # TODO - each of these types should be broken out into their individual modules @dataclass(frozen=True) class TestMetricGroupType(GroupType): type_id = 1 diff --git a/tests/sentry/workflow_engine/models/test_detector.py b/tests/sentry/workflow_engine/models/test_detector.py index 638442a9d1ee42..9f3a7ddf2ef791 100644 --- a/tests/sentry/workflow_engine/models/test_detector.py +++ b/tests/sentry/workflow_engine/models/test_detector.py @@ -157,6 +157,19 @@ def test_get_error_detector_for_project__cache_hit(self) -> None: ) mock_cache_get.assert_called_once_with(expected_cache_key) + def test_settings(self) -> None: + detector = self.create_detector() + assert detector.settings + + def test_settings__no_settings__invaild_settings(self) -> None: + # This is an issue type w/o a detector association + detector = self.create_detector( + type="profile_json_decode_main_thread", name="Invalid Detector" + ) + + with pytest.raises(ValueError, match="Registered grouptype has no detector settings"): + assert detector.settings + def test_get_detector_project_type_cache_key() -> None: project_id = 123 diff --git a/tests/sentry/workflow_engine/processors/test_detector.py b/tests/sentry/workflow_engine/processors/test_detector.py index f3448ca9d6a3b3..5714f39a131ca7 100644 --- a/tests/sentry/workflow_engine/processors/test_detector.py +++ b/tests/sentry/workflow_engine/processors/test_detector.py @@ -199,31 +199,18 @@ def test_state_results_multi_group(self, mock_produce_occurrence_to_kafka: Magic any_order=True, ) - def test_no_issue_type(self) -> None: - detector = self.create_detector(type=self.handler_state_type.slug) - data_packet = self.build_data_packet() - with ( - mock.patch("sentry.workflow_engine.models.detector.logger") as mock_logger, - mock.patch( - "sentry.workflow_engine.models.Detector.group_type", - return_value=None, - new_callable=mock.PropertyMock, - ), - ): - results = process_detectors(data_packet, [detector]) - assert mock_logger.error.call_args[0][0] == "No registered grouptype for detector" - assert results == [] - def test_no_handler(self) -> None: detector = self.create_detector(type=self.no_handler_type.slug) data_packet = self.build_data_packet() with mock.patch("sentry.workflow_engine.models.detector.logger") as mock_logger: - results = process_detectors(data_packet, [detector]) - assert ( - mock_logger.error.call_args[0][0] - == "Registered grouptype for detector has no detector_handler" - ) - assert results == [] + with pytest.raises(ValueError): + results = process_detectors(data_packet, [detector]) + assert ( + mock_logger.error.call_args[0][0] + == "Registered grouptype for detector has no detector_handler" + ) + + assert results == [] def test_sending_metric_before_evaluating(self) -> None: detector = self.create_detector(type=self.handler_type.slug) @@ -334,11 +321,13 @@ def test_doesnt_send_metric(self) -> None: data_packet = self.build_data_packet() with mock.patch("sentry.utils.metrics.incr") as mock_incr: - process_detectors(data_packet, [detector]) - calls = mock_incr.call_args_list - # We can have background threads emitting metrics as tasks are scheduled - filtered_calls = list(filter(lambda c: "taskworker" not in c.args[0], calls)) - assert len(filtered_calls) == 0 + with pytest.raises(ValueError): + process_detectors(data_packet, [detector]) + + calls = mock_incr.call_args_list + # We can have background threads emitting metrics as tasks are scheduled + filtered_calls = list(filter(lambda c: "taskworker" not in c.args[0], calls)) + assert len(filtered_calls) == 0 @django_db_all