-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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.
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
Conversation
| 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, | ||
| ) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a comment about test cleanup but not gonna block on it
* master: (249 commits) feat(source-maps): Do not show pagination together with empty state (#92287) ref(project-creation): Introduce useCreateProjectRules hook (#92186) feat(agent-insights): Handle new keys (#92613) feat(source-maps): Introduce new empty state copies and react-native callout (#92286) ref(issues): Remove project from group activity type (#92600) feat(ourlogs): Use /trace-logs endpoint (#92577) feat(issues): Only update group hasSeen when user is member (#92597) fix(workflow_engine): Graceful Data Condition Eval Handling (#92591) feat(uptime): Implement detector handler (#91107) chore(autofix): Remove logs from response payload (#92589) fix(search): Fix issue with tags name 'constructor' (#92586) fix(autofix): Fix condition for onboarding check (#92584) fix(ourlogs): Return the same format as /events & limit 1000 for trace-logs (#92580) fix(autofix): Fix automation onboarding condition (#92579) feat(explore): Remove group by timestamp from explore (#92546) feat(trace-items): Autocomplete for semver attributes (#92515) feat(processing) Define EventProcessingStore.__all__ (#92555) feat(autofix): Better errored state (#92571) chore(autofix): Seer beta banner copy changes (#92576) feat(crons): Add endpoint to return counts by status (#92574) ...
# Description - Introduce a new pattern for `DataConditionEvaluationException` to allow us to raise exceptions to interrupt the control flow - Override a specific problem in `issue_priority_deescalating_handler` - Includes tests to ensure we are gracefully handling `DataConditionEvaluationException`s
Description
DataConditionEvaluationExceptionto allow us to raise exceptions to interrupt the control flowissue_priority_deescalating_handlerDataConditionEvaluationExceptions