Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache Metrics Broken on JDK > 11 #605

Open
broadcom-tony opened this issue Jun 8, 2023 · 9 comments
Open

Cache Metrics Broken on JDK > 11 #605

broadcom-tony opened this issue Jun 8, 2023 · 9 comments
Assignees
Labels
closed: notabug The issue is not a bug

Comments

@broadcom-tony
Copy link

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:

#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

  • Linux/MacOs
  • JDK 17

`
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
Copy link
Contributor

sdelamo commented Jul 26, 2023

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

@Richardson-e
Copy link

Richardson-e commented Jul 26, 2023

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
Copy link
Contributor

sdelamo commented Jul 26, 2023

do you have a sample application @Richardson-e ?

@ben-manes
Copy link

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
Copy link

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
Copy link
Contributor

sdelamo commented Aug 24, 2023

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

@guillermocalvo
Copy link
Contributor

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,

  • cache_size: reported properly.
  • cache_gets: reported properly.
  • cache_puts: NOT collected by Caffeine.
  • cache_evictions: represents automatic evictions only.

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?

@guillermocalvo guillermocalvo added the closed: notabug The issue is not a bug label Aug 31, 2023
@broadcom-tony
Copy link
Author

@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.

@sdelamo sdelamo moved this to In Progress in 4.2.0 Release Aug 31, 2023
@ben-manes
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed: notabug The issue is not a bug
Projects
No open projects
Status: No status
Development

No branches or pull requests

5 participants