feat(perf_issues): Add post_process pipeline for performance issues#39895
Conversation
wedamija
requested review from
barkbarkimashark,
ceorourke,
rjo100 and
udameli
github-actions
Bot
added
the
Scope: Backend
There was a problem hiding this comment.
I spent a while trying to generate these cases dynamically but couldn't figure it out in a reasonable amount of time. It might be easier using pure pytest instead of unittest
|
How does |
That's kind of what we need to work out. The code here https://github.com/getsentry/sentry/blob/d86d5b669653d0a025813f7a703d43b7d8603f14/src/sentry/tasks/post_process.py#L688-L703 looks like:
I'm also not really sure if we even want to send performance issues here at all, but I don't know what the downstream effects are there. |
|
@wedamija re: |
That's kind of how I feel too. I think that for sure we don't want to send transactions here, since they're too high volume. Allowing users to control which issue types are sent probably makes sense both here and for service hooks, although I know nothing about how users actually configure these. I'm ok with putting this off as well |
This adds in a post process pipeline for performance issues. We don't need most steps, just snoozes, inbox and processing issue alert rules. We might want to add assignment in the future if we update the system to be able to match on parts of the transaction. Same for suspect commits if we can figure out how to make tha twork. `update_existing_attachments` doesn't make sense for perf issues since there can be multiple issues per transaction, and this function assumes that there's only one issue. We don't have attachments to worry about here, afaik, so if this requirement comes up we can add it later. We'll want to enable `process_plugins` in the future, but for now we can keep it disabled to limit any noise. We'll probably want to enable `process_service_hooks` and `process_resource_change_bounds` later once we understand if we want to send them to the integrations.
wedamija
force-pushed
the
danf/perf_issues_post_process_pipeline
branch
from
d86d5b6 to
c1b3615
Compare
| group_states = ( | ||
| [ | ||
| { | ||
| "id": group_id, | ||
| "is_new": is_new, | ||
| "is_regression": is_regression, | ||
| "is_new_group_environment": is_new_group_environment, | ||
| } | ||
| ] | ||
| if group_id | ||
| else None | ||
| ) |
There was a problem hiding this comment.
Hmm, any reason not to add group_states as an optional arg on call_post_process_group.
The test PostProcessGroupPerformanceTest.test_full_pipeline_with_group_states passes in two GroupState objects but this won't support that.
There was a problem hiding this comment.
I suppose we could allow group_states to be passed as an override. tbh I was just calling it directly from that test since it's a special case
3 participants
This adds in a post process pipeline for performance issues. We don't need most steps, just snoozes, inbox and processing issue alert rules.
We might want to add assignment in the future if we update the system to be able to match on parts of the transaction. Same for suspect commits if we can figure out how to make tha twork.
update_existing_attachmentsdoesn't make sense for perf issues since there can be multiple issues per transaction, and this function assumes that there's only one issue. We don't have attachments to worry about here, afaik, so if this requirement comes up we can add it later.We'll want to enable
process_pluginsin the future, but for now we can keep it disabled to limit any noise.We'll probably want to enable
process_service_hooksandprocess_resource_change_boundslater once we understand if we want to send them to the integrations.