feat(metrics): add compaction io + H4 file-size telemetry (RFC 0009 §3.6) - #113
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR instruments compaction operations with byte-volume metrics across three layers: ChangesCompaction I/O and File-Size Metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ourios-ingester/src/metrics.rs (1)
130-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord
ourios.compaction.ioeven when a sweep fails.
iodatapoints are currently skipped onErr(IngestError), so first-sweep visibility is lost on fatal scan failures and direction series may disappear during outage windows. Emitread=0/write=0outside theOk(report)block, then override values fromreportwhen present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-ingester/src/metrics.rs` around lines 130 - 147, Currently io datapoints are only emitted inside the Ok(result) branch, so on Err(IngestError) the read/write series vanish; move the self.io.add calls so a default zero-valued pair is emitted unconditionally before matching result (emit self.io.add(0, &[KeyValue::new(semconv::OURIOS_IO_DIRECTION, "read")]) and self.io.add(0, &[KeyValue::new(semconv::OURIOS_IO_DIRECTION, "write")]) ), then in the Ok(report) branch compute bytes_written and call self.io.add(report.bytes_read, ..) and self.io.add(bytes_written, ..) to override the zeros; update the code around result handling where bytes_written, report, and self.io.add are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-ingester/src/metrics.rs`:
- Line 139: The bytes_written aggregation uses .sum() which can overflow; change
the computation of bytes_written from report.compacted_files.iter().map(|f|
f.bytes).sum() to a saturating fold so it never wraps (e.g., use iter().map(|f|
f.bytes).fold(0u64, |acc, b| acc.saturating_add(b))); update the variable
bytes_written and related logic in metrics.rs to match the saturating
accumulation semantics used by run_sweep in compactor.rs.
---
Outside diff comments:
In `@crates/ourios-ingester/src/metrics.rs`:
- Around line 130-147: Currently io datapoints are only emitted inside the
Ok(result) branch, so on Err(IngestError) the read/write series vanish; move the
self.io.add calls so a default zero-valued pair is emitted unconditionally
before matching result (emit self.io.add(0,
&[KeyValue::new(semconv::OURIOS_IO_DIRECTION, "read")]) and self.io.add(0,
&[KeyValue::new(semconv::OURIOS_IO_DIRECTION, "write")]) ), then in the
Ok(report) branch compute bytes_written and call self.io.add(report.bytes_read,
..) and self.io.add(bytes_written, ..) to override the zeros; update the code
around result handling where bytes_written, report, and self.io.add are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 07864e9e-4e60-4616-95f6-1e9348688c7e
📒 Files selected for processing (3)
crates/ourios-ingester/src/compactor.rscrates/ourios-ingester/src/metrics.rscrates/ourios-parquet/src/compaction.rs
There was a problem hiding this comment.
Pull request overview
Adds the remaining RFC 0009 §3.6 compaction telemetry by plumbing byte-volume measurements out of ourios-parquet compaction and emitting new OpenTelemetry instruments in ourios-ingester, including the H4 small-file detector.
Changes:
- Extend
CompactionOutcomewith best-effortbytes_read/bytes_writtenand compute them duringcompact_partition. - Extend sweep reporting to carry aggregate read volume plus per-committed-output
(tenant, bytes)samples. - Add
ourios.compaction.iocounter (read/write viaourios.io.direction) andourios.storage.parquet.file.sizeper-tenant histogram, with tests validating export.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/ourios-parquet/src/compaction.rs |
Computes and returns best-effort compaction read/write byte volumes; adds unit tests for byte-volume reporting. |
crates/ourios-ingester/src/compactor.rs |
Propagates byte-volume data into SweepReport, including per-tenant consolidated file-size samples. |
crates/ourios-ingester/src/metrics.rs |
Defines and records new io counter and file_size histogram instruments; extends in-memory exporter tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
69952ec to
6e44502
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…3.6) Complete the compaction metric set with the two byte-volume instruments RFC 0009 §3.6 deferred: - `ourios.storage.parquet.file.size` (Histogram, By, `ourios.tenant`) — the H4 small-file detector: a per-tenant distribution of consolidated output sizes, so the "alert when > 5 % of files < 128 MiB" rule (the small-file hazard) is a derived alert over this distribution. - `ourios.compaction.io` (Counter, By, `ourios.io.direction`) — bytes read from the merged-away inputs and written to the consolidated file. `compact_partition` now reports `bytes_read` / `bytes_written` on `CompactionOutcome` (best-effort `stat`: a metric inaccuracy on a file we just read or wrote never fails a committed compaction), the `SweepReport` carries them through plus a per-tenant `CompactedFile` per output, and `CompactionMetrics::record_sweep` records both instruments. Names/units come from the generated `ourios-semconv` constants — no registry change. `ourios.compaction.backlog` (an UpDownCounter for sealed-but-uncompacted lag — a cross-sweep gauge, not a per-compaction quantity) stays the one §3.6 metric deferred. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6e44502 to
b87299e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
Completes the compaction metric set (RFC 0009 §3.6) with the two byte-volume instruments the earlier telemetry slice deferred — headlined by the H4 small-file detector.
ourios.storage.parquet.file.sizeByourios.tenantourios.compaction.ioByourios.io.directionourios.storage.parquet.file.size— the H4 detector (hazards.md docs: apply RFC maturity-model amendments #4, small-file problem). A per-tenant distribution of consolidated output-file sizes; the "alert when > 5 % of files < 128 MiB" rule is a derived alert over this distribution, not a base metric.ourios.compaction.io— bytes read from the merged-away inputs vs. written to the consolidated output, split byourios.io.direction(read/write).How
compact_partitionnow reportsbytes_read/bytes_writtenonCompactionOutcome. Sizing is best-effort (stat→0on failure) by design: a metric inaccuracy on a file we just read or wrote must never turn a committed compaction into a failure.SweepReportcarries the aggregatebytes_readplus one per-tenantCompactedFile { tenant, bytes }per committed output (the per-tenant histogram samples).CompactionMetricsbuilds theiocounter +file_sizehistogram and records both inrecord_sweep. Neither is zero-seeded (required attribute / histogram), consistent withsweeps/duration;iosurfaces on the first sweep with a 0-byte point.ourios-semconvconstants (already present) — no registry / weaver change.Invariants (CLAUDE.md §4, hazard H4)
This is the H4 ("small file problem") detection metric. It is observe-only — it changes no compaction behaviour, only exposes the per-tenant file-size distribution the H4 alert needs. Row conservation, the audit event, and the atomic manifest swap are untouched.
ourios.compaction.backlog(an UpDownCounter for sealed-but-uncompacted lag — a cross-sweep gauge, not a per-compaction quantity) remains the one §3.6 metric still deferred.Tests
ourios-parquet:compact_partitionreports a non-zero read volume and a write volume equal to the consolidated file's on-disk size; a no-op reports zero volumes.ourios-ingester: the in-memory exporter shows all 8 compaction metrics;iocarries read (4096) and write (3072 = Σ outputs) direction points;file.sizeis a per-tenant histogram with one sample per output. (Single test / single global provider —init_in_memoryinstalls the global meter.)Local:
cargo fmt --all --check,cargo clippy --workspace --all-targets --all-features -D warnings,cargo test --workspace --all-featuresall green.Part of epic #94.
🤖 Generated with Claude Code
Summary by CodeRabbit