WIP: Correctly instrument non-lazy Futures functions #1865
Conversation
There was a problem hiding this comment.
overall, this solution looks correct, i don't see any issues with the change. i did comment on some naming and other style nits --- currently, a lot of the code references the async-trait crate specifically, but this code will now work with all functions returning async blocks, regardless of whether they are generated by async-trait. so, it would be nice for the naming and comments to be updated to reflect that.
tracing-attributes/tests/async_fn.rs
Outdated
| #[instrument] | ||
| fn manual_impl_future() -> impl Future<Output = ()> { | ||
| async move { | ||
| tracing::trace!(poll = true); | ||
| } | ||
| } | ||
| #[instrument] | ||
| fn manual_box_pin() -> Pin<Box<dyn Future<Output = ()>>> { | ||
| Box::pin(async move { | ||
| tracing::trace!(poll = true); | ||
| }) | ||
| } |
There was a problem hiding this comment.
nit, take it or leave it: personally, i think it would be nice if the boxed and non-boxed versions were in separate tests, so that we can always test both cases even if one fails, and know which one is broken. for example, my guess is that the Box::pin version currently does work on master, while the impl Future version doesn't; it would be nice if the tests indicated that.
tracing-attributes/tests/async_fn.rs
Outdated
| #[instrument] | ||
| fn hybrid_non_lazy_async() -> impl Future<Output = ()> { | ||
| tracing::trace!(call = true); | ||
| async move { | ||
| tracing::trace!(poll = true); | ||
| } | ||
| } | ||
| #[instrument] | ||
| fn hybrid_non_lazy_async_boxed() -> Pin<Box<dyn Future<Output = ()>>> { | ||
| tracing::trace!(call = true); | ||
| Box::pin(async move { | ||
| tracing::trace!(poll = true); | ||
| }) | ||
| } |
There was a problem hiding this comment.
similarly, i have a slight preference for having separate tests for the pinned and unpinned cases.
tracing-attributes/src/expand.rs
Outdated
| enum AsyncTraitKind<'a> { | ||
| // old construction. Contains the function | ||
| Function(&'a ItemFn), | ||
| // new construction. Contains a reference to the async block | ||
| Async(&'a ExprAsync), | ||
| // new construction. Contains a reference to the async block, and a bool | ||
| // indicating if the return value is a `Box::pin` | ||
| Async { | ||
| async_expr: &'a ExprAsync, | ||
| pinned_box: bool, | ||
| }, | ||
| } |
There was a problem hiding this comment.
i'm not sure if this should still be called AsyncTraitKind. since #[instrument] now handles all functions returning async blocks, not just those generated by the #[async_trait] macro, i think it would be nice to rename this type (and other types) to reflect that.
tracing-attributes/src/expand.rs
Outdated
| Async { | ||
| async_expr: &'a ExprAsync, | ||
| pinned_box: bool, | ||
| }, | ||
| } | ||
|
|
||
| pub(crate) struct AsyncTraitInfo<'block> { |
There was a problem hiding this comment.
similarly, it seems like this is now used for all functions returning async blocks, not just those generated by async-trait. we should probably rename it.
tracing-attributes/src/expand.rs
Outdated
| // new construction. Contains a reference to the async block, and a bool | ||
| // indicating if the return value is a `Box::pin` |
There was a problem hiding this comment.
maybe also re-word this comment to not be async-trait specific, as well?
tracing-attributes/src/expand.rs
Outdated
| // check that the move 'keyword' is present | ||
| async_expr.capture?; |
There was a problem hiding this comment.
hmm, should we try to handle functions returning async blocks without the move keyword, too?
Swatinem
force-pushed
the
fix/lazy-async
branch
from
9071a40 to
94754de
Compare
|
@Swatinem do you want to keep this PR open? AFAICT, it's been superseded by your other branches, is that correct? |
|
I will just close this for now. It has a failing test that I want to eventually succeed, but I want to work through some refactors first. |
So far this is just a failing testcase of how I think the testcase from #1863 should behave at runtime.
I will be working on fixing up the code generation next.