ref(aci): remove passing in detector to action.trigger attempt 2 #103099

cathteng · 2025-11-10T21:54:23Z

But adds in fetching the detector correctly for Activities. Detector is specifically needed to send activity notifications correctly for metric issue resolution

cursor · 2025-11-10T21:56:33Z

src/sentry/workflow_engine/processors/detector.py

+        return Detector.objects.get(project_id=group.project_id, type=group.issue_type.slug)
+    except Detector.DoesNotExist:
+        # return issue stream detector
+        return Detector.objects.get(project_id=group.project_id, type=IssueStreamGroupType.slug)


Bug: Error handling: Logging noise and crashes

The get_detector_by_group function uses logger.exception() for expected control flow when DetectorGroup.DoesNotExist is raised. This logs full stack traces to error tracking for normal fallback behavior, polluting monitoring systems. Additionally, the final fallback Detector.objects.get(project_id=group.project_id, type=IssueStreamGroupType.slug) can raise an unhandled Detector.DoesNotExist exception if no issue stream detector exists for the project, causing crashes instead of graceful error handling.

it should crash if there's no detector at the end

codecov · 2025-11-10T22:12:11Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
29667	1	29666	243

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

tests.sentry.workflow_engine.endpoints.test_organization_test_fire_action.TestFireActionsEndpointTest::test_pagerduty_action

Stack Traces | 4.21s run time

#x1B[1m#x1B[.../workflow_engine/endpoints/test_organization_test_fire_action.py#x1B[0m:103: in test_pagerduty_action
    assert pagerduty_data["payload"]["summary"].startswith(
#x1B[1m#x1B[31mE   AssertionError: assert False#x1B[0m
#x1B[1m#x1B[31mE    +  where False = <built-in method startswith of str object at 0x7f35fc45c150>('[Cool Gibbon]:')#x1B[0m
#x1B[1m#x1B[31mE    +    where <built-in method startswith of str object at 0x7f35fc45c150> = '[Golden Hare]: This is an example None exception'.startswith#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-11-10T22:57:58Z

src/sentry/workflow_engine/processors/detector.py

+        pass
+
+    try:
+        return Detector.objects.get(project_id=group.project_id, type=group.issue_type.slug)


What other types of detectors (besides the issue stream type handled later) are there only 1 per project? Is it just the error detector type or are there others?

The performance detectors will probably be 1 per project(?)

Ok, so there's an assumption that anything found for DetectorGroup is for metric issues, then it tries the other types and finally falls back to issue stream group types? Would an uptime or cron detector potentially hit a MultipleObjectsReturned? Maybe it'd be safer to pass a list of types

I would think uptime and crons hit DetectorGroup too

I updated it so that this is only used for activity notifications (e.g. resolve for metric issues)

We only process updates if the detector_id is attached to the StatusChangeMessage

sentry/src/sentry/workflow_engine/handlers/workflow/workflow_status_update_handler.py

Lines 37 to 43 in 646bc83

detector_id = status_change_message.get("detector_id")

if detector_id is None:

# We should not hit this case, it's should only occur if there is a bug

# passing it from the workflow_engine to the issue platform.

metrics.incr("workflow_engine.tasks.error.no_detector_id")

return

If detector_id exists, I believe it must have existed when the occurrence to create the issue was sent to issue platform, meaning we would have already written the DetectorGroup row

sentry/src/sentry/issues/ingest.py

Lines 256 to 257 in 646bc83

if is_new and occurrence.evidence_data and "detector_id" in occurrence.evidence_data:

associate_new_group_with_detector(group, occurrence.evidence_data["detector_id"])

src/sentry/workflow_engine/processors/detector.py

kcons · 2025-11-13T00:25:49Z

src/sentry/workflow_engine/models/action.py

+    def trigger(self, event_data: WorkflowEventData) -> None:
+        from sentry.workflow_engine.processors.detector import get_detector_by_group
+
+        detector = get_detector_by_group(event_data.group)


Have we considered tracking how often a Detector is found here?
We know coverage isn't 100%, so while this is probably going to work for newer groups, maybe not older ones.

Oops, I should update this to also use either get_detector_by_group or get_detector_by_event depending on whether the event is a GroupEvent or Activity

We will only send the activity through workflow engine if a detector_id exists in issue platform

sentry/src/sentry/workflow_engine/handlers/workflow/workflow_status_update_handler.py

Lines 39 to 49 in e5270d6

if detector_id is None:

# We should not hit this case, it's should only occur if there is a bug

# passing it from the workflow_engine to the issue platform.

metrics.incr("workflow_engine.tasks.error.no_detector_id")

return

process_workflow_activity.delay(

activity_id=activity.id,

group_id=group.id,

detector_id=detector_id,

)

I am assuming that if that's the case, then we would have already written the DetectorGroup row

Tracking is good, yes

kcons

My main concern here is that we have known incompleteness of DetectorGroup that we were investigating yesterday, so moving to rely on it exclusively for action triggering will be a bit of a regression.
For error it's nbd because there's an easy fallback, but for others it seems worth being confident on coverage of DetectorGroup before making it load-bearing.

kcons · 2025-11-13T19:25:07Z

src/sentry/workflow_engine/processors/detector.py

+        )
+        raise Detector.DoesNotExist("Detector not found for event data")
+
+    raise TypeError(f"Cannot determine the detector from {type(event_data.event)}.")


this makes more sense as an else above.

kcons · 2025-11-13T19:26:38Z

src/sentry/workflow_engine/processors/detector.py

+                "group_id": event_data.group.id,
+            },
+        )
+        raise Detector.DoesNotExist("Detector not found for event data")


It seems slightly better to re-raise, as we've already sent an exception to sentry, and if we reraise and someone tries to send it again it should be dropped by dedup, vs raising a new one which will result in potential double-reporting.

kcons · 2025-11-13T19:27:38Z

src/sentry/workflow_engine/processors/detector.py

+    try:
+        return DetectorGroup.objects.get(group=group).detector
+    except DetectorGroup.DoesNotExist:
+        logger.exception(


This'll probably block deploy. We know DetectorGroup coverage to be incomplete for metric and error issues.
I think it's okay to report that, just making sure we're aware.

+1, this will fail to fetch detectors for most metric issue groups.

kcons · 2025-11-13T19:37:16Z

src/sentry/workflow_engine/processors/detector.py

+
+    try:
+        return Detector.objects.get(project_id=group.project_id, type=group.issue_type.slug)
+    except Detector.DoesNotExist:


In my mind, this sort of logic wouldn't need to depend on our awareness of current group type conventions..
it'd be something like

if detector_type := group.issue_type.singleton_detector_type:
return Detector.objects.get(....)

that is, each type explicitly is configured to use a detectors or not. The scheme for detector association being knowable based on GroupType seems like a nice thing to have. "Which groups use the issue stream detector?" would be answerable with a grep.

mifu67

I know you're already aware of this, but just calling out that we shouldn't merge this until the backfill is done, or metric actions will break.

mifu67 · 2025-11-13T21:22:04Z

src/sentry/workflow_engine/processors/detector.py

+    try:
+        return DetectorGroup.objects.get(group=group).detector
+    except DetectorGroup.DoesNotExist:
+        logger.exception(


+1, this will fail to fetch detectors for most metric issue groups.

cathteng requested review from a team as code owners November 10, 2025 21:54

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

vercel bot deployed to Preview November 10, 2025 21:56 View deployment

cursor bot reviewed Nov 10, 2025

View reviewed changes

cathteng requested review from ceorourke and mifu67 November 10, 2025 22:21

vercel bot deployed to Preview November 10, 2025 22:36 View deployment

ceorourke reviewed Nov 10, 2025

View reviewed changes

cathteng requested a review from saponifi3d November 10, 2025 23:16

cathteng added 6 commits November 11, 2025 14:46

remove passing in detector to action.trigger

afd75ab

fix tests

96eb655

remove project_id as actions task param

f935f11

update comment

72a7ee7

handle activity notifications requiring detector

9fa77bc

fix tests

065ac2a

cathteng force-pushed the cathy/aci/remove-detector-action-trigger branch from c64e0f8 to 065ac2a Compare November 11, 2025 22:48

cursor bot reviewed Nov 11, 2025

View reviewed changes

src/sentry/workflow_engine/processors/detector.py Show resolved Hide resolved

vercel bot deployed to Preview November 11, 2025 22:51 View deployment

cathteng mentioned this pull request Nov 11, 2025

feat(aci): associate groups without matching detector to the issue stream detector #103191

Closed

fetch detector depending on groupevent or activity

646bc83

vercel bot deployed to Preview November 11, 2025 23:58 View deployment

kcons self-requested a review November 12, 2025 19:25

kcons reviewed Nov 13, 2025

View reviewed changes

cathteng added 3 commits November 12, 2025 16:31

get detector from event data

78e552e

logging

3d613a4

log name

c197ba3

vercel bot deployed to Preview November 13, 2025 00:36 View deployment

fix mock

e256097

vercel bot deployed to Preview November 13, 2025 00:56 View deployment

fix test

4e4b3da

vercel bot deployed to Preview November 13, 2025 01:49 View deployment

cathteng requested a review from a team November 13, 2025 17:39

kcons reviewed Nov 13, 2025

View reviewed changes

mifu67 reviewed Nov 13, 2025

View reviewed changes

	detector_id = status_change_message.get("detector_id")

	if detector_id is None:
	# We should not hit this case, it's should only occur if there is a bug
	# passing it from the workflow_engine to the issue platform.
	metrics.incr("workflow_engine.tasks.error.no_detector_id")
	return

	if is_new and occurrence.evidence_data and "detector_id" in occurrence.evidence_data:
	associate_new_group_with_detector(group, occurrence.evidence_data["detector_id"])

Uh oh!

ref(aci): remove passing in detector to action.trigger attempt 2 #103099

Are you sure you want to change the base?

ref(aci): remove passing in detector to action.trigger attempt 2 #103099

Uh oh!

Conversation

cathteng commented Nov 10, 2025

Uh oh!

cursor bot Nov 10, 2025

Choose a reason for hiding this comment

Bug: Error handling: Logging noise and crashes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

❌ 1 Tests Failed:

Uh oh!

ceorourke Nov 10, 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!

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!

Choose a reason for hiding this comment

Uh oh!

kcons left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mifu67 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Nov 10, 2025 •

edited

Loading

ceorourke Nov 10, 2025 •

edited

Loading