feat(alerts): ACI dual write helpers #81953

ceorourke · 2024-12-11T01:14:10Z

Add helper methods to create rows in the new ACI models that will be called just after creating the old models.

codecov · 2024-12-11T01:39:34Z

Codecov Report

Attention: Patch coverage is 90.21739% with 9 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ry/workflow_engine/migration_helpers/alert_rule.py	89.77%	9 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #81953    +/-   ##
========================================
  Coverage   80.42%   80.43%            
========================================
  Files        7300     7305     +5     
  Lines      321936   322196   +260     
  Branches    21015    21015            
========================================
+ Hits       258931   259169   +238     
- Misses      62600    62622    +22     
  Partials      405      405

src/sentry/workflow_engine/metric_helpers.py

ceorourke · 2024-12-11T23:34:25Z

src/sentry/workflow_engine/metric_helpers.py

+        condition=condition,
+        comparison=alert_rule_trigger.alert_threshold,
+        condition_result=condition_result,
+        type=ConditionType.METRIC_CONDITION,


I added this but wasn't totally sure if we had other plans for the type field

I'm using the Conditions enum right now, just haven't had a chance to link it to the type's choices yet. I was thinking of renaming a couple of the fields on the data_condition cause they feel a bit confusing (but also dreading the migrations 🤣)

🤞 i get a bit more coding time today to tie up these loose ends.

src/sentry/workflow_engine/metric_helpers.py

iamrajjoshi · 2024-12-12T00:16:03Z

src/sentry/workflow_engine/metric_helpers.py

