Initial Caffeine CacheStats recorder #1897

schlosna · 2024-03-07T18:49:32Z

Before this PR

Begin to address #1895

Supersedes #1075

After this PR

==COMMIT_MSG==
Initial Caffeine CacheStats recorder

Example usage:

TaggedMetricRegistry taggedMetricRegistry = ...
Cache<Integer, String> cache = Caffeine.newBuilder()
        .recordStats(CacheStats.of(taggedMetricRegistry, "unique-cache-name"))
        .build();

LoadingCache<String, Integer> loadingCache = Caffeine.newBuilder()
        .recordStats(CacheStats.of(taggedMetricRegistry, "unique-loading-cache-name"))
        .build(key::length);

==COMMIT_MSG==

Possible downsides?

changelog-app · 2024-03-07T18:49:36Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Initial Caffeine CacheStats recorder

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2024-03-07T20:04:53Z

tritium-caffeine/src/main/java/com/palantir/tritium/metrics/caffeine/CacheStats.java

+    public static CacheStats of(CacheMetrics metrics, String name) {
+        return new CacheStats(metrics, name);
+    }


I'd prefer if we made CacheMetrics private -- metric-schema doesn't make any guarantees around backward compatibility of generated code because it's expected to be consumed by the project which declares the metrics.

I suppose we may want to reuse the same metrics for guava and caffeine caches. If that's the case, I might recommend a new jar module with internal in the name, like tritium-cache-internal, where metrics may be defined and generated as public classes. The tritium-cache-internal module can be used as an implementation dep into the tritium modules which instrument specific cache implementations, so that the classes aren't directly visible to consumers.

Makes sense, I can move the metric-schema definition and make package private.

Generally I'd like to deprecate all uses of Guava caches and migrate any remaining to Caffeine, so not sure its worth adding similar for Guava caches. We already have BanGuavaCaches in baseline

Perfect, thanks!

carterkozak · 2024-03-07T20:05:37Z

tritium-caffeine/src/main/java/com/palantir/tritium/metrics/caffeine/CacheStats.java

+     * @return Caffeine stats instance to register via
+     * {@link com.github.benmanes.caffeine.cache.Caffeine#recordStats(Supplier)}.
+     */
+    public static CacheStats of(TaggedMetricRegistry taggedMetricRegistry, String name) {


Might as well sprinkle safety annotations:

Suggested change

public static CacheStats of(TaggedMetricRegistry taggedMetricRegistry, String name) {

public static CacheStats of(TaggedMetricRegistry taggedMetricRegistry, @Safe String name) {

carterkozak · 2024-03-07T20:36:37Z

tritium-caffeine/src/main/metrics/cache-metrics.yml

+      hit.count:
+        type: counter
+        tags: [cache]
+        docs: Count of cache hits


Sorry I missed this in the previous round of review, I think all the counters should be updated to type: meter. Counter is interpreted as a gauge in most backends because it doesn't guarantee monotonic increasing values, where our Meter, confusingly, uses the Prometheus Counter type for slightly easier indexing.

We may want to remove the .count name suffixes as well (otherwise we'll end up with queries for cache_hit_count_count), since those are described by both the counter and meter metric types.

yeah, I'm trying to keep metric name compatibility with the existing metrics produced by com.palantir.tritium.metrics.caffeine.CaffeineCacheStats#registerCache(com.palantir.tritium.metrics.registry.TaggedMetricRegistry, com.github.benmanes.caffeine.cache.Cache<?,?>, java.lang.String) for now as I'm not sure how complex the migration will be across the fleet.

carterkozak · 2024-03-07T20:41:25Z

tritium-caffeine/src/main/metrics/cache-metrics.yml

+      load.success.count:
+        type: counter
+        tags: [cache]
+        docs: Count of successful cache loads
+      load.failure.count:
+        type: counter
+        tags: [cache]
+        docs: Count of failed cache loads


Thinking out loud here, entirely up to you either way:

We could declare a single metric along these lines, using a tag to distinguish success and failure:

load: type: meter tags: - name: cache - name: result values: [success, failure]

Sometimes this makes it easier to track total interactions

carterkozak · 2024-03-07T20:57:25Z

tritium-caffeine/src/main/java/com/palantir/tritium/metrics/caffeine/CacheStats.java

+    @Override
+    public void recordLoadFailure(@NonNegative long loadTime) {
+        loadFailureMeter.mark();
+        totalLoadNanos.add(loadTime);


I suppose we could make the meter into a timer, which also provides a “count” value in addition to percentiles, more timeseries though

Invalidated by push of 1bcf527

svc-autorelease · 2024-03-07T21:17:57Z

Released 0.83.0

schlosna added 2 commits March 7, 2024 13:41

Initial Caffeine CacheStats recorder

66a4888

readme

0af16d2

schlosna requested review from ash211 and carterkozak March 7, 2024 18:49

probot-autolabeler bot added the autorelease label Mar 7, 2024

schlosna added the do not merge label Mar 7, 2024

schlosna added 2 commits March 7, 2024 13:51

self supply

683c166

Remove gauges & simplify

61e72e5

schlosna force-pushed the ds/cache-metrics branch from 2eb59bc to 61e72e5 Compare March 7, 2024 19:21

javadoc

7c83c7a

carterkozak reviewed Mar 7, 2024

View reviewed changes

enum map

0f65783

carterkozak reviewed Mar 7, 2024

View reviewed changes

svc-changelog and others added 6 commits March 7, 2024 20:12

Add generated changelog entries

9f1a698

private metric schema

7ad96c1

safe

ff78d58

deprecate older cache registration mechanisms

031b770

trigger cache cleanup & evictions in tests

c89fdc2

update readme

cd7e68b

carterkozak reviewed Mar 7, 2024

View reviewed changes

schlosna added 2 commits March 7, 2024 15:41

meters

4530a28

Cleanup metric names

af50aaf

carterkozak reviewed Mar 7, 2024

View reviewed changes

relax deprecation for now

1fd78b9

carterkozak previously approved these changes Mar 7, 2024

View reviewed changes

schlosna marked this pull request as ready for review March 7, 2024 20:59

load timer

1bcf527

carterkozak approved these changes Mar 7, 2024

View reviewed changes

schlosna added merge when ready and removed do not merge labels Mar 7, 2024

bulldozer-bot bot merged commit a672c71 into develop Mar 7, 2024
5 checks passed

bulldozer-bot bot deleted the ds/cache-metrics branch March 7, 2024 21:17

schlosna mentioned this pull request Mar 7, 2024

README entry for how to instrument a cache #1895

Closed

This was referenced Mar 27, 2024

Add cache size metrics to CacheStats #1919

Merged

Combine hit and miss cache metrics into single tagged metric #1921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Caffeine CacheStats recorder #1897

Initial Caffeine CacheStats recorder #1897

schlosna commented Mar 7, 2024 •

edited

changelog-app bot commented Mar 7, 2024 •

edited by schlosna

carterkozak Mar 7, 2024

schlosna Mar 7, 2024

carterkozak Mar 7, 2024

schlosna Mar 7, 2024

carterkozak Mar 7, 2024

schlosna Mar 7, 2024

carterkozak Mar 7, 2024

schlosna Mar 7, 2024

carterkozak Mar 7, 2024

carterkozak Mar 7, 2024

svc-autorelease commented Mar 7, 2024

	public static CacheStats of(TaggedMetricRegistry taggedMetricRegistry, String name) {
	public static CacheStats of(TaggedMetricRegistry taggedMetricRegistry, @Safe String name) {

Initial Caffeine CacheStats recorder #1897

Initial Caffeine CacheStats recorder #1897

Conversation

schlosna commented Mar 7, 2024 • edited

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Mar 7, 2024 • edited by schlosna

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-autorelease commented Mar 7, 2024

schlosna commented Mar 7, 2024 •

edited

changelog-app bot commented Mar 7, 2024 •

edited by schlosna

Generate changelog in `changelog/@unreleased`