From 06c9e63ac3bdab04129b8a617eecb9df73ed44a0 Mon Sep 17 00:00:00 2001 From: Josh Callender <1569818+saponifi3d@users.noreply.github.com> Date: Fri, 30 May 2025 14:28:34 -0700 Subject: [PATCH] Improve error handling around the no open periods case --- .../issue_priority_deescalating_handler.py | 4 +- .../workflow_engine/models/data_condition.py | 27 ++++++++++-- src/sentry/workflow_engine/types.py | 2 +- .../models/test_data_condition.py | 44 ++++++++++++++++++- 4 files changed, 68 insertions(+), 9 deletions(-) diff --git a/src/sentry/workflow_engine/handlers/condition/issue_priority_deescalating_handler.py b/src/sentry/workflow_engine/handlers/condition/issue_priority_deescalating_handler.py index c469a2f2620304..7b9df2b06602a3 100644 --- a/src/sentry/workflow_engine/handlers/condition/issue_priority_deescalating_handler.py +++ b/src/sentry/workflow_engine/handlers/condition/issue_priority_deescalating_handler.py @@ -2,7 +2,7 @@ from sentry.models.group import GroupStatus from sentry.models.groupopenperiod import get_latest_open_period -from sentry.workflow_engine.models.data_condition import Condition +from sentry.workflow_engine.models.data_condition import Condition, DataConditionEvaluationException from sentry.workflow_engine.registry import condition_handler_registry from sentry.workflow_engine.types import DataConditionHandler, WorkflowEventData @@ -21,7 +21,7 @@ def evaluate_value(event_data: WorkflowEventData, comparison: Any) -> bool: current_priority = group.priority open_period = get_latest_open_period(group) if open_period is None: - raise Exception("No open period found") + raise DataConditionEvaluationException("No open period found") # use this to determine if we've breached the comparison priority before highest_seen_priority = open_period.data.get("highest_seen_priority", current_priority) diff --git a/src/sentry/workflow_engine/models/data_condition.py b/src/sentry/workflow_engine/models/data_condition.py index 8e9e1e2342abcc..258986a85283bc 100644 --- a/src/sentry/workflow_engine/models/data_condition.py +++ b/src/sentry/workflow_engine/models/data_condition.py @@ -10,13 +10,17 @@ from sentry.backup.scopes import RelocationScope from sentry.db.models import DefaultFieldsModel, region_silo_model, sane_repr -from sentry.utils.registry import NoRegistrationExistsError +from sentry.utils import metrics, registry from sentry.workflow_engine.registry import condition_handler_registry from sentry.workflow_engine.types import DataConditionResult, DetectorPriorityLevel logger = logging.getLogger(__name__) +class DataConditionEvaluationException(Exception): + pass + + class Condition(StrEnum): # Base conditions - Most DETECTOR_TRIGGERS will use these EQUAL = "eq" @@ -182,14 +186,29 @@ def evaluate_value(self, value: T) -> DataConditionResult: # Otherwise, we need to get the handler and evaluate the value try: handler = condition_handler_registry.get(condition_type) - except NoRegistrationExistsError: + except registry.NoRegistrationExistsError: logger.exception( "No registration exists for condition", extra={"type": self.type, "id": self.id}, ) return None - result = handler.evaluate_value(value, self.comparison) + try: + result = handler.evaluate_value(value, self.comparison) + except DataConditionEvaluationException as e: + metrics.incr("workflow_engine.data_condition.evaluation_error") + logger.info( + "A known error occurred while evaluating a data condition", + extra={ + "condition_id": self.id, + "type": self.type, + "comparison": self.comparison, + "value": value, + "error": str(e), + }, + ) + return None + return self.get_condition_result() if result else None @@ -205,7 +224,7 @@ def enforce_data_condition_json_schema(data_condition: DataCondition) -> None: try: handler = condition_handler_registry.get(condition_type) - except NoRegistrationExistsError: + except registry.NoRegistrationExistsError: logger.exception( "No registration exists for condition", extra={"type": data_condition.type, "id": data_condition.id}, diff --git a/src/sentry/workflow_engine/types.py b/src/sentry/workflow_engine/types.py index a15c322f120984..9313a0fe0b6bb5 100644 --- a/src/sentry/workflow_engine/types.py +++ b/src/sentry/workflow_engine/types.py @@ -81,7 +81,7 @@ class DataSourceTypeHandler(Generic[T]): @staticmethod def bulk_get_query_object(data_sources) -> dict[int, T | None]: """ - Bulk fetch related data-source models reutrning a dict of the + Bulk fetch related data-source models returning a dict of the `DataSource.id -> T`. """ raise NotImplementedError diff --git a/tests/sentry/workflow_engine/models/test_data_condition.py b/tests/sentry/workflow_engine/models/test_data_condition.py index c921237096c9c1..b9b11818c99232 100644 --- a/tests/sentry/workflow_engine/models/test_data_condition.py +++ b/tests/sentry/workflow_engine/models/test_data_condition.py @@ -3,8 +3,8 @@ import pytest from sentry.testutils.cases import TestCase -from sentry.workflow_engine.models.data_condition import Condition -from sentry.workflow_engine.types import DetectorPriorityLevel +from sentry.workflow_engine.models.data_condition import Condition, DataConditionEvaluationException +from sentry.workflow_engine.types import DataConditionHandler, DetectorPriorityLevel class GetConditionResultTest(TestCase): @@ -67,3 +67,43 @@ def test_condition_result_comparison_fails(self): type=Condition.GREATER, comparison=1.0, condition_result="wrong" ) assert dc.evaluate_value(2) is None + + @mock.patch("sentry.workflow_engine.models.data_condition.condition_handler_registry") + def test_condition_evaluation__data_condition_exception(self, mock_registry): + class DataConditionHandlerMock(DataConditionHandler[int]): + @staticmethod + def evaluate_value(value: int, comparison: int) -> bool: + raise DataConditionEvaluationException("Something went wrong") + + mock_registry.get.return_value = DataConditionHandlerMock() + + dc = self.create_data_condition( + type=Condition.LEVEL, # this will be overridden by the mock, cannot be a operator + comparison=1.0, + condition_result=DetectorPriorityLevel.HIGH, + ) + + with mock.patch("sentry.workflow_engine.models.data_condition.logger.info") as mock_logger: + dc.evaluate_value(2) + assert ( + mock_logger.call_args[0][0] + == "A known error occurred while evaluating a data condition" + ) + + @mock.patch("sentry.workflow_engine.models.data_condition.condition_handler_registry") + def test_condition_evaluation___exception(self, mock_registry): + class DataConditionHandlerMock(DataConditionHandler[int]): + @staticmethod + def evaluate_value(value: int, comparison: int) -> bool: + raise Exception("Something went wrong") + + mock_registry.get.return_value = DataConditionHandlerMock() + + dc = self.create_data_condition( + type=Condition.LEVEL, # this will be overridden by the mock, cannot be a operator + comparison=1.0, + condition_result=DetectorPriorityLevel.HIGH, + ) + + with pytest.raises(Exception): + dc.evaluate_value(2)