From ec5f115061f1021d4154bdeb0a7bef0265b31534 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Tue, 8 Oct 2024 17:42:15 -0700 Subject: [PATCH] Update DataPacket to hold data vs being an interface Write tests --- src/sentry/incidents/grouptype.py | 5 +- src/sentry/testutils/factories.py | 6 +- .../workflow_engine/models/data_source.py | 11 +- src/sentry/workflow_engine/models/detector.py | 15 +- .../processors/test_detector.py | 155 ++++++++++++++++++ 5 files changed, 184 insertions(+), 8 deletions(-) create mode 100644 tests/sentry/workflow_engine/processors/test_detector.py diff --git a/src/sentry/incidents/grouptype.py b/src/sentry/incidents/grouptype.py index e05f1b5a2b0d7..7a7d0dc6900d5 100644 --- a/src/sentry/incidents/grouptype.py +++ b/src/sentry/incidents/grouptype.py @@ -4,12 +4,15 @@ from sentry.issues.grouptype import GroupCategory, GroupType from sentry.ratelimits.sliding_windows import Quota from sentry.types.group import PriorityLevel +from sentry.workflow_engine.models import DataPacket from sentry.workflow_engine.models.detector import DetectorEvaluationResult, DetectorHandler # TODO: This will be a stateful detector when we build that abstraction class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]): - def evaluate(self, data_packet: QuerySubscriptionUpdate) -> list[DetectorEvaluationResult]: + def evaluate( + self, data_packet: DataPacket[QuerySubscriptionUpdate] + ) -> list[DetectorEvaluationResult]: # TODO: Implement return [] diff --git a/src/sentry/testutils/factories.py b/src/sentry/testutils/factories.py index 90624c6d0b44b..db8603ec86802 100644 --- a/src/sentry/testutils/factories.py +++ b/src/sentry/testutils/factories.py @@ -2138,7 +2138,11 @@ def create_detector( if name is None: name = petname.generate(2, " ", letters=10).title() return Detector.objects.create( - organization=organization, name=name, owner_user_id=owner_user_id, owner_team=owner_team + organization=organization, + name=name, + owner_user_id=owner_user_id, + owner_team=owner_team, + **kwargs, ) @staticmethod diff --git a/src/sentry/workflow_engine/models/data_source.py b/src/sentry/workflow_engine/models/data_source.py index 4fa93e397afed..13c409865ae57 100644 --- a/src/sentry/workflow_engine/models/data_source.py +++ b/src/sentry/workflow_engine/models/data_source.py @@ -1,4 +1,5 @@ -from typing import Protocol +import dataclasses +from typing import Generic, TypeVar from django.db import models @@ -11,9 +12,13 @@ ) from sentry.workflow_engine.models.data_source_detector import DataSourceDetector +T = TypeVar("T") -class DataPacket(Protocol): - query_id: int + +@dataclasses.dataclass +class DataPacket(Generic[T]): + query_id: str + packet: T @region_silo_model diff --git a/src/sentry/workflow_engine/models/detector.py b/src/sentry/workflow_engine/models/detector.py index cd59cdec5db6a..dcabca1c66f54 100644 --- a/src/sentry/workflow_engine/models/detector.py +++ b/src/sentry/workflow_engine/models/detector.py @@ -13,6 +13,7 @@ from sentry.issues import grouptype from sentry.models.owner_base import OwnerModel from sentry.types.group import PriorityLevel +from sentry.workflow_engine.models import DataPacket if TYPE_CHECKING: from sentry.workflow_engine.models.detector_state import DetectorStatus @@ -57,14 +58,22 @@ def detector_handler(self) -> DetectorHandler | None: if not group_type: logger.error( "No registered grouptype for detector", - extra={"group_type": group_type, "detector_id": self.id}, + extra={ + "group_type": str(group_type), + "detector_id": self.id, + "detector_type": self.type, + }, ) return None if not group_type.detector_handler: logger.error( "Registered grouptype for detector has no detector_handler", - extra={"group_type": group_type, "detector_id": self.id}, + extra={ + "group_type": str(group_type), + "detector_id": self.id, + "detector_type": self.type, + }, ) return None return group_type.detector_handler(self) @@ -100,5 +109,5 @@ def __init__(self, detector: Detector): self.detector = detector @abc.abstractmethod - def evaluate(self, data_packet: T) -> list[DetectorEvaluationResult]: + def evaluate(self, data_packet: DataPacket[T]) -> list[DetectorEvaluationResult]: pass diff --git a/tests/sentry/workflow_engine/processors/test_detector.py b/tests/sentry/workflow_engine/processors/test_detector.py new file mode 100644 index 0000000000000..f835b1d56d65c --- /dev/null +++ b/tests/sentry/workflow_engine/processors/test_detector.py @@ -0,0 +1,155 @@ +from unittest import mock + +from sentry.issues.grouptype import GroupCategory, GroupType +from sentry.types.group import PriorityLevel +from sentry.workflow_engine.models import DataPacket +from sentry.workflow_engine.models.detector import ( + DetectorEvaluationResult, + DetectorHandler, + DetectorStateData, +) +from sentry.workflow_engine.models.detector_state import DetectorStatus +from sentry.workflow_engine.processors.detector import process_detectors +from tests.sentry.issues.test_grouptype import BaseGroupTypeTest + + +class TestProcessDetectors(BaseGroupTypeTest): + def setUp(self): + super().setUp() + + class NoHandlerGroupType(GroupType): + type_id = 1 + slug = "no_handler" + description = "no handler" + category = GroupCategory.METRIC_ALERT.value + + class MockDetectorHandler(DetectorHandler[dict]): + def evaluate(self, data_packet: DataPacket[dict]) -> list[DetectorEvaluationResult]: + return [DetectorEvaluationResult(True, PriorityLevel.HIGH, data_packet)] + + class HandlerGroupType(GroupType): + type_id = 2 + slug = "handler" + description = "handler" + category = GroupCategory.METRIC_ALERT.value + detector_handler = MockDetectorHandler + + class MockDetectorStateHandler(DetectorHandler[dict]): + def evaluate(self, data_packet: DataPacket[dict]) -> list[DetectorEvaluationResult]: + group_keys = data_packet.packet.get("group_keys", [None]) + return [ + DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData( + group_key, + True, + DetectorStatus.OK, + 100, + {}, + ), + ) + for group_key in group_keys + ] + + class HandlerStateGroupType(GroupType): + type_id = 3 + slug = "handler_with_state" + description = "handler with state" + category = GroupCategory.METRIC_ALERT.value + detector_handler = MockDetectorStateHandler + + self.no_handler_type = NoHandlerGroupType + self.handler_type = HandlerGroupType + self.handler_state_type = HandlerStateGroupType + + def build_data_packet(self, **kwargs): + query_id = "1234" + return DataPacket[dict](query_id, {"query_id": query_id, "some": "data", **kwargs}) + + def test(self): + detector = self.create_detector(type=self.handler_type.slug) + data_packet = self.build_data_packet() + results = process_detectors(data_packet, [detector]) + assert results == [ + (detector, [DetectorEvaluationResult(True, PriorityLevel.HIGH, data_packet)]) + ] + + def test_state_results(self): + detector = self.create_detector(type=self.handler_state_type.slug) + data_packet = self.build_data_packet() + results = process_detectors(data_packet, [detector]) + result = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData(None, True, DetectorStatus.OK, 100, {}), + ) + assert results == [ + ( + detector, + [result], + ) + ] + + def test_state_results_multi_group(self): + detector = self.create_detector(type=self.handler_state_type.slug) + data_packet = self.build_data_packet(group_keys=["group_1", "group_2"]) + results = process_detectors(data_packet, [detector]) + result_1 = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData("group_1", True, DetectorStatus.OK, 100, {}), + ) + result_2 = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData("group_2", True, DetectorStatus.OK, 100, {}), + ) + assert results == [ + ( + detector, + [result_1, result_2], + ) + ] + + def test_state_results_multi_group_dupe(self): + detector = self.create_detector(type=self.handler_state_type.slug) + data_packet = self.build_data_packet(group_keys=["dupe", "dupe"]) + with mock.patch("sentry.workflow_engine.processors.detector.logger") as mock_logger: + results = process_detectors(data_packet, [detector]) + assert mock_logger.error.call_args[0][0] == "Duplicate detector state group keys found" + result = DetectorEvaluationResult( + True, + PriorityLevel.HIGH, + data_packet, + DetectorStateData("dupe", True, DetectorStatus.OK, 100, {}), + ) + assert results == [ + ( + detector, + [result, result], + ) + ] + + def test_no_issue_type(self): + detector = self.create_detector(type="invalid 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] == "No registered grouptype for detector" + assert results == [] + + def test_no_handler(self): + 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 == []