Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl<T: Stream> Stream for Instrumented<T> #2540

Closed
evgeniy-terekhin opened this issue Mar 29, 2023 · 7 comments
Closed

impl<T: Stream> Stream for Instrumented<T> #2540

evgeniy-terekhin opened this issue Mar 29, 2023 · 7 comments

Comments

@evgeniy-terekhin
Copy link

evgeniy-terekhin commented Mar 29, 2023

it would be nice to have impl<T: Stream> Stream for Instrumented<T> just as we have impl<T: Stream> Stream for Instruemnted<T> otherwise it is really unergonomic to propagate tracing spans into streams

I have this use case: actix-web streaming response

let span = tracing::Span::current();
Ok(
    HttpResponse::Ok().streaming(ReceiverStream::new(rx).map_ok(web::Bytes::from).map_err(
        move |err| {
            span.in_scope(|| {
                tracing::error!("some error happened: {err:#}");
            });

            service::errors::internal_server_error()
        },
    )),
)

what I would like to have is

use tracing::Instrument;

Ok(HttpResponse::Ok().streaming(
    ReceiverStream::new(rx)
        .in_current_span()
        .map_ok(web::Bytes::from)
        .map_err(|err| {
            tracing::error!("some error happened: {err:#}");

            service::errors::internal_server_error()
        }),
))

however this doesn't compile

error[E0599]: the method `map_ok` exists for struct `Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>`, but its trait bounds were not satisfied
    |
71  |               .map_ok(web::Bytes::from)
    |                ^^^^^^ method cannot be called on `Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>` due to unsatisfied trait bounds
    |
   ::: /home/evgeniy/.cargo/registry/src/github.com-1ecc6299db9ec823/tracing-0.1.37/src/instrument.rs:247:1
    |
247 | / pin_project! {
248 | |     /// A [`Future`] that has been instrumented with a `tracing` [`Span`].
249 | |     ///
250 | |     /// This type is returned by the [`Instrument`] extension trait. See that
...   |
261 | |     }
262 | | }
    | | -
    | | |
    | |_doesn't satisfy `_: TryStreamExt`
    |   doesn't satisfy `_: TryStream`
    |
    = note: the following trait bounds were not satisfied:
            `Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>: TryStream`
            which is required by `Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>: TryStreamExt`
            `&Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>: TryStream`
            which is required by `&Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>: TryStreamExt`
            `&mut Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>: TryStream`
            which is required by `&mut Instrumented<ReceiverStream<Result<Vec<u8>, anyhow::Error>>>: TryStreamExt`

warning: unused import: `futures_util::strea
@ilslv
Copy link
Contributor

ilslv commented Mar 30, 2023

@evgeniy-terekhin
Copy link
Author

evgeniy-terekhin commented Mar 30, 2023

isn't it different from tracing::instrument::Instrumented?

@ilslv
Copy link
Contributor

ilslv commented Mar 30, 2023

They are a different type and trait, but are drop-in replacement for analogs from tracing.

AFAIK tracing-futures predates tracing::Instrument and was partially copied into tracing: #808

@evgeniy-terekhin
Copy link
Author

ok so are you proposing to use tracing_futures::Instrument trait instead of tracing::Ibstrument if I want to have Instrumented implemented for Stream?

I am not sure this suggestion is good in long-term perspective

why have two Instrument trait implementations anyways? shouldn't tge one from tracing-futures be deprecated?

@hawkw
Copy link
Member

hawkw commented Mar 30, 2023

The tracing-futures crate implements compatibility between tracing and the futures crate.

The tracing crate attempts to minimize dependencies exposed in its public API, for stability reasons. It provides an Instrument trait that returns an Instrumented type which only implements the Future trait, because Future is in the standard library. Stream, on the other hand, is provided by the futures crate, and is not in the standard library. If tracing implemented traits from futures and a new incompatible version of the futures crate is released, this would be a breaking change for tracing, which we try to avoid whenever possible. On the other hand, the tracing-futures crate can release new versions more easily, and we can make breaking changes when a new futures version is released.

When the AsyncIterator (née Stream) trait is added to the Rust standard library, tracing's version of Instrumented will implement that trait.

@evgeniy-terekhin
Copy link
Author

@hawkw thanks for the explanation! it now makes much more sense. I am closing the issue.

@hawkw
Copy link
Member

hawkw commented Mar 30, 2023

Glad that was helpful! I think the tracing-futures crate probably ought to document the relationship between its Instrument/Instrumented and the similarly named APIs in tracing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants