diff --git a/src/sentry/grouping/grouptype.py b/src/sentry/grouping/grouptype.py index 17f44f67143728..4a95e537d0393e 100644 --- a/src/sentry/grouping/grouptype.py +++ b/src/sentry/grouping/grouptype.py @@ -5,23 +5,20 @@ from sentry.models.group import DEFAULT_TYPE_ID from sentry.types.group import PriorityLevel from sentry.workflow_engine.endpoints.validators.error_detector import ErrorDetectorValidator -from sentry.workflow_engine.handlers.detector.base import DetectorHandler -from sentry.workflow_engine.models.data_source import DataPacket -from sentry.workflow_engine.types import ( - DetectorEvaluationResult, - DetectorGroupKey, - DetectorSettings, +from sentry.workflow_engine.handlers.detector.base import ( + DetectorHandler, + GroupedDetectorEvaluationResult, ) +from sentry.workflow_engine.models.data_source import DataPacket +from sentry.workflow_engine.types import DetectorSettings T = TypeVar("T") class ErrorDetectorHandler(DetectorHandler): - def evaluate( - self, data_packet: DataPacket[T] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: + def evaluate_impl(self, data_packet: DataPacket[T]) -> GroupedDetectorEvaluationResult: # placeholder - return {} + return GroupedDetectorEvaluationResult(result={}, tainted=False) @dataclass(frozen=True) diff --git a/src/sentry/workflow_engine/handlers/detector/__init__.py b/src/sentry/workflow_engine/handlers/detector/__init__.py index 5e8fba3a7867bd..04d70f4a9e5e5f 100644 --- a/src/sentry/workflow_engine/handlers/detector/__init__.py +++ b/src/sentry/workflow_engine/handlers/detector/__init__.py @@ -4,8 +4,15 @@ "DetectorHandler", "DetectorOccurrence", "DetectorStateData", + "GroupedDetectorEvaluationResult", "StatefulDetectorHandler", ] -from .base import DataPacketEvaluationType, DataPacketType, DetectorHandler, DetectorOccurrence +from .base import ( + DataPacketEvaluationType, + DataPacketType, + DetectorHandler, + DetectorOccurrence, + GroupedDetectorEvaluationResult, +) from .stateful import DetectorStateData, StatefulDetectorHandler diff --git a/src/sentry/workflow_engine/handlers/detector/base.py b/src/sentry/workflow_engine/handlers/detector/base.py index 7dbb7bd851eb90..f33787eff12c33 100644 --- a/src/sentry/workflow_engine/handlers/detector/base.py +++ b/src/sentry/workflow_engine/handlers/detector/base.py @@ -11,6 +11,7 @@ from sentry.issues.grouptype import GroupType from sentry.issues.issue_occurrence import IssueEvidence, IssueOccurrence from sentry.types.actor import Actor +from sentry.utils import metrics from sentry.workflow_engine.models import DataConditionGroup, DataPacket, Detector from sentry.workflow_engine.processors.data_condition_group import ProcessedDataConditionGroup from sentry.workflow_engine.types import ( @@ -77,6 +78,12 @@ def to_issue_occurrence( ) +@dataclass(frozen=True) +class GroupedDetectorEvaluationResult: + result: dict[DetectorGroupKey, DetectorEvaluationResult] + tainted: bool + + # TODO - @saponifi3d - Change this class to be a pure ABC and remove the `__init__` method. # TODO - @saponifi3d - Once the change is made, we should introduce a `BaseDetector` class to evaluate simple cases class DetectorHandler(abc.ABC, Generic[DataPacketType, DataPacketEvaluationType]): @@ -102,10 +109,27 @@ def __init__(self, detector: Detector): else: self.condition_group = None - @abc.abstractmethod def evaluate( self, data_packet: DataPacket[DataPacketType] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult] | None: + ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: + tags = { + "detector_type": self.detector.type, + "result": "unknown", + } + try: + value = self.evaluate_impl(data_packet) + tags["result"] = "tainted" if value.tainted else "success" + metrics.incr("workflow_engine_detector.evaluation", tags=tags, sample_rate=1.0) + return value.result + except Exception: + tags["result"] = "failure" + metrics.incr("workflow_engine_detector.evaluation", tags=tags, sample_rate=1.0) + raise + + @abc.abstractmethod + def evaluate_impl( + self, data_packet: DataPacket[DataPacketType] + ) -> GroupedDetectorEvaluationResult: """ This method is used to evaluate the data packet's value against the conditions on the detector. """ diff --git a/src/sentry/workflow_engine/handlers/detector/stateful.py b/src/sentry/workflow_engine/handlers/detector/stateful.py index 478167911098c0..230e3ccbbc0524 100644 --- a/src/sentry/workflow_engine/handlers/detector/stateful.py +++ b/src/sentry/workflow_engine/handlers/detector/stateful.py @@ -19,6 +19,7 @@ DetectorHandler, DetectorOccurrence, EventData, + GroupedDetectorEvaluationResult, ) from sentry.workflow_engine.models import DataPacket, Detector, DetectorState from sentry.workflow_engine.processors.data_condition_group import ( @@ -371,14 +372,16 @@ def _build_workflow_engine_evidence_data( ], } - def evaluate( + def evaluate_impl( self, data_packet: DataPacket[DataPacketType] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: + ) -> GroupedDetectorEvaluationResult: dedupe_value = self.extract_dedupe_value(data_packet) group_data_values = self._extract_value_from_packet(data_packet) state = self.state_manager.get_state_data(list(group_data_values.keys())) results: dict[DetectorGroupKey, DetectorEvaluationResult] = {} + tainted = False + for group_key, data_value in group_data_values.items(): state_data: DetectorStateData = state[group_key] if dedupe_value <= state_data.dedupe_value: @@ -391,7 +394,10 @@ def evaluate( group_data_values[group_key] ) - if condition_results is None or condition_results.logic_result is False: + if condition_results is not None and condition_results.logic_result.is_tainted(): + tainted = True + + if condition_results is None or condition_results.logic_result.triggered is False: # Invalid condition result, nothing we can do # Or if we didn't match any conditions in the evaluation continue @@ -441,7 +447,7 @@ def evaluate( ) self.state_manager.commit_state_updates() - return results + return GroupedDetectorEvaluationResult(result=results, tainted=tainted) def _create_resolve_message( self, @@ -624,7 +630,7 @@ def _evaluation_detector_conditions( }, ) - if condition_evaluation.logic_result: + if condition_evaluation.logic_result.triggered: validated_condition_results: list[DetectorPriorityLevel] = [ condition_result.result for condition_result in condition_evaluation.condition_results diff --git a/src/sentry/workflow_engine/models/data_condition.py b/src/sentry/workflow_engine/models/data_condition.py index cc6e72aad99f45..8b1f6d4b6f8f26 100644 --- a/src/sentry/workflow_engine/models/data_condition.py +++ b/src/sentry/workflow_engine/models/data_condition.py @@ -14,7 +14,7 @@ from sentry.db.models import DefaultFieldsModel, region_silo_model, sane_repr from sentry.utils import metrics, registry from sentry.workflow_engine.registry import condition_handler_registry -from sentry.workflow_engine.types import DataConditionResult, DetectorPriorityLevel +from sentry.workflow_engine.types import ConditionError, DataConditionResult, DetectorPriorityLevel from sentry.workflow_engine.utils import scopedstats logger = logging.getLogger(__name__) @@ -145,7 +145,7 @@ def get_snapshot(self) -> dict[str, Any]: "condition_result": self.condition_result, } - def get_condition_result(self) -> DataConditionResult: + def get_condition_result(self) -> DataConditionResult | ConditionError: match self.condition_result: case float() | bool(): return self.condition_result @@ -159,15 +159,15 @@ def get_condition_result(self) -> DataConditionResult: "Invalid condition result", extra={"condition_result": self.condition_result, "id": self.id}, ) + return ConditionError(msg="Invalid condition result") - return None - - def _evaluate_operator(self, condition_type: Condition, value: T) -> DataConditionResult: + def _evaluate_operator( + self, condition_type: Condition, value: T + ) -> DataConditionResult | ConditionError: # If the condition is a base type, handle it directly op = CONDITION_OPS[condition_type] - result = None try: - result = op(cast(Any, value), self.comparison) + return op(cast(Any, value), self.comparison) except TypeError: logger.exception( "Invalid comparison for data condition", @@ -178,11 +178,12 @@ def _evaluate_operator(self, condition_type: Condition, value: T) -> DataConditi "condition_id": self.id, }, ) - - return result + return ConditionError(msg="Invalid comparison for data condition") @scopedstats.timer() - def _evaluate_condition(self, condition_type: Condition, value: T) -> DataConditionResult: + def _evaluate_condition( + self, condition_type: Condition, value: T + ) -> DataConditionResult | ConditionError: try: handler = condition_handler_registry.get(condition_type) except registry.NoRegistrationExistsError: @@ -190,7 +191,7 @@ def _evaluate_condition(self, condition_type: Condition, value: T) -> DataCondit "No registration exists for condition", extra={"type": self.type, "id": self.id}, ) - return None + return ConditionError(msg="No registration exists for condition") should_be_fast = not is_slow_condition(self) start_time = time.time() @@ -212,7 +213,7 @@ def _evaluate_condition(self, condition_type: Condition, value: T) -> DataCondit "error": str(e), }, ) - return None + return ConditionError(msg=str(e)) finally: duration = time.time() - start_time if should_be_fast and duration >= FAST_CONDITION_TOO_SLOW_THRESHOLD.total_seconds(): @@ -230,7 +231,7 @@ def _evaluate_condition(self, condition_type: Condition, value: T) -> DataCondit return result - def evaluate_value(self, value: T) -> DataConditionResult: + def evaluate_value(self, value: T) -> DataConditionResult | ConditionError: try: condition_type = Condition(self.type) except ValueError: @@ -238,9 +239,9 @@ def evaluate_value(self, value: T) -> DataConditionResult: "Invalid condition type", extra={"type": self.type, "id": self.id}, ) - return None + return ConditionError(msg="Invalid condition type") - result: DataConditionResult + result: DataConditionResult | ConditionError if condition_type in CONDITION_OPS: result = self._evaluate_operator(condition_type, value) else: diff --git a/src/sentry/workflow_engine/models/workflow.py b/src/sentry/workflow_engine/models/workflow.py index b08e74f7c3489d..c55bff06579289 100644 --- a/src/sentry/workflow_engine/models/workflow.py +++ b/src/sentry/workflow_engine/models/workflow.py @@ -18,7 +18,8 @@ from sentry.models.owner_base import OwnerModel from sentry.workflow_engine.models.data_condition import DataCondition, is_slow_condition from sentry.workflow_engine.models.data_condition_group import DataConditionGroup -from sentry.workflow_engine.types import WorkflowEventData +from sentry.workflow_engine.processors.data_condition_group import TriggerResult +from sentry.workflow_engine.types import ConditionError, WorkflowEventData from .json_config import JSONConfigBase @@ -93,7 +94,7 @@ def get_audit_log_data(self) -> dict[str, Any]: def evaluate_trigger_conditions( self, event_data: WorkflowEventData, when_data_conditions: list[DataCondition] | None = None - ) -> tuple[bool, list[DataCondition]]: + ) -> tuple[TriggerResult, list[DataCondition]]: """ Evaluate the conditions for the workflow trigger and return if the evaluation was successful. If there aren't any workflow trigger conditions, the workflow is considered triggered. @@ -104,7 +105,7 @@ def evaluate_trigger_conditions( ) if self.when_condition_group_id is None: - return True, [] + return TriggerResult.TRUE, [] workflow_event_data = replace(event_data, workflow_env=self.environment) try: @@ -116,7 +117,7 @@ def evaluate_trigger_conditions( "DataConditionGroup does not exist", extra={"id": self.when_condition_group_id}, ) - return False, [] + return TriggerResult(False, ConditionError(msg="DataConditionGroup does not exist")), [] group_evaluation, remaining_conditions = process_data_condition_group( group, workflow_event_data, when_data_conditions ) diff --git a/src/sentry/workflow_engine/processors/data_condition_group.py b/src/sentry/workflow_engine/processors/data_condition_group.py index eaf4320ca3314f..ae8cd5a97b34f6 100644 --- a/src/sentry/workflow_engine/processors/data_condition_group.py +++ b/src/sentry/workflow_engine/processors/data_condition_group.py @@ -1,6 +1,7 @@ import dataclasses import logging -from typing import TypeVar +from collections.abc import Callable, Iterable +from typing import ClassVar, NoReturn, TypeVar import sentry_sdk @@ -8,7 +9,7 @@ from sentry.workflow_engine.models import DataCondition, DataConditionGroup from sentry.workflow_engine.models.data_condition import is_slow_condition from sentry.workflow_engine.processors.data_condition import split_conditions_by_speed -from sentry.workflow_engine.types import DataConditionResult +from sentry.workflow_engine.types import ConditionError, DataConditionResult from sentry.workflow_engine.utils import scopedstats logger = logging.getLogger(__name__) @@ -16,16 +17,166 @@ T = TypeVar("T") +def _find_error( + items: list["TriggerResult"], predicate: Callable[["TriggerResult"], bool] +) -> ConditionError | None: + """Helper to find an error from items matching the predicate.""" + return next((item.error for item in items if predicate(item)), None) + + +@dataclasses.dataclass(frozen=True) +class TriggerResult: + """ + Represents the result of a trigger evaluation with taint tracking. + + The triggered field indicates whether the trigger condition was met. + + The error field contains error information if the evaluation was tainted. + When error is not None, it indicates that the result may not be accurate due to + errors encountered during evaluation. Note that there may have been additional + errors beyond the one captured here - this field contains a representative error + from the evaluation, not necessarily all errors that occurred. + """ + + triggered: bool + error: ConditionError | None + + # Constant untainted TriggerResult values (initialized after class definition). + # These represent clean success/failure with no errors. + TRUE: ClassVar["TriggerResult"] + FALSE: ClassVar["TriggerResult"] + + def is_tainted(self) -> bool: + """ + Returns True if this result is less trustworthy due to an error during + evaluation. + """ + return self.error is not None + + def with_error(self, error: ConditionError) -> "TriggerResult": + """ + Returns a new TriggerResult with the same triggered value but the given error. + If the result is already tainted, the error is ignored. + """ + if self.is_tainted(): + return self + return TriggerResult(triggered=self.triggered, error=error) + + @staticmethod + def any(items: Iterable["TriggerResult"]) -> "TriggerResult": + """ + Like `any()`, but for TriggerResult. If any inputs had errors that could + impact the result, the result will contain an error from one of them. + """ + items_list = list(items) + result = any(item.triggered for item in items_list) + + if result: + # Result is True. If we have any untainted True, the result is clean. + # Only tainted if all Trues are tainted. + if any(item.triggered and not item.is_tainted() for item in items_list): + return TriggerResult(triggered=True, error=None) + # All Trues are tainted + return TriggerResult( + triggered=True, error=_find_error(items_list, lambda x: x.triggered) + ) + else: + # Result is False. Any tainted item could have changed the result. + return TriggerResult( + triggered=False, + error=_find_error(items_list, lambda x: x.is_tainted()), + ) + + @staticmethod + def all(items: Iterable["TriggerResult"]) -> "TriggerResult": + """ + Like `all()`, but for TriggerResult. If any inputs had errors that could + impact the result, the result will contain an error from one of them. + """ + items_list = list(items) + result = all(item.triggered for item in items_list) + + if result: + # Result is True. Any tainted item could have changed the result. + return TriggerResult( + triggered=True, + error=_find_error(items_list, lambda x: x.is_tainted()), + ) + else: + # Result is False. If we have any untainted False, the result is clean. + # Only tainted if all Falses are tainted. + if any(not item.triggered and not item.is_tainted() for item in items_list): + return TriggerResult(triggered=False, error=None) + # All Falses are tainted + return TriggerResult( + triggered=False, + error=_find_error(items_list, lambda x: not x.triggered), + ) + + @staticmethod + def none(items: Iterable["TriggerResult"]) -> "TriggerResult": + """ + Like `not any()`, but for TriggerResult. If any inputs had errors that could + impact the result, the result will contain an error from one of them. + """ + items_list = list(items) + + # No items is guaranteed True, no possible error. + if not items_list: + return TriggerResult(triggered=True, error=None) + + result = all(not item.triggered for item in items_list) + + if result: + # Result is True (no conditions triggered) + # Any tainted item could have changed the result + return TriggerResult( + triggered=True, + error=_find_error(items_list, lambda x: x.is_tainted()), + ) + else: + # Result is False (at least one condition triggered) + # If we have any untainted True, the result is clean + if any(item.triggered and not item.is_tainted() for item in items_list): + return TriggerResult(triggered=False, error=None) + # All triggered items are tainted + return TriggerResult( + triggered=False, + error=_find_error(items_list, lambda x: x.triggered), + ) + + def __or__(self, other: "TriggerResult") -> "TriggerResult": + """ + OR operation, equivalent to TriggerResult.any([self, other]). + """ + return TriggerResult.any([self, other]) + + def __and__(self, other: "TriggerResult") -> "TriggerResult": + """ + AND operation, equivalent to TriggerResult.all([self, other]). + """ + return TriggerResult.all([self, other]) + + def __bool__(self) -> NoReturn: + raise AssertionError("TriggerResult cannot be used as a boolean") + + +# Constant untainted TriggerResult values for common cases. +# These are singleton instances representing clean success/failure with no errors. +TriggerResult.TRUE = TriggerResult(triggered=True, error=None) +TriggerResult.FALSE = TriggerResult(triggered=False, error=None) + + @dataclasses.dataclass() class ProcessedDataCondition: - logic_result: bool + logic_result: TriggerResult condition: DataCondition result: DataConditionResult @dataclasses.dataclass() class ProcessedDataConditionGroup: - logic_result: bool + logic_result: TriggerResult condition_results: list[ProcessedDataCondition] @@ -76,35 +227,37 @@ def evaluate_condition_group_results( condition_results: list[ProcessedDataCondition], logic_type: DataConditionGroup.Type, ) -> ProcessedDataConditionGroup: - logic_result = False + logic_result = TriggerResult.FALSE group_condition_results: list[ProcessedDataCondition] = [] if logic_type == DataConditionGroup.Type.NONE: # if we get to this point, no conditions were met # because we would have short-circuited - logic_result = True + logic_result = TriggerResult.none( + condition_result.logic_result for condition_result in condition_results + ) elif logic_type == DataConditionGroup.Type.ANY: - logic_result = any( - [condition_result.logic_result for condition_result in condition_results] + logic_result = TriggerResult.any( + condition_result.logic_result for condition_result in condition_results ) - if logic_result: + if logic_result.triggered: group_condition_results = [ condition_result for condition_result in condition_results - if condition_result.logic_result + if condition_result.logic_result.triggered ] elif logic_type == DataConditionGroup.Type.ALL: conditions_met = [condition_result.logic_result for condition_result in condition_results] - logic_result = all(conditions_met) + logic_result = TriggerResult.all(conditions_met) - if logic_result: + if logic_result.triggered: group_condition_results = [ condition_result for condition_result in condition_results - if condition_result.logic_result + if condition_result.logic_result.triggered ] return ProcessedDataConditionGroup( @@ -126,33 +279,44 @@ def evaluate_data_conditions( if len(conditions_to_evaluate) == 0: # if we don't have any conditions, always return True - return ProcessedDataConditionGroup(logic_result=True, condition_results=[]) + return ProcessedDataConditionGroup(logic_result=TriggerResult.TRUE, condition_results=[]) for condition, value in conditions_to_evaluate: evaluation_result = condition.evaluate_value(value) - is_condition_triggered = evaluation_result is not None + cleaned_result: DataConditionResult + if isinstance(evaluation_result, ConditionError): + cleaned_result = None + else: + cleaned_result = evaluation_result + trigger_result = TriggerResult( + triggered=cleaned_result is not None, + error=evaluation_result if isinstance(evaluation_result, ConditionError) else None, + ) - if is_condition_triggered: + if trigger_result.triggered: # Check for short-circuiting evaluations if logic_type == DataConditionGroup.Type.ANY_SHORT_CIRCUIT: condition_result = ProcessedDataCondition( - logic_result=True, + logic_result=trigger_result, condition=condition, - result=evaluation_result, + result=cleaned_result, ) return ProcessedDataConditionGroup( - logic_result=is_condition_triggered, + logic_result=trigger_result, condition_results=[condition_result], ) if logic_type == DataConditionGroup.Type.NONE: - return ProcessedDataConditionGroup(logic_result=False, condition_results=[]) + return ProcessedDataConditionGroup( + logic_result=TriggerResult(triggered=False, error=trigger_result.error), + condition_results=[], + ) result = ProcessedDataCondition( - logic_result=is_condition_triggered, + logic_result=trigger_result, condition=condition, - result=evaluation_result, + result=cleaned_result, ) condition_results.append(result) @@ -177,7 +341,10 @@ def process_data_condition_group( "Invalid DataConditionGroup.logic_type found in process_data_condition_group", extra={"logic_type": group.logic_type}, ) - return ProcessedDataConditionGroup(logic_result=False, condition_results=[]), [] + trigger_result = TriggerResult( + triggered=False, error=ConditionError(msg="Invalid DataConditionGroup.logic_type") + ) + return ProcessedDataConditionGroup(logic_result=trigger_result, condition_results=[]), [] # Check if conditions are already prefetched before using cache all_conditions: list[DataCondition] @@ -197,7 +364,7 @@ def process_data_condition_group( # there are only slow conditions to evaluate, do not evaluate an empty list of conditions # which would evaluate to True condition_group_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=condition_results, ) return condition_group_result, split_conds.slow @@ -208,8 +375,8 @@ def process_data_condition_group( logic_result = processed_condition_group.logic_result # Check to see if we should return any remaining conditions based on the results - is_short_circuit_all = not logic_result and logic_type == DataConditionGroup.Type.ALL - is_short_circuit_any = logic_result and logic_type in ( + is_short_circuit_all = not logic_result.triggered and logic_type == DataConditionGroup.Type.ALL + is_short_circuit_any = logic_result.triggered and logic_type in ( DataConditionGroup.Type.ANY, DataConditionGroup.Type.ANY_SHORT_CIRCUIT, ) diff --git a/src/sentry/workflow_engine/processors/delayed_workflow.py b/src/sentry/workflow_engine/processors/delayed_workflow.py index a8d8d824923bbc..c29978266befe1 100644 --- a/src/sentry/workflow_engine/processors/delayed_workflow.py +++ b/src/sentry/workflow_engine/processors/delayed_workflow.py @@ -533,7 +533,7 @@ def _group_result_for_dcg( return evaluate_data_conditions( conditions_to_evaluate, DataConditionGroup.Type(dcg.logic_type) - ).logic_result + ).logic_result.triggered @sentry_sdk.trace diff --git a/src/sentry/workflow_engine/processors/detector.py b/src/sentry/workflow_engine/processors/detector.py index e4bd46f1fe9956..864a4cea018314 100644 --- a/src/sentry/workflow_engine/processors/detector.py +++ b/src/sentry/workflow_engine/processors/detector.py @@ -322,9 +322,6 @@ def process_detectors[T]( ): detector_results = handler.evaluate(data_packet) - if detector_results is None: - return results - for result in detector_results.values(): logger_extra = { "detector": detector.id, diff --git a/src/sentry/workflow_engine/processors/workflow.py b/src/sentry/workflow_engine/processors/workflow.py index 9983c49164e9fe..fb8de5ac48216c 100644 --- a/src/sentry/workflow_engine/processors/workflow.py +++ b/src/sentry/workflow_engine/processors/workflow.py @@ -224,7 +224,7 @@ def evaluate_workflow_triggers( }, ) else: - if evaluation: + if evaluation.triggered: triggered_workflows.add(workflow) if dual_processing_logs_enabled: try: @@ -357,7 +357,7 @@ def evaluate_workflows_action_filters( }, ) else: - if group_evaluation.logic_result: + if group_evaluation.logic_result.triggered: if delayed_workflow_item := queue_items_by_workflow.get(workflow): if delayed_workflow_item.delayed_when_group_id: # If there are already delayed when conditions, diff --git a/src/sentry/workflow_engine/types.py b/src/sentry/workflow_engine/types.py index d4f29ab6382a71..2b3ac3c9656789 100644 --- a/src/sentry/workflow_engine/types.py +++ b/src/sentry/workflow_engine/types.py @@ -48,6 +48,20 @@ class DetectorPriorityLevel(IntEnum): DataConditionResult = DetectorPriorityLevel | int | float | bool | None +@dataclass(frozen=True) +class ConditionError: + """ + Represents the failed evaluation of a data condition. + Not intended to be detailed or comprehensive; code returning this + is assumed to have already reported the error. + + A message is provided for clarity and to aid in debugging; a singleton placeholder + value would also work, but would be less clear. + """ + + msg: str + + @dataclass(frozen=True) class DetectorEvaluationResult: # TODO - Should group key live at this level? @@ -160,6 +174,11 @@ class Subgroup(StrEnum): @staticmethod def evaluate_value(value: T, comparison: Any) -> DataConditionResult: + """ + Evaluate the value of a data condition. + Any error that results in a failure to provide a correct result should + raise a DataConditionEvaluationException. + """ raise NotImplementedError diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py index b4cbe5b07e10d0..172201bf35be08 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_types.py @@ -13,13 +13,16 @@ from sentry.testutils.cases import APITestCase from sentry.testutils.silo import region_silo_test from sentry.uptime.grouptype import UptimeDomainCheckFailure -from sentry.workflow_engine.handlers.detector import DetectorHandler, DetectorOccurrence +from sentry.workflow_engine.handlers.detector import ( + DetectorHandler, + DetectorOccurrence, + GroupedDetectorEvaluationResult, +) from sentry.workflow_engine.handlers.detector.base import EventData from sentry.workflow_engine.models import DataPacket from sentry.workflow_engine.processors.data_condition_group import ProcessedDataConditionGroup from sentry.workflow_engine.types import ( DetectorEvaluationResult, - DetectorGroupKey, DetectorPriorityLevel, DetectorSettings, ) @@ -40,10 +43,13 @@ def setUp(self) -> None: self.registry_patcher.start() class MockDetectorHandler(DetectorHandler[dict[Never, Never], bool]): - def evaluate( + def evaluate_impl( self, data_packet: DataPacket[dict[Never, Never]] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: - return {None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)} + ) -> GroupedDetectorEvaluationResult: + return GroupedDetectorEvaluationResult( + result={None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)}, + tainted=False, + ) def extract_value(self, data_packet: DataPacket[dict[Never, Never]]) -> bool: return True diff --git a/tests/sentry/workflow_engine/handlers/detector/test_base.py b/tests/sentry/workflow_engine/handlers/detector/test_base.py index 14491ea693f672..a3f8f936944481 100644 --- a/tests/sentry/workflow_engine/handlers/detector/test_base.py +++ b/tests/sentry/workflow_engine/handlers/detector/test_base.py @@ -10,6 +10,7 @@ DataPacketEvaluationType, DetectorHandler, DetectorOccurrence, + GroupedDetectorEvaluationResult, StatefulDetectorHandler, ) from sentry.workflow_engine.handlers.detector.stateful import DetectorCounters @@ -99,10 +100,13 @@ class NoHandlerGroupType(GroupType): category_v2 = GroupCategory.METRIC_ALERT.value class MockDetectorHandler(DetectorHandler[dict, int]): - def evaluate( + def evaluate_impl( self, data_packet: DataPacket[dict] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: - return {None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)} + ) -> GroupedDetectorEvaluationResult: + return GroupedDetectorEvaluationResult( + result={None: DetectorEvaluationResult(None, True, DetectorPriorityLevel.HIGH)}, + tainted=False, + ) def extract_value(self, data_packet: DataPacket[dict]) -> int: return data_packet.packet.get("value", 0) @@ -120,9 +124,9 @@ def extract_dedupe_value(self, data_packet: DataPacket[dict]) -> int: return data_packet.packet.get("dedupe", 0) class MockDetectorWithUpdateHandler(DetectorHandler[dict, int]): - def evaluate( + def evaluate_impl( self, data_packet: DataPacket[dict] - ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: + ) -> GroupedDetectorEvaluationResult: status_change = StatusChangeMessage( "test_update", project_id, @@ -130,11 +134,14 @@ def evaluate( None, ) - return { - None: DetectorEvaluationResult( - None, True, DetectorPriorityLevel.HIGH, status_change - ) - } + return GroupedDetectorEvaluationResult( + result={ + None: DetectorEvaluationResult( + None, True, DetectorPriorityLevel.HIGH, status_change + ) + }, + tainted=False, + ) def create_occurrence( self, diff --git a/tests/sentry/workflow_engine/models/test_data_condition.py b/tests/sentry/workflow_engine/models/test_data_condition.py index 3dfd20873942df..23f58a3e402542 100644 --- a/tests/sentry/workflow_engine/models/test_data_condition.py +++ b/tests/sentry/workflow_engine/models/test_data_condition.py @@ -5,7 +5,7 @@ from sentry.testutils.cases import TestCase from sentry.workflow_engine.models.data_condition import Condition, DataConditionEvaluationException -from sentry.workflow_engine.types import DetectorPriorityLevel +from sentry.workflow_engine.types import ConditionError, DetectorPriorityLevel from tests.sentry.workflow_engine.test_base import BaseWorkflowTest, DataConditionHandlerMixin @@ -18,7 +18,7 @@ class GetConditionResultTest(TestCase): def test_str(self) -> None: dc = self.create_data_condition(condition_result="wrong") with mock.patch("sentry.workflow_engine.models.data_condition.logger") as mock_logger: - assert dc.get_condition_result() is None + assert dc.get_condition_result() == ConditionError(msg="Invalid condition result") assert mock_logger.error.call_args[0][0] == "Invalid condition result" def test_int(self) -> None: @@ -94,7 +94,7 @@ def test_condition_result_comparison_fails(self) -> None: dc = self.create_data_condition( type=Condition.GREATER, comparison=1.0, condition_result="wrong" ) - assert dc.evaluate_value(2) is None + assert dc.evaluate_value(2) == ConditionError(msg="Invalid condition result") def test_condition_evaluation__data_condition_exception(self) -> None: def evaluate_value(value: int, comparison: int) -> bool: diff --git a/tests/sentry/workflow_engine/models/test_workflow.py b/tests/sentry/workflow_engine/models/test_workflow.py index f687371e47b5ce..fb323240ba1c07 100644 --- a/tests/sentry/workflow_engine/models/test_workflow.py +++ b/tests/sentry/workflow_engine/models/test_workflow.py @@ -32,21 +32,21 @@ def test_queryset(self) -> None: def test_evaluate_trigger_conditions__condition_new_event__True(self) -> None: evaluation, _ = self.workflow.evaluate_trigger_conditions(self.event_data) - assert evaluation is True + assert evaluation.triggered is True def test_evaluate_trigger_conditions__condition_new_event__False(self) -> None: # Update event to have been seen before self.group_event.group.times_seen = 5 evaluation, _ = self.workflow.evaluate_trigger_conditions(self.event_data) - assert evaluation is False + assert evaluation.triggered is False def test_evaluate_trigger_conditions__no_conditions(self) -> None: self.workflow.when_condition_group = None self.workflow.save() evaluation, _ = self.workflow.evaluate_trigger_conditions(self.event_data) - assert evaluation is True + assert evaluation.triggered is True def test_evaluate_trigger_conditions__slow_condition(self) -> None: # Update group to _all_, since the fast condition is met @@ -60,7 +60,7 @@ def test_evaluate_trigger_conditions__slow_condition(self) -> None: self.event_data ) - assert evaluation is True + assert evaluation.triggered is True assert remaining_conditions == [slow_condition] def test_full_clean__success(self) -> None: diff --git a/tests/sentry/workflow_engine/processors/test_data_condition_group.py b/tests/sentry/workflow_engine/processors/test_data_condition_group.py index fa274cb0ef8669..00fcbbefd7c3e5 100644 --- a/tests/sentry/workflow_engine/processors/test_data_condition_group.py +++ b/tests/sentry/workflow_engine/processors/test_data_condition_group.py @@ -1,3 +1,4 @@ +import unittest from unittest import mock from sentry.testutils.cases import TestCase @@ -6,12 +7,13 @@ from sentry.workflow_engine.processors.data_condition_group import ( ProcessedDataCondition, ProcessedDataConditionGroup, + TriggerResult, evaluate_data_conditions, get_data_conditions_for_group, get_slow_conditions_for_groups, process_data_condition_group, ) -from sentry.workflow_engine.types import DetectorPriorityLevel +from sentry.workflow_engine.types import ConditionError, DetectorPriorityLevel class TestGetDataConditionsForGroup(TestCase): @@ -32,7 +34,7 @@ def test_process_data_condition_group__exists__fails(self) -> None: ) expected_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=[], ) expected_remaining_conditions: list[DataCondition] = [] @@ -50,10 +52,10 @@ def test_process_data_condition_group__exists__passes(self) -> None: condition_result=DetectorPriorityLevel.HIGH, ) expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[ ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.condition, result=DetectorPriorityLevel.HIGH, ) @@ -128,15 +130,15 @@ def get_conditions_to_evaluate(self, value: int) -> list[tuple[DataCondition, in class TestEvaluateConditionGroupTypeAny(TestEvaluationConditionCase): def test_evaluate_data_conditions__passes_all(self) -> None: expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[ ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.data_condition, result=DetectorPriorityLevel.HIGH, ), ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.data_condition_two, result=DetectorPriorityLevel.LOW, ), @@ -152,10 +154,10 @@ def test_evaluate_data_conditions__passes_all(self) -> None: def test_evaluate_data_conditions__passes_one(self) -> None: expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[ ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.data_condition_two, result=DetectorPriorityLevel.LOW, ) @@ -171,7 +173,7 @@ def test_evaluate_data_conditions__passes_one(self) -> None: def test_evaluate_data_conditions__fails_all(self) -> None: expected_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=[], ) @@ -185,7 +187,7 @@ def test_evaluate_data_conditions__fails_all(self) -> None: def test_evaluate_data_conditions__passes_without_conditions(self) -> None: result = evaluate_data_conditions([], self.data_condition_group.logic_type) expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[], ) @@ -201,10 +203,10 @@ def test_evaluate_data_conditions__passes_all(self) -> None: assert evaluate_data_conditions( self.get_conditions_to_evaluate(10), self.data_condition_group.logic_type ) == ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[ ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.data_condition, result=DetectorPriorityLevel.HIGH, ) @@ -218,10 +220,10 @@ def test_evaluate_data_conditions__passes_one(self) -> None: ) expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[ ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.data_condition_two, result=DetectorPriorityLevel.LOW, ) @@ -235,14 +237,14 @@ def test_evaluate_data_conditions__fails_all(self) -> None: self.data_condition_group.logic_type, ) expected_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=[], ) assert result == expected_result def test_evaluate_data_conditions__passes_without_conditions(self) -> None: expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[], ) result = evaluate_data_conditions([], self.data_condition_group.logic_type) @@ -260,15 +262,15 @@ def test_evaluate_data_conditions__passes_all(self) -> None: ) expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[ ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.data_condition, result=DetectorPriorityLevel.HIGH, ), ProcessedDataCondition( - logic_result=True, + logic_result=TriggerResult.TRUE, condition=self.data_condition_two, result=DetectorPriorityLevel.LOW, ), @@ -281,7 +283,7 @@ def test_evaluate_data_conditions__passes_one(self) -> None: self.get_conditions_to_evaluate(4), self.data_condition_group.logic_type ) expected_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=[], ) assert result == expected_result @@ -292,7 +294,7 @@ def test_evaluate_data_conditions__fails_all(self) -> None: ) expected_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=[], ) assert result == expected_result @@ -300,7 +302,7 @@ def test_evaluate_data_conditions__fails_all(self) -> None: def test_evaluate_data_conditions__passes_without_conditions(self) -> None: result = evaluate_data_conditions([], self.data_condition_group.logic_type) expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[], ) assert result == expected_result @@ -317,7 +319,7 @@ def test_evaluate_data_conditions__all_conditions_pass__fails(self) -> None: ) expected_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=[], ) assert result == expected_result @@ -327,7 +329,7 @@ def test_evaluate_data_conditions__one_condition_pass__fails(self) -> None: self.get_conditions_to_evaluate(4), self.data_condition_group.logic_type ) expected_result = ProcessedDataConditionGroup( - logic_result=False, + logic_result=TriggerResult.FALSE, condition_results=[], ) assert result == expected_result @@ -337,12 +339,26 @@ def test_evaluate_data_conditions__no_conditions_pass__passes(self) -> None: self.get_conditions_to_evaluate(1), self.data_condition_group.logic_type ) expected_result = ProcessedDataConditionGroup( - logic_result=True, + logic_result=TriggerResult.TRUE, condition_results=[], ) assert result == expected_result + def test_evaluate_data_conditions__error_with_no_pass__tainted_true(self) -> None: + error = ConditionError(msg="test error") + with ( + mock.patch.object(self.data_condition, "evaluate_value", return_value=None), + mock.patch.object(self.data_condition_two, "evaluate_value", return_value=error), + ): + result = evaluate_data_conditions( + self.get_conditions_to_evaluate(10), self.data_condition_group.logic_type + ) + + assert result.logic_result.triggered is True + assert result.logic_result.error == error + assert result.condition_results == [] + class TestEvaluateConditionGroupWithSlowConditions(TestCase): def setUp(self) -> None: @@ -366,7 +382,7 @@ def setUp(self) -> None: def test_basic_remaining_conditions(self) -> None: expected_condition_result = ProcessedDataCondition( - logic_result=True, condition=self.data_condition, result=True + logic_result=TriggerResult.TRUE, condition=self.data_condition, result=True ) group_evaluation, remaining_conditions = process_data_condition_group( @@ -374,7 +390,7 @@ def test_basic_remaining_conditions(self) -> None: 10, ) - assert group_evaluation.logic_result is True + assert group_evaluation.logic_result.triggered is True assert ( group_evaluation.condition_results[0].condition.id == expected_condition_result.condition.id @@ -388,7 +404,7 @@ def test_basic_only_slow_conditions(self) -> None: 10, ) - assert group_evaluation.logic_result is False + assert group_evaluation.logic_result.triggered is False assert group_evaluation.condition_results == [] assert remaining_conditions == [self.slow_condition] @@ -398,7 +414,7 @@ def test_short_circuit_with_all(self) -> None: 1, ) - assert group_evaluation.logic_result is False + assert group_evaluation.logic_result.triggered is False assert group_evaluation.condition_results == [] assert remaining_conditions == [] @@ -409,9 +425,11 @@ def test_short_circuit_with_any(self) -> None: 10, ) - assert group_evaluation.logic_result is True + assert group_evaluation.logic_result.triggered is True assert group_evaluation.condition_results == [ - ProcessedDataCondition(logic_result=True, condition=self.data_condition, result=True) + ProcessedDataCondition( + logic_result=TriggerResult.TRUE, condition=self.data_condition, result=True + ) ] assert remaining_conditions == [] @@ -456,3 +474,97 @@ def test_multiple_dcgs(self) -> None: dcg2.id: [condition2, condition4], dcg3.id: [condition5], } + + +# Constants to make TestTriggerResult easier to read +TRUE = TriggerResult.TRUE +FALSE = TriggerResult.FALSE +ERR = ConditionError(msg="test error") + + +class TestTriggerResult(unittest.TestCase): + + def test_any_all_untainted_true_returns_untainted_true(self) -> None: + assert TriggerResult.any([FALSE, TRUE, FALSE]) == TRUE + + def test_any_one_untainted_true_returns_untainted_true(self) -> None: + assert TriggerResult.any([TRUE, TRUE.with_error(ERR)]) == TRUE + assert TriggerResult.any([TRUE.with_error(ERR), TRUE]) == TRUE + + def test_any_only_tainted_true_returns_tainted_true(self) -> None: + assert TriggerResult.any([FALSE, TRUE.with_error(ERR), FALSE]) == TRUE.with_error(ERR) + + def test_any_no_true_returns_false_with_error_if_present(self) -> None: + assert TriggerResult.any([FALSE, FALSE.with_error(ERR), FALSE]) == FALSE.with_error(ERR) + + def test_any_all_false_untainted_returns_untainted_false(self) -> None: + assert TriggerResult.any([FALSE, FALSE, FALSE]) == FALSE + + def test_all_all_untainted_true_returns_untainted_true(self) -> None: + assert TriggerResult.all([TRUE, TRUE, TRUE]) == TRUE + + def test_all_any_tainted_returns_tainted(self) -> None: + assert TriggerResult.all([TRUE, TRUE.with_error(ERR), TRUE]) == TRUE.with_error(ERR) + + def test_all_with_untainted_false_and_tainted_true_returns_clean_false(self) -> None: + # Clean because we have untainted False + assert TriggerResult.all([TRUE, FALSE, TRUE.with_error(ERR)]) == FALSE + + def test_all_with_only_tainted_false_returns_tainted_false(self) -> None: + assert TriggerResult.all([TRUE, FALSE.with_error(ERR)]) == FALSE.with_error(ERR) + + def test_all_all_false_untainted_returns_untainted_false(self) -> None: + assert TriggerResult.all([FALSE, FALSE, FALSE]) == FALSE + + def test_any_with_generator_preserves_error(self) -> None: + assert TriggerResult.any(iter([FALSE, FALSE.with_error(ERR), FALSE])) == FALSE.with_error( + ERR + ) + + def test_any_untainted_true_with_tainted_false_returns_clean_true(self) -> None: + assert TriggerResult.any([TRUE, FALSE.with_error(ERR)]) == TRUE + + def test_all_untainted_false_with_tainted_true_returns_clean_false(self) -> None: + assert TriggerResult.all([FALSE, TRUE.with_error(ERR)]) == FALSE + + def test_all_with_generator_preserves_error(self) -> None: + assert TriggerResult.all(iter([TRUE, TRUE.with_error(ERR), TRUE])) == TRUE.with_error(ERR) + + def test_none_empty_returns_untainted_true(self) -> None: + assert TriggerResult.none([]) == TRUE + + def test_none_all_false_untainted_returns_untainted_true(self) -> None: + assert TriggerResult.none([FALSE, FALSE, FALSE]) == TRUE + + def test_none_all_false_with_error_returns_tainted_true(self) -> None: + assert TriggerResult.none([FALSE, FALSE.with_error(ERR), FALSE]) == TRUE.with_error(ERR) + + def test_none_one_true_returns_untainted_false(self) -> None: + assert TriggerResult.none([FALSE, TRUE, FALSE]) == FALSE + + def test_none_one_true_with_error_returns_tainted_false(self) -> None: + assert TriggerResult.none([FALSE, TRUE.with_error(ERR), FALSE]) == FALSE.with_error(ERR) + + def test_none_untainted_true_with_tainted_false_returns_clean_false(self) -> None: + assert TriggerResult.none([TRUE, FALSE.with_error(ERR)]) == FALSE + + def test_or_with_untainted_true_returns_clean_true(self) -> None: + # Clean because we have untainted True + assert (TRUE | FALSE.with_error(ERR)) == TRUE + + def test_or_with_only_tainted_true_returns_tainted_true(self) -> None: + # Tainted because only True is tainted + assert (TRUE.with_error(ERR) | FALSE) == TRUE.with_error(ERR) + + def test_and_with_untainted_false_returns_clean_false(self) -> None: + # Clean because we have untainted False + assert (FALSE & TRUE.with_error(ERR)) == FALSE + + def test_and_with_only_tainted_false_returns_tainted_false(self) -> None: + # Tainted because only False is tainted + assert (TRUE & FALSE.with_error(ERR)) == FALSE.with_error(ERR) + + def test_none_with_generator_preserves_error(self) -> None: + assert TriggerResult.none(iter([FALSE, FALSE.with_error(ERR), FALSE])) == TRUE.with_error( + ERR + ) diff --git a/tests/sentry/workflow_engine/processors/test_delayed_workflow.py b/tests/sentry/workflow_engine/processors/test_delayed_workflow.py index bf96b63373dcfa..88ffef353020e1 100644 --- a/tests/sentry/workflow_engine/processors/test_delayed_workflow.py +++ b/tests/sentry/workflow_engine/processors/test_delayed_workflow.py @@ -39,6 +39,7 @@ ) from sentry.workflow_engine.processors.data_condition_group import ( ProcessedDataConditionGroup, + TriggerResult, get_slow_conditions_for_groups, ) from sentry.workflow_engine.processors.delayed_workflow import ( @@ -940,7 +941,7 @@ def test_fire_actions_for_groups__fire_actions(self, mock_trigger: MagicMock) -> @patch("sentry.workflow_engine.processors.workflow.process_data_condition_group") def test_fire_actions_for_groups__workflow_fire_history(self, mock_process: MagicMock) -> None: mock_process.return_value = ( - ProcessedDataConditionGroup(logic_result=True, condition_results=[]), + ProcessedDataConditionGroup(logic_result=TriggerResult.TRUE, condition_results=[]), [], ) diff --git a/tests/sentry/workflow_engine/processors/test_detector.py b/tests/sentry/workflow_engine/processors/test_detector.py index 6e2b25c15a516f..050955b0554138 100644 --- a/tests/sentry/workflow_engine/processors/test_detector.py +++ b/tests/sentry/workflow_engine/processors/test_detector.py @@ -220,10 +220,15 @@ def test_sending_metric_before_evaluating(self) -> None: with mock.patch("sentry.utils.metrics.incr") as mock_incr: process_detectors(data_packet, [detector]) - mock_incr.assert_called_once_with( + mock_incr.assert_any_call( "workflow_engine.process_detector", tags={"detector_type": detector.type}, ) + mock_incr.assert_any_call( + "workflow_engine_detector.evaluation", + tags={"detector_type": detector.type, "result": "success"}, + sample_rate=1.0, + ) @mock.patch("sentry.workflow_engine.processors.detector.produce_occurrence_to_kafka") @mock.patch("sentry.workflow_engine.processors.detector.metrics")