✨ feat(aci): Invoke Rule Registry from Notification Action #84524

iamrajjoshi · 2025-02-04T17:37:30Z

This PR introduces a new ABC which we will inherit and create new handlers to invoke the legacy issue alert registry.

Basically after the notification action determines that it needs to invoke the issue alert registry (rule registry), it will call BaseIssueAlertHandler.invoke_legacy_registry which will shape the data into a rule instance and then safe_execute the callback returned by the RuleFuture.

the method mentioned above is based on

sentry/src/sentry/rules/processing/processor.py

Lines 147 to 184 in aa95456

    
           def activate_downstream_actions( 
        
               rule: Rule, 
        
               event: GroupEvent, 
        
               notification_uuid: str | None = None, 
        
               rule_fire_history: RuleFireHistory | None = None, 
        
           ) -> MutableMapping[ 
        
               str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]] 
        
           ]: 
        
               grouped_futures: MutableMapping[ 
        
                   str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]] 
        
               ] = {} 
        
               for action in rule.data.get("actions", ()): 
        
                   action_inst = instantiate_action(rule, action, rule_fire_history) 
        
                   if not action_inst: 
        
                       continue 
        
                   results = safe_execute( 
        
                       action_inst.after, 
        
                       event=event, 
        
                       notification_uuid=notification_uuid, 
        
                   ) 
        
                   if results is None: 
        
                       logger.warning("Action %s did not return any futures", action["id"]) 
        
                       continue 
        
                   for future in results: 
        
                       key = future.key if future.key is not None else future.callback 
        
                       rule_future = RuleFuture(rule=rule, kwargs=future.kwargs) 
        
                       if key not in grouped_futures: 
        
                           grouped_futures[key] = (future.callback, [rule_future]) 
        
                       else: 
        
                           grouped_futures[key][1].append(rule_future) 
        
               return grouped_futures

spec

closes https://getsentry.atlassian.net/browse/ACI-150

iamrajjoshi · 2025-02-04T17:37:59Z

tests/sentry/workflow_engine/handlers/action/notification/test_issue_alert.py

+        "sentry.workflow_engine.handlers.action.notification.issue_alert.instantiate_action"
+    )
+    @mock.patch("uuid.uuid4")
+    def test_invoke_legacy_registry(self, mock_uuid, mock_instantiate_action, mock_safe_execute):


based off of

sentry/src/sentry/rules/processing/processor.py

Lines 147 to 184 in aa95456

def activate_downstream_actions(

rule: Rule,

event: GroupEvent,

notification_uuid: str | None = None,

rule_fire_history: RuleFireHistory | None = None,

) -> MutableMapping[

str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]]

]:

grouped_futures: MutableMapping[

str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]]

] = {}

for action in rule.data.get("actions", ()):

action_inst = instantiate_action(rule, action, rule_fire_history)

if not action_inst:

continue

results = safe_execute(

action_inst.after,

event=event,

notification_uuid=notification_uuid,

)

if results is None:

logger.warning("Action %s did not return any futures", action["id"])

continue

for future in results:

key = future.key if future.key is not None else future.callback

rule_future = RuleFuture(rule=rule, kwargs=future.kwargs)

if key not in grouped_futures:

grouped_futures[key] = (future.callback, [rule_future])

else:

grouped_futures[key][1].append(rule_future)

return grouped_futures

codecov · 2025-02-04T17:41:31Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
23509	1	23508	289

View the top 1 failed test(s) by shortest run time

tests.sentry.workflow_engine.handlers.action.notification.test_issue_alert.TestBaseIssueAlertHandler::test_create_rule_instance_from_action

Stack Traces | 3.23s run time

#x1B[1m#x1B[.../action/notification/test_issue_alert.py#x1B[0m:46: in test_create_rule_instance_from_action
    assert rule.id == self.detector.id
#x1B[1m#x1B[31mE   AssertionError: assert 17 == 9#x1B[0m
#x1B[1m#x1B[31mE    +  where 17 = <Rule at 0x7f9c204401b0: id=17, project_id=4555528490188802, label='Smashing Bass'>.id#x1B[0m
#x1B[1m#x1B[31mE    +  and   9 = <Detector at 0x7f9c386e3950: id=9>.id#x1B[0m
#x1B[1m#x1B[31mE    +    where <Detector at 0x7f9c386e3950: id=9> = <action.notification.test_issue_alert.TestBaseIssueAlertHandler testMethod=test_create_rule_instance_from_action>.detector#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

ceorourke · 2025-02-04T19:08:33Z

src/sentry/workflow_engine/handlers/action/notification/issue_alert.py

