-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(symbolicator): Check manual low priority queue kill switch when pre-processing events #28586
Conversation
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.
i think there's a bunch of tests to fix because of the new options
src/sentry/tasks/store.py
Outdated
symbolicate_event_from_reprocessing_low_priority | ||
# if from_reprocessing | ||
# else symbolicate_event_low_priority |
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.
i was thinking of ignoring reprocessing for the lowpriority and only use it for brand new events. but... maybe that's naive and you're right. It does mean we have to make sure to handle a bunch more cases.
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.
Yeah, my impression was that there's nothing that explicitly prevents events in the low priority queue from being reprocessed. It doesn't make sense to promote reprocessed LPQ events up to the regular queue for obvious reasons, and my understanding was that we deliberately have a separate code path for reprocessing for metrics or tracking purposes.
What are some of the cases you're thinking of as a result of this? I'll admit that I have a fairly surface-level understanding of reprocessing right now. I'm assuming this diff doesn't cover most of them.
e17f37f
to
86f35ac
Compare
86f35ac
to
c34dbdf
Compare
…pre-processing events
0127460
to
9c82029
Compare
b8cb387
to
980ed52
Compare
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.
good testing! ❤️
Queue( | ||
"events.reprocessing.symbolicate_event_low_priority", | ||
routing_key="events.reprocessing.symbolicate_event_low_priority", | ||
), |
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.
we will need ops to make sure that this queue is handled exclusively by the same workers which handle the events.symbolicate_event_low_priority
queue.
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.
good point, thanks for the heads up! when you phrase it this way it seems like maybe it's better to just throw reprocessing requests back into the LPQ... i'll start a conversation about this and change this later if it turns out that would be preferable. i assume by having a separate queue for reprocessing requests we could least easily drop everything in that queue without negatively impacting new requests.
Starter work to push symbolicator events to the low priority queue when the appropriate manual kill switch is specified.
Some code is commented out because it relies on changes made in #28561.
This follows up on the work done in #28576; This PR is the companion that finally uses the switches registered in #28576.