feat(ACI): Update metric issue occurrence #93368

ceorourke · 2025-06-11T20:25:52Z

The main change from #92702 with some slight changes I'll call out in comments.

codecov · 2025-06-11T20:41:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #93368      +/-   ##
==========================================
+ Coverage   85.77%   85.82%   +0.04%     
==========================================
  Files       10286    10307      +21     
  Lines      593019   594318    +1299     
  Branches    23028    23028              
==========================================
+ Hits       508677   510045    +1368     
+ Misses      83732    83663      -69     
  Partials      610      610

ceorourke · 2025-06-12T22:31:06Z

src/sentry/workflow_engine/handlers/detector/stateful.py

@@ -415,6 +415,7 @@ def _build_detector_evaluation_result(
            )

            # Set the event data with the necessary fields
+            event_data["environment"] = self.detector.config.get("environment")


The event schema expects environment to be set so we may as well set it on the base class

This might be a little tricky since not every detector would have to store the env in the config. is it okay if it's set to None is some cases? if so, then this seems good 👍

I hard coded it to none and re-ran my tests so it seems ok! According to the docs it must be set.

tests/sentry/workflow_engine/handlers/detector/test_metric_issue_detector_handler.py

ceorourke · 2025-06-12T22:59:22Z

src/sentry/incidents/grouptype.py

+                assignee=assignee,
+                priority=priority,
+            ),
+            {},


In the previous attempt I was setting some event data:

sentry/src/sentry/incidents/grouptype.py

Lines 83 to 87 in 8303446

event_data = {

"environment": self.detector.config.get("environment"),

"platform": None,

"sdk": None,

} # XXX: may need to add to this

it seems that having a "platform" of None was the main problem and the other ones either aren't necessary or are covered by the base class

ceorourke · 2025-06-12T23:00:19Z

tests/sentry/workflow_engine/handlers/detector/test_metric_issue_detector_handler.py

+    @with_feature("organizations:workflow-engine-metric-alert-processing")
+    @with_feature("organizations:workflow-engine-process-metric-issue-workflows")
+    @patch("sentry.workflow_engine.processors.workflow.process_workflows")
+    def test_occurrence_post_process(self, mock_process_workflows):


This test covers the reason the original PR was reverted - it wasn't getting into post processing and processing the workflows as expected. This test makes sure it does!

saponifi3d

generally looks good, i think we should move the tests into the incident module, and the extra credit bits would be to split stuff out into separate files. (might be best to just split the test out to that file, and then do the handler in another pr)

saponifi3d · 2025-06-13T17:21:30Z

src/sentry/workflow_engine/handlers/detector/stateful.py

@@ -415,6 +415,7 @@ def _build_detector_evaluation_result(
            )

            # Set the event data with the necessary fields
+            event_data["environment"] = self.detector.config.get("environment")


This might be a little tricky since not every detector would have to store the env in the config. is it okay if it's set to None is some cases? if so, then this seems good 👍

saponifi3d · 2025-06-13T17:23:23Z

src/sentry/workflow_engine/handlers/detector/stateful.py

+            if validated_condition_results:
+                new_priority = max(new_priority, *validated_condition_results)


Do we need to add this? it's just checking if there are any items in the list -- which is probably a similar amount of processing power to splat an empty list and max it.

Yeah this is carried over from the last PR, if there are no results because there was an error or the threshold was not met this causes a problem #92702 (comment)

ah, sorry forgot -- thanks for the link!

tests/sentry/workflow_engine/handlers/detector/test_metric_issue_detector_handler.py

saponifi3d · 2025-06-13T17:35:42Z

src/sentry/incidents/grouptype.py

@@ -41,6 +43,12 @@ def create_occurrence(
        data_packet: DataPacket[QuerySubscriptionUpdate],
        priority: DetectorPriorityLevel,
    ) -> tuple[DetectorOccurrence, EventData]:
+        try:


🤔 this class is starting to get a bit large, should we pull it out into its own file and then import it here? That might also make it clearer with tests / what's being tested. Thoughts on something like: incidents/metric_detector.py then incidents/test_metric_detector.py for the code / tests?

I'm running into circular import issues trying to do this :/

ceorourke · 2025-06-13T18:40:51Z

tests/sentry/incidents/test_metric_issue_detector_handler.py

