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

Allow override of scheduler #2931

Merged

Conversation

mikebell90
Copy link
Contributor

@mikebell90 mikebell90 commented Oct 15, 2022

#1524 introduced a fix to queuing issues and ends up doing atFixedDelay rather than atFixedRate. The logic is sound enough, but leads to frustrating unanticipated issues.

Issues with binning exist in Graphite (and probably others). These accumulate "drift", and so metric gaps occur. See #2664 for two examples.

Hence allowing this to be configurable is desirable.

This is the lamest but easiest form of this - moving the method to an overridable method.

Usage would be (example graphite reporter)

  • Create your own subclass of GraphiteReporter, override getScheduledFuture.
  • Said class will also need to explicitly set the constructor instead of using the builder.

Pseudo code:

public class MyReporter extends GraphiteReporter {
     public MyReporter(....) {
         super(...)
    }

    @Override 
    public ScheduledFuture<?> getScheduledFuture(long initialDelay, long period, TimeUnit unit, Runnable runnable) {
        return executor.scheduleWithFixedRate(runnable, initialDelay, period, unit);
   }
}

I looked at other options but they all involved breaking or overloading constructors too much:

  • making it an abstract method, and implementing at concrete classes.
    • There are about 5 , and their constructors are hidden by builders. One could add to builders (with a lot of tedious copy/paste) but the constructors are heavily overloaded and would break other subclassers or add lots of jargon
    • Move the logic to base class. That would involve changing another heavily overloadd constructor (ScheduledReporter)

There's a lot of unfortunate outdated api design in DropWizard

  • Not enough interfaces.
  • Too many overloaded constructors instead of config objects.
  • Lack of centralized configuration options.
  • Often using concrete classes instead of interfaces, which imposes many constraints.

Those are not fixable without a major breaking version change. But we can at least offer something here.

@mikebell90 mikebell90 requested review from a team as code owners October 15, 2022 00:34
@gitpod-io
Copy link

gitpod-io bot commented 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 joschi added this to the 4.2.14 milestone Dec 9, 2022
Copy link
Member

@joschi joschi left a comment

Choose a reason for hiding this comment

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

@mikebell90 Thanks for your contribution!

@joschi joschi merged commit 6256089 into dropwizard:release/4.2.x Dec 9, 2022
@mikebell90
Copy link
Contributor Author

@joschi unfortunately (in the middle of an incident) but I think

https://github.com/mikebell90/metrics/blob/9049dd3bd402d8668c1b6a41ebc50bba59aa2340/metrics-core/src/main/java/com/codahale/metrics/ScheduledReporter.java#L59

needs to be corrected. That needs to be protected or overriding class can't access. Thought that was submitted.

joschi pushed a commit that referenced this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants