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

Add weighted size counter to caffeine instrumentation #1246

Open
jhominal opened this issue Jan 3, 2025 · 4 comments
Open

Add weighted size counter to caffeine instrumentation #1246

jhominal opened this issue Jan 3, 2025 · 4 comments

Comments

@jhominal
Copy link

jhominal commented Jan 3, 2025

Hello,

I am using caffeine in a context where I am using a Weigher to bound the cache instance memory consumption.

In that context, I find that the weighted size (which can be found on the EvictionPolicy object in caffeine) is an interesting statistic to track.

I would be happy to author a PR to add that stat to caffeine instrumentation. (guava cache does not seem to expose an API that would make that statistic available)

@zeitlinger
Copy link
Member

Great - PR would be welcome 😄

@jhominal
Copy link
Author

jhominal commented Jan 7, 2025

@zeitlinger While looking at the details of the metrics collected in caffeine-instrumentation, I have come to believe that caffeine_cache_eviction_weight should be a Counter instead of a Gauge (Weigher.weigh is annotated as returning a non-negative value.)

Would you approve a change of metric type from gauge to counter?
What provisions, if any, should be made for backward compatibility?

@zeitlinger
Copy link
Member

@zeitlinger While looking at the details of the metrics collected in caffeine-instrumentation, I have come to believe that caffeine_cache_eviction_weight should be a Counter instead of a Gauge (Weigher.weigh is annotated as returning a non-negative value.)

Would you approve a change of metric type from gauge to counter? What provisions, if any, should be made for backward compatibility?

Gauge seems to be correct, because the method returns the current accumulated value rather than a difference

 /**
   * Returns the sum of weights of evicted entries. This total does not include manual
   * {@linkplain Cache#invalidate invalidations}.
   *
   * @return the sum of weights of evicted entities
   */

@jhominal
Copy link
Author

jhominal commented Jan 7, 2025

@zeitlinger Semantically the eviction weight is also described in the CacheStats class doc string

/**
 * Statistics about the performance of a {@link Cache}.
 * <p>
 * Cache statistics are incremented according to the following rules:
 * <ul>
 *   <li>When an entry is evicted from the cache, {@code evictionCount} is incremented and the
 *       weight added to {@code evictionWeight}.
 * </ul>

That is, each time there is an eviction:

  • evictionCount is incremented by 1;
  • evictionWeight is incremented by the evicted entry's weight. By contract (on the Weigher interface), the weight of an entry must be non-negative.

Because of that, I do believe that evictionWeight is a monotically increasing counter.

In other words, if eviction is a counter (which it is, even though it returns an accumulated count of evicted entries), then eviction_weight should, in my opinion, also be a counter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants