[shippingservice] fix context propagation

[julianocosta89]

Changes

Fix #1318.

As I was touching this service I've refactored the code to make it more readable.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

• CHANGELOG.md updated to document new feature additions
• Appropriate documentation updates in the docs
• Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

[julianocosta89]

@TommyCpp would you be able to take a look at this one?

[julianocosta89]

We need to implement some quote mock in order to run the tests, and as I'm not a Rust expert I've opted to remove the non-working tests as they were not working since we added quoteservice which is a lot!
quoteservice was added before we release v1 and it seems nobody was running the tests anyways.

[cijothomas]

Will have to decide if we should rewrite this service using OTel Tracing API. It looks like its currently using an external instrumentation library which converts spans emitted using tokio/tracing crate into OTel spans. It is undecided how such usage is going to be supported going forward.

This might also explain why logs are not correlated to spans, as Spans are not produced using OTel Tracing APIs.

[TommyCpp]

Sorry missed this earlier

It looks like its currently using an external instrumentation library which converts spans emitted using tokio/tracing crate into OTel spans

It's a third party instrument on http clients IIRC. Probably better if we rewrite to handle the propagation ourselfs

[austinlparker]

What's the status here, should this be merged?

[cijothomas]

We should using tracing and not log crate, if we plan to add logging in this PR.
https://github.com/open-telemetry/opentelemetry-demo/pull/1433/files#r1516487971

or scope down to just fixing context propagation issue alone.

[cijothomas]

What's the status here, should this be merged?

No. I left a blocking comment just now.

Outside of that I'd love to get some opinions from demo maintainers about the Rust app - it is NOT using OTel Tracing API, but uses tokio-tracing along with a "bridge" (maintained outside OTel repos) to map them to OTel Tracing. OTel Rust SIG is undecided on how to navigate this problem : open-telemetry/opentelemetry-rust#1571

Options:

1. Modify shipping service to only OTel Tracing API. If OTel Rust SIG ultimately decides to sacrifice OTel Tracing API, then this requires rework.
2. Keep the existing - but we need to add owners of the respective packages as code reviewers here. (I don't know if they have interest/time). Again, rework is require if OTel Rust SIG decides to go forward with OTel Tracing API and ignore tokio-tracing.
3. Something else? Please share.

Either requires rework (after all, no signal is stable in OTel Rust). I'd suggest to do 1.

@open-telemetry/rust-approvers Please share your thoughts too.

[julianocosta89]

I'll take a look at it again soon.

[austinlparker]

I see. I'd probably opt for option 1 then.

[julianocosta89]

Either requires rework (after all, no signal is stable in OTel Rust). I'd suggest to do 1.

Do we have any instrumentation library in OTel Rust ATM?
If not, I'd keep the tokio-tracing in order to demonstrate how it instruments automatically Rust code.

[cijothomas]

Either requires rework (after all, no signal is stable in OTel Rust). I'd suggest to do 1.

Do we have any instrumentation library in OTel Rust ATM? If not, I'd keep the tokio-tracing in order to demonstrate how it instruments automatically Rust code.

Nothing maintained by opentelemetry.
Using tokio-tracing also requires another library (tracing-opentelemetry), not maintained by opentelemetry.

[julianocosta89]

I'd say we can do that in a follow-up PR.
I've added the opentelemetry-appender-tracing and got some logs with Span and Trace ID

This should be good to go, and we can continue the discussion in an issue. WDYT?

[julianocosta89]

Just my 2 cents here:

I'm in favour of getting rid of tokio-tracing.
It is really cumbersome using it and making sure all the dependencies are aligned, as they almost never are on the latest OTel rust release.

[julianocosta89]

@cijothomas I'd appreciate if you could double check the code whenever you have some minutes.

[hdost]

Looks good from my perspective. I'll try running it later today.

[julianocosta89]

@open-telemetry/rust-approvers would anyone be able to give us a green light on this one?