+        return None
+
+    data = {
+        "type": alert_rule_trigger_action.type,


@saponifi3d we are adding the integration "provider" to the data blob here. my current plan is to not use this and deduce it via integration_id and target_type. i am still open to switching over and using this "type" if you think its a good idea for future work.

this might also make the UI portion of ACI a little easier since we will have a provider that we can do filtering from 🤔

the only problem with using this to search / do lookups on is that it's in the data json blob, so those lookups will be reaaalllly slow compared to a column. I feel like there are more reasons for us to have this as a column than to keep the action this abstract though 🤔

maybe we just go back to ActionType.SLACK, rather than a single type for all notifications? that way we could keep this model generally abstract, while making this property more accessible.

We could have a list of "notifications" like NOTIFICATION_ACTIONS=[ActionType.SLACK, ...] and to trigger the generalized notification action, i can just check if the type is in that list instead of relying solely on the type.

would that work for y'all?

if we split up the types per integration provider, would we still then have a split between "issue" and "metric" as we discussed yesterday?

discussed in office hours: we will add a legacy field on an Action for "issue" and "metric"

mifu67 · 2024-12-12T22:15:47Z

src/sentry/workflow_engine/metric_helpers.py

+    )
+    threshold_type = alert_rule_trigger.alert_rule.threshold_type
+    condition = (
+        Condition.GREATER if threshold_type == AlertRuleThresholdType.ABOVE else Condition.LESS


For UI purposes, do we need to include a case here for AlertRuleThresholdType.ABOVE_AND_BELOW?

Yes, good catch. I should reach out to design too and make sure the anomaly detection UI includes this (and anything else specific to it).

I ended up leaving a note to add this for anomaly detection later on since adding Condition.ABOVE_OR_BELOW includes creating the condition handler for anomaly detection which is a separate piece of work.

src/sentry/workflow_engine/metric_helpers.py

saponifi3d · 2024-12-13T05:28:26Z

src/sentry/workflow_engine/metric_helpers.py

+        condition=condition,
+        comparison=alert_rule_trigger.alert_threshold,
+        condition_result=condition_result,
+        type=ConditionType.METRIC_CONDITION,


I'm using the Conditions enum right now, just haven't had a chance to link it to the type's choices yet. I was thinking of renaming a couple of the fields on the data_condition cause they feel a bit confusing (but also dreading the migrations 🤣)

🤞 i get a bit more coding time today to tie up these loose ends.

src/sentry/workflow_engine/metric_helpers.py

saponifi3d · 2024-12-13T05:55:42Z

src/sentry/workflow_engine/metric_helpers.py

+        return None
+
+    data = {
+        "type": alert_rule_trigger_action.type,


the only problem with using this to search / do lookups on is that it's in the data json blob, so those lookups will be reaaalllly slow compared to a column. I feel like there are more reasons for us to have this as a column than to keep the action this abstract though 🤔

maybe we just go back to ActionType.SLACK, rather than a single type for all notifications? that way we could keep this model generally abstract, while making this property more accessible.

We could have a list of "notifications" like NOTIFICATION_ACTIONS=[ActionType.SLACK, ...] and to trigger the generalized notification action, i can just check if the type is in that list instead of relying solely on the type.

would that work for y'all?

saponifi3d · 2024-12-13T05:59:32Z

src/sentry/workflow_engine/models/data_condition.py

@@ -27,6 +31,7 @@ class Condition(StrEnum):
    LESS = "lt"
    NOT_EQUAL = "ne"
    GROUP_EVENT_ATTR_COMPARISON = "group_event_attr_comparison"
+    ABOVE_AND_BELOW = "above_and_below"


you'll need to also register a method that can evaluate this. if it's all custom logic, take a look at the GROUP_EVENT_ATTR_COMPARISON condition to get an example of how to register / use this.

I took this out for now since we'll be adding special handling for the anomaly detection condition handler

tests/sentry/workflow_engine/test_metric_helpers.py

src/sentry/workflow_engine/metric_helpers.py

src/sentry/workflow_engine/types.py

src/sentry/workflow_engine/models/data_condition.py

src/sentry/workflow_engine/types.py

saponifi3d · 2024-12-16T20:56:21Z

src/sentry/workflow_engine/migration_helpers/alert_rule.py

+    )
+
+
+def migrate_alert_rule(


i really like how you re-structure this 💯

ceorourke · 2024-12-19T00:52:10Z

This has gotten quite long 😓 I'll break it up tomorrow

A smaller chunk of #81953 that creates the helper methods to migrate the `AlertRule` and not the `AlertRuleTrigger` or `AlertRuleTriggerAction` just yet.

ceorourke · 2024-12-20T20:17:28Z

Closing in favor of #82400 and #82492

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

vercel bot deployed to Preview December 11, 2024 01:22 View deployment

ceorourke force-pushed the ceorourke/aci-dual-write-helpers branch from ed3ff80 to d447c15 Compare December 11, 2024 20:54

vercel bot deployed to Preview December 11, 2024 20:58 View deployment

vercel bot deployed to Preview December 11, 2024 23:17 View deployment

ceorourke marked this pull request as ready for review December 11, 2024 23:19

ceorourke requested a review from a team December 11, 2024 23:19

ceorourke commented Dec 11, 2024

View reviewed changes

src/sentry/workflow_engine/metric_helpers.py Outdated Show resolved Hide resolved

ceorourke commented Dec 11, 2024

View reviewed changes

src/sentry/workflow_engine/metric_helpers.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview December 11, 2024 23:31 View deployment

ceorourke commented Dec 11, 2024

View reviewed changes

src/sentry/workflow_engine/metric_helpers.py Outdated Show resolved Hide resolved

iamrajjoshi reviewed Dec 12, 2024

View reviewed changes

iamrajjoshi requested a review from a team December 12, 2024 00:16

mifu67 mentioned this pull request Dec 12, 2024

feat(workflow_engine): Add process_data_packets method #82002

Merged

mifu67 reviewed Dec 12, 2024

View reviewed changes

vercel bot deployed to Preview December 13, 2024 02:13 View deployment

vercel bot deployed to Preview December 13, 2024 02:22 View deployment

saponifi3d reviewed Dec 13, 2024

View reviewed changes

ceorourke force-pushed the ceorourke/aci-dual-write-helpers branch from b10ddac to 8b337f9 Compare December 16, 2024 19:42

vercel bot deployed to Preview December 16, 2024 19:46 View deployment

vercel bot deployed to Preview December 16, 2024 20:01 View deployment

vercel bot deployed to Preview December 16, 2024 20:16 View deployment

ceorourke requested a review from saponifi3d December 16, 2024 20:48

vercel bot deployed to Preview December 16, 2024 20:51 View deployment

saponifi3d reviewed Dec 16, 2024

View reviewed changes

vercel bot deployed to Preview December 16, 2024 22:26 View deployment

vercel bot deployed to Preview December 17, 2024 00:30 View deployment

ceorourke force-pushed the ceorourke/aci-dual-write-helpers branch from 7ba8ac8 to 35b4991 Compare December 17, 2024 19:16

ceorourke added 19 commits December 18, 2024 14:27

rm comment

1de67e1

fix typing, add conditiontype enum

f3e7164

typing in tests

d15d8cf

Add try/except

5ea8051

handle above and below condition for anomaly detection

e0a530a

typing

0a1e830

move file

a163391

make more smaller methods

6fa78fa

add logging, typing, return tuples

4d721f1

remove anomaly detection stuff for now

8c5a53e

add tests for unhappy path

23731f2

only set one

54a8823

rename test file

e4185b1

Add return

7f6242c

update tes

f2849e0

use Action.Type

8744557

add todo comment

110a7e5

update data source type

b058303

condition -> type, add action.types, update tests

a11952b

ceorourke force-pushed the ceorourke/aci-dual-write-helpers branch from 35b4991 to a11952b Compare December 19, 2024 00:43

vercel bot deployed to Preview December 19, 2024 00:52 View deployment

fix import

29219ba

ceorourke force-pushed the ceorourke/aci-dual-write-helpers branch from 4804948 to 29219ba Compare December 19, 2024 19:21

🛠️ apply pre-commit fixes

78dd40c

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

ceorourke mentioned this pull request Dec 19, 2024

feat(alerts): ACI dual write alert rule helpers #82400

Merged

ceorourke added a commit that referenced this pull request Dec 20, 2024

feat(alerts): ACI dual write alert rule helpers (#82400)

526a059

A smaller chunk of #81953 that creates the helper methods to migrate the `AlertRule` and not the `AlertRuleTrigger` or `AlertRuleTriggerAction` just yet.

ceorourke closed this Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(alerts): ACI dual write helpers #81953

feat(alerts): ACI dual write helpers #81953

ceorourke commented Dec 11, 2024

codecov bot commented Dec 11, 2024 •

edited

Loading

ceorourke Dec 11, 2024

saponifi3d Dec 13, 2024

iamrajjoshi Dec 12, 2024 •

edited

Loading

saponifi3d Dec 13, 2024

iamrajjoshi Dec 13, 2024

iamrajjoshi Dec 13, 2024

mifu67 Dec 12, 2024

ceorourke Dec 13, 2024

ceorourke Dec 16, 2024

saponifi3d Dec 13, 2024

saponifi3d Dec 13, 2024

saponifi3d Dec 13, 2024

ceorourke Dec 16, 2024

saponifi3d Dec 16, 2024

ceorourke commented Dec 19, 2024

ceorourke commented Dec 20, 2024

		)


		def migrate_alert_rule(

feat(alerts): ACI dual write helpers #81953

feat(alerts): ACI dual write helpers #81953

Conversation

ceorourke commented Dec 11, 2024

codecov bot commented Dec 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamrajjoshi Dec 12, 2024 • edited Loading

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

ceorourke commented Dec 19, 2024

ceorourke commented Dec 20, 2024

codecov bot commented Dec 11, 2024 •

edited

Loading

iamrajjoshi Dec 12, 2024 •

edited

Loading