feat(fastrace-futures): add StreamExt::enter_on_poll#174
Conversation
bce0f7f to
6bca79a
Compare
tisonkun
left a comment
There was a problem hiding this comment.
Generally LGTM. Thanks for your contribution @TennyZhuang!
There was a problem hiding this comment.
Pull request overview
Adds stream-level poll instrumentation to fastrace-futures, enabling lightweight per-poll_next local spans similar to fastrace::future::FutureExt::enter_on_poll, primarily to observe wake/poll frequency in long-lived streams.
Changes:
- Add
StreamExt::enter_on_pollandEnterOnPollStreamadapter to create aLocalSpanon eachpoll_next. - Add unit tests verifying span creation, parent/child relationships, and span completion on drop.
- Update dev-dependencies to enable
fastraceand run reporter-dependent tests serially.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| fastrace-futures/src/lib.rs | Implements enter_on_poll for streams and adds tests validating behavior and span relationships. |
| fastrace-futures/Cargo.toml | Adds dev-deps needed for span-reporting tests (fastrace with enable, serial_test). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fastrace = { workspace = true, features = ["enable"] } | ||
| async-stream = { version = "0.3" } | ||
| futures = { version = "0.3" } | ||
| serial_test = { version = "3.1" } |
There was a problem hiding this comment.
I think we can avoid adding serial_test here. This adapter can be tested through a local collection path instead of the process-global reporter: start a LocalCollector, poll a simple stream under a local parent, then inspect LocalSpans::to_span_records(...). That would keep the test focused and avoid a new dev dependency.
There was a problem hiding this comment.
Done. serial_test removed; tests now use LocalCollector::start() → collector.collect() → to_span_records(...).
| } | ||
| } | ||
|
|
||
| /// Starts a [`LocalSpan`] at every [`Stream::poll_next()`]. |
There was a problem hiding this comment.
Could you mention that this needs an outer in_span or another existing local parent? The example uses the right order, but spelling it out would help avoid .in_span(...).enter_on_poll(...), where the local span is entered before the local parent exists.
There was a problem hiding this comment.
Done. Added an "Important: Local parent required" section that spells out the need for an outer in_span or existing local parent, shows the correct .enter_on_poll().in_span() order, and warns against the reversed order.
6bca79a to
c81ad09
Compare
Adds `enter_on_poll` to `StreamExt`, mirroring `FutureExt::enter_on_poll`. Creates a `LocalSpan` on every `Stream::poll_next()`. Changes from review: - Remove serial_test dev-dependency - Rewrite tests to use LocalCollector + LocalSpans::to_span_records() - Strengthen docs to warn about composition order and local parent requirement Closes fast#172
c81ad09 to
b75d339
Compare
|
@andylokandy Both review items addressed in the latest push:
CI is re-running now. Thanks for the review! |
|
@TennyZhuang Thank you! |
Adds
StreamExt::enter_on_polltofastrace-futures, mirroringfastrace::future::FutureExt::enter_on_poll.EnterOnPollStreamcreates aLocalSpanon everyStream::poll_next(), useful for observing how often a stream is woken.Closes #172