Skip to content
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

feat(aci): registry for dual writing conditions #82021

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 i think this module name might be a little confusing - it seems like this is creating instances of conditions for the issue rules?

Maybe we can make a migrations/rule.py or migrations/rule/* if you want a full module. colleen has a similar pr that also has a bunch of migration helpers, and i feel like all that code could be co-located as they can probs be removed after the rollout is completed as part of the "Cleanup" milestone.

Copy link
Member Author

Choose a reason for hiding this comment

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

migrations is a good name

Empty file.
54 changes: 54 additions & 0 deletions src/sentry/workflow_engine/conditions/dual_write.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
from collections.abc import Callable
from typing import Any

from sentry.rules.conditions.every_event import EveryEventCondition
from sentry.rules.conditions.reappeared_event import ReappearedEventCondition
from sentry.rules.conditions.regression_event import RegressionEventCondition
from sentry.utils.registry import Registry
from sentry.workflow_engine.models.data_condition import Condition, DataCondition
from sentry.workflow_engine.models.data_condition_group import DataConditionGroup

data_condition_translator_registry = Registry[
Callable[[dict[str, Any], DataConditionGroup], DataCondition]
]()


def translate_to_data_condition(data: dict[str, Any], dcg: DataConditionGroup):
translator = data_condition_translator_registry.get(data["id"])
return translator(data, dcg)


@data_condition_translator_registry.register(EveryEventCondition.id)
def create_every_event_condition(data: dict[str, Any], dcg: DataConditionGroup) -> DataCondition:
return DataCondition.objects.create(
condition=Condition.TRUTH,
comparison=True,
condition_result=True,
condition_group=dcg,
)


@data_condition_translator_registry.register(ReappearedEventCondition.id)
def create_group_comparison_reappeared_condition(
data: dict[str, Any], dcg: DataConditionGroup
) -> DataCondition:
return DataCondition.objects.create(
type=Condition.GROUP_EVENT_ATTR_COMPARISON,
condition="state.has_reappeared",
comparison=True,
condition_result=True,
condition_group=dcg,
)


@data_condition_translator_registry.register(RegressionEventCondition.id)
def create_group_comparison_regression_condition(
data: dict[str, Any], dcg: DataConditionGroup
) -> DataCondition:
return DataCondition.objects.create(
type=Condition.GROUP_EVENT_ATTR_COMPARISON,
Copy link
Member Author

Choose a reason for hiding this comment

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

my worry with using the same type for different conditions is that they'll need to be serialized differently, but i think we can figure it out by making an enum for the possible condition values here. we also have conditions you can no longer select in the UI but we'll still have to show because we don't delete them

condition="state.is_regression",
comparison=True,
condition_result=True,
condition_group=dcg,
)
14 changes: 11 additions & 3 deletions src/sentry/workflow_engine/handlers/condition/group_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from sentry.eventstore.models import GroupEvent
from sentry.workflow_engine.models.data_condition import Condition
from sentry.workflow_engine.registry import condition_handler_registry
from sentry.workflow_engine.types import DataConditionHandler
from sentry.workflow_engine.types import DataConditionHandler, DetectorPriorityLevel


def get_nested_value(data: Any, path: str, default: Any = None) -> Any | None:
Expand All @@ -24,6 +24,14 @@ def get_nested_value(data: Any, path: str, default: Any = None) -> Any | None:
@condition_handler_registry.register(Condition.GROUP_EVENT_ATTR_COMPARISON)
class GroupEventConditionHandler(DataConditionHandler[GroupEvent]):
@staticmethod
def evaluate_value(data: GroupEvent, comparison: Any, data_filter: str) -> bool:
event_value = get_nested_value(data, data_filter)
def evaluate_value(
value: GroupEvent, comparison: Any, condition: str, **kwargs: Any
) -> DetectorPriorityLevel | int | float | bool | None:
Copy link
Member Author

Choose a reason for hiding this comment

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

my local mypy was complaining about this

Copy link
Contributor

Choose a reason for hiding this comment

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

DataConditionResult is probs the best one to use from sentry.workflow_engine.types (it's the same as what you have)

# conditions stores the path to the attribute in the event
event_value = get_nested_value(value, condition)
if event_value is None:
event_value = get_nested_value(
kwargs, condition
) # TODO: remove when GroupEvent contains EventState
Comment on lines +31 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to split here? could we just invoke the condition with the kwargs as the value? (if it's a different input type, we can add another method here with a different type and reuse the get_nested_value method in that handler)

Copy link
Member Author

Choose a reason for hiding this comment

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

sure i can do that

Copy link
Member Author

Choose a reason for hiding this comment

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

i put this here in the case that we actually do decide to move these out of a separate kwarg and into the GroupEvent itself


return event_value == comparison
16 changes: 12 additions & 4 deletions src/sentry/workflow_engine/models/data_condition.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import operator
from collections.abc import Callable
from enum import StrEnum
from inspect import signature
from typing import Any, TypeVar, cast

from django.db import models
Expand All @@ -27,15 +28,17 @@ class Condition(StrEnum):
LESS = "lt"
NOT_EQUAL = "ne"
GROUP_EVENT_ATTR_COMPARISON = "group_event_attr_comparison"
TRUTH = "truth"


condition_ops = {
condition_ops: dict[Condition, Callable] = {
Condition.EQUAL: operator.eq,
Condition.GREATER_OR_EQUAL: operator.ge,
Condition.GREATER: operator.gt,
Condition.LESS_OR_EQUAL: operator.le,
Condition.LESS: operator.lt,
Condition.NOT_EQUAL: operator.ne,
Condition.TRUTH: operator.truth,
}

T = TypeVar("T")
Expand Down Expand Up @@ -94,7 +97,7 @@ def get_condition_handler(self) -> DataConditionHandler[T] | None:

return condition_handler_registry.get(condition_type)

def evaluate_value(self, value: T) -> DataConditionResult:
def evaluate_value(self, value: T, **kwargs) -> DataConditionResult:
condition_handler: DataConditionHandler[T] | None = None
op: Callable | None = None

Expand All @@ -107,9 +110,14 @@ def evaluate_value(self, value: T) -> DataConditionResult:
op = condition_ops.get(condition, None)

if condition_handler is not None:
result = condition_handler.evaluate_value(value, self.comparison, self.condition)
result = condition_handler.evaluate_value(
value, self.comparison, self.condition, **kwargs
)
elif op is not None:
result = op(cast(Any, value), self.comparison)
if len(signature(op).parameters) == 1:
result = op(cast(Any, value)) == self.comparison
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain this a bit? i'm wondering if we can use one of the existing operators rather than introducing this if else nesting for the 1 operator.

Copy link
Member Author

Choose a reason for hiding this comment

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

operator.truth and some other operators like abs take in 1 argument, the other operators take in 2. if you want to compare the ones with a single argument to the comparison then we need special logic

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but do we need to add the truth operator? could we just say Value === True instead of adding the truth operator? we also don't have a use case for abs, since the goal of data conditions is to compare a dynamic value (value) to a static value (comparison), it feels a little weird to have a single evaluation.

else:
result = op(cast(Any, value), self.comparison)
else:
logger.error(
"Invalid Data Condition Evaluation",
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/workflow_engine/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def bulk_get_query_object(data_sources) -> dict[int, T | None]:

class DataConditionHandler(Generic[T]):
@staticmethod
def evaluate_value(value: T, comparison: Any, condition: str) -> DataConditionResult:
def evaluate_value(value: T, comparison: Any, condition: str, **kwargs) -> DataConditionResult:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain how these kwargs are being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

we discussed this before, we can pass the other parts of PostProcessJob that aren't the GroupEvent as kwargs so they can be evaluated in workflow conditions, and keep process_workflows general for other group types that don't have kwargs. there is an EventState object we check for RegressionCondition and ReappearedEventCondition today, for example

Copy link
Contributor

Choose a reason for hiding this comment

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

okay cool, wanted to confirm that was the intent -- at this level, i don't think we should be referring to them as kwargs though, instead we should be evaluating them as the value (whichever kwarg you want to evaluate).

this also seems like it's only adding the passing of that stuff in a limited scope, instead we should pull this out as a separate PR an look at that feature as a whole. then we can start to build off of it, otherwise there's a lot of noise and it's hard to follow eevrything.

raise NotImplementedError


Expand Down
42 changes: 42 additions & 0 deletions tests/sentry/workflow_engine/handlers/condition/test_base.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
from typing import Any

from sentry.rules.base import RuleBase
from sentry.testutils.cases import TestCase
from sentry.workflow_engine.conditions.dual_write import (
translate_to_data_condition as dual_write_condition,
)
from sentry.workflow_engine.models.data_condition import Condition, DataCondition
from sentry.workflow_engine.models.data_condition_group import DataConditionGroup


class ConditionTestCase(TestCase):
@property
def condition(self) -> Condition:
raise NotImplementedError

@property
def rule_cls(self) -> type[RuleBase]:
# for mapping purposes, can delete later
raise NotImplementedError

@property
def payload(self) -> dict[str, Any]:
# for dual write, can delete later
raise NotImplementedError

def translate_to_data_condition(
self, data: dict[str, Any], dcg: DataConditionGroup
) -> DataCondition:
return dual_write_condition(data, dcg)

def assert_passes(self, data_condition: DataCondition, value, **kwargs) -> None:
assert (
data_condition.evaluate_value(value, **kwargs) == data_condition.get_condition_result()
)

def assert_does_not_pass(self, data_condition: DataCondition, value, **kwargs) -> None:
assert (
data_condition.evaluate_value(value, **kwargs) != data_condition.get_condition_result()
)

# TODO: activity
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from sentry.rules.conditions.every_event import EveryEventCondition
from sentry.workflow_engine.models.data_condition import Condition
from tests.sentry.workflow_engine.handlers.condition.test_base import ConditionTestCase


class EveryEventOperatorTest(ConditionTestCase):
condition = Condition.TRUTH
rule_cls = EveryEventCondition
payload = {"id": EveryEventCondition.id}

def test(self):
dc = self.create_data_condition(
comparison=True, condition_result=True, condition=self.condition
)

self.assert_passes(dc, self.event)

def test_dual_write(self):
dcg = self.create_data_condition_group()
dc = self.translate_to_data_condition(self.payload, dcg)

assert dc.condition == self.condition
assert dc.comparison is True
assert dc.condition_result is True
assert dc.condition_group == dcg
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
from sentry.rules.base import EventState
from sentry.rules.conditions.reappeared_event import ReappearedEventCondition
from sentry.rules.conditions.regression_event import RegressionEventCondition
from sentry.workflow_engine.models.data_condition import Condition
from tests.sentry.workflow_engine.handlers.condition.test_base import ConditionTestCase


class TestReappearedEventCondition(ConditionTestCase):
condition = Condition.GROUP_EVENT_ATTR_COMPARISON
rule_cls = ReappearedEventCondition
payload = {"id": ReappearedEventCondition.id}

def test(self):
state = EventState(
is_new=False,
is_regression=False,
is_new_group_environment=False,
has_reappeared=True,
has_escalated=False,
)
dc = self.create_data_condition(
type=Condition.GROUP_EVENT_ATTR_COMPARISON,
condition="state.has_reappeared",
comparison=True,
condition_result=True,
)

self.assert_passes(dc, self.event, state=state)

state.has_reappeared = False
self.assert_does_not_pass(dc, self.event, state=state)

def test_dual_write(self):
dcg = self.create_data_condition_group()
dc = self.translate_to_data_condition(self.payload, dcg)

assert dc.type == self.condition
assert dc.condition == "state.has_reappeared"
assert dc.comparison is True
assert dc.condition_result is True
assert dc.condition_group == dcg


class TestRegressionEventCondition(ConditionTestCase):
condition = Condition.GROUP_EVENT_ATTR_COMPARISON
rule_cls = RegressionEventCondition
payload = {"id": RegressionEventCondition.id}

def test(self):
state = EventState(
is_new=False,
is_regression=True,
is_new_group_environment=False,
has_reappeared=True,
has_escalated=False,
)
dc = self.create_data_condition(
type=Condition.GROUP_EVENT_ATTR_COMPARISON,
condition="state.is_regression",
comparison=True,
condition_result=True,
)

self.assert_passes(dc, self.event, state=state)

state.is_regression = False
self.assert_does_not_pass(dc, self.event, state=state)

def test_dual_write(self):
dcg = self.create_data_condition_group()
dc = self.translate_to_data_condition(self.payload, dcg)

assert dc.type == self.condition
assert dc.condition == "state.is_regression"
assert dc.comparison is True
assert dc.condition_result is True
assert dc.condition_group == dcg
Loading