feat(aci milestone 3): ACI alert rule dual update helper #82982

mifu67 · 2025-01-07T00:04:25Z

Add helper method to update the relevant ACI tables after updating an alert rule.
Also fixes some tests that weren't running because I named them incorrectly in my last PR 😅

codecov · 2025-01-07T00:32:35Z

Codecov Report

Attention: Patch coverage is 92.85714% with 8 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	81.39%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #82982      +/-   ##
==========================================
+ Coverage   84.98%   87.65%   +2.66%     
==========================================
  Files        9487     9447      -40     
  Lines      538638   537897     -741     
  Branches    21200    21000     -200     
==========================================
+ Hits       457766   471490   +13724     
+ Misses      80430    66043   -14387     
+ Partials      442      364      -78

cathteng

i'm not familiar with the threshold type -> detector trigger update logic, but i do have some other questions

src/sentry/workflow_engine/migration_helpers/alert_rule.py

cathteng · 2025-01-14T17:10:50Z

src/sentry/workflow_engine/migration_helpers/alert_rule.py

@@ -374,6 +375,99 @@ def migrate_alert_rule(
    )


+def update_migrated_alert_rule(alert_rule: AlertRule, updated_fields: dict[str, Any]) -> (


will we update the workflow + actions here too?

These are dual updated with the AlertRuleTriggers/AlertRuleActions

src/sentry/workflow_engine/migration_helpers/alert_rule.py

cathteng · 2025-01-14T17:15:30Z

src/sentry/workflow_engine/migration_helpers/alert_rule.py

+            )
+            resolve_threshold_type = (
+                Condition.LESS_OR_EQUAL
+                if updated_fields["threshold_type"] == AlertRuleThresholdType.ABOVE.value


should this be resolve_threshold_type?

No, the resolve threshold type is the opposite as the specified threshold type

I think this is just "less" and "greater" rather than with "or equal" for both - https://github.com/getsentry/sentry/blob/master/src/sentry/workflow_engine/migration_helpers/alert_rule.py#L208

i think this logic is overall correct - it's inverting the logic type from the alert rule to create the resolution condition; for example: greater than 500 is Priority.HIGH would have a resolution threshold: less than or equal to 500 is Priority.RESOLVE -- and that means we don't need to do the +/- 0.000001 trick from before.

since the logic is always > or < in metric alerts, we can safely assume the equality operator should be applied for the inverse with resolution.

cathteng · 2025-01-14T17:16:47Z

src/sentry/workflow_engine/migration_helpers/alert_rule.py

+            resolve_condition = data_conditions.filter(condition_result=DetectorPriorityLevel.OK)
+            resolve_condition.update(comparison=updated_fields["resolve_threshold"])
+
+    detector.update(**updated_detector_fields)


if this fails, should the alert rule be updated? should updating the alert rule + the detector happen in a transaction so either both or neither is updated?

That makes sense

@cathteng how is that done if you're updating 2 models? cause usually it's like with transaction.atomic(router.db_for_write(MyModel)):

cathteng · 2025-01-14T17:17:40Z

src/sentry/workflow_engine/migration_helpers/alert_rule.py

@@ -374,6 +375,99 @@ def migrate_alert_rule(
    )


+def update_migrated_alert_rule(alert_rule: AlertRule, updated_fields: dict[str, Any]) -> (


how will you get these updated fields?

Dual updated with the alert rule

ok because the tests make it seem like you're generating them manually so i wasn't sure

Ah yeah, we're just testing the actual dual writing code separately so that the tests in test_logic.py can be cleaner (we also haven't hooked up the dual writing logic yet)

ceorourke · 2025-01-15T17:47:12Z

tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py

        """
-        metric_alert = self.create_alert_rule(threshold_type=AlertRuleThresholdType.ABOVE)
+        metric_alert = self.create_alert_rule(threshold_type=AlertRuleThresholdType.BELOW)


why has this switched?

The default threshold type is ABOVE, so I changed it in this test to BELOW so we're not testing the same case twice. The test wasn't running because I named it incorrectly, so I missed that it was failing the test for the resolve threshold being GTE (happens when the threshold type is BELOW)

tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py

src/sentry/workflow_engine/migration_helpers/alert_rule.py

saponifi3d · 2025-01-15T19:05:53Z

src/sentry/workflow_engine/migration_helpers/alert_rule.py

+            )
+            resolve_threshold_type = (
+                Condition.LESS_OR_EQUAL
+                if updated_fields["threshold_type"] == AlertRuleThresholdType.ABOVE.value


i think this logic is overall correct - it's inverting the logic type from the alert rule to create the resolution condition; for example: greater than 500 is Priority.HIGH would have a resolution threshold: less than or equal to 500 is Priority.RESOLVE -- and that means we don't need to do the +/- 0.000001 trick from before.

since the logic is always > or < in metric alerts, we can safely assume the equality operator should be applied for the inverse with resolution.

ceorourke · 2025-01-16T21:22:01Z

src/sentry/workflow_engine/migration_helpers/alert_rule.py

+    detector_state.update(active=False, state=DetectorPriorityLevel.OK)
+
+    # TODO: do we need to create an audit log entry here?
+    # do we need to return the detector's data conditions/DCG?


re: audit log we probably should but I think we can punt that to another task as we'll need to create new audit log types entirely for the whole project

idk that we need to return the dc / dcg here since the user hitting this code path will be unaware of ACI as a whole, so it's not like we need to return this data in the api response. we will need to make a new serializer when we switch over though, so maybe then? I wouldn't worry about it right now though

ceorourke

lgtm besides adding a transaction which I think would be nice

Add helper method to update the relevant ACI tables after updating an alert rule. Also fixes some tests that weren't running because I named them incorrectly in my last PR 😅 --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>

wip

4a7fa6e

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

🛠️ apply pre-commit fixes

17794d3

vercel bot deployed to Preview January 7, 2025 00:09 View deployment

more progress

782c669

vercel bot deployed to Preview January 8, 2025 00:42 View deployment

don't forget threshold_type

87b43cf

vercel bot deployed to Preview January 8, 2025 00:52 View deployment

add data condition stuff

3194c4c

vercel bot deployed to Preview January 8, 2025 20:17 View deployment

mifu67 and others added 2 commits January 10, 2025 11:28

Merge branch 'master' into mifu67/aci/dual-update-helpers

449e805

🛠️ apply pre-commit fixes

e582567

vercel bot deployed to Preview January 10, 2025 19:37 View deployment

mifu67 added 3 commits January 10, 2025 13:03

add back logging statements

e62708d

changes to update fn

791918e

remove brain dump

9db54c7

vercel bot deployed to Preview January 11, 2025 00:47 View deployment

test changes to detector

ac4959e

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

Merge branch 'master' into mifu67/aci/dual-update-helpers

bad3440

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

tests and fixes

a274225

mifu67 requested review from ceorourke and saponifi3d January 13, 2025 22:26

mifu67 marked this pull request as ready for review January 13, 2025 22:26

mifu67 requested a review from a team as a code owner January 13, 2025 22:26

vercel bot deployed to Preview January 13, 2025 22:29 View deployment

mypy ¯\_(ツ)_/¯

317a505

vercel bot deployed to Preview January 13, 2025 23:17 View deployment

cathteng reviewed Jan 14, 2025

View reviewed changes

PR comments + oops

237300a

vercel bot deployed to Preview January 14, 2025 18:46 View deployment

ceorourke reviewed Jan 15, 2025

View reviewed changes

tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py Show resolved Hide resolved

saponifi3d reviewed Jan 15, 2025

View reviewed changes

Merge branch 'master' into mifu67/aci/dual-update-helpers

5703fff

vercel bot deployed to Preview January 16, 2025 18:23 View deployment

PR comments

37a19bc

mifu67 requested review from ceorourke, saponifi3d and a team January 16, 2025 18:42

vercel bot deployed to Preview January 16, 2025 18:46 View deployment

ceorourke reviewed Jan 16, 2025

View reviewed changes

ceorourke approved these changes Jan 16, 2025

View reviewed changes

cleanup

7a47aca

mifu67 enabled auto-merge (squash) January 16, 2025 21:33

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

mifu67 merged commit e1cf8b5 into master Jan 16, 2025
49 checks passed

mifu67 deleted the mifu67/aci/dual-update-helpers branch January 16, 2025 22:00

mifu67 restored the mifu67/aci/dual-update-helpers branch January 23, 2025 19:20

mifu67 deleted the mifu67/aci/dual-update-helpers branch January 23, 2025 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aci milestone 3): ACI alert rule dual update helper #82982

feat(aci milestone 3): ACI alert rule dual update helper #82982

mifu67 commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 7, 2025 •

edited

Loading

cathteng left a comment

cathteng Jan 14, 2025

mifu67 Jan 14, 2025

cathteng Jan 14, 2025

mifu67 Jan 14, 2025

ceorourke Jan 15, 2025 •

edited

Loading

saponifi3d Jan 15, 2025

cathteng Jan 14, 2025

mifu67 Jan 14, 2025

ceorourke Jan 16, 2025

cathteng Jan 14, 2025

mifu67 Jan 14, 2025

cathteng Jan 14, 2025

mifu67 Jan 14, 2025

ceorourke Jan 15, 2025

mifu67 Jan 15, 2025

saponifi3d Jan 15, 2025

ceorourke Jan 16, 2025

ceorourke left a comment

		@@ -374,6 +375,99 @@ def migrate_alert_rule(
		)


		def update_migrated_alert_rule(alert_rule: AlertRule, updated_fields: dict[str, Any]) -> (

feat(aci milestone 3): ACI alert rule dual update helper #82982

feat(aci milestone 3): ACI alert rule dual update helper #82982

Conversation

mifu67 commented Jan 7, 2025 • edited Loading

codecov bot commented Jan 7, 2025 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

ceorourke Jan 15, 2025 • 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

Choose a reason for hiding this comment

ceorourke left a comment

Choose a reason for hiding this comment

mifu67 commented Jan 7, 2025 •

edited

Loading

codecov bot commented Jan 7, 2025 •

edited

Loading

ceorourke Jan 15, 2025 •

edited

Loading