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

ScheduledReporter queues up runnables when gauge is slow #1524

Closed
ChadiEM opened this issue Dec 17, 2019 · 4 comments · Fixed by #1590
Closed

ScheduledReporter queues up runnables when gauge is slow #1524

ChadiEM opened this issue Dec 17, 2019 · 4 comments · Fixed by #1590
Milestone

Comments

@ChadiEM
Copy link

ChadiEM commented Dec 17, 2019

Problem Statement

The current implementation of ScheduledReporter relies on a ScheduledExecutorService scheduled at fixed rate:

this.scheduledFuture = executor.scheduleAtFixedRate(runnable, initialDelay, period, unit);

Every period, a thread will report, except when one of the Gauge implementations (getValue()) is slower than that period (for example, network latency). In that case, threads will start to accumulate. When the latency is resolved, there will be a spike of report requests.

Reproducible Scenario

To reproduce, have:

  • a gauge that sleeps 10 seconds the first three times, then 0 the upcoming times before returning a value
  • a ScheduledReporter with a delay of 5 seconds that prints all gauges.

You will get reports at 0, 10, 20, 30, 30, 30, 30, 35, and 40.

Possible solution

I suggest to replace executor.scheduleAtFixedRate with executor.scheduleWithFixedDelay, so that the next call to report is separated by period with respect to the previous call, instead of having an absolute period.

@joschi joschi added this to the 4.2.0 milestone May 13, 2020
@joschi
Copy link
Member

joschi commented May 13, 2020

@ChadiEM Thanks for reporting this!

@dropwizard/metrics @dropwizard/committers Any opinions or reservations on changing the reporting strategy of ScheduledReporter as described by @ChadiEM?

@the-thing
Copy link
Contributor

I think this is a good idea. If they gauge is slow, one should use CachedGauge, but sometimes this can happen unexpectedly on production. Those reporting burst are undesired.

@richwklein
Copy link

Should this issue be resolved by #1590?

@ChadiEM
Copy link
Author

ChadiEM commented Oct 11, 2020

Yes! Thank you.

@ChadiEM ChadiEM closed this as completed Oct 11, 2020
mikebell90 added a commit to mikebell90/metrics that referenced this issue Oct 15, 2022
dropwizard#1524 introduced a fix to queuing issues and ends up doing atFixedDelay rather than atFixedRate.

Issues with binning exist in Graphite (and probably others). See dropwizard#2664
joschi pushed a commit to mikebell90/metrics that referenced this issue Nov 16, 2022
dropwizard#1524 introduced a fix to queuing issues and ends up doing atFixedDelay rather than atFixedRate.

Issues with binning exist in Graphite (and probably others). See dropwizard#2664
joschi pushed a commit to mikebell90/metrics that referenced this issue Dec 9, 2022
dropwizard#1524 introduced a fix to queuing issues and ends up doing atFixedDelay rather than atFixedRate.

Issues with binning exist in Graphite (and probably others). See dropwizard#2664
joschi pushed a commit that referenced this issue Dec 9, 2022
#1524 introduced a fix to queuing issues and ends up doing atFixedDelay rather than atFixedRate.

Issues with binning exist in Graphite (and probably others).

See #2664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants