Add 'record-native-stats' option to jcache configuration.

[krm1312]

When true, this enables stats collection (Caffeine.recordStats() on
the underlying caches. These caches can then be registered with
micrometer and, imo, are more useful than the Jcache metrics.

PS: The CLA link in contributing.md is broken.

[ben-manes]

Thanks! How would you register it into Micrometer, use Cache.unwrap?

A new option should be described and defaulted in reference.conf.

There is a section for monitoring, which right now enables JCache's JMX beans. Should we skip the extra flag and enable native stats when that is set? Or maybe move your flag there as native-statistics? (The JCache ones can be enabled/disabled dynamically by CacheManager, whereas the native ones are enabled only at build time).

caffeine/jcache/src/main/resources/reference.conf

Lines 49 to 56 in fe0131b

	# The JMX monitoring configuration
	monitoring {
	# If cache statistics should be recorded and externalized
	statistics = false
	# If the configuration should be externalized
	management = false
	}

Thanks for the reminder of CLAhub disappearing. It is not important, but I should find an alternative. It's too bad how it quietly died since so I can't migrate the signatures, which kind of defeated the purpose...

[krm1312]

Having monitoring.statistics also turn on Caffeine's stats collection seems like a good solution and what most users would be expecting. Certainly works for my use case. The downside is you can't do it on a per-cache basis that way, but, that seems like an unlikely requirement. You could always filter out metrics from caches you don't care about down stream if that really mattered. Feel free to close this if that's how you want to go.

In regards to how I'm registering, yes, I'm using unwrap:

CachingProvider provider = Caching.getCachingProvider(CaffeineCachingProvider.class.getName());
CacheManager cacheManager = provider.getCacheManager();
cacheManager.getCacheNames().forEach(name -> {
    CaffeineCacheMetrics.monitor(meterRegistry, cacheManager.getCache(name).unwrap(Cache.class), name);
});

[ben-manes]

A quirk with the jcache adapter is that it predates the native variable expiration support. To match the spec team's providers, which used lazy expiration by relying on a size constraint, the jcache adapter adds a wrapper and manually validates the expiration. This means your stats will be a little off.

I'd like to migrate to the native expiration and remove the lazy wrapper. This wasn't done yet because it appeared that it would cause backwards incompatibility with the existing configurations, so deferred until a major release. It may be possible with a fresh look, as I forget the details.

The monitoring config is at a per-cache level. A quirk with JCache is that it can be enabled/disabled at runtime, too, which one aspect that makes it incompatible with our own. For native stats, we could make that build time and allow specifying the StatsCounter in the configuration. The StatsCounter is push-based, so you could avoid having to unwrap the cache, so it might be more amenable to an external configuration.

I'd propose we add the following option,

management {
  # native stats are disabled, default
  stats-collector = null

  # native stats enabled, built-in & needs unwrapping
  stats-collector = com.github.benmanes.caffeine.cache.stats.ConcurrentStatsCounter

  # native stats enabled, Micrometer
  stats-collector = foo.bar.baz.MicrometerStatsCounter
}

wdyt?

[krm1312]

You are right of course about it being per-cache. Not sure where I got that in my head.

In regards to allowing the configuration of the StatsCounter implementation, it would need additional context (cache name, and Cache) to do anything useful when it receives say recordHits(count). Maybe that interface gets a forCache(String name, Cache cache) that is invoked upon creation. Not entirely convinced this is that useful.

What I think might be useful is to receive a notification when a Cache is created or even when being built. Something like:

management {
  lifecyle-listener =  foo.bar.baz.CacheLifecyleListener
}

// Called after all configuration from config file is done
Caffeine preCreate(String name, caffeine.Caffeine builder)

// Called after .build() - register with micrometer
void postCreate(String name, caffeine.Cache cache)

// Called before close (not sure this is useful)
void preClose(String name, caffeine.Cache cache)

// Called after close - could de-register with micrometer
void postClose(String name, caffeine.Cache cache)

Then one could customize (break ;)) things in preCreate. For example, one could enable stats there. In postCreate we could register it with the micrometer's CaffeineCacheMetrics.

We also might just be making this over-complicated. The unwrap is not that ugly for me at least.

[ben-manes]

Oh you're right, I forgot that the name is needed and wouldn't be supplied without a new interface. ☹️

In theory I like the idea of the lifecycle listener, except that modifying the builder could cause various breakages. For example the builder rejects if a setting is made twice (like maximumSize) and perhaps features like weakKeys might break some JCache assumption (I forget if so, but seems plausible error prone). If the only safe setting is recordStats(?) on preCreate then it's probably not the right approach.

Your original proposal seems the best then, with a minor rename to management.native-statistics for consistency. That way its not enabled by default since most wouldn't unwrap.

I promised the Quarkus team a release over the weekend for them (added a jandex index for their graal AOT). If you don't get a chance to update this PR, I'll take it over and merge it with those minor tweaks.

Thanks! 😄

[krm1312]

Changed the name and moved it under monitoring. Feel free to change it more if you need/want. Thanks.

[ben-manes]

Thanks! I tweaked it slightly to mirror other JCache idioms and will release today or tomorrow.

[ben-manes]

Released in 2.8.6