feat: add aggregate observability counters to the cache layer - #24188
Draft
adriangb wants to merge 3 commits into
Draft
feat: add aggregate observability counters to the cache layer#24188adriangb wants to merge 3 commits into
adriangb wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24188 +/- ##
========================================
Coverage 81.05% 81.05%
========================================
Files 1106 1106
Lines 382287 382518 +231
Branches 382287 382518 +231
========================================
+ Hits 309851 310041 +190
- Misses 54121 54159 +38
- Partials 18315 18318 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds a `CacheStatistics` struct holding aggregate, lifetime counters for a cache (hits, misses, evictions, bytes evicted, and inserts rejected for being larger than the whole limit) plus a `Cache::statistics()` accessor. The accessor has a default implementation returning `None` so that external implementations of the (public) `Cache` trait keep compiling. `None` means "not instrumented", which a reporting tool can distinguish from an instrumented cache whose counters are genuinely all zero. No implementation yet; `DefaultCache` is wired up in the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Counts hits, misses, evictions, bytes evicted and inserts rejected for exceeding the whole cache limit, and exposes them via `Cache::statistics()` and the inherent `DefaultCache::cache_statistics()`. The counters live in `DefaultCacheState`, so they are updated under the mutex that every `get`/`put` already takes rather than adding a second point of synchronization on the hot path. Semantics, covered by the new tests: - a lookup of an absent *or expired* key is a miss; `contains_key` is a probe and is not counted at all - only capacity-driven drops are evictions, including those caused by lowering the limit via `update_cache_limit`. `remove`, `clear`, `drop_table_entries` and replacing an entry are not evictions - an insert whose entry is larger than the entire limit is counted separately: no amount of eviction can ever make room for it, so it is a distinct failure mode worth surfacing on its own - counters are monotonic for the lifetime of the cache and are not reset by `clear` Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds `get_file_statistic_cache_statistics`, `get_file_metadata_cache_statistics` and `get_list_files_cache_statistics`, so the counters are reachable from the place users already go for cache limits. Each returns `None` when the cache is disabled or when the configured implementation does not track statistics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adriangb
force-pushed
the
claude/cache-observability-counters
branch
2 times, most recently
from
August 8, 2026 19:48
89371ee to
22de14f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
if a maintainer prefers to track it separately.
Rationale for this change
DataFusion has two parquet caches whose working sets scale linearly with schema
width, but whose limits are fixed byte budgets:
file_statistics_cache(default 20 MiB) and
file_metadata_cache(default 50 MiB). On a1024-column × 256-file parquet table, measured locally:
working set against the 50 MiB default
sweep over every file, once per query) is LRU's pathological case, so the
degradation is a cliff and not a gradient: raising the metadata limit to
200 MiB against a 307 MiB working set still delivers 0% of the benefit.
with both caches disabled, 483 ms; with both fully resident, 8.8 ms.
The point of this PR is the last bullet: at the defaults the caches were pure
overhead, and a user in that regime today has no way to discover it.
DefaultCacheStatekeeps a per-entryhits: HashMap<K, usize>, but there areno aggregate counters anywhere, and the only cache introspection in the project
is datafusion-cli's
metadata_cache()table function — which lists the entriesthat are currently resident, and therefore says nothing at all about the ones
that were evicted a microsecond after being inserted. Cache thrashing is
currently invisible and undiagnosable.
This PR adds the counters. It does not change any caching behaviour.
What changes are included in this PR?
Three small commits:
CacheStatistics+Cache::statistics()(cache/mod.rs). A plainstruct of aggregate, lifetime counters:
hitsgetcalls that returned a valuemissesgetcalls that did not — key absent or entry expiredevictionsbytes_evictedinserts_rejected_too_largeplus
lookups()andhit_rate()helpers.inserts_rejected_too_largeis called out separately on purpose: that pathalready exists in
DefaultCacheState::put, and a non-zero value means thecache can never hold that entry no matter how much of it is free. That is a
different problem from ordinary eviction pressure and wants a different fix
(raise the limit vs. shrink the working set), so folding it into
evictionswould lose the most diagnostic signal in the set. It is exactly what happens
when one wide-schema file's metadata exceeds the entire cache budget.
DefaultCachetracks them (cache/default_cache.rs).CacheManagerexposes them (cache/cache_manager.rs):get_file_metadata_cache_statistics(),get_file_statistic_cache_statistics()andget_list_files_cache_statistics(), alongside the existing*_limitaccessors. Each returns
Nonewhen the cache is disabled or when theconfigured implementation is not instrumented.
On not breaking the
CachetraitCacheis public API with potential external implementors, sostatistics()is a new trait method with a default implementation ratherthan a required one — existing implementations keep compiling untouched. It
returns
Option<CacheStatistics>(defaultNone) rather thanCacheStatistics::default()so that "this implementation is not instrumented"stays distinguishable from "this instrumented cache has genuinely done nothing
yet"; reporting all-zero counters for a cache that never counted anything would
be actively misleading. A test in
cache/mod.rsimplementsCachewithoutoverriding
statistics(), both to pin the non-breaking guarantee and to assertthe
None.On the hot path
The counters are fields of
DefaultCacheState, i.e. they live behind theMutexthat everyget/putalready takes, and are incremented inside thatexisting critical section. No new synchronisation, no atomics, no second lock.
Are these changes tested?
Yes — new unit tests in
cache/default_cache.rs, one per counter:test_statistics_hits_and_misses— hits, misses on absent keys,clear()neither resetting the counters nor counting as eviction, and that
contains_keyis a probe that is deliberately not countedtest_statistics_expired_entry_counts_as_miss— a TTL-expired entry is amiss, not an eviction
test_statistics_evictions—evictions/bytes_evictedfor LRU eviction andfor eviction caused by lowering the limit, and that
remove,drop_table_entriesand same-key replacement are not evictionstest_statistics_inserts_rejected_too_large— the rejection path, includingthe case where the rejected insert also drops a stale entry under that key
plus
test_cache_statistics_reachable_from_cache_managerincache/cache_manager.rsand the trait-default tests incache/mod.rs.cargo test -p datafusion-execution,cargo fmt --alland./ci/scripts/rust_clippy.shall pass.Are there any user-facing changes?
Additive only, no behaviour change:
CacheStatisticsstructCache::statistics()with a default implementation, so this is not abreaking change for external implementors
CacheManageraccessorsDeliberately left out, to keep this reviewable:
EXPLAIN ANALYZE. Surfacing cache hit rate per query is the naturalfollow-up and the thing that would actually put this in front of users, but it
needs a decision about per-query vs. process-lifetime accounting (these
counters are process-lifetime and monotonic) and touches the metrics plumbing.
Separate PR.
metadata_cache(). That table function emits one row perresident entry; aggregate counters do not fit that shape without either
repeating them on every row or adding a second table function. Both grow the
diff more than they are worth here.
there is a consumer for it — see the
EXPLAIN ANALYZEitem.DefaultCache::putsilently drops anyvalue whose
size()is 0 and reports no previous value, which forCachedFileListmeans an empty directory listing is never cached — so anempty prefix is re-listed on every query, invisibly. That is pre-existing
behaviour rather than something these counters change, and whether the fix is
a sixth counter or making zero-size entries cacheable is a separate
discussion.
🤖 Generated with Claude Code