✨ poc(nowa): poc for issue alert migration #81288

iamrajjoshi · 2024-11-26T00:49:12Z

this is a poc to see how issue alerts notifications will work using NOWA in ACI.

i am planning to build this out so we can fire notifications, but currently i have the migration path from Rule -> Action and the first part of the NOWA logic, which translates an Action to legacy models (Rule and RuleFireHistory)

edit: added the invoke_nowa function and was able to successfully send a notification using the new models.

currently i convert a Rule to Action and back to a Rule, and send a notification using the new rule.

iamrajjoshi · 2024-11-26T00:49:29Z

src/sentry/workflow_engine/models/action.py

@@ -21,8 +39,36 @@ class Action(DefaultFieldsModel):

    # TODO (@saponifi3d): Don't hardcode these values, and these are incomplete values
    class Type(models.TextChoices):


this can change. just have this here so i can use this

👍 yeah - let's update this a bit, happy to chat through ideas!

src/sentry/workflow_engine/nowa/logic.py

src/sentry/workflow_engine/nowa/mappings.py

codecov · 2024-11-26T01:02:16Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
322	1	321	9

View the top 1 failed tests by shortest run time

::tests.sentry.workflow_engine.actions.notification_action.test_notification_action

Stack Traces | 0s run time

No failure message available

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

src/sentry/workflow_engine/nowa/logic.py

src/sentry/rules/processing/processor.py

src/sentry/workflow_engine/nowa/logic.py

iamrajjoshi · 2024-11-27T15:14:13Z

src/sentry/workflow_engine/actions/notification_action/notification_action_handler.py

+        Action.Type.NOTIFICATION_EMAIL,
+    ]
+)
+def notification_action_handler(action: Action, group_event: GroupEvent):


this is the entrypoint of NOWA, it will be invoked when someone calls trigger_action with one of the registered action types

i am having trouble with this registry registering in 1 of my tests, if i import it it claims i registered this function twice. if i don't, it says this isn't registered 😕

i think it might be the way you're using the registry. i'd just do something like, @action_handler_registry.register(Action.Type.NOTIFICATION) to encapsulate any notification.

you can also stack the decorators if we keep many kinds of notifications:

@action_handler_registry.register(Action.Type.NOTIFICATION_SLACK) @action_handler_registry.register(Action.Type.NOTIFICATION_DISCORD) @action_handler_registry.register(Action.Type.NOTIFICATION_MSTEAMS) def notification_action_handler(action: Action, group_event: GroupEvent): pass

iamrajjoshi · 2024-11-27T15:14:43Z

src/sentry/workflow_engine/actions/action_handlers_registry.py

+        raise ValueError(f"No handler found for action type: {action.type}")
+
+
+def trigger_action(action: Action, group_event: GroupEvent):


other parts of ACI can call this function which will trigger nowa if the action.type is correct

iamrajjoshi · 2024-11-27T15:20:08Z

src/sentry/workflow_engine/actions/notification_action/logic.py

@@ -0,0 +1,137 @@
+import uuid


meat of nowa

saponifi3d · 2024-11-27T16:43:03Z

src/sentry/workflow_engine/actions/action_handlers_registry.py

+def register_action_handler(action_types: list[Action.Type]):
+    """
+    Decorator to register a handler for specific action types.
+
+    Usage:
+    @register_action_handler([Action.Type.NOTIFICATION_SLACK, Action.Type.NOTIFICATION_DISCORD])
+    def notification_action_handler(action: Action, group_event: GroupEvent):
+        ...
+    """
+
+    def decorator(handler_class: ActionHandler):
+        for action_type in action_types:
+            action_handler_registry.register(action_type.value)(handler_class)
+        return handler_class
+
+    return decorator
+
+
+def get_handler(action: Action) -> ActionHandler:
+    """Get the handler instance for a given action."""
+    try:
+        handler_class = action_handler_registry.get(action.type.value)
+        return handler_class()
+    except NoRegistrationExistsError:
+        raise ValueError(f"No handler found for action type: {action.type}")


you shouldn't nee to create these, you can just use the action_handler_registry directly.

@action_handler_registry.register(Action.Type) def handler(action: Action, group_event: GroupEvent): pass

and

action_handler_register.get(Action.Type)

(i can also create these as they're part of milestone 1 -- i'm about to need this registry on my branch, i'll probably just copy pasta bits of this 🎉)

saponifi3d · 2024-11-27T16:47:29Z

src/sentry/workflow_engine/actions/notification_action/logic.py

+from sentry.workflow_engine.models.detector import Detector
+
+
+def build_rule_data_blob(action: Action) -> list[dict[str, Any]]:


is this dict shaped like anything in particular? it seems like we always have an id, uuid, integration_id_key (based on the action type), etc -- can we make a type that encapsulates the expected shape?

src/sentry/workflow_engine/actions/notification_action/migration_utils.py

src/sentry/workflow_engine/actions/notification_action/mappings.py

saponifi3d · 2024-11-27T16:53:41Z

src/sentry/workflow_engine/actions/notification_action/logic.py

@@ -0,0 +1,137 @@
+import uuid
+from typing import Any


After moving around the other files, we could then update this to be notification_action.py -- that might be nice to keep it actions/<action_implementation>.py - and keeping it flat is one of the big wins of using a registry for just the handler method.

src/sentry/workflow_engine/actions/notification_action/migration_utils.py

saponifi3d · 2024-11-27T17:17:05Z

src/sentry/workflow_engine/actions/notification_action/migration_utils.py

+    }
+
+
+def build_notification_actions_from_rule_data(actions: list[dict[str, Any]]) -> list[Action]:


how can we improve the typing here? Is there a specific format for each action?

saponifi3d · 2024-11-27T17:18:48Z

src/sentry/workflow_engine/actions/notification_action/logic.py

+    :param group_event: GroupEvent model instance
+    """
+
+    rule, rule_fire_history, notification_uuid = form_issue_alert_models(


for production code, let's add a span to this method so we can see how long we're spending creating legacy models per notification.

saponifi3d · 2024-11-27T17:21:19Z

src/sentry/workflow_engine/actions/notification_action/logic.py

+    :param group_event: GroupEvent model instance
+    """
+
+    rule, rule_fire_history, notification_uuid = form_issue_alert_models(


also, should we add a check to see if we need to create a new rule vs fetching an existing rule from the db?

saponifi3d · 2024-11-27T17:23:27Z

src/sentry/workflow_engine/actions/notification_action/logic.py

+    )
+
+    # TODO(iamrajjoshi): Add a check to see if the rule has only one action
+    assert len(rule.data.get("actions", [])) == 1


a little nervous that this will really increase the number of rules we have in the db; it seems like for each notification we're sending; we'll be creating a new rule (so if the workflow has 5 actions, we'll create 5 rules with 1 action each). It also seems like we're creating it every time, but not removing it and not reusing it.

iamrajjoshi · 2024-12-03T19:07:30Z

src/sentry/workflow_engine/typings/notification_action.py

+}
+
+
+class RuleDataBlob:


working through this at the moment, need to figure out how we can handle dynamic fields

iamrajjoshi · 2024-12-03T19:07:36Z

src/sentry/workflow_engine/typings/notification_action.py

+"""
+IntegrationIdKey is the key used to identify the integration in the rule data.
+"""
+IntegrationIdKey = Union[


not used atm

getsantry · 2024-12-31T08:00:32Z

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

iamrajjoshi added the Do Not Merge Don't merge label Nov 26, 2024

iamrajjoshi requested a review from a team November 26, 2024 00:49

iamrajjoshi self-assigned this Nov 26, 2024