Skip to content

Add (remote) cache metrics#3033

Merged
gbrodman merged 1 commit into
google:masterfrom
gbrodman:cacheMetrics
May 11, 2026
Merged

Add (remote) cache metrics#3033
gbrodman merged 1 commit into
google:masterfrom
gbrodman:cacheMetrics

Conversation

@gbrodman
Copy link
Copy Markdown
Collaborator

@gbrodman gbrodman commented May 5, 2026

This only applies to the CacheModule-provided caches because we don't want to have to deal with all the various other caches. We'll want to know the various ratios between types of cache hits/misses when evaluating the usefulness of the remote caching.


This change is Reviewable

@gbrodman gbrodman requested a review from ptkach May 5, 2026 20:12
@gbrodman gbrodman force-pushed the cacheMetrics branch 2 times, most recently from 44ba793 to d61cc72 Compare May 8, 2026 15:43
@gbrodman gbrodman requested a review from jicelhay May 8, 2026 20:46
Copy link
Copy Markdown
Collaborator

@jicelhay jicelhay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jicelhay reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: 4 of 7 files reviewed, 2 unresolved discussions (waiting on gbrodman and ptkach).


core/src/main/java/google/registry/cache/CacheMetrics.java line 38 at r1 (raw file):

      ImmutableSet.of(
          LabelDescriptor.create("cache_name", "The type of the cache (domain/host)."),
          LabelDescriptor.create("hit_type", "The type of cache hit, if the object was found."));

"if the object was found" I don't think that description is accurate if you're trackig misses in the label, right?


core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java line 72 at r1 (raw file):

    possibleValue = loadFromDatabase(key);
    if (possibleValue.isEmpty()) {
      cacheMetrics.recordLookup(clazz.getSimpleName(), CacheMetrics.CacheHitType.MISS_NONEXISTENT);

What's the reason to track MISS_NONEXISTENT?

This only applies to the CacheModule-provided caches because we don't
want to have to deal with all the various other caches. We'll want to
know the various ratios between types of cache hits/misses when
evaluating the usefulness of the remote caching.
Copy link
Copy Markdown
Collaborator Author

@gbrodman gbrodman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gbrodman made 2 comments.
Reviewable status: 4 of 7 files reviewed, 2 unresolved discussions (waiting on jicelhay and ptkach).


core/src/main/java/google/registry/cache/CacheMetrics.java line 38 at r1 (raw file):

Previously, jicelhay (Juan Celhay) wrote…

"if the object was found" I don't think that description is accurate if you're trackig misses in the label, right?

Done


core/src/main/java/google/registry/cache/MultilayerEppResourceCache.java line 72 at r1 (raw file):

Previously, jicelhay (Juan Celhay) wrote…

What's the reason to track MISS_NONEXISTENT?

Requests for domains that don't exist will always fall through to the database. It looks like for now these aren't a huge portion of requests (maybe 15-20%?) but if that percentage goes up, the caches won't be of as much use and the database will still get hammered.

In that case we will need to come up with different solutions, which could include maybe caching nonexistence or using some complicated system of nested bloom filters.

Copy link
Copy Markdown
Collaborator

@jicelhay jicelhay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jicelhay reviewed 4 files and all commit messages, and resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on ptkach).

@gbrodman gbrodman enabled auto-merge May 11, 2026 17:47
@gbrodman gbrodman added this pull request to the merge queue May 11, 2026
Merged via the queue into google:master with commit 5854ccf May 11, 2026
10 checks passed
@gbrodman gbrodman deleted the cacheMetrics branch May 11, 2026 19:27
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

Successfully merging this pull request may close these issues.

2 participants