This change exposes BenchSession and Observer API#104
Open
adityashah1212 wants to merge 2 commits into
Open
Conversation
This change exposes BenchSession and Observer API to as public interfaces, so that the bench can be executed without relying on run method. This allows to customize how the bench execution occurs and also allows access to BenchReport generated after the run. The Observer API gives even more visibility as it has access to results of each iteration and the user can define their custom Observer to observe results of each iteration if they so desire.
|
Warning Review limit reached
Your plan includes 2 reviews of capacity. Refill in 11 minutes and 29 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR removes the async-trait dependency and refactors the crate to an Observer + BenchSession architecture: new suite and observer traits, Runner reports via Observer, Aggregator/TUI accept futures channels, and examples/docs updated to drop async-trait usage. ChangesCore architecture refactoring
Examples and documentation update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 |
1 task
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
examples/custom_observer.rs (1)
44-48: ⚡ Quick winAssert observer notifications explicitly in the example.
The example wires a custom observer but never verifies it was called. Add a post-run counter assertion to validate observer behavior end-to-end.
💡 Suggested fix
let opts = BenchOpts::builder().iterations(1000).build()?; let observer = CountingObserver::default(); + let observed = observer.clone(); let session = BenchSession::new(SimpleBench).opts(opts).observer(observer); let report = session.run().await?; + assert_eq!(observed.0.load(Ordering::Relaxed), 1000); assert_eq!(report.stats.overall.iters, 1000); assert_eq!(report.status_dist[&Status::server_error(500)], 200); assert_eq!(report.status_dist[&Status::client_error(400)], 200);Also applies to: 52-52
🤖 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 `@examples/custom_observer.rs` around lines 44 - 48, The example registers a CountingObserver but doesn't assert it was invoked; after running the session (the call to BenchSession::new(SimpleBench).opts(opts).observer(observer) and session.run().await), add an explicit assertion on the observer's counter (the CountingObserver instance) to verify notifications were received (e.g., check its count increased or equals expected iterations) so the example validates observer behavior end-to-end.src/aggregator.rs (1)
1-11: 💤 Low valueDocumentation references removed types.
The module-level documentation still references
SilentCollector, but this type has been renamed toAggregatorand madepub(crate). Update the doc comments to reflect the current implementation.🤖 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 `@src/aggregator.rs` around lines 1 - 11, Update the module doc comment to reflect the renamed and visibility-changed type: replace all references to `SilentCollector` with `Aggregator` and note that `Aggregator` is `pub(crate)` (non-public) where relevant; also ensure references to `JsonReporter` remain accurate (e.g., `crate::reporter::JsonReporter`) and adjust any example/use-case wording that implied a publicly exported `SilentCollector` to indicate the internal `Aggregator` instead.src/tui/mod.rs (1)
67-71: ⚡ Quick winUse
nonzero!macro instead ofunsafeblock.The
nonzeromacro is already imported (line 38) and provides a safe, compile-time checked alternative tounsafe { NonZeroU8::new_unchecked(32) }.Suggested fix
impl Default for TuiSettings { fn default() -> Self { - Self { fps: unsafe { NonZeroU8::new_unchecked(32) }, auto_quit: true } + Self { fps: nonzero!(32u8), auto_quit: true } } }🤖 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 `@src/tui/mod.rs` around lines 67 - 71, Replace the unsafe NonZeroU8 construction in the Default impl for TuiSettings: inside fn default() of impl Default for TuiSettings swap unsafe { NonZeroU8::new_unchecked(32) } for the safe compile-time nonzero! macro (e.g., nonzero!(32u8) or nonzero!(32)) so the fps field is created without unsafe.src/session.rs (1)
18-18: 💤 Low valueTypo: "conslidated" → "consolidated".
Fix
-/// Define a bench session that can be run and get a conslidated report +/// Define a bench session that can be run and get a consolidated report🤖 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 `@src/session.rs` at line 18, Update the top-of-file/doc-comment in src/session.rs to fix the typo "conslidated" → "consolidated"; locate the doc string that currently reads "Define a bench session that can be run and get a conslidated report" (near the BenchSession/module comment) and replace the misspelled word so the comment reads "consolidated".
🤖 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 `@examples/custom_observer.rs`:
- Around line 22-26: The status selection uses per-worker ordering
(info.worker_seq) which makes aggregate counts non-deterministic under parallel
workers; change the selector to use the global iteration sequence
(info.runner_seq) in the status assignment (the match that sets `status`) and
the other occurrence mentioned (lines ~49-51) so status generation is
iteration-global and deterministic, or alternatively force a single worker if
you intentionally want worker-local behavior; update references of
`info.worker_seq` to `info.runner_seq` in the `status` logic and the other
identical spot.
In `@src/cli.rs`:
- Around line 274-279: The TUI is being enabled based only on cli.quiet which
ignores non-TTY stdout; update the logic that decides between session.run() and
session.with_tui(...) to use the existing cli.collector() / Collector::Silent
behavior (or explicitly check stdout().is_tty()) so the TUI is not enabled when
stdout is not a TTY or when cli.quiet is true; locate the code around
BenchSession::new(suite).opts(opts) and replace the current cli.quiet-only
branch (which calls session.with_tui(TuiSettings { fps: cli.fps, auto_quit:
!cli.quit_manually })) with a decision that calls session.with_tui only when
collector indicates interactive (i.e., not Collector::Silent / stdout().is_tty()
&& !cli.quiet), otherwise call session.run().
In `@src/observer.rs`:
- Around line 11-12: The documentation in observer.rs references removed types
crate::collector::TuiCollector and crate::collector::SilentCollector; update
those stale links to the current public API by either removing the specific type
links or replacing them with the appropriate exposed symbols (e.g.,
crate::collector::Collector trait or the current concrete collector types) so
the docs compile and reflect the new architecture—search for the TuiCollector
and SilentCollector mentions in observer.rs and change them to the current
collector interface or a generic crate::collector module reference.
- Line 15: The file imports and calls tracing::warn / warn! unconditionally even
though tracing is optional; gate the import and any warn! invocations behind
#[cfg(feature = "tracing")] (or define a small cfg-based shim: when feature
"tracing" is set re-export tracing::warn, otherwise provide a no-op warn macro)
so the crate builds with default-features = false, and update the rustdoc
references that point to non-existent crate::collector::TuiCollector and
crate::collector::SilentCollector by removing or replacing them with the
correct, current types/modules (or plain text) so docs no longer link to a
missing collector module.
In `@src/suite.rs`:
- Around line 46-49: The blanket impl for BenchSuite is overly restrictive
because it requires Sync; update the impl<T> BenchSuite for T where clause to
remove the Sync bound so it reads something like T: StatelessBenchSuite + Send +
'static; specifically edit the impl that references BenchSuite and
StatelessBenchSuite to drop the Sync constraint so stateless suites that are
Send but not Sync can implement BenchSuite.
---
Nitpick comments:
In `@examples/custom_observer.rs`:
- Around line 44-48: The example registers a CountingObserver but doesn't assert
it was invoked; after running the session (the call to
BenchSession::new(SimpleBench).opts(opts).observer(observer) and
session.run().await), add an explicit assertion on the observer's counter (the
CountingObserver instance) to verify notifications were received (e.g., check
its count increased or equals expected iterations) so the example validates
observer behavior end-to-end.
In `@src/aggregator.rs`:
- Around line 1-11: Update the module doc comment to reflect the renamed and
visibility-changed type: replace all references to `SilentCollector` with
`Aggregator` and note that `Aggregator` is `pub(crate)` (non-public) where
relevant; also ensure references to `JsonReporter` remain accurate (e.g.,
`crate::reporter::JsonReporter`) and adjust any example/use-case wording that
implied a publicly exported `SilentCollector` to indicate the internal
`Aggregator` instead.
In `@src/session.rs`:
- Line 18: Update the top-of-file/doc-comment in src/session.rs to fix the typo
"conslidated" → "consolidated"; locate the doc string that currently reads
"Define a bench session that can be run and get a conslidated report" (near the
BenchSession/module comment) and replace the misspelled word so the comment
reads "consolidated".
In `@src/tui/mod.rs`:
- Around line 67-71: Replace the unsafe NonZeroU8 construction in the Default
impl for TuiSettings: inside fn default() of impl Default for TuiSettings swap
unsafe { NonZeroU8::new_unchecked(32) } for the safe compile-time nonzero! macro
(e.g., nonzero!(32u8) or nonzero!(32)) so the fps field is created without
unsafe.
🪄 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
Run ID: 4204df50-d9a1-4be1-a06e-01b463b590fd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.tomlREADME.mdexamples/baseline.rsexamples/custom_observer.rsexamples/http_hyper.rsexamples/http_reqwest.rsexamples/logging.rsexamples/postgres.rsexamples/simple_stateless.rsexamples/warmup.rssrc/aggregator.rssrc/baseline/mod.rssrc/cli.rssrc/clock.rssrc/collector/mod.rssrc/histogram.rssrc/lib.rssrc/observer.rssrc/report.rssrc/runner.rssrc/session.rssrc/suite.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/render.rssrc/tui/state.rssrc/tui/terminal.rssrc/tui/tui_log.rs
💤 Files with no reviewable changes (8)
- examples/postgres.rs
- examples/logging.rs
- examples/http_hyper.rs
- src/collector/mod.rs
- examples/baseline.rs
- examples/http_reqwest.rs
- README.md
- examples/warmup.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cli.rs (1)
275-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse resolved collector when choosing run mode.
Line 275 still branches only on
cli.quiet, so non-TTY output and--collector silentcan incorrectly enter the TUI path. Branch usingcli.collector()(or include!stdout().is_tty()) to keep behavior consistent with CLI semantics.Suggested fix
let session = BenchSession::new(suite).opts(opts); - let report = if cli.quiet { + let report = if matches!(cli.collector(), Collector::Silent) { session.run().await } else { session.with_tui(TuiSettings { fps: cli.fps, auto_quit: !cli.quit_manually || !stdout().is_tty() }).run().await }?;🤖 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 `@src/cli.rs` around lines 275 - 279, The current branch uses cli.quiet to decide whether to call session.run() or session.with_tui(...).run(), but this ignores the resolved collector setting and TTY status; update the branching to use the resolved collector (call cli.collector()) and/or include stdout().is_tty() so that when the collector is "silent" or stdout is not a TTY we call session.run(), otherwise use session.with_tui(TuiSettings { fps: cli.fps, auto_quit: !cli.quit_manually || !stdout().is_tty() }).run().await; ensure you reference cli.collector(), cli.quiet, session.with_tui, TuiSettings and stdout().is_tty() when making the change so the CLI semantics are preserved.
🤖 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.
Duplicate comments:
In `@src/cli.rs`:
- Around line 275-279: The current branch uses cli.quiet to decide whether to
call session.run() or session.with_tui(...).run(), but this ignores the resolved
collector setting and TTY status; update the branching to use the resolved
collector (call cli.collector()) and/or include stdout().is_tty() so that when
the collector is "silent" or stdout is not a TTY we call session.run(),
otherwise use session.with_tui(TuiSettings { fps: cli.fps, auto_quit:
!cli.quit_manually || !stdout().is_tty() }).run().await; ensure you reference
cli.collector(), cli.quiet, session.with_tui, TuiSettings and stdout().is_tty()
when making the change so the CLI semantics are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1a129fe-f482-4cb1-8f4b-80de61843755
📒 Files selected for processing (4)
examples/custom_observer.rssrc/cli.rssrc/observer.rssrc/suite.rs
Author
|
@wfxr, can you please review this? |
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
1 participant
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.
Description
This change exposes BenchSession and Observer API to as public interfaces, so that the bench can be executed without relying on run method. This allows to customize how the bench execution occurs and also allows access to BenchReport generated after the run.
The Observer API gives even more visibility as it has access to results of each iteration and the user can define their custom Observer to observe results of each iteration if they so desire.
Breaking Change: This change removes the need for async-trait as it remove
ReportCollectorand hence the need for it to bedyncompatible. As forBenchSuiteandStatelessBenchSuite, they had no reason to be markedasync_traitas neither neededdyncompatibility to begin with. This simplifies the dependencies for the user crates.Related Issue
#103
Checklist
Summary by CodeRabbit
New Features
Refactor
Documentation