@@ -0,0 +1,262 @@
+from datetime import UTC, datetime, timedelta


diff got kinda large when I moved this because I copy/pasted the tests into new files rather than moving the whole original file and then copy/pasting the post process tests, but nothing has changed. I tried to put it back in the original file and move it to fix up the diff but it didn't detect any changes at that point 🤷

no worries, thanks for moving these into this module.

saponifi3d · 2025-06-13T18:51:02Z

src/sentry/workflow_engine/handlers/detector/stateful.py

+            if validated_condition_results:
+                new_priority = max(new_priority, *validated_condition_results)


ah, sorry forgot -- thanks for the link!

saponifi3d · 2025-06-13T18:52:12Z

tests/sentry/incidents/test_metric_issue_detector_handler.py

@@ -0,0 +1,262 @@
+from datetime import UTC, datetime, timedelta


no worries, thanks for moving these into this module.

The main change from #92702 with some slight changes I'll call out in comments.

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

ceorourke added 3 commits June 11, 2025 13:45

ref(ACI): Update metric issue occurrence

534f213

typo

3946ac2

update base test

68e81cf

ceorourke force-pushed the ceorourke/update-metric-issue-occurrence-take2 branch from eaa4185 to 68e81cf Compare June 11, 2025 20:52

vercel bot deployed to Preview June 11, 2025 20:53 View deployment

update test

d9d7399

vercel bot deployed to Preview June 11, 2025 23:22 View deployment

update test again

517e81e

vercel bot deployed to Preview June 11, 2025 23:55 View deployment

just trying to run the test in CI

4d37944

vercel bot deployed to Preview June 12, 2025 17:52 View deployment

idk man

6ce4a6d

vercel bot deployed to Preview June 12, 2025 21:02 View deployment

test actually works

216fb71

vercel bot deployed to Preview June 12, 2025 22:20 View deployment

ensure we fall into the assignee code

fdb5f02

ceorourke commented Jun 12, 2025

View reviewed changes

tests/sentry/workflow_engine/handlers/detector/test_metric_issue_detector_handler.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview June 12, 2025 22:32 View deployment

typing

cfe6236

vercel bot deployed to Preview June 12, 2025 22:36 View deployment

ceorourke marked this pull request as ready for review June 12, 2025 22:57

ceorourke requested review from a team as code owners June 12, 2025 22:57

ceorourke commented Jun 12, 2025

View reviewed changes

saponifi3d reviewed Jun 13, 2025

View reviewed changes

break tests out into own files

f646557

ceorourke requested a review from saponifi3d June 13, 2025 18:19

vercel bot deployed to Preview June 13, 2025 18:19 View deployment

ceorourke commented Jun 13, 2025

View reviewed changes

saponifi3d approved these changes Jun 13, 2025

View reviewed changes

ceorourke merged commit 07c96c0 into master Jun 13, 2025
64 checks passed

ceorourke deleted the ceorourke/update-metric-issue-occurrence-take2 branch June 13, 2025 18:55

billyvg pushed a commit that referenced this pull request Jun 18, 2025

feat(ACI): Update metric issue occurrence (#93368)

16db5c5

The main change from #92702 with some slight changes I'll call out in comments.

andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025

feat(ACI): Update metric issue occurrence (#93368)

0d245dd

The main change from #92702 with some slight changes I'll call out in comments.

	event_data = {
	"environment": self.detector.config.get("environment"),
	"platform": None,
	"sdk": None,
	} # XXX: may need to add to this

		if validated_condition_results:
		new_priority = max(new_priority, *validated_condition_results)

		@@ -0,0 +1,262 @@
		from datetime import UTC, datetime, timedelta

Uh oh!

feat(ACI): Update metric issue occurrence #93368

feat(ACI): Update metric issue occurrence #93368

Uh oh!

Conversation

ceorourke commented Jun 11, 2025

Uh oh!

codecov bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saponifi3d Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saponifi3d left a comment

Choose a reason for hiding this comment

Uh oh!

saponifi3d Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saponifi3d Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ceorourke Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 11, 2025 •

edited

Loading

saponifi3d Jun 13, 2025 •

edited

Loading

saponifi3d Jun 13, 2025 •

edited

Loading

saponifi3d Jun 13, 2025 •

edited

Loading

ceorourke Jun 13, 2025 •

edited

Loading