Conversation
…B-3768)
Every hit site but one fed cache_operation_size_bytes with the length of the
envelope it served: sync L1 and async L1 use len(l1_bytes), async L2 uses the
raw bytes get_cached_value_async already returns. The sync L2 site recorded
len(str(value).encode()) instead, a repr estimate of the deserialized value,
because the sync handler returned only (True, value). One process mixing sync
and async callers, or any secure cache (ciphertext envelope vs plaintext repr),
got a bimodal histogram on the same series: 14 vs 168 bytes for {"answer": 42}.
get_cached_value and get_cached_value_with_freshness now return
(True, value, size_bytes), the served L2 envelope length. It is a length, not
the envelope itself, because the mmap fast path's view dangles once its handle
closes (#171) and returning bytes there would mean a full copy; the sync path
has no L1 backfill (#164) so nothing needs the bytes. The async variant keeps
carrying raw bytes because it does backfill.
The estimate computed before log_cache_operation was dead code: that call takes
**kwargs and no site passes size_bytes. Deleted rather than wired in, so the
log call stays symmetric with the L1 sites.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. WalkthroughCache read hits now return the served envelope byte length. Freshness results carry the same value. Sync L2 metrics record this length instead of estimating it from the deserialised value. ChangesCache hit size propagation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The cache-hit size propagation change has no identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… (LAB-3765) The sync L2 site measures the served envelope as of #298; this comment described the state it was written against and would read as a lie once both land. The remaining sentence is about this site only.
…ain (LAB-3768) The previous commit claimed the async hit sites already record the served envelope length. On this base they do not; #297 brings them there. Docstring, wrapper comment and test docstring now state the fact that holds today: the same quantity the sync L1 site records via len(l1_bytes). The hit site unpacks the tuple by name, mirroring the async site, instead of reading [1] and [2]. The test runs with l1_enabled=False so the L2 site is reached without an L1 singleton to prime and clear, and the one-use fixture is inlined with its rationale kept as a comment.
d76b56e
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
1 participant
Problem
cache_operation_size_bytes{operation="get",serializer="rust"}was fed a different quantity at the sync L2 hit site than at the sync L1 hit site. The L1 site recordslen(l1_bytes), the envelope it served (#297 brings the async hit sites to the same quantity). The sync L2 site recordedlen(str(value).encode("utf-8")), a repr estimate of the deserialized value, because the sync handler returned only(True, value)with no envelope.One process mixing L1 and L2 hits, or any
@cache.securecaller (ciphertext envelope vs plaintext repr), got a bimodal histogram on the same series. Measured on a trivial{"answer": 42}: the estimate reported 14 bytes, the served envelope is 168.Fix
get_cached_valueandget_cached_value_with_freshnessreturn(True, value, size_bytes), the length of the L2 envelope actually served. On the mmap fast path that ishandle.view.nbytes, read beforeclose(); on the bytes pathlen(cached_data). Both arest_size - HEADER_SIZEon the File backend, so the two paths report the same number for the same entry.log_cache_operationwas dead code (that call takes**kwargsand no site passessize_bytes); deleted rather than wired in so the log call stays symmetric with the L1 sites.Why a length and not the raw bytes like the async variant: the mmap view dangles once its handle closes (#171) and must never leave the read frame, so returning bytes there means a full copy of the payload, which is what the mmap path exists to avoid (
tests/performance/test_large_object_memory.pymeasures peak memory on this exact call). The sync path performs no L1 backfill (#164), so nothing downstream needs the bytes. The async variant keeps carrying raw bytes because it does backfill.Tests
tests/unit/test_sync_l2_hit_size_bytes.py: a sync L2 hit recordssize_bytes == len(stored envelope), patched atFeatureOrchestrator.record_cache_operation. Fails on the previous code (14 == 168).test_mmap_read_path.pyreal-mmap end-to-end now asserts the reported size equals the stored envelope length.test_decrypt_fail_policy.py,test_swr_decorator.py,test_mmap_read_path.py.Local:
ruff check/ruff format --checkclean;tests/unit -m "not slow"2261 passed;tests/critical -m "not slow"244 passed; doctest + markdown-docs collection green; File read-path memory-budget perf tests green.Docs
Internal handler return shape only; docstrings updated in the same diff.
docs/api-reference.mdanddocs/features/prometheus-metrics.mdalready describecache_operation_size_bytesas payload size, which is now true at every sync site. No docs.cachekit.io change.Summary by CodeRabbit
Summary
This PR fixes a metrics inconsistency where the sync L2 cache hit path was recording an incorrect
size_bytesvalue.Problem
When a sync cache hit occurred at the L2 level, the
size_bytesmetric was recordinglen(str(value).encode())— a repr-based estimate of the deserialized value — rather than the actual length of the served L2 envelope. For secure caches, this meant comparing ciphertext envelope sizes against plaintext repr sizes, producing a bimodal histogram on a single metric series.Changes
cache_handler.py: Updated theget_cached_valuedocstring to clarify that the returnedsize_bytesis the L2 envelope length, aligning it with the L1 hit site which recordslen(l1_bytes). Removed the now-inaccurate reference to the async path.wrapper.py: Refactored the sync wrapper to unpack the(True, value, size_bytes)tuple into named variables (_found,result,size_bytes) instead of using positional index access (cached_result[1],cached_result[2]), improving readability of the hit-metric recording.test_sync_l2_hit_size_bytes.py: Simplified the test by disabling L1 (l1_enabled=False) instead of relying on L1-clearing fixtures to force the code path to the L2 hit site. Removed the_clear_l1autouse fixture and the separaterecordedfixture, inlining the orchestrator patching directly into the test.Impact
The sync L2 hit metric now measures the same quantity as the L1 hit site, ensuring consistent
size_bytesreporting across cache tiers and avoiding misleading histograms for secure caches.