-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
IA-98: add rollout for sentry task latency #83244
base: master
Are you sure you want to change the base?
Conversation
# Use a try/catch here to contain the blast radius of an exception being unhandled through the options lib | ||
# Unhandled exception could cause all tasks to be effected and not work |
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.
the code this comment refers to was deleted in a previous PR
src/sentry/tasks/base.py
Outdated
do_record_timing_rollout = False | ||
record_timing_rollout = options.get("sentry.tasks.record.timing.rollout") | ||
if record_timing_rollout and record_timing_rollout > random(): | ||
do_record_timing_rollout = True |
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.
setting record_timing = True
here made it so the wrapped function wouldn't use the parameter record_timing
, so I added do_record_timing_rollout
for the rollout logic
src/sentry/tasks/base.py
Outdated
record_timing_rollout = options.get("sentry.tasks.record.timing.rollout") | ||
if record_timing_rollout and record_timing_rollout > random(): |
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.
You could use sentry.features.rollout.in_random_rollout
to handle the comparison with random and handling unset option values.
❌ 4 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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.
Looks good to me.
In #71778 the option to allow latency metrics from a
SentryTask
was added, but defaulted to false and has since only been enabled on 3 tasks.This PR adds a rollout option to increase the rate at which all tasks in a region report latency metrics. This will allow us to configure monitors based off of task latency instead of backlog similar to what we have for kafka.