♻️ feat(workflow_engine): issue alert action migration util for slack #83123

iamrajjoshi · 2025-01-08T21:23:16Z

this pr starts the implementation of migration util for issue alert migration to aci.

added some comments to the code for context.

replaces: #82505

src/sentry/workflow_engine/migration_helpers/rule_action.py

iamrajjoshi · 2025-01-08T21:24:23Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+        return build_slack_data_blob(action).__dict__
+    # # Otherwise, we can just return the action data as is, removing the keys we don't want to save
+    else:
+        return {


popping keys we save elsewhere or don't want to save

src/sentry/workflow_engine/typings/notification_action.py

codecov · 2025-01-08T22:01:10Z

Codecov Report

Attention: Patch coverage is 93.41317% with 11 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...try/workflow_engine/typings/notification_action.py	91.54%	6 Missing ⚠️
...gine/migration_helpers/test_migrate_rule_action.py	92.85%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #83123      +/-   ##
==========================================
- Coverage   87.48%   87.48%   -0.01%     
==========================================
  Files        9406     9409       +3     
  Lines      537270   537437     +167     
  Branches    21158    21158              
==========================================
+ Hits       470047   470188     +141     
- Misses      66866    66892      +26     
  Partials      357      357

ceorourke

left some nits but overall lgtm

src/sentry/workflow_engine/migration_helpers/rule_action.py

src/sentry/workflow_engine/typings/notification_action.py

src/sentry/workflow_engine/migration_helpers/rule_action.py

saponifi3d · 2025-01-10T23:50:38Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+            integration_id_key = ACTION_TYPE_TO_INTEGRATION_ID_KEY.get(action_type)
+            if integration_id_key is None:
+                # we should always have an integration id key if target type is specific
+                # TODO(iamrajjoshi): Should we fail loudly here?


I think it depends on how this method is being used to migrate a rule. I'd think of this as flow control, if we want to continue creating the new action / workflow then we should just log and continue like you have. If we want to stop creation of the workflow because it will be malformed, we should throw an error to stop the migration (or handle the issue in another area)

talked with cathy, we will log the error and continue to build actions that we can. actions that we store in Rule that are malformed don't fire in the first place and we don't have any system that heals these actions, so we just won't migrate them.

saponifi3d · 2025-01-10T23:56:01Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+            integration_id=integration_id,
+            target_identifier=target_identifier,
+            target_display=target_display,
+            target_type=target_type,


would we want to create the action if any of these weren't set? (given the current code flow, that could be a possibility)

saponifi3d · 2025-01-10T23:57:26Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+        # FWIW, we don't use target type for issue alerts
+        target_type = ACTION_TYPE_TO_TARGET_TYPE_RULE_REGISTRY.get(action_type)
+
+        if target_type == ActionTarget.SPECIFIC:


since we're not using the target_type could we just say if the target type isn't set to specific, then continue or throw an exception? i think that might help make this code a little more readable since we could dedent a lot of it, but i'm not sure if we want to since the notification_action can be created w/o these properties.

src/sentry/workflow_engine/typings/notification_action.py

tests/sentry/workflow_engine/migration_helpers/test_migrate_rule_action.py

cathteng

the logic looks good, i just have a few suggestions for organization

cathteng · 2025-01-10T23:45:51Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+            # Get the target_identifier if it exists
+            target_identifier_key = ACTION_TYPE_TO_TARGET_IDENTIFIER_KEY.get(action_type)
+            if target_identifier_key is not None:
+                target_identifier = action.get(target_identifier_key)


i think target_identifier might need to be initialized outside of this if block, similarly with target_display below

src/sentry/workflow_engine/migration_helpers/rule_action.py

cathteng · 2025-01-10T23:48:45Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+            not in [
+                ACTION_TYPE_TO_INTEGRATION_ID_KEY.get(action_type),
+                ACTION_TYPE_TO_TARGET_IDENTIFIER_KEY.get(action_type),
+                ACTION_TYPE_TO_TARGET_DISPLAY_KEY.get(action_type),
+                *EXCLUDED_ACTION_DATA_KEYS,
+            ]


do we remove these because these are used in metric alerts?

we remove them b/c we already store them in some other field. i want to keep the blob as small as possible, containing extra context that we need to send a notification.

src/sentry/workflow_engine/typings/notification_action.py

cathteng

some more thoughts 🤔

cathteng · 2025-01-13T22:10:27Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+        try:
+            translator_class = issue_alert_action_translator_registry.get(registry_id)
+            translator = translator_class()
+        except KeyError:


the Registry class will throw NoRegistrationExists error if the key doesn't exist

cathteng · 2025-01-13T22:12:08Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+
+        try:
+            translator_class = issue_alert_action_translator_registry.get(registry_id)
+            translator = translator_class()


maybe you can pass in the action to the translator class so you can do this in one go

target_display = None if translator.target_display_key: target_display = action.get(translator.target_display_key)

becomes

target_display = translator.target_display_key

src/sentry/workflow_engine/migration_helpers/rule_action.py

cathteng

see big comment

cathteng · 2025-01-14T18:11:51Z

src/sentry/workflow_engine/migration_helpers/rule_action.py

+        notification_action = Action(
+            type=translator.action_type,
+            data=translator.get_sanitized_data(),
+            integration_id=translator.integration_id,
+            target_identifier=translator.target_identifier,
+            target_display=translator.target_display,
+            target_type=translator.target_type,
+        )


so clean 😻

src/sentry/workflow_engine/typings/notification_action.py

…#83123)

♻️ ref: create migration util helper for slack

ef7859f

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

iamrajjoshi commented Jan 8, 2025

View reviewed changes

src/sentry/workflow_engine/migration_helpers/rule_action.py Outdated Show resolved Hide resolved

iamrajjoshi commented Jan 8, 2025

View reviewed changes

vercel bot deployed to Preview January 8, 2025 21:25 View deployment

♻️ ref: use bulk create

fc203b5

iamrajjoshi requested a review from a team January 8, 2025 21:27

iamrajjoshi marked this pull request as ready for review January 8, 2025 21:27

vercel bot deployed to Preview January 8, 2025 21:30 View deployment

iamrajjoshi commented Jan 8, 2025

View reviewed changes

src/sentry/workflow_engine/typings/notification_action.py Show resolved Hide resolved

ceorourke approved these changes Jan 8, 2025

View reviewed changes

🔍 nit: rename mapping and helper func

4de89b9

vercel bot deployed to Preview January 9, 2025 18:55 View deployment

iamrajjoshi requested a review from a team January 9, 2025 19:38

🔧 chore: use .get

1c0317d

iamrajjoshi requested a review from cathteng January 9, 2025 19:49

vercel bot deployed to Preview January 9, 2025 19:50 View deployment

saponifi3d reviewed Jan 11, 2025

View reviewed changes

cathteng approved these changes Jan 11, 2025

View reviewed changes

♻️ ref: use registry

0088734

vercel bot deployed to Preview January 13, 2025 21:16 View deployment

cathteng reviewed Jan 13, 2025

View reviewed changes

✨ feat: moved more stuff to abc and wrote more tests

4d390bb

vercel bot deployed to Preview January 14, 2025 04:34 View deployment

Merge branch 'master' into raj/issue-alert-migration-helper-slack

c5bde4b

vercel bot deployed to Preview January 14, 2025 04:51 View deployment

iamrajjoshi requested review from cathteng and saponifi3d January 14, 2025 17:35

cathteng approved these changes Jan 14, 2025

View reviewed changes

🔧 chore: cleanup required & missing fields

41d2ac6

iamrajjoshi marked this pull request as draft January 14, 2025 19:24

iamrajjoshi marked this pull request as ready for review January 14, 2025 19:24

iamrajjoshi enabled auto-merge (squash) January 14, 2025 19:24

vercel bot deployed to Preview January 14, 2025 19:29 View deployment

iamrajjoshi merged commit ca71de7 into master Jan 14, 2025
49 checks passed

iamrajjoshi deleted the raj/issue-alert-migration-helper-slack branch January 14, 2025 19:52

This was referenced Jan 14, 2025

✨ feat(aci): add discord/msteam issue alert migration helper #83455

Merged

✨ feat(aci): add pagerduty/opsegnie migration utils #83565

Merged

andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025

♻️ feat(workflow_engine): issue alert action migration util for slack (…

d9317fe

…#83123)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ feat(workflow_engine): issue alert action migration util for slack #83123

♻️ feat(workflow_engine): issue alert action migration util for slack #83123

iamrajjoshi commented Jan 8, 2025 •

edited

Loading

iamrajjoshi Jan 8, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

ceorourke left a comment

saponifi3d Jan 10, 2025

iamrajjoshi Jan 13, 2025

saponifi3d Jan 10, 2025

saponifi3d Jan 10, 2025

cathteng left a comment

cathteng Jan 10, 2025

cathteng Jan 10, 2025

iamrajjoshi Jan 13, 2025

cathteng left a comment

cathteng Jan 13, 2025

cathteng Jan 13, 2025

cathteng left a comment

cathteng Jan 14, 2025

♻️ feat(workflow_engine): issue alert action migration util for slack #83123

♻️ feat(workflow_engine): issue alert action migration util for slack #83123

Conversation

iamrajjoshi commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

ceorourke 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

cathteng 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

cathteng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathteng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamrajjoshi commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading