ref(aci): remove passing in detector to action.trigger #103000
Conversation
github-actions
bot
added
the
Scope: Backend
|
|
||
| project_id = detector.project_id | ||
| if project_id is None: | ||
| project_id = detector.project_id |
There was a problem hiding this comment.
The other use for detector here is to fetch the project id and to get logs using detector.type. We can replace these with get_detector_by_event and passing in project_id to the task
There was a problem hiding this comment.
Do you need to check that detector exists since it's optional now?
There was a problem hiding this comment.
We're still passing in detector_id to the task in this PR, but yeah can check if one of them exists
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #103000 +/- ##
===========================================
+ Coverage 79.62% 80.66% +1.03%
===========================================
Files 9137 9141 +4
Lines 392777 392887 +110
Branches 24966 24966
===========================================
+ Hits 312768 316934 +4166
+ Misses 79608 75552 -4056
Partials 401 401 |
| raise ValueError("Exactly one of event_id or activity_id must be provided") | ||
|
|
||
| if detector is None: | ||
| detector = get_detector_by_event(event_data) |
There was a problem hiding this comment.
Bug: Event type mismatch breaks activity processing.
When activity_id is provided with project_id but without detector_id, the code calls get_detector_by_event with an Activity event, but get_detector_by_event only accepts GroupEvent and will raise a TypeError. The fallback to get_detector_by_event at line 128-129 assumes the event is always a GroupEvent, but when processing activities (line 117-119), the event_data contains an Activity object instead.
There was a problem hiding this comment.
fire action shouldn't work anyway, so I think this is fine? Not sure of the status of activity notifications
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
overall looking good, wonder if we can remove the project_id as well, and just have everything based off of the group info.
| has_reappeared: bool, | ||
| has_escalated: bool, | ||
| detector_id: int | None = None, | ||
| project_id: int | None = None, |
There was a problem hiding this comment.
🤔 could we just get the project_id from the group? https://github.com/getsentry/sentry/blob/master/src/sentry/models/group.py#L619
That would allow us to simplify this signature / decouple another model as well. My thought process here is "we want to notify about an issue" -- so i'm just trying to pare it down to the fewest dependencies needed for that. if the action needs additional data from other areas, it can look up that information as needed per action.
| # Set up a timeout grouping context because we want to make sure any Sentry timeout reporting | ||
| # in this scope is grouped properly. | ||
| with timeout_grouping_context(action.type): | ||
| action.trigger(event_data, detector) |
| has_reappeared: bool, | ||
| has_escalated: bool, | ||
| start_timestamp_seconds: float | None = None, | ||
| project_id: int | None = None, |
There was a problem hiding this comment.
looks like we could omit this here now too 🎉 --dependencies;
mifu67
left a comment
mifu67
left a comment
There was a problem hiding this comment.
Wanted to confirm that there are currently no required changes to the charts?
|
@mifu67 no changes |
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
|
PR reverted: a1e05e7 |
…)" This reverts commit bfaacc5. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>
…)" This reverts commit bfaacc5. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>
…)" This reverts commit bfaacc5. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>
This begins decoupling having the
detectorbe prop drilled fromaction.triggerall the way down to metric alert charts.First we add an optional
project_idfield to thetrigger_actiontask that we will populate in the next PR, and also remove the need fordetectorinaction.trigger