+        detector: Detector,
+    ) -> None:
+        """
+        This method will create a rule instance from the Action model, and then invoke the legacy registry.


which part of this is doing the invocation? it looks like it's just executing the futures like we do currently

i added a lot more docstrings just now. the tldr is we consolidate the post_process process_rules, activate_downstream_actions into this method which will handle building a rule instance from an Action, getting the action handler class, getting the rule futures and executing them.

i'm not seeing the use of any registry here, maybe that's what colleen was asking about?

ah yes, good point. the rule registry is quite literally invoked in the activate_downstream_actions with the instantiate_action method call it makes. do you think there is a better way for me to be explicit about this?

src/sentry/workflow_engine/handlers/action/notification/notification.py

src/sentry/workflow_engine/handlers/action/notification/issue_alert.py

cathteng · 2025-02-05T17:10:03Z

src/sentry/workflow_engine/handlers/action/notification/issue_alert.py

+            futures = cls.get_rule_futures(job, rule, notification_uuid)
+
+            # Execute the futures
+            cls.execute_futures(job, futures)


do these need to be separate? is it for having separate spans?

yes but also- i want to decouple the logic here, i have a feeling when we move to notif platform, we will slowly remove each of these calls.

It also makes it easier to test each of these steps with mock data, so I've been encouraging this kind of decoupling. It can also allow us to add metrics surrounding each call to see what the rough failure rate is for each step.

cathteng · 2025-02-05T17:14:42Z

src/sentry/workflow_engine/handlers/action/notification/issue_alert.py

+        detector: Detector,
+    ) -> None:
+        """
+        This method will create a rule instance from the Action model, and then invoke the legacy registry.


i'm not seeing the use of any registry here, maybe that's what colleen was asking about?

src/sentry/workflow_engine/handlers/action/notification/notification.py

cathteng · 2025-02-05T17:18:11Z

src/sentry/workflow_engine/handlers/action/notification/issue_alert.py

+        """
+
+        rule = Rule(
+            id=detector.id,


can you remind me why this needs to be detector.id? is it possible that this collides with an existing Rule id?

actually i need this to be action.id so i can fetch the correct action in our slack threads implementation & so i build links correctly. this won't collide with anything since this will be a rule instance not saved in the DB.

cathteng · 2025-02-05T17:20:16Z

tests/sentry/workflow_engine/handlers/action/notification/test_issue_alert.py

+        mock_safe_execute.assert_called_once_with(mock_callback, self.job["event"], mock_futures)
+
+
+class TestDiscordIssueAlertHandler(BaseWorkflowTest):


should we test that the correct after() method is called?

we shouldn't need to. activate_downstream_actions is well tested and as long as we are certain that the rule action id we are sending it (for example, "sentry.integrations.discord.notify_action.DiscordNotifyServiceAction" for Discord), we can be certain it will call the correct after method

GabeVillalobos · 2025-02-10T18:55:54Z

src/sentry/workflow_engine/handlers/action/__init__.py

@@ -2,4 +2,4 @@
    "NotificationActionHandler",
 ]

-from .notification import NotificationActionHandler
+from .notification.notification import NotificationActionHandler


I find this file path a little confusing, since notification/notification doesn't tell me a lot about what's in this module.

updating to handler

GabeVillalobos · 2025-02-10T18:57:53Z

src/sentry/workflow_engine/handlers/action/notification/issue_alert.py

+            futures = cls.get_rule_futures(job, rule, notification_uuid)
+
+            # Execute the futures
+            cls.execute_futures(job, futures)


It also makes it easier to test each of these steps with mock data, so I've been encouraging this kind of decoupling. It can also allow us to add metrics surrounding each call to see what the rough failure rate is for each step.

src/sentry/workflow_engine/handlers/action/notification/issue_alert.py

GabeVillalobos · 2025-02-10T19:01:14Z

src/sentry/workflow_engine/handlers/action/notification/notification.py

+            logger.exception(
+                "No issue alert handler found for action type: %s",
+                action.type,
+                extra={"action_id": action.id},
+            )


Should this swallow the exception? Does the caller assume these calls never fail?

good point, let me raise exceptions, we can swallow if needed later

src/sentry/workflow_engine/handlers/action/notification/notification.py

…all integrations (#84829) builds on #84524 adds legacy rule registry invokers/handlers for Messaging and Oncall Integrations

iamrajjoshi requested a review from GabeVillalobos February 4, 2025 17:37

iamrajjoshi self-assigned this Feb 4, 2025

iamrajjoshi requested a review from a team as a code owner February 4, 2025 17:37

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 4, 2025

iamrajjoshi commented Feb 4, 2025

View reviewed changes

vercel bot deployed to Preview February 4, 2025 17:45 View deployment

ceorourke reviewed Feb 4, 2025

View reviewed changes

iamrajjoshi added 3 commits February 4, 2025 14:14

✨ feat: create legacy issue alert handler

ab20a0b

🐛 fix: update import in test

141a394

✨ feat: decompose logic, fix mypy, handle registry functions correctly

157ca3c

iamrajjoshi force-pushed the raj/aci/invoke-rule-registry branch from 6394eb6 to 157ca3c Compare February 5, 2025 00:22

iamrajjoshi commented Feb 5, 2025

View reviewed changes

src/sentry/workflow_engine/handlers/action/notification/notification.py Outdated Show resolved Hide resolved

🔍 nit: typo

d983c01

iamrajjoshi requested a review from a team February 5, 2025 00:26

vercel bot deployed to Preview February 5, 2025 00:30 View deployment

🐛 fix: update tests

f0c00d0

vercel bot deployed to Preview February 5, 2025 01:27 View deployment

cathteng reviewed Feb 5, 2025

View reviewed changes

iamrajjoshi requested review from ceorourke and cathteng February 7, 2025 22:55

🔍 nit: pr feedback

541bb52

vercel bot deployed to Preview February 7, 2025 22:59 View deployment

iamrajjoshi mentioned this pull request Feb 7, 2025

✨ feat(aci): create legacy issue alert handlers for messaging and oncall integrations #84829

Merged

🔍 nit: small test fix

afd322e

vercel bot deployed to Preview February 7, 2025 23:36 View deployment

✨ feat: add support for environment id

16d962a

vercel bot deployed to Preview February 10, 2025 18:17 View deployment

GabeVillalobos reviewed Feb 10, 2025

View reviewed changes

♻️ ref: rename and raise exceptions

6c334c4

iamrajjoshi requested a review from GabeVillalobos February 10, 2025 21:52

GabeVillalobos approved these changes Feb 10, 2025

View reviewed changes

vercel bot deployed to Preview February 10, 2025 21:55 View deployment

iamrajjoshi enabled auto-merge (squash) February 10, 2025 21:56

iamrajjoshi merged commit da60e50 into master Feb 10, 2025
49 checks passed

iamrajjoshi deleted the raj/aci/invoke-rule-registry branch February 10, 2025 22:21

iamrajjoshi added a commit that referenced this pull request Feb 11, 2025

✨ feat(aci): create legacy issue alert handlers for messaging and onc…

d791c85

…all integrations (#84829) builds on #84524 adds legacy rule registry invokers/handlers for Messaging and Oncall Integrations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ feat(aci): Invoke Rule Registry from Notification Action #84524

✨ feat(aci): Invoke Rule Registry from Notification Action #84524

iamrajjoshi commented Feb 4, 2025

iamrajjoshi Feb 4, 2025

codecov bot commented Feb 4, 2025 •

edited

Loading

ceorourke Feb 4, 2025

iamrajjoshi Feb 5, 2025

cathteng Feb 5, 2025

iamrajjoshi Feb 7, 2025

cathteng Feb 5, 2025

iamrajjoshi Feb 7, 2025

GabeVillalobos Feb 10, 2025

cathteng Feb 5, 2025

cathteng Feb 5, 2025

iamrajjoshi Feb 7, 2025

cathteng Feb 5, 2025

iamrajjoshi Feb 7, 2025

GabeVillalobos Feb 10, 2025

iamrajjoshi Feb 10, 2025

GabeVillalobos Feb 10, 2025

GabeVillalobos Feb 10, 2025

iamrajjoshi Feb 10, 2025

	def activate_downstream_actions(
	rule: Rule,
	event: GroupEvent,
	notification_uuid: str \| None = None,
	rule_fire_history: RuleFireHistory \| None = None,
	) -> MutableMapping[
	str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]]
	]:
	grouped_futures: MutableMapping[
	str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]]
	] = {}

	for action in rule.data.get("actions", ()):
	action_inst = instantiate_action(rule, action, rule_fire_history)
	if not action_inst:
	continue

	results = safe_execute(
	action_inst.after,
	event=event,
	notification_uuid=notification_uuid,
	)
	if results is None:
	logger.warning("Action %s did not return any futures", action["id"])
	continue

	for future in results:
	key = future.key if future.key is not None else future.callback
	rule_future = RuleFuture(rule=rule, kwargs=future.kwargs)

	if key not in grouped_futures:
	grouped_futures[key] = (future.callback, [rule_future])
	else:
	grouped_futures[key][1].append(rule_future)

	return grouped_futures

		mock_safe_execute.assert_called_once_with(mock_callback, self.job["event"], mock_futures)


		class TestDiscordIssueAlertHandler(BaseWorkflowTest):

✨ feat(aci): Invoke Rule Registry from Notification Action #84524

✨ feat(aci): Invoke Rule Registry from Notification Action #84524

Conversation

iamrajjoshi commented Feb 4, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2025 • edited Loading

❌ 1 Tests Failed:

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

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

codecov bot commented Feb 4, 2025 •

edited

Loading