test(bench): ingest write-path criterion benches - #248
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new ChangesIngest Write-Path Benchmarks
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-bench/benches/ingest_write_path.rs`:
- Around line 35-139: The non-trivial benchmark helper functions wal_config and
rec lack unit tests to validate their correctness and assumptions. Add a
#[cfg(test)] module in this file with focused unit tests that verify the
wal_config function returns expected configuration values, the rec function
correctly generates MinedRecord instances with the varying parameter i properly
reflected in the output to ensure non-degenerate data for compression testing,
and validate any critical benchmark setup invariants such as proper TempDir
lifecycle management during benchmark execution. Place these tests adjacent to
the code they test to comply with coding guidelines.
- Around line 108-127: The issue is that r.clone() is being called inside the
measured closure for the ParquetRecordSink benchmark, so the cloning cost is
included in the timed sink.emit() operations which distorts the benchmark
signal. Move the cloning of records outside the timed path by cloning all
records in the setup closure (the first parameter to iter_batched) so that the
benchmark only measures the actual emit and flush operations without the clone
overhead. The measured closure should iterate over pre-cloned records instead.
🪄 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: 44f543b1-232a-4b0b-beb2-0b2d7dc2efd0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/ourios-bench/Cargo.tomlcrates/ourios-bench/benches/ingest_write_path.rs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new Criterion benchmark suite to ourios-bench to measure ingest write-path hot spots (OTLP→WAL append+fsync, and WAL→Parquet sink emit+flush), aligning with the performance harness needs described in CLAUDE.md §6.2.
Changes:
- Add
ingest_write_pathCriterion benches coveringwal_appendandsink_write/{1000,10000}. - Add
ourios-ingesteras a dev-dependency for access toParquetRecordSink. - Update
Cargo.lockfor the new dependency edge.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/ourios-bench/Cargo.toml | Registers the new ingest_write_path bench and adds ourios-ingester as a dev-dep. |
| crates/ourios-bench/benches/ingest_write_path.rs | Implements the new WAL append+sync and Parquet sink write-path Criterion benchmarks. |
| Cargo.lock | Adds ourios-ingester to the resolved dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds `ourios-bench/benches/ingest_write_path.rs` covering CLAUDE.md §6.2's
named ingest hot paths, alongside the existing b1/b2/recovery/compaction
benches:
- `wal_append` — OTLP → WAL: append one batch frame + fsync (the
WAL-before-ack unit), mirroring `ourios.wal.append.duration`.
- `sink_write/{1000,10000}` — WAL → Parquet: feed N mined records to a
`ParquetRecordSink` and flush to one Parquet object, mirroring
`ourios.sink.flush.*`. Throughput in records.
Both rebuild their fixture in the untimed `iter_batched` setup so only the
durable append / emit + flush is measured. Supportive **wall-clock**
evidence (like recovery/b1/b2) — to be run indicatively on the CI runner
first; the authoritative baseline-hardware numbers are a separate, opt-in
run (the `bench-on-ci-runner-first` discipline). Adds `ourios-ingester` as
a dev-dep for the sink half.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jensholdgaard
force-pushed
the
bench/ingest-write-path
branch
from
June 18, 2026 15:11
ffe01ca to
461651f
Compare
jensholdgaard
changed the title
Address review on #248: - Clone the record batch in the untimed `iter_batched` setup; the timed `sink_write` routine now emits owned records, so per-record clone cost no longer pollutes the WAL→Parquet signal (sink throughput rises to ~1.09 M rec/s at 10k, previously understated). - `wal_append` warms up with one append + sync in setup so the timed sync measures steady state — the first sync on a fresh WAL also fsyncs the segment directory (entry durability), which steady-state syncs don't. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 2 review on #248: `iter_batched` ran fixture teardown (dropping `TempDir`/`Wal`/sink → file close, Parquet unlink, dir delete) inside the timed routine. Switch both benches to `iter_custom` with an explicit `Instant` around just the append+fsync / emit+flush, so build and teardown are excluded and only one fixture is alive at a time (deferring drops via iter_batched outputs would instead accumulate open WALs/temp dirs across a sample batch). Teardown was a large fraction — `sink_write/1000` ~halves and sink throughput rises to ~1.56 M rec/s at 10k. Module doc updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(review) Round 3 review on #248: `ParquetRecordSink::flush_all` swallows encode/ store errors and retains the buffer, so a silent flush failure would still report a (bogus) `sink_write` timing for the error path. Add an untimed `assert_eq!(sink.buffered_records(), 0)` right after the timer — a failed flush now panics the bench instead of benchmarking the retry path. (`wal_append` is already covered: its append/sync `.expect()`s panic on error.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
Adds
ourios-bench/benches/ingest_write_path.rs— criterion benches for CLAUDE.md §6.2's named ingest hot paths, alongside the existingb1/b2/recovery/compactionbenches. This is the performance-test harness the perf-metrics work (#247) was paving the way for.wal_append— OTLP → WAL: append one batch frame + fsync (the WAL-before-ack unit; one batch → one frame → one sync). Mirrorsourios.wal.append.duration. Throughput in bytes.sink_write/{1000,10000}— WAL → Parquet: feed N mined records to aParquetRecordSinkand flush to one Parquet object (encode + put). Mirrorsourios.sink.flush.*. Throughput in records.Both rebuild their fixture in the untimed
iter_batchedsetup, so only the durable append / emit + flush is measured.Indicative local numbers (dev machine, real fsync — NOT a baseline)
wal_append≈ 4.4 ms/batch — fsync-bound; production amortizes one fsync across concurrent batches via group-commit.sink_write/1000≈ 222 K rec/s;sink_write/10000≈ 0.9 M rec/s — encode+put scales well as fixed overhead amortizes.Per the bench-on-ci-runner-first discipline, these are supportive wall-clock evidence (like
recovery/b1/b2), to be run indicatively on the CI runner; the authoritative baseline-hardware run is separate and opt-in.Notes
ourios-ingesteras aourios-benchdev-dep (forParquetRecordSink); the WAL half usesourios-waldirectly.docs/benchmarks.mdrow can follow if you want it tracked there alongside the thesis gates.🤖 Generated with Claude Code
Summary by CodeRabbit
ingest_write_pathperformance benchmarks for the ingest write hot paths, including:wal_append: measures steady-state WAL append + durable sync throughput (bytes).sink_write/{N}: measures Parquet record sink emit +flush_all()performance (records), with warmed-up setup and timing focused on the critical work.