Skip to content
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): Add the ability to manipulate the automatic killswitch for the LPQ #28757

Merged
merged 16 commits into from
Oct 1, 2021

Conversation

relaxolotl
Copy link
Contributor

@relaxolotl relaxolotl commented Sep 22, 2021

Builds off of #28565 changes. Broken out of #28714 to reduce the size of that PR's diff.

This adds in methods to RealtimeMetricsStore to fetch, add, and remove projects from the automated kill switch (automated option?) stored on redis which specifies which projects should be sent to symbolicator's low priority queue.

This does not include code to actually use these methods. Actual usages of the code in this PR will be included in #28714.

Open questions have been left as TODOs on the code, and feedback on those would be much appreciated. I also haven't done anything explicit to ensure any sort of locking on this value, which may be a problem. I'm currently unsure if this is something that's already taken care of by Redis, or if additional work needs to be done to ensure that this value is being safely manipulated in redis.

Comment on lines 90 to 91
if len(project_ids) == 0:
return set()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this special case actually matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code below can definitely handle this - it's mostly a preoptimization so we skip talking to redis entirely if for some reason no project IDs are fed in. i'm assuming it's relatively costly to perform that round trip but i could be wrong and this specific check isn't necessary.

@relaxolotl relaxolotl force-pushed the feat/lpq/redis-killswitch branch from ab5cb38 to c6e4437 Compare September 22, 2021 19:44
@relaxolotl relaxolotl force-pushed the feat/lpq/redis-killswitch branch from becce7c to 6605e67 Compare September 22, 2021 19:53
@relaxolotl relaxolotl force-pushed the feat/lpq/redis-killswitch branch from 9efab96 to 804cbbf Compare September 22, 2021 23:17
@relaxolotl relaxolotl force-pushed the feat/lpq/redis-killswitch branch from 5f4e036 to 20af86a Compare September 23, 2021 23:12
Base automatically changed from feat/low-priority-queue-counter to master September 28, 2021 11:45
@loewenheim loewenheim requested a review from a team September 28, 2021 11:45
@relaxolotl relaxolotl merged commit 5b26ee2 into master Oct 1, 2021
@relaxolotl relaxolotl deleted the feat/lpq/redis-killswitch branch October 1, 2021 22:54
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants