Add reactor-pool-micrometer module with recorder and gauges

[simonbasle]

This commit adds a new module depending directly on Micrometer which
provides out-of-the-box implementations of PoolMetricsRecorder and other
utilities to instrument a Pool with Micrometer.

Fixes #161.

[jonatan-ivanov]

Some of these types seem to be disagreeing with the behavior, i.e.: this seems to be registered as a gauge not a counter.

Also, I think this might be a good spot to call out a few differences between counters and gauges. Our rule of thumb is usually "never gauge something that you can count".

A counter is always monotonic (it can't decrease) while a gauge can increase and decrease. Because of these properties i this case a counter can tell how many resources were acquired in a time window (basically the rate: N resources/second) while a gauge can only tell you how many resources are acquired right now.

With another (http example): a counter can tell you the incoming request rate while a gauge can tell you how many requests are you processing concurrently right now.

I'm not sure what is the intention at the moment, I just wanted to share this so that we can pick the right instrument for each case.

Fyi: we also have a FunctionCounter for getting the data from an external method (like the gauge does) instead of manually calling increment but the monotonic property should hold there too which might not be the case (i.e.: acquiredSize returns the current size instead of all the acquired resources since startup).

[simonbasle]

I've fixed the disagreeing types, but I do need to carefully consider the points that you bring about counters vs gauges. I'm not sure how to apply such a transition here, precisely because PoolMetrics interface exposes a "view" into the current state of the pool for the 4 metrics: ACQUIRED, ALLOCATED, IDLE, PENDING_ACQUIRE.

These numbers can indeed grow and shrink depending on when the methods are called.

How would you go about that one?

[jonatan-ivanov]

If they are non-monotonic (can grow and shrink), it's straightforward, they need to be a Gauge since Counters must be monotonic.

I think this also agrees with their meaning: what is the actual value in that very moment when you call the method that provides the data.
If they would be a Counter, their meaning would be something like: how many time did X happen since app start/last method call.

[jonatan-ivanov]

I don't know if it is an issue here, (I think it is not) but I want to call a common issue with gauges out: they keep a WeekReference to their state object (i.e.: poolMetrics). This means that if nothing holds a strong reference on the state object, it might be GC'ed and the Gauge will not be use it anymore.

[simonbasle]

mmh the readily available InstrumentedPool implementation (the one we offer) is the PoolMetrics, so in this particular case it is rather desirable (we wouldn't want the meters to retain a reference to a whole pool which is otherwise disposed for instance).

if we keep the gauges, maybe we should call out this fact so that users can understand the behavior with any hypothetical 3rd party implementation that produces a standalone PoolMetrics instance

[violetagg]

Will this clash with the metrics in Reactor Netty, which we would like to keep? Or if we do not use this module there is no issue?

[simonbasle]

it should not clash now that we use the reactor.pool. hardcoded prefix. tag keys might still clash, although for now I've used pool. prefix in tags. in any case, the module is entirely optional. it should be pulled in by end users directly, who can make that decision or even use a dedicated meterRegistry with a meter-id filter if needed.

[shakuzen]

A lot of the names seem to be very generic, which requires the user to carefully provide a good prefix to be able to quickly understand the metrics when looking at them in a metrics backend. I wonder if adding some more context by adding reactor.pool. to the beginning of meter names will help. I also wonder if allowing a customizable prefix is necessary in this case. I assume the motivation behind it is to allow identifying metrics for different pools, but I think that could be achieved with a pool.name tag or similar, in lieu of a prefix. What do you think?
Previously, we needed to add a prefix to ExecutorServiceMetrics in Micrometer because different libraries/users were using it with a different set of extraTags. If we know the set of tags is consistent (because we don't accept arbitrary extra ones) then we can avoid this complexity, I think. /cc @jonatan-ivanov

[simonbasle]

ok. I think this doesn't change the need for a separate module because we need to ask the user for a MeterRegistry anyway, but interesting point about naming meters less generically and turning the prefix into a name tag.

metricsPrefix is indeed intended as a way to identify distinct pools.

as a side note, what about tags naming?

[simonbasle]

I've implemented the change so that meter names are now hardcoded using the reactor.pool.* qualifier and the differentiator is now a pool.name tag that is common to all the meters in that module.

I used dot-separated "prefixes" in tag keys, but that might be hit or miss as the general wisdom seems to be that for tags, keys should be as simple as possible... on the other hand @violetagg also raised a concern that a tag key-value that is too generic (eg. name=http) might create some confusion, so some amount of differentiator could be a good thing.
@shakuzen @jonatan-ivanov any advice?

[simonbasle]

follow-up:

• add the module to the BOM : Add reactor-pool-micrometer to BOM reactor#726