Add support for micrometer metrics with reactor schedulers

[kkocel]

In Reactor 3.2.3 there is possibility to enable metrics. There could be property introduced eg. reactor.schedulers.metrics: enabled that would enable metrics by calling Schedulers.enableMetrics()

[philwebb]

We discussed this a little as a team today and we're not sure that we like the global nature of Schedulers.enableMetrics(). We're especially concerned about the fact that SchedulerMetricDecorator uses Metrics.globalRegistry which has the following javadoc:

For use especially in places where dependency injection of MeterRegistry is not possible

That doesn't seem like a very Spring friendly way of providing integration.

We'd still like to provide metrics support, but we're not sure exactly how yet.

[philwebb]

/cc @smaldini @simonbasle in case any alternative non-static integration is under consideration already.

[simonbasle]

@philwebb from my initial discussions with @jkschneider and what we did back when we added metrics to Flux and Mono, the globalRegistry seemed the way to go, since any registry added by the application should be added to it and thus will instrumentation data forwarded.

I might be mistaken. But keep in mind that for us the challenge is that we cannot expose the Micrometer classes in the "main" API (Flux#metrics(), Schedulers#enableMetrics(), etc...) since we only want an optional dependency to it.

Happy to hear if you have ideas about how to integrate metrics in a non-static way though 🙇

[jkschneider]

Agreed. The static registry was added for precisely this case, where a core library cannot leak Micrometer types through its API. I don't see an alternative, but happy to hear suggestions.

[wilkinsona]

If Reactor exposed SchedulerMetricDecorator as public API and allowed an instance to be created for a specific MeterRegistry, Boot could then enable metrics for that MeterRegistry by passing the SchedulerMetricDecorator that it has created into a call to addExecutorServiceDecorator.

[simonbasle]

That could be a workaround (with the danger of exposing a public class that would break the runtime the first time it is loaded in an app that doesn't depend on Micrometer), but would introduce a discrepancy between Flux metrics (which do need the global registry) and scheduler metrics...

Edit: to clarify that do need statement above: Flux cannot have traces of Micrometer in its public API, because it is 100% sure to be loaded and thus to trigger loading of micrometer-related classes (and associated NoClassDefFoundError). So it has to rely on the globalRegistry.

Only alternative I can think of is to have a completely untyped API at the Metrics level, like Metrics.setDefaultRegistry(Object maybeARegistryOrNotWhatever).

[simonbasle]

@wilkinsona @philwebb @jkschneider I have opened a PR that explores that last avenue, please chime in and tell me if you think it is worth the ugly untyped API: reactor/reactor-core#1464

[simonbasle]

After comments and discussions around my PR above, I don't see any good way to abide by the following constraints, other than using the globalRegistry as we're currently doing:

• Reactor wants to avoid an explicit dependency on Micrometer
• Classes of Micrometer cannot leak into the Reactor APIs and public classes (otherwise it will lead to NoClassDefFoundError in the wild)
• Reactor shouldn't be in the business of implementing an interface set for metrics (reinventing most of the wheels in Micrometer)

Any idea?

[wilkinsona]

Classes of Micrometer cannot leak into the Reactor APIs and public classes (otherwise it will lead to NoClassDefFoundError in the wild)

If it were me, I would relax this restriction. It's quite common in the Spring world to have a class that requires an optional dependency to be on the classpath for it to be usable. If SchedulerMetricDecorator were named such that its dependency on Micrometer was more obvious, I think it would be reasonable for a NoClassDefFoundError or ClassNotFoundException to be thrown if someone attempts to use that class without Micrometer on the classpath.

[cybuch]

How about simple wrapper for Scheduler? So the user have control over schedulers - which he wants to measure and which not.

[simonbasle]

The simple wrapper solution is not viable because the only thing that is currently instrumented/instrumentable is the underlying ScheduledExecutorService some Schedulers use. This is NOT exposed by the Schedulers and thus cannot be accessed by a wrapper. Reactor-core exposes a hook to intercept when a Scheduler instantiates a ScheduledExecutorService, which can be used to change it to a Micrometer-instrumented wrapper, but that is applied to all `Schedulers.

[kkocel]

@simonbasle Is there any update on this since there is a wrapper implemented?

[simonbasle]

this would be up to the boot team or contributors to take up that task, but yes since last week io.projectreactor:reactor-core-micrometer:1.0.0 optional module is available alongside reactor-core:3.5.0. It exposes a TimedScheduler wrapper that could be used on a case by case basis without depending on the global registry. Something like:

    @Bean
    public static SimpleMeterRegistry metricsRegistry() {
        return new SimpleMeterRegistry();
    }

    @Bean
    public static Scheduler parallel() {
        return Schedulers.newParallel("my.parallel.scheduler");
    }

    @Bean
    public static Scheduler instrumentedParallel(MeterRegistry registry, Scheduler parallel) {
        String metricsPrefix = "my.instrumented.parallel.scheduler";

        return Micrometer.timedScheduler(
                parallel,
                registry,
                metricsPrefix
        );
    }

[kkocel]

@wilkinsona It looks like spring boot can now take advantage of TimedScheduler wrapper :)

[rubasace]

What is the state of this effort? And what is the expected outcome from it? Is there any intention on providing some sort of decoration out-of-the-box so schedulers are monitored out-of-the-box?

At the moment, I don't seem to find a replacement for Schedulers.enableMetrics() that instruments default schedulers, unless I manually decorate them with TimedScheduler everywhere I use them (or provide them as beans after decoration)

[kkocel]

@rubasace Hi, I created a PR in reactor to support this - https://github.com/reactor/reactor-core/pull/3288

[jonathannaguin]

I just hit this wall as well, I don't really see a nice replacement for Schedulers.enableMetrics() for those schedulers that are used by Spring.