Skip to content

fix(workflow_engine): Graceful Data Condition Eval Handling #92591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 30, 2025
Merged

fix(workflow_engine): Graceful Data Condition Eval Handling #92591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/sentry/workflow_engine/handlers/condition/issue_priority_deescalating_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)

Expand Down
27 changes: 23 additions & 4 deletions src/sentry/workflow_engine/models/data_condition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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


Expand All @@ -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},
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/workflow_engine/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
44 changes: 42 additions & 2 deletions tests/sentry/workflow_engine/models/test_data_condition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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,
)
Comment on lines +73 to +84
Copy link
Member

@ceorourke ceorourke May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to put this in setup since it's duplicated in the next test? I know the mock handler might be tricky but at least the data condition should be fine

Copy link
Contributor Author

@saponifi3d saponifi3d May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was able to get it working like this:

# in workflow_engine/test_base.py
def get_condition_handler_mock(
    evaluate_value: Callable[[Any, Any], DataConditionResult]
) -> DataConditionHandler:
    class MockConditionHandler(DataConditionHandler):
        @staticmethod
        def evaluate_value(value: Any, comparison: Any) -> DataConditionResult:
            return evaluate_value(value, comparison)

    return MockConditionHandler()

# in a test_file.py:
class TestExample(TestCase):
    @mock.patch("sentry.workflow_engine.models.data_condition.condition_handler_registry")
    def test(self, mock_registry):
        def evaluate_value(value: int, comparison: int) -> bool:
            raise DataConditionEvaluationException("Something went wrong")

        mock_condition = get_condition_handler_mock(evaluate_value)
        mock_registry.get.return_value = mock_condition
        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"
            )

it feels like that's not sharing much though, especially since we'd have to pass in evaluate value as well.

mind if i leave it as is for this pr? and then do a follow up PR with a base tests or mixin to make this more generic.


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