Closes: #52

This is suggested by the Opentelemetry spec, which requires "Therefore, HTTP client spans SHOULD be using conservative, low cardinality names formed from the available parameters of an HTTP request, such as "HTTP {METHOD_NAME}". Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name. "

Context: https://github.com/LemmyNet/lemmy/pull/1809#issuecomment-1146234710

[## Bug description

I get the following error when trying to use reqwest-retry with RetryTransientMiddleware

Middleware error: Request object is not clonable. Are you passing a streaming body?

To Reproduce

Do a streaming body request:

let res = client_req.send().await.map_err(error::ErrorBadRequest);
  Ok(HttpResponse::build(res.status()).body(BodyStream::new(res.bytes_stream())))
}

Expected behavior

Not give an error, as when I don't have RetryTransientMiddleware

Environment

• OS: Arch Linux
• Rust version: 1.61.0

Additional context

Context: https://github.com/apollographql/router/pull/656

In the router, we saw that opentelemetry headers were not sent to backend servers in some cases, and it turns out that the log level was not set for reqwest-tracing, so this span was marked as disabled, then here the SpanExt::context method would fail on with_subscriber because the span is disabled.

I understand why it is designed that way, but it feels surprising that the log level in reqwest-tracing affects opentelemetry propagation in the calling application, and I am wondering if instead it could be enabled or disabled depending on the parent span?

Maybe with something like Span::or_current? Any other suggestions?

if current Span exist trace_id,does reqwest-middleware extend Current span trace_id?

E.g:

use reqwest_tracing::{default_on_request_end, reqwest_otel_span, ReqwestOtelSpanBackend, TracingMiddleware};
use opentelemetry::sdk::export::trace::stdout;
use reqwest::{Request, Response};
use reqwest_middleware::{ClientBuilder, Result};
use std::time::Instant;
use task_local_extensions::Extensions;
use tracing::{info_span, Span};
use tracing_subscriber::layer::SubscriberExt;
use tracing_subscriber::Registry;

pub struct TimeTrace;

impl ReqwestOtelSpanBackend for TimeTrace {
    fn on_request_start(req: &Request, extension: &mut Extensions) -> Span {
        extension.insert(Instant::now());
        reqwest_otel_span!(req, time_elapsed = tracing::field::Empty)
    }

    fn on_request_end(span: &Span, outcome: &Result<Response>, extension: &mut Extensions) {
        let time_elapsed = extension.get::<Instant>().unwrap().elapsed().as_millis() as i64;
        default_on_request_end(span, outcome);
        span.record("time_elapsed", &time_elapsed);
    }
}

#[tokio::main]
async fn main() {
    let tracer = stdout::new_pipeline().install_simple();
    let telemetry = tracing_opentelemetry::layer().with_tracer(tracer);
    let subscriber = Registry::default().with(telemetry);
    tracing::subscriber::set_global_default(subscriber).unwrap();

+    let span = info_span!("trace_id","1234");
    let span = span.enter();
    run().await;
}

async fn run() {
    let span = Span::current();
+    println!("--->{:?}",span);
    let client = ClientBuilder::new(reqwest::Client::new())
        .with(TracingMiddleware::default())
        .build();

  +  client.get("https://truelayer.com").send().await.unwrap();
}

• Current Output

--->Span { name: "trace_id", level: Level(Info), target: "demo", id: Id(1), module_path: "demo", line: 33, file: "src/main.rs" }
SpanData { span_context: SpanContext { trace_id: 9b94f068662cee9c5076363a8896a114, span_id: 49ae88e2b0dcbef2, trace_flags: TraceFlags(1), is_remote: false, trace_state: TraceState(None) }, parent_span_id: 0000000000000000, span_kind: Internal, name: "encode_headers", start_time: SystemTime { tv_sec: 1668667233, tv_nsec: 334094000 }, end_time: SystemTime { tv_sec: 1668667233, tv_nsec: 334356000 },...
SpanData { span_context: SpanContext { trace_id: b318146ecd8c4380ebce06a05d77f0f8, span_id: 0c389105b17853b6, trace_flags: TraceFlags(1), is_remote: false, trace_state: TraceState(None) }, parent_span_id: 0000000000000000, span_kind: Internal, name: "parse_headers", start_time: SystemTime { tv_sec: 1668667233, tv_nsec: 594372000 }, end_time: SystemTime { tv_sec: 1668667233, tv_nsec: 594609000 },...
SpanData { span_context: SpanContext { trace_id: 5d0aaf9994579e8efc422e0fb9016508, span_id: d169884eca2ec6b0, trace_flags: TraceFlags(1), is_remote: false, trace_state: TraceState(None) }, parent_span_id: 0000000000000000, span_kind: Internal, name: "parse_headers", start_time: SystemTime { tv_sec: 1668667233, tv_nsec: 594712000 }, end_time: SystemTime { tv_sec: 1668667233, tv_nsec: 595089000 },...

• Expect Output

SpanData { span_context: SpanContext { trace_id: 1234, span_id: 49ae88e2b0dcbef2, trace_flags: TraceFlags(1), is_remote: false, trace_state: TraceState(None) }, parent_span_id: 0000000000000000, span_kind: Internal, name: "encode_headers", start_time: SystemTime { tv_sec: 1668667233, tv_nsec: 334094000 }, end_time: SystemTime { tv_sec: 1668667233, tv_nsec: 334356000 },...
SpanData { span_context: SpanContext { trace_id: 1234, span_id: 0c389105b17853b6, trace_flags: TraceFlags(1), is_remote: false, trace_state: TraceState(None) }, parent_span_id: 0000000000000000, span_kind: Internal, name: "parse_headers", start_time: SystemTime { tv_sec: 1668667233, tv_nsec: 594372000 }, end_time: SystemTime { tv_sec: 1668667233, tv_nsec: 594609000 },...
...

Motivations

I am trying to test my middleware implementation but I can't because I can't call handle because I can't construct a Next.

Solution

Provide an easy-to-use mechanism for testing middleware implementations.

Alternatives

Make Next public for construction so I can at least call handle myself.

Motivations

I would like to have a option to customize middleware behaviour per each request. For example, I could set per request retry policy, set some custom request identifier for logging, etc...

Solution

I imagine this could be somehow solved by adding ability to set initial values for extensions. Either by specifying initial Extensions value with new send_with_extensions method or maybe nicer by somehow wraping reqwest:: RequestBuilder to have context/data method (similar to actix_web::App.app_data)

Alternatives

Additional context

As suggested in https://github.com/TrueLayer/reqwest-middleware/pull/54#issuecomment-1231396940

Adds a set of Extensions to RequestBuilder so that requests can have some default extensions provided.

Adds a new way to configure requests: RequestInitialiser - it's a simple sync trait that just applies basic settings to RequestBuilder.

Adds a Extension RequestInitialiser which calls insert during the init chain

Bug description

When copy/pasting the example from the readme into a test project, it does not work because the dependencies cannot be satisfied. Adjusting the dependencies results in a building issue.

To Reproduce

cd /tmp
cargo new rtexample
cd rtexample/
code -n .
# Copy/paste the example from the readme

At this point, trying to build results in the following error:

rtexample on  master [?] via 🦀 v1.59.0
❯ cargo build
    Updating crates.io index
error: failed to select a version for `reqwest-tracing`.
    ... required by package `rtexample v0.1.0 (/private/tmp/rtexample)`
versions that meet the requirements `^0.1.2` are: 0.1.3, 0.1.2

the package `rtexample` depends on `reqwest-tracing`, with features: `opentelemetry_0_17` but `reqwest-tracing` does not have these features.


failed to select a version for `reqwest-tracing` which could resolve this conflict

Adjusting the dependency to:

reqwest-tracing = { version = "0.2.1", features = ["opentelemetry_0_17"] }

then produces the following build error:

error[E0277]: the trait bound `opentelemetry::sdk::trace::Tracer: opentelemetry::trace::tracer::Tracer` is not satisfied
   --> src/main.rs:10:62
    |
10  |   let telemetry = tracing_opentelemetry::layer().with_tracer(tracer);
    |                                                  ----------- ^^^^^^ the trait `opentelemetry::trace::tracer::Tracer` is not implemented for `opentelemetry::sdk::trace::Tracer`
    |                                                  |
    |                                                  required by a bound introduced by this call
    |
note: required by a bound in `OpenTelemetryLayer::<S, T>::with_tracer`
   --> /Users/rgreinhofer/.cargo/registry/src/github.com-1ecc6299db9ec823/tracing-opentelemetry-0.15.0/src/layer.rs:308:17
    |
308 |         Tracer: otel::Tracer + PreSampledTracer + 'static,
    |                 ^^^^^^^^^^^^ required by this bound in `OpenTelemetryLayer::<S, T>::with_tracer`

error[E0277]: the trait bound `opentelemetry::sdk::trace::Tracer: PreSampledTracer` is not satisfied
   --> src/main.rs:10:62
    |
10  |   let telemetry = tracing_opentelemetry::layer().with_tracer(tracer);
    |                                                  ----------- ^^^^^^ the trait `PreSampledTracer` is not implemented for `opentelemetry::sdk::trace::Tracer`
    |                                                  |
    |                                                  required by a bound introduced by this call
    |

[...] 

Expected behavior

The example in the readme should work right away to allow new users to quickly experience the library.

Adding an example that could be ran with cargo run --example and tested via the CI would be ideal.

Environment

• OS: macOS 12.2.1 (21D62)
• Rust version1.59.0

Bug description

When sending a request, as I understand, a traceparent header should be added to the request. When sending a request, no such header is added.

Example of a dummy request

To Reproduce

    let filter = EnvFilter::try_from_default_env()
        .or_else(|_| EnvFilter::try_new("info"))
        .unwrap();

    let opentelemetry_endpoint =
        env::var("OPENTELEMETRY_ENDPOINT").expect("Missing env var 'OPENTELEMETRY_ENDPOINT''");

    let exporter = opentelemetry_otlp::new_exporter()
        .tonic()
        .with_endpoint(opentelemetry_endpoint);

    let config =
        opentelemetry::sdk::trace::config().with_resource(opentelemetry::sdk::Resource::new(vec![
            opentelemetry::KeyValue::new("service.name", "gateway"),
            opentelemetry::KeyValue::new("service.namespace", "cloud-conference"),
        ]));

    let tracer = opentelemetry_otlp::new_pipeline()
        .tracing()
        .with_exporter(exporter)
        .with_trace_config(config)
        .install_batch(opentelemetry::runtime::Tokio)
        .expect("Failed to create opentelemtry pipeline");

    let telemetry = tracing_opentelemetry::layer().with_tracer(tracer);

    let subscriber = tracing_subscriber::fmt::layer()
        .json()
        .with_current_span(true)
        .event_format(tracing_bridge::TraceIdFormat);

    tracing_subscriber::registry()
        .with(telemetry)
        .with(filter)
        .with(subscriber)
        .try_init()
        .unwrap();

I'm using a proxy here to check if the header is present because the traces I got on grafana only included one service, so I used that proxy to debug, and I found that the traceparent header isn't sent.

    let reqwest = reqwest::Client::builder()
        .timeout(std::time::Duration::from_secs(10))
        .proxy(reqwest::Proxy::http("172.16.6.96:9090").unwrap())
        .build()
        .expect("Failed to build reqwest");
    let reqwest = ClientBuilder::new(reqwest).with(TracingMiddleware).build();
    let reqwest = Arc::new(reqwest);

Then I put this reqwest client in a state, that I clone each time I want to make a request.

Expected behavior

Having the traceparent header on every requests.

Environment

• OS: macOS 12.0.1 (21A559)
• Rust version: 1.56.1

Additional context

tracing = "0.1"
tracing-futures = "0.2"
tracing-log = { version = "0.1", features = ["env_logger"] }
tracing-subscriber = { version = "0.3.5", features = ["env-filter", "json", "fmt"] }
tracing-error = "0.2.0"
tracing-serde = "0.1.2"
tracing-opentelemetry = "0.16.0"
opentelemetry = { version = "0.16.0", features = ["rt-tokio"] }
opentelemetry-otlp = { version = "0.9.0" }
reqwest-middleware = "0.1.3"
reqwest-tracing = { version = "0.2.0", features = ["opentelemetry_0_16"] }

Motivations

ClientWithMiddleware does not implement the Debug trait, which prevents other wrapper structs from automatically implementing the Debug trait. Consider this code for example:

#[derive(Debug)]
pub struct SomeApiClient {
    client: ClientWithMiddleware,
}

This code fails to compile because SomeApiClient cannot derive(Debug) when a field does not implement Debug.

Solution

ClientWithMiddleware implements Debug.

Alternatives

At the application level, write a Debug implementation for SomeApiClient by hand, ignoring the client field intentionally. This is quite tedious, repetitive and prone to mistakes.

Additional context

Currently, ClientWithMiddleware is defined like this:

#[derive(Clone)]
pub struct ClientWithMiddleware {
    inner: reqwest::Client,
    middleware_stack: Box<[Arc<dyn Middleware>]>,
}

It's probably necessary to make Debug a supertrait of Middleware. If that is not possible, the Debug implementation of ClientWithMiddleware could instead skip the middleware_stack field, although I think hiding fields from the debug output should be avoided if possible.

fix #17

this update requires moving to tracing-subscriber 0.3. It removes chrono from transitive dependencies, following https://rustsec.org/advisories/RUSTSEC-2020-0159.html

Context:

• tracing-opentelemetry 0.16 release: https://github.com/tokio-rs/tracing/pull/1682
• tracing subscriber 0.3 release with the removal of chrono: https://github.com/tokio-rs/tracing/pull/1677

This replaces task-local-extensions with http's extensions, as http was already in the dependency closure anyway and the other features of task-local-extensions (that required an incompatible-with-wasm part of tokio) were not used anyway.

I tried it out in the project that triggered this PR, and it seems to work perfectly fine!

Motivations

This is a random thought, but it might be worth tracking.

We have task-local-extensions to provide a type agnostic data for middlewares. http::Request also has their own.

Solution

By building with http::Request instead of reqwest::Request, we can reuse their extensions and simplify our own code and get more ecosystem support. There is a TryFrom between the two and it should in theory never fail (the only difference is the Url types. But they should both be spec compliant).

Also, we would lose the extensions when sending through the conversion, so I propose we wrap the Response type to store the state of the extensions at the end

Motivations

The local Error is a simple enum between the underlying reqwest::Error and middleware errors. It would be nice if this Error implemented the same interface as reqwest::Error, esp adding .status()

c.f. https://github.com/oxidecomputer/progenitor/pull/252 for why this could be useful.

Solution

Alternatives

Additional context

There is currently no wrapper for reqwest::Response. As a result, resp.body().await? call is not retried. In the case where the response is large, resp.body() call involves a lot of tcp socket listening and can potentially throw network errors

Motivations

I'm building a type-safe API client around an untyped REST API, and I'm trying to accept dynamic Middleware. Because this client's builder's stack is dynamic, the middleware needs to be represented as Box<dyn Middleware>, which Middleware is not implemented for.

Solution

Middleware should be implemented for common derivatives and wrapper types:

• &T
• &mut T
• Arc<T>
• Box<T>
• Cell<T>
• Cow<T>
• MutexGuard<T>
• Pin<T>
• Rc<T>
• Ref<T>
• RefMut<T>

Alternatives

I'm currently wrapping the Box<dyn Middleware> with a newtype, like:

struct BoxedMiddleware(Box<dyn Middleware>);

and impl Middleware for BoxedMiddleware, but this will need to be replicated for each crate (I'll be building multiple wrapper clients), so it'd be much cleaner to have the impls in the source crate!

Additional context

Happy to submit a PR! 😁

I tried to add a new middleware for handling rate-limit headers. The idea was to use https://github.com/mre/rate-limits for extracting these headers and sleeping for a certain duration if we got rate-limited.

Even in the case of getting rate-limited, we can still return the response immediately. Only on the next request would we have to sleep until the limit got lifted. The way I thought I could implement it was to use a HashMap<String, Duration> from which I would look up the wait time for the given host. However it looks like the Middleware trait uses a non-mutable &self parameter, so I can't update the HashMap field stored in self unless I'm missing something.

The other alternative I considered was to store the HashMap inside Extensions, but that turned out to be quite unergonomic.

Am I missing anything obvious? Would love to get some feedback.