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

[cathteng]

Same as #103000

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

[codecov]

Codecov Report

❌ Patch coverage is 80.95238% with 8 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/workflow_engine/processors/detector.py	77.27%	5 Missing ⚠️
src/sentry/workflow_engine/tasks/actions.py	75.00%	2 Missing ⚠️
src/sentry/api/endpoints/project_rule_actions.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           master   #103099       +/-   ##
============================================
+ Coverage   66.02%    80.44%   +14.42%     
============================================
  Files        9256      9267       +11     
  Lines      395040    396589     +1549     
  Branches    25212     25212               
============================================
+ Hits       260808    319035    +58227     
+ Misses     133803     77125    -56678     
  Partials      429       429               

[ceorourke]

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?

[cathteng]

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

[ceorourke]

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

[cathteng]

I would think uptime and crons hit DetectorGroup too

[cathteng]

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"])

[kcons]

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.

[cathteng]

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

[cathteng]

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]

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.

[cathteng]

@kcons @mifu67 the backfill has been completed

[kcons]

I think my concerns were addressed.

[kcons]

I think this merits a comment "Returns Detector associated with this group, either based on DetectorGroup, (project, type), or if those fail, returns the Issue Stream detector". or something.