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

cathteng · 2024-12-12T19:05:05Z

(and migrate every event, regression, reappeared conditions)

Add a registry for dual writing a condition on a Rule to a DataCondition based on the condition id.

EveryEventCondition is an example where we can replace the condition with a Python operator (operator.truth), since we are evaluating on a GroupEvent which is always truthy.

Regression and ReappearedCondition are simple conditions that directly evaluate on GroupState which will be passed in kwargs to DataCondition.evaluate_value.

cathteng · 2024-12-12T19:05:25Z

src/sentry/workflow_engine/handlers/condition/group_event.py

-        event_value = get_nested_value(data, data_filter)
+    def evaluate_value(
+        value: GroupEvent, comparison: Any, condition: str, **kwargs: Any
+    ) -> DetectorPriorityLevel | int | float | bool | None:


my local mypy was complaining about this

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

codecov · 2024-12-12T19:09:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #82021   +/-   ##
=======================================
  Coverage   80.35%   80.36%           
=======================================
  Files        7279     7280    +1     
  Lines      321319   321381   +62     
  Branches    20958    20958           
=======================================
+ Hits       258206   258275   +69     
+ Misses      62699    62692    -7     
  Partials      414      414

cathteng · 2024-12-12T19:46:17Z

src/sentry/workflow_engine/conditions/dual_write.py

+    data: dict[str, Any], dcg: DataConditionGroup
+) -> DataCondition:
+    return DataCondition.objects.create(
+        type=Condition.GROUP_EVENT_ATTR_COMPARISON,


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

saponifi3d

if it's not toooooo troublesome.. min splitting the changes to the platform and the changes for the migration into 2 prs? i think that'd help me understand what code is being added for each task.

saponifi3d · 2024-12-13T06:13:53Z

src/sentry/workflow_engine/conditions/__init__.py

🤔 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.

migrations is a good name

saponifi3d · 2024-12-13T06:17:40Z

src/sentry/workflow_engine/handlers/condition/group_event.py

-        event_value = get_nested_value(data, data_filter)
+    def evaluate_value(
+        value: GroupEvent, comparison: Any, condition: str, **kwargs: Any
+    ) -> DetectorPriorityLevel | int | float | bool | None:


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

saponifi3d · 2024-12-13T06:24:25Z

src/sentry/workflow_engine/models/data_condition.py

        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


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.

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

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.

saponifi3d · 2024-12-13T16:58:29Z

src/sentry/workflow_engine/types.py

@@ -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:


can you explain how these kwargs are being used?

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

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.

saponifi3d · 2024-12-13T17:02:13Z

src/sentry/workflow_engine/handlers/condition/group_event.py

+        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


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)

sure i can do that

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

cathteng · 2024-12-13T17:57:29Z

@saponifi3d i feel like it's helpful to know why we are creating data conditions a certain way if you can see how the fields are being used in the handler, lmk if you feel differently

cathteng · 2024-12-13T18:01:46Z

i can split it

cathteng · 2024-12-20T22:56:26Z

too many merge conflicts. replaced by #82511

cathteng added 5 commits December 12, 2024 09:38

EveryEventCondition via operator.truth

9ca8c8b

migrate reappeared + regression conditions

99005da

fix typing

312b3c7

update comment

67b81cf

clean up tests

b5de3fc

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 12, 2024

cathteng commented Dec 12, 2024

View reviewed changes

vercel bot deployed to Preview December 12, 2024 19:07 View deployment

add init file

2979200

vercel bot deployed to Preview December 12, 2024 19:41 View deployment

cathteng commented Dec 12, 2024

View reviewed changes

cathteng marked this pull request as ready for review December 12, 2024 20:16

cathteng requested a review from a team December 12, 2024 20:16

saponifi3d reviewed Dec 13, 2024

View reviewed changes

cathteng marked this pull request as draft December 13, 2024 18:03

cathteng closed this Dec 20, 2024

cathteng deleted the cathy/aci/condition-translator-registry branch December 23, 2024 16:21

github-actions bot locked and limited conversation to collaborators Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cathteng commented Dec 12, 2024

cathteng Dec 12, 2024

saponifi3d Dec 13, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

cathteng Dec 12, 2024

saponifi3d left a comment

saponifi3d Dec 13, 2024

cathteng Dec 13, 2024

saponifi3d Dec 13, 2024

saponifi3d Dec 13, 2024

cathteng Dec 13, 2024

saponifi3d Dec 13, 2024

saponifi3d Dec 13, 2024

cathteng Dec 13, 2024

saponifi3d Dec 13, 2024

saponifi3d Dec 13, 2024

cathteng Dec 13, 2024

cathteng Dec 13, 2024

cathteng commented Dec 13, 2024

cathteng commented Dec 13, 2024

cathteng commented Dec 20, 2024

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

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

Conversation

cathteng commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

saponifi3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathteng commented Dec 13, 2024

cathteng commented Dec 13, 2024

cathteng commented Dec 20, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading