feat(workflow_engine): Add Detector Life Cycle Hooks to DetectorSettings

src/sentry/deletions/defaults/detector.py

@@ -27,4 +27,10 @@ def get_child_relations(self, instance: Detector) -> list[BaseRelation]:
                 ModelRelation(DataConditionGroup, {"id": instance.workflow_condition_group.id})
             )
 
+        # When a detector is deleted, we want to invoke the life cycle for deletion
+        # This is helpful when cleaning up related models or billing changes
+        hooks = instance.settings.hooks
+        if hooks and hooks.on_delete:
+            hooks.on_delete(instance.id)
+
         return model_relations

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):

src/sentry/workflow_engine/endpoints/validators/base/detector.py

@@ -141,6 +141,12 @@ def update(self, instance: Detector, validated_data: dict[str, Any]):
             event=audit_log.get_event_id("DETECTOR_EDIT"),
             data=instance.get_audit_log_data(),
         )
+
+        # This hook is used for _after_ a detector has been updated
+        hooks = instance.settings.hooks
+        if hooks and hooks.on_update:
+            hooks.on_update(instance)
+
         return instance
 
     def _create_data_source(self, validated_data_source, detector: Detector):
@@ -217,4 +223,10 @@ def create(self, validated_data):
                 event=audit_log.get_event_id("DETECTOR_ADD"),
                 data=detector.get_audit_log_data(),
             )
+
+            hooks = detector.settings.hooks
+
+            if hooks and hooks.on_create:
+                hooks.on_create(detector)
+
         return detector

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}

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,16 @@ class SnubaQueryDataSourceType(TypedDict):
     event_types: list[SnubaQueryEventType]
 
 
+@dataclass(frozen=True)
+class DetectorLifeCycleHooks:
+    on_create: Callable[[Detector], None] | None  # invoked on validator.save()
+    on_delete: Callable[[int], None] | None  # invoked when the `deletion` code is invoked
+    on_update: Callable[[Detector], None] | None  # invoked on the validator.update()
+
+
 @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)

tests/sentry/uptime/endpoints/test_detector.py

@@ -13,12 +13,14 @@ def _get_valid_data(project_id, environment_name, **overrides):
         "projectId": project_id,
         "name": "Test Uptime Detector",
         "type": UptimeDomainCheckFailure.slug,
-        "dataSource": {
-            "timeout_ms": 30000,
-            "name": "Test Uptime Detector",
-            "url": "https://www.google.com",
-            "interval_seconds": UptimeSubscription.IntervalSeconds.ONE_MINUTE,
-        },
+        "dataSources": [
+            {
+                "timeout_ms": 30000,
+                "name": "Test Uptime Detector",
+                "url": "https://www.google.com",
+                "interval_seconds": UptimeSubscription.IntervalSeconds.ONE_MINUTE,
+            }
+        ],
         "conditionGroup": {
             "logicType": "any",
             "conditions": [

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()

tests/sentry/workflow_engine/detectors/test_life_cycle_hooks.py

@@ -0,0 +1,119 @@
+from unittest.mock import Mock, PropertyMock, patch
+
+from django.test.client import RequestFactory
+
+from sentry.deletions.tasks.scheduled import run_scheduled_deletions
+from sentry.issues.grouptype import PerformanceSlowDBQueryGroupType
+from sentry.testutils.cases import TestCase
+from sentry.testutils.hybrid_cloud import HybridCloudTestMixin
+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(
+                on_delete=Mock(),
+                on_create=Mock(),
+                on_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.on_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.on_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")
+
+        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"]
+            detector.settings.hooks.on_update.assert_called_with(detector)
+
+
+class TestDetectorLifeCycleDeletionHooks(TestCase, HybridCloudTestMixin):
+    def setUp(self) -> None:
+        super().setUp()
+
+        self.detector = self.create_detector(name="Test Detector")
+        self.detector_settings = DetectorSettings(
+            hooks=DetectorLifeCycleHooks(
+                on_delete=Mock(),
+                on_create=Mock(),
+                on_update=Mock(),
+            )
+        )
+
+    def test_delete(self) -> None:
+        detector_id = self.detector.id
+        with patch.object(Detector, "settings", new_callable=PropertyMock) as mock_settings:
+            mock_settings.return_value = self.detector_settings
+            self.ScheduledDeletion.schedule(instance=self.detector, days=0)
+
+            with self.tasks():
+                run_scheduled_deletions()
+
+            # The deletion worked
+            assert not Detector.objects.filter(id=detector_id).exists()
+            self.detector_settings.hooks.on_delete.assert_called_with(detector_id)  # type: ignore[union-attr]
+
+    def test_delete__no_hooks(self) -> None:
+        with patch.object(Detector, "settings", new_callable=PropertyMock) as mock_settings:
+            mock_settings.return_value = DetectorSettings()
+            self.ScheduledDeletion.schedule(instance=self.detector, days=0)
+
+            with self.tasks():
+                run_scheduled_deletions()
+
+            # The deletion still works
+            assert not Detector.objects.filter(id=self.detector.id).exists()

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

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

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