Skip to content

ref(crons): Simplify monitor clock implementation #54388

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

Conversation

evanpurkhiser
Copy link
Member

This is a follow up to GH-54204 as suggested by @fpacifici

#53661 (comment)

Can we just have the pulse message running in both modes and treat
everything as high volume mode?

Instead of having two modes, we can simply always use the same logic for
dispatching the monitor tasks on the minute roll-over, using the
consumer as a clock.

Previously the worry here was that in low-volume check-in situations
nothing would drive the clock and we would need to have an external
clock, with a different way to dispatch the tasks. But there is no need
for a different way to dispatch the tasks, we can have an external clock
that pulses messages into the topic and we can simply use the same logic
already implemented to use the topic messages as a clock.

This change removes the concept of "high volume" / "low volume" and adds
the concept of a "clock_pulse" message to the consumer.

In a follow up PR we will introduce the celery beat task which produces
the clock_pulse messages.

@evanpurkhiser evanpurkhiser requested a review from a team as a code owner August 8, 2023 18:46
@evanpurkhiser evanpurkhiser requested a review from a team August 8, 2023 18:46
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 8, 2023
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #54388 (a2b55a6) into master (6612cd8) will increase coverage by 0.00%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #54388   +/-   ##
=======================================
  Coverage   79.74%   79.74%           
=======================================
  Files        4991     4991           
  Lines      211939   211944    +5     
  Branches    36115    36117    +2     
=======================================
+ Hits       169008   169018   +10     
+ Misses      37731    37728    -3     
+ Partials     5200     5198    -2     
Files Changed Coverage Δ
src/sentry/monitors/consumers/monitor_consumer.py 89.40% <100.00%> (+1.09%) ⬆️

... and 3 files with indirect coverage changes

@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/ref-crons-simplify-monitor-clock-implementation branch from 9fd307b to 6de6726 Compare August 9, 2023 22:35
This is a follow up to GH-54204 as suggested by @fpacifici

#53661 (comment)

> Can we just have the pulse message running in both modes and treat
> everything as high volume mode?

Instead of having two modes, we can simply always use the same logic for
dispatching the monitor tasks on the minute roll-over, using the
consumer as a clock.

Previously the worry here was that in low-volume check-in situations
nothing would drive the clock and we would need to have an external
clock, with a different way to dispatch the tasks. But there is no need
for a different way to dispatch the tasks, we can have an external clock
that pulses messages into the topic and we can simply use the same logic
already implemented to use the topic messages as a clock.

This change removes the concept of "high volume" / "low volume" and adds
the concept of a "clock_pulse" message to the consumer.

In a follow up PR we will introduce the celery beat task which produces
the clock_pulse messages.
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/ref-crons-simplify-monitor-clock-implementation branch from 6de6726 to a2b55a6 Compare August 10, 2023 15:49
@evanpurkhiser evanpurkhiser merged commit 48ca2ea into master Aug 10, 2023
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/ref-crons-simplify-monitor-clock-implementation branch August 10, 2023 16:23
evanpurkhiser added a commit that referenced this pull request Aug 10, 2023
evanpurkhiser added a commit that referenced this pull request Aug 10, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants