refactor(metrics): add dedicated range tombstone block counters#36
Merged
polaz merged 9 commits intoMar 21, 2026
Merged
Conversation
- Add range_tombstone_block_load_io, _load_cached, _io_requested fields to Metrics struct - Add accessor methods: range_tombstone_block_io(), range_tombstone_block_load_count(), range_tombstone_block_load_cached_count(), range_tombstone_block_cache_hit_rate() - Update aggregate methods (block_io, block_load_io_count, block_load_cached_count) to include range tombstone counters - Split RangeTombstone out of Data|Meta match arms in load_block() Closes #13 Closes #34
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds dedicated range-tombstone metrics and accessors, updates aggregate block-level metric methods to include range-tombstone counts/bytes, splits metrics accounting in Changes
Sequence Diagram(s)sequenceDiagram
participant Table as Table::recover / load_block
participant BlockIO as Block::from_file
participant Metrics as Metrics
participant Cache as Cache
Table->>BlockIO: read range-tombstone block
BlockIO-->>Table: block(bytes, type=RangeTombstone)
Table->>Metrics: increment range_tombstone_block_load_io (cfg(feature="metrics"))
Table->>Metrics: add range_tombstone_block_io_requested(bytes) (cfg(feature="metrics"))
Table->>Cache: populate block cache
Note right of Cache: subsequent load_block() call
Table->>Cache: try load from cache
Cache-->>Table: hit
Table->>Metrics: increment range_tombstone_block_load_cached (cfg(feature="metrics"))
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors table block-load metrics to track BlockType::RangeTombstone separately from Data|Meta, improving observability for delete-heavy workloads and making BlockType matching more compile-time-exhaustive.
Changes:
- Split
BlockType::RangeTombstoneinto dedicated match arms inload_block()(cache-hit and I/O paths). - Add new
Metricscounters for range tombstone block cache hits, disk/page-cache reads, and requested bytes. - Update aggregate/accessor methods (
block_io, load counts, cache hit rates) to include the new range tombstone counters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/table/util.rs |
Routes range tombstone block cache/I/O events to dedicated metrics counters via explicit BlockType match arms. |
src/metrics.rs |
Adds new atomic counters and updates aggregate/accessor methods to include range tombstone metrics. |
- Test default zero values, load count summation, cache hit rate - Test aggregate methods (block_io, block_load_io_count, block_load_cached_count) include RT counters - Add NOTE comments in load_block() explaining that RT blocks currently bypass this path (loaded eagerly at table recovery) Closes #13 Closes #34
|
🧠 Learnings used✅ Actions performedFull review triggered. |
…e-blocktype-match-exhaustive-in-tableu
…disk" All *_load_cached_count() methods return block-cache hits, not disk reads. The pre-existing docstrings incorrectly said "from disk or OS page cache" — corrected to "served from block cache".
Table recovery loads range tombstone blocks via Block::from_file(), bypassing load_block(). Increment range_tombstone_block_load_io and range_tombstone_block_io_requested directly at the recovery call site so that real-world RT block reads are reflected in metrics. Remove stale NOTE comments from load_block() match arms and update test assertions to account for the recovery-time increment.
…bleu' of github.com:structured-world/lsm-tree into feat/#13-refactor-make-blocktype-match-exhaustive-in-tableu
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/table/mod.rs`:
- Around line 582-589: The startup recover() path is incorrectly recording its
eager RT read into
range_tombstone_block_load_io/range_tombstone_block_io_requested even though it
bypasses load_block() and the block cache; change the metric to a
recovery-specific counter instead: add new metrics (e.g.
range_tombstone_recovery_block_load_io and
range_tombstone_recovery_block_io_requested) and update the block in question to
increment those instead of
range_tombstone_block_load_io/range_tombstone_block_io_requested so
load_block(), block_cache_hit_rate(), and range_tombstone_block_cache_hit_rate()
remain semantically correct; reference recover(), the current increments of
range_tombstone_block_load_io and range_tombstone_block_io_requested, and
load_block() when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c20380-2460-401c-a770-79db4ce617c2
📒 Files selected for processing (4)
src/metrics.rssrc/table/mod.rssrc/table/tests.rssrc/table/util.rs
Recovery reads RT blocks via Block::from_file(), bypassing the block cache. Counting these as range_tombstone_block_load_io skews block_cache_hit_rate() (unpaired IO without cached counterpart). Revert the recovery-site increment so RT counters track only load_block() calls — consistent with filter/index recovery which also bypass metrics. Add #[expect(clippy::unwrap_used)] on writer.finish() in the test.
This was referenced Mar 21, 2026
3 tasks
9 tasks
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.
Summary
BlockType::RangeTombstoneout ofData|Metamatch arms inload_block()into dedicated branchesMetrics:range_tombstone_block_load_cached,range_tombstone_block_load_io,range_tombstone_block_io_requestedblock_io,block_load_io_count,block_load_cached_count) to include range tombstone countersCloses #13
Closes #34
Test plan
cargo check --all-featurespassescargo test --all-features— 27/27 tests passBlockTypeadditions at compile timeSummary by CodeRabbit
New Features
Tests