Blanket-implement tracing::instrument::WithSubscriber

[nightkr]

This provides a blanket implementation of WithSubscriber, as already implied by the docs, discussed at https://canary.discord.com/channels/500028886025895936/627649734592561152/850435247216001074, and already implemented by tracing-futures.

Motivation

The current docs are confusing since they describe behaviour that is effectively unusable.

[nightkr]

The bound here is copied from tracing-futures. It might be worth adding a Future bound to avoid polluting autocompletes too much though?

[hawkw]

The tracing-futures version does not have a Future bound because it also allows wrapping types implementing other traits, such as Stream, with a subscriber. However, tracing's version only provides implementations for Future, because tracing depends only on the standard library, not the futures crate. So, it should be fine to restrict the blanket impl to T: Future, here.

However, when the Stream trait eventually does make it into std, we'll probably want to add a Stream impl for WithSubscriber<T: Stream>, as well. Then, we'll have to make the blanket impl more general. Technically, I think this is a breaking change, as it could break user WithSubscriber impls for types that don't implement Future. To protect against that, I think we would need to seal the trait so that downstream code can't add WithSubscriber impls. Then, it would be fine to add a Future bound on the blanket impl, for now.

[hawkw]

Thanks, this looks good --- not having it was definitely an oversight.

In re: your comment about adding a Future bound to the blanket impl, I think we could do that, but it might require a little more complexity to avoid breaking changes if we want to add WithSubscriber impls for other traits. I left a more detailed comment as a reply to yours.

I'd be happy to merge this as-is, or we could make the additional changes I suggested in my comment, whatever you think is better?

[hawkw]

The tracing-futures version does not have a Future bound because it also allows wrapping types implementing other traits, such as Stream, with a subscriber. However, tracing's version only provides implementations for Future, because tracing depends only on the standard library, not the futures crate. So, it should be fine to restrict the blanket impl to T: Future, here.

However, when the Stream trait eventually does make it into std, we'll probably want to add a Stream impl for WithSubscriber<T: Stream>, as well. Then, we'll have to make the blanket impl more general. Technically, I think this is a breaking change, as it could break user WithSubscriber impls for types that don't implement Future. To protect against that, I think we would need to seal the trait so that downstream code can't add WithSubscriber impls. Then, it would be fine to add a Future bound on the blanket impl, for now.

[nightkr]

Yeah.. good point about Stream breaking it eventually anyway. Sealing seems like overkill, especially since we'd have to revert to the blanket-blanket later anyway.

I suppose we should just go ahead then.