#[instrument] - Future-proofing async-trait support in tracing-attributes

[nightmared]

Not a bug yet, but async-trait is working towards changing the way they wrap an async function in a trait impl (see dtolnay/async-trait#143 for more details).
More specifically, they are moving away from a design where they were wrapping the insides of async function into an async function nested inside a "classical" function, and returning the async function, boxing it along the way. Because some code is probably much clearer than anything I can say, this means that the impl of the following code will be translated from:

#[async_trait]
pub trait FakeTrait {
    async fn foo(&mut self);
}

struct Impl;

#[async_trait]
impl FakeTrait for Impl {
    async fn foo(&mut self) {
    }
}

to something like:

impl FakeTrait for Impl {
    fn foo(&mut self) -> Box<Pin<dyn Future<Output = ()>>> {
      async fn _foo(_self: &mut Self) {}
      Box::pin(_foo(self))
    }
}

If it can help to demangle my shoddy explanations above, the support for accepting this kind of expressions was added in #711.

The new version would (from what I understand, please correct me if I'm mistaken !) translate the previous code as:

impl FakeTrait for Impl {
    fn foo(&mut self) -> Box<Pin<dyn Future<Output = ()>>> {
      Box::pin(async move {})
    }
}

While we could support this code in the same fashion as we support the current version of async-trait, I fear this might lead to some confusion if a user deliberately want to instrument a function returning a future and end up instrumented the future instead. Not quite sure what to do here. Maybe we have to consider adding an argument to the #[instrument] macro that would opt-in to instrument the returned closure ?
It could be something like:

#[async_trait]
impl FakeTrait for Impl {
   #[instrument] // instrument the outer function (foo)
    async fn foo(&mut self) {
    }
}

#[async_trait]
impl FakeTrait for Impl {
   #[instrument(instrument_return_future)] // instrument the return (async move {}) instead of foo (the wrapper)
    async fn foo(&mut self) {
    }
}

But that's just some thoughts in the wind :)
Any opinion ?

[hawkw]

Hmm. IMO, I think instrumenting the outer function and not the returned future is almost never the behavior that users will want, and doing that by default seems like it would be very surprising. In particular, when #[async_trait] is being used, there is never any actual code in the outer function, so entering the span inside of the outer function without instrumenting the future would result in no user code running inside the span, which seems pretty wrong.

I'd rather err on the side of making the default do what I think people will want a significant majority of the time — if we're going to add an option to configure the behavior here, I'd make it opt out of instrumenting the returned future. Maybe #[instrument(function_only)] or something. Or, we could just always instrument the returned future. People who don't want that behavior can always just write

fn my_fn(arg: Arg) -> Box<dyn Future<...>> {
    let span = tracing::info_span!("my_fn", ?arg);
    let _e = span.enter();
    // ...do stuff inside the span...
    Box::new(async move {
        // ...outside the span...
    })
}

[nightmared]

Thanks for the clarification, you're probably right in that this behavior would be more correct (in the case of a sync function returning a boxed future at least, I'm not sure if there is many uses of functions that do a great deal of work to "prepare" the future, and that the user would want to instrument ?). In any case, if tracing-attributes were to default to instrumenting the returned future, letting the user enter the span manually instead of adding another parameter to #[instrument] is probably the right call.
In that case, i will try to hack something together this weekend.

[hawkw]

That would be great! Thanks for working on this.