feat(symbolicator): Register manual kill switches for symbolicator's low priority queue #28576
Conversation
src/sentry/options/defaults.py
Outdated
| register("store.load-shed-process-event-projects", type=Any, default=[]) | ||
| register("store.load-shed-symbolicate-event-projects", type=Any, default=[]) | ||
| register("store.symbolicate-event-dlq-never", type=Any, default=[]) | ||
| register("store.symbolicate-event-dlq-always", type=Any, default=[]) |
There was a problem hiding this comment.
I went looking at the code of register and now wonder why type=Any? It seems if you don't provide a type it'll derive it from the default arg and end up with Sequence?
There was a problem hiding this comment.
The other reason I went looking at what register is and how options works is because of the [] default. I guess this is the actual type we'd put in there, but if we fill this with project IDs this becomes O(n) to find if your project is in this list. That's not good.
A Python set() would fix this easily, but seems that's not a thing in sentry options.
This also led me to look even further into this and... ugh. We can only set() and get() this list. This is maybe fine for the killswitches but probably not for the -selected variation that'll come later. If we need to stick with options for this we'd have to implement external locking for the set part.
There was a problem hiding this comment.
I went looking at the code of
registerand now wonder whytype=Any? It seems if you don't provide a type it'll derive it from thedefaultarg and end up withSequence?
I'll admit I mostly followed the example of the existing kill switches that use the same implementation and just copied what they did, but I don't see there being any issues with being explicit about the type on these two options. I'll use Sequence in this case.
If we need to stick with options for this we'd have to implement external locking for the set part.
Would you happen to have any ideas on where or how else you'd rather see this implemented? I'm starting to develop the conclusion that options may not be the best place to implement the behaviour we want from these "options" for the LPQ, especially when it comes to the -selected list. I want to avoid having to build something from scratch, but I'm unsure about what alternatives we have to options if there are any.
There was a problem hiding this comment.
From talking to people I'm coming to the conclusion that these two which are actually killswitches should probably stay with this and they will have to swallow the O(n) cost. For the selected list we can move to redis as storage backend for this.
…pre-processing events (#28586) This adds in logic to begin reading and making decisions based on the manual kill switches defined in #28576. The PR also introduces a new queue meant for reprocessing low priority queue events in order to prevent projects pushed to the low priority queue from promoting their events to the regular queue via reprocessing requests.
kill switches added:
store.symbolicate-event-lpq-neverstore.symbolicate-event-lpq-alwaysThis registers the two manual kill switches meant for ops use. As
mentioned in their description, adding a project to these switches
will either always or never redirect all symbolication(?) events for
that project towards symbolicator's low priority queue. The low
priority queue provides "worse" guarantees on how quickly an
event will be processed, i.e. it will take longer for an event to be
completed in the low priority queue.
I've added @untitaker as a reviewer because this uses killswitch
infra that he's most familiar with, being its author and all that. It
would be helpful to know whether I'm using that correctly, and
if it's better to use the "legacy" kill switch for what I'm trying to
accomplish here.
Note that this is merely the registration step. Actual use of this
switch will follow in a second PR.