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()

Comment From: 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.

Comment From: philwebb

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

Comment From: 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 🙇

Comment From: 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.

Comment From: 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.

Comment From: 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).

Comment From: 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

Comment From: 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?

Comment From: 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.

Comment From: cybuch

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

Comment From: 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.

Comment From: kkocel

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

Comment From: 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
        );
    }

Comment From: kkocel

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

Comment From: 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)

Comment From: kkocel

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

Comment From: 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.

Comment From: snicoll

I don't really see what's left here. Schedulers.enableMetrics() has been deprecated for removal in favor of TimedScheduler.

Such wrapper has to be applied on a scheduler per scheduler basis and we need more insights to validate if there is something Spring Boot can reasonably do. WDYT @chemicL?

Comment From: chemicL

I am just catching up on this subject, so apologies if something is off-base, but I will try to provide my personal take given just a brief skimming and getting acquainted with the subject.

Step 1

If Spring Boot creates instances of Scheduler then they should be injectable (ConditionalOnMissing?), e.g. to provide metrics, using the mentioned TimedScheduler.

Step 2

If Spring Boot uses Schedulers from the global pool (e.g. Schedulers.boundedElastic()) then it's worth considering actually providing the user the means to replace them with something they can configure and monitor on their own. The same principle as above.

Step 3

Once the above steps are implemented, it makes sense to give a configuration option to enable metrics and possibly custom tags (according to the TimedScheduler constructor) for these instances controlled by Boot. However, that would only have an effect if reactor-core-micrometer is on the user's classpath and it is most probably not provided by Spring Boot.

Next steps

Now, my concern is with all the other instances of Scheduler that the user code and other libraries might use. And I suppose this is the interesting and requested feature as it includes the sprinkled Schedulers.parallel() and other calls in the user's codebase.

The nuance of Reactor and low-level libraries like that, including the JDK itself, is that there is no lifecycle management that we are all accustomed to with Spring Boot – instead, static global instances and configurations exist. In the case of Reactor's Schedulers, it is reactor.core.scheduler.Schedulers#setFactory which can influence all the created instances, including the ones like Schedulers.boundedElastic(). This call, in order to have the right effect, has to be called early in the app's lifecycle in order to have the desired total impact. The issue is that it might collide with some other caller. Nevertheless, we have a configuration for Hooks.enableAutomaticContextPropagation() called by Spring Boot's configuration early in the lifecycle, so it's worth considering if that's what users want. Since the Micrometer.timedScheduler(...) call can be fed with the Spring Boot's owned MeterRegistry, this is something worth experimenting with.

However, the metrics prefix would be shared across all instances of a particular Scheduler type, which is probably not desired. It feels like this becomes an entire domain of Schedulers to now consider and configure, e.g. named instances, etc. With that, my take is that Spring Boot should limit itself to Step 1 and Step 2, while leaving Step 4 as something the user can do on their own, e.g. by using Schedulers.setFactory or managing beans of Scheduler instances which are instrumented according to the user's rules.

Hope this is helpful, albeit long and not straightforward :) Let me know your thoughts @snicoll