-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(aci): remove passing in detector to action.trigger attempt 2 #103099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
afd75ab
96eb655
f935f11
72a7ee7
9fa77bc
065ac2a
646bc83
78e552e
3d613a4
c197ba3
e256097
4e4b3da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |||||||||||||||||||
| from sentry.issues.issue_occurrence import IssueOccurrence | ||||||||||||||||||||
| from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka | ||||||||||||||||||||
| from sentry.locks import locks | ||||||||||||||||||||
| from sentry.models.activity import Activity | ||||||||||||||||||||
| from sentry.models.group import Group | ||||||||||||||||||||
| from sentry.models.project import Project | ||||||||||||||||||||
| from sentry.services.eventstore.models import GroupEvent | ||||||||||||||||||||
|
|
@@ -139,6 +140,47 @@ def get_detector_by_event(event_data: WorkflowEventData) -> Detector: | |||||||||||||||||||
| return detector | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def get_detector_by_group(group: Group) -> Detector: | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| return DetectorGroup.objects.get(group=group).detector | ||||||||||||||||||||
| except DetectorGroup.DoesNotExist: | ||||||||||||||||||||
| logger.exception( | ||||||||||||||||||||
| "DetectorGroup not found for group", | ||||||||||||||||||||
| extra={"group_id": group.id}, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| pass | ||||||||||||||||||||
|
|
||||||||||||||||||||
| try: | ||||||||||||||||||||
| return Detector.objects.get(project_id=group.project_id, type=group.issue_type.slug) | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The performance detectors will probably be 1 per project(?)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so there's an assumption that anything found for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think uptime and crons hit DetectorGroup too
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated it so that this is only used for activity notifications (e.g. resolve for metric issues) We only process updates if the sentry/src/sentry/workflow_engine/handlers/workflow/workflow_status_update_handler.py Lines 37 to 43 in 646bc83
If sentry/src/sentry/issues/ingest.py Lines 256 to 257 in 646bc83
cathteng marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
| except Detector.DoesNotExist: | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my mind, this sort of logic wouldn't need to depend on our awareness of current group type conventions.. if detector_type := group.issue_type.singleton_detector_type: 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. |
||||||||||||||||||||
| # return issue stream detector | ||||||||||||||||||||
| return Detector.objects.get(project_id=group.project_id, type=IssueStreamGroupType.slug) | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Error handling: Logging noise and crashesThe
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should crash if there's no detector at the end |
||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| def get_detector_from_event_data(event_data: WorkflowEventData) -> Detector: | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| if isinstance(event_data.event, GroupEvent): | ||||||||||||||||||||
| return get_detector_by_event(event_data) | ||||||||||||||||||||
| elif isinstance(event_data.event, Activity): | ||||||||||||||||||||
| return get_detector_by_group(event_data.group) | ||||||||||||||||||||
| except Detector.DoesNotExist: | ||||||||||||||||||||
| logger.exception( | ||||||||||||||||||||
| "Detector not found for event data", | ||||||||||||||||||||
| extra={ | ||||||||||||||||||||
| "type": type(event_data.event), | ||||||||||||||||||||
| "id": ( | ||||||||||||||||||||
| event_data.event.event_id | ||||||||||||||||||||
| if isinstance(event_data.event, GroupEvent) | ||||||||||||||||||||
| else event_data.event.id | ||||||||||||||||||||
| ), | ||||||||||||||||||||
| "group_id": event_data.group.id, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| raise Detector.DoesNotExist("Detector not found for event data") | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||
|
|
||||||||||||||||||||
| raise TypeError(f"Cannot determine the detector from {type(event_data.event)}.") | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this makes more sense as an |
||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| class _SplitEvents(NamedTuple): | ||||||||||||||||||||
| events_with_occurrences: list[tuple[GroupEvent, int]] | ||||||||||||||||||||
| error_events: list[GroupEvent] | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, this will fail to fetch detectors for most metric issue groups.