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

[ShippingService] Parent span not set properly #1318

Closed
julianocosta89 opened this issue Jan 2, 2024 · 8 comments · Fixed by #1433
Closed

[ShippingService] Parent span not set properly #1318

julianocosta89 opened this issue Jan 2, 2024 · 8 comments · Fixed by #1433
Labels
bug Something isn't working rust Pull requests that update Rust code

Comments

@julianocosta89
Copy link
Member

Bug Report

Which version of the demo you are using?
v1.7.0

Symptom

When shippingservice calls quoteservice the parent_id sent is not from the caller, but from the caller's parent.
The trace is currenly looking like this:

image

But it should be something like this:

checkoutservice oteldemo.ShippingService/GetQuote
    |--> shippingservice oteldemo.ShippingService/GetQuote
        |--> shippingservice POST
            |--> quoteservice POST /getquote
                |--> quoteservice {closure}
                    |--> quoteservice calculate-quote

Reproduce

Simply run the demo with docker compose up and check any trace containing shippingservice spans.

@julianocosta89 julianocosta89 added bug Something isn't working rust Pull requests that update Rust code labels Jan 2, 2024
@julianocosta89
Copy link
Member Author

In the following snippet, shouldn't the current context be the POST context?

async fn request_quote(count: u32) -> Result<f64, Box<dyn std::error::Error>> {
    // TODO: better testing here and default quote_service_addr
    let quote_service_addr: String = format!(
        "{}{}",
        env::var("QUOTE_SERVICE_ADDR").expect("$QUOTE_SERVICE_ADDR is not set"),
        "/getquote"
    );

    let mut reqbody = HashMap::new();
    reqbody.insert("numberOfItems", count);

    let client = ClientBuilder::new(reqwest::Client::new())
        .with(TracingMiddleware::<SpanBackendWithUrl>::new())
        .build();

    let req = client.request(Method::POST, quote_service_addr);

    let mut headers = HeaderMap::new();

    let cx = Context::current();
    global::get_text_map_propagator(|propagator| {
        propagator.inject_context(&cx, &mut HeaderInjector(&mut headers))
    });

    let resp = req
        .json(&reqbody)
        .headers(headers)
        .send()
        .await?
        .text_with_charset("utf-8")
        .await?;

    debug!("{:?}", resp);

    match resp.parse::<f64>() {
        Ok(f) => Ok(f),
        Err(error) => Err(Box::new(error)),
    }
}

https://github.com/open-telemetry/opentelemetry-demo/blob/main/src/shippingservice/src/shipping_service/quote.rs#L64-L67

@TommyCpp do you spot where we are doing the context propagation wrong here?

@TommyCpp
Copy link
Contributor

TommyCpp commented Jan 2, 2024

Feel free to assign this to me. I can take a look this week

@julianocosta89
Copy link
Member Author

Thank you @TommyCpp!

@TommyCpp
Copy link
Contributor

TommyCpp commented Jan 8, 2024

I think the issue lie with reqwest-tracing create, which priovides a middle layer to create a child span for HTTP calls.

    let client = ClientBuilder::new(reqwest::Client::new())
        .with(TracingMiddleware::<SpanBackendWithUrl>::new())
        .build();

But that happens after we setup the header

    let cx = Context::current();
    global::get_text_map_propagator(|propagator| {
        propagator.inject_context(&cx, &mut HeaderInjector(&mut headers))
    });

and somewhere before the request is sent.

The quick patch would be disable the .with(TracingMiddleware::<SpanBackendWithUrl>::new()) but that means we will delete the POST span from shippingservice. If this can wait I think it's better to fix stuff from reqwest-tracing side

@TommyCpp
Copy link
Contributor

TommyCpp commented Jan 8, 2024

By default the propgation on the POST span should be included automatically by the middleware(as we didn't use DisableOtelPropagation extension here)

@julianocosta89
Copy link
Member Author

Thank you @TommyCpp!

We can definitely wait for the better fix.
This is in the demo for some time already, so it won't be an issue to keep it as is for a bit longer.
Should I create an issue in the reqwest-tracing?
Is that this repo: https://github.com/TrueLayer/reqwest-middleware?

@TommyCpp
Copy link
Contributor

TommyCpp commented Jan 9, 2024

Yep

I can open the issue on their side. I also want to construct some examples between rust client and rust server to help investigation

@julianocosta89
Copy link
Member Author

@TommyCpp did you happen to have opened a ticket on their side?
If not, I could do it.

Do I need to provide any extra info other than what you've detailed here: #1318 (comment)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants