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(crons): Fan out check_missing task to each monitor_environment #55924

Merged
merged 8 commits into from
Sep 19, 2023

Conversation

rjo100
Copy link
Contributor

@rjo100 rjo100 commented Sep 8, 2023

Currently this task has a time-limit of 15s, this can be very sensitive to database issues where things "slow down" and the task hits it's time-limit.

This task currently works by finding all monitors that are past their expected check-in time and updates and marks the monitor as having missed a check-in. We can improve performance here by fanning out tasks for each monitor that it needs to mark as missed.

image

Currently we average around ~250 missed monitors per minute. This number will only grow linearly as the product usage grows. This would be an extra 250 tasks to process each minute.

@rjo100 rjo100 requested a review from a team as a code owner September 8, 2023 17:12
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 8, 2023
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #55924 (e43d5f4) into master (2ce9eec) will increase coverage by 0.00%.
Report is 14 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master   #55924    +/-   ##
========================================
  Coverage   79.94%   79.95%            
========================================
  Files        5061     5061            
  Lines      217361   217468   +107     
  Branches    36786    36806    +20     
========================================
+ Hits       173778   173874    +96     
- Misses      38258    38267     +9     
- Partials     5325     5327     +2     
Files Changed Coverage
src/sentry/monitors/tasks.py 100.00%

@evanpurkhiser evanpurkhiser changed the title feat(crons): Run check missing for each environment in parallel feat(crons): Fan out check_missing task to each monitor_environment Sep 8, 2023
logger.info(
"monitor.missed-checkin", extra={"monitor_environment_id": monitor_environment.id}
)
check_missing_environment.delay(monitor_environment.id)
Copy link
Member

Choose a reason for hiding this comment

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

not a celery expert but what's the overhead of a task here? would it make sense at all to batch them?

is the overall goal of this PR simply to increase parallelism?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now we're only at a few hundred but could always switch to chunks later
https://docs.celeryq.dev/en/latest/userguide/canvas.html#chunks

Comment on lines 220 to 221
monitor_environment = MonitorEnvironment.objects.get(id=monitor_environment_id)
monitor = monitor_environment.monitor
Copy link
Member

Choose a reason for hiding this comment

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

Let's add select_related('monitor') so we don't have to make a second query here.

This reverts commit 2e0a9a9.
@evanpurkhiser
Copy link
Member

evanpurkhiser commented Sep 8, 2023

Let's give it a shot, but if possible let's keep an eye on when the tasks are being run.

@rjo100 rjo100 merged commit 5041cef into master Sep 19, 2023
58 checks passed
@rjo100 rjo100 deleted the rjo100/run-check-missing-parallel branch September 19, 2023 18:23
@asottile-sentry asottile-sentry added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Sep 19, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: c738ff5

getsentry-bot added a commit that referenced this pull request Sep 19, 2023
…vironment` (#55924)"

This reverts commit 5041cef.

Co-authored-by: asottile-sentry <103459774+asottile-sentry@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants