micronaut-projects / micronaut-cache

This project includes caching support for Micronaut
Apache License 2.0
27 stars 30 forks source link

Cache Metrics Broken on JDK > 11 #605

Open broadcom-tony opened 1 year ago

broadcom-tony commented 1 year ago

Expected Behavior

All caching metrics should work cache_size, cache_puts, hits, etc.

Actual Behaviour

cache_size metrics work but cache_puts, hits, etc do not increment. This issue describes the behavior:

https://github.com/micronaut-projects/micronaut-cache/issues/94

I believe the issue is that javax was pulled out of the JDK in version 11 and greater however the JCacheMetricsBinder.java and potentially other places @Require javax.cache.CacheManager to load their bean.

See:

https://github.com/micronaut-projects/micronaut-cache/blob/master/cache-core/src/main/java/io/micronaut/cache/jcache/metrics/JCacheMetricsBinder.java#L42

Steps To Reproduce

No response

Environment Information

`
id "io.micronaut.minimal.application" version "3.7.8" micronaut { version = "3.9.1" } implementation "io.micronaut.micrometer:micronaut-micrometer-core" implementation "io.micronaut.micrometer:micronaut-micrometer-registry-prometheus" implementation "io.micronaut.cache:micronaut-cache-core" implementation "io.micronaut.cache:micronaut-cache-caffeine"

`

Example Application

No response

Version

3.9.1

sdelamo commented 1 year ago

does it work if you add javax.cache:cache-api:1.1.1to your application?

Richardson-e commented 1 year ago

I was able to reproduce this issue with micronaut 4.0.0 I added implementation ('javax.cache:cache-api:1.1.1') to the build.gradle file, checked it was in my classpath and it did not resolve the problem. Metric cache.size works(as it did previously), but cache.gets nor cache.puts increment as it should.

sdelamo commented 1 year ago

do you have a sample application @Richardson-e ?

ben-manes commented 1 year ago

fyi, in JCache you have to enable stats as part of the cache configuration. For Caffeine, if using the config file,

caffeine.jcache.default.monitoring.statistics = true

where default is the base template, or can be a specific cache name.

Since JCache requires JMX (management = true) you can enable and verify in jconsole separately from other reporters.

Richardson-e commented 1 year ago

do you have a sample application @Richardson-e ?

https://github.com/Richardson-e/hello-world-caffeine-cache

I'll take a look at explicitly enabling stats for jcache as part of the cache config.

sdelamo commented 1 year ago

@Richardson-e assigning this to @guillermocalvo who has more availability

guillermocalvo commented 1 year ago

So I've been looking into this issue and I believe cache metrics are working as expected, as far as Micronaut layer is concerned. It just so happens that Caffeine collects "performance statistics", which are not exactly the same as "usage statistics".

Here's what cache statistics look like for Caffeine:

On the one hand, there's simply no cache_puts recorded by Caffeine. On the other hand, Caffeine doesn't keep track of manually removed cache entries, which means that cache_evictions will not be altered as a result of invoking methods annotated with @CacheInvalidate. However, we will observe changes when some entry is evicted automatically by Caffeine (for example, if maximum size is reached). @ben-manes Please correct me if I'm wrong.

On the bright side though, Caffeine does report metrics for cache hits and cache misses. Which means that cache_gets will be incremented over time, as @Cacheable methods are invoked. Finally, cache_size is accurately reported too.

In summary,

Here's a sample application that collects cache metrics and exposes them through /prometheus:

After playing around with it for a while, /prometheus reports the next cache metrics:

cache_size{cache="headlines",} 2.0

cache_gets_total{cache="headlines",result="hit",} 6.0
cache_gets_total{cache="headlines",result="miss",} 5.0

cache_puts_total{cache="headlines",} 0.0

cache_evictions_total{cache="headlines",} 11.0
cache_eviction_weight_total{cache="headlines",} 11.0

@broadcom-tony Please note that in order to activate metrics for a specific cache, we need to explicitly set config property micronaut.caches.*.record-stats to true. Here's the full Configuration Reference.

@sdelamo @Richardson-e I think it's safe to say that Micronaut is doing what it possibly can to retrieve and expose Caffeine metrics. Any thoughts before I close this ticket?

broadcom-tony commented 1 year ago

@guillermocalvo Thanks for the in-depth description and investigation. I just looked in to BoundedLocalCache.put() and I agree that it doesn't appear to be recording puts. Tragic ... I agree you can close this but at least we have a record now if anyone else runs in to it.

ben-manes commented 1 year ago

The cache primarily tries to capture statistics that users couldn't easily obtain themselves, like the hit rate and number of evictions, and anything else of interest can be added by the user. The usage is meant to be a hidden implementation detail of the class, like one would use ConcurrentHashMap, so the caller might wrap it for the outside world. Is there any reason to not enrich the native stats by adding to the provider's DefaultSyncCache.put?

A reason for this approach is that the more flexible of an api that is provided, the more ambiguous it is to map into statistics. The Map interface's put, replace, and computeIfPresent might be viewed as both a put and removal, though not everyone agrees (JCache's variants say they are only puts). It also becomes a slippery issue of everyone wanting their scenarios covered in the common stats, which might bloat it or require changes as edge cases are debated. In Guava we decided to cop out by making that a user's problem and recommend treating it as a data structure akin to using Java Collections. That worked pretty well, with the occasional annoyance of maybe being too minimal but that being easy to resolve with obvious code.