Propagate span_id & trace_id in logs records when converting trace events

[maximebedard]

Fixes #1378.

Changes

Depends on tokio-rs/tracing-opentelemetry#78.

This PR updates the dependency of tracing-opentelemetry to 0.22 and add a support for propagating the trace context when bridging trace events to logs. This is useful when sending otlp log records to tools such as datadog, which would then use the trace_id and span_id to correlate logs and traces.

To do so, I'm pulling the OtelData from the current span and converting it to a TraceContext that can be set on the log record.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: maximebedard / name: Maxime Bedard (d212518, 868ca59, 74f97ed, c959980, 762b99a, 39081ec, fdfe9b7)

[cijothomas]

@maximebedard Could you sign the easycla part, which is a pre-req before accepting any contributions!

[cijothomas]

this looks odd to depend on tracing-opentelemetry!

[maximebedard]

Definitely. I believe it's necessary since it's where the OtelData is injected into the current span extension and where the TraceId and SpanId is generated. If there's a better way to retrieve the data, I would be happy to update the PR and remove this dependency.

[lalitb]

Can we just copy the OtelData definition from tracing-opentelemetry, doesn't seems to be a deep-rooted definition. And this will help get rid of dependency.

#[derive(Debug, Clone)]
pub struct OtelData {
    /// The parent otel `Context` for the current tracing span.
    pub parent_cx: opentelemetry::Context,

    /// The otel span data recorded during the current tracing span.
    pub builder: opentelemetry::trace::SpanBuilder,
}

In the future, it would be good to have a single subscriber/layer for both traces and logs and so consolidate at that time.

[maximebedard]

I'm not sure this works as the extension type internally uses a HashMap<TypeId, _>, and the TypeId of the copied definition would happen to be different. I tried an simpler example here and the types are indeed not equal.

[lalitb]

Thanks for the explanation. You are right, copying OtelData definition won't work here. Do you think we can make this dependency optional? And then check for the tracing context only if this dependency is met. This will ensure that the users who are using this crate solely for logging purpose are not paying cost of context lookup when no span is actually created. And this is valid also for users who are directly using the otel tracing API (though it is not recommended).

As a side-note, as of now, when using bothtracing-opentelemetry and opentelemetry-appender-tracing, the logs emitted with tracing macros (trace!, info!, event! etc) are exported both through SpanExporter as span-events, and the LogExporter as logs. This mayn't be desirable. It would be eventually good to integrate both tracing-opentelemetry and opentelemetry-appender-tracing crates.

[lalitb]

@ewoolsey Can you validate if the span_id and trace_id are populated correctly?

[ewoolsey]

Yes it's working correctly. I've got a project depending on this PR and it's functioning as expected.

[lalitb]

Ok, interesting. I think there won't be any error thrown by the Rust compiler for the circular dependency. Rust strong type system just allows that, but still this is a bad design. One option could be to reopen the PR for review to unblock, and follow up the circular dependency problem with a separate GitHub issue. @maximebedard - if you have any comment?

[ewoolsey]

I would love that personally. This is a pretty critical feature for my project and finding a way around it has been really tough.

[maximebedard]

The dependency issue is specific to the workspace, it would not be an issue if you use a released version of the opentelemetry crates and tracing-opentelemetry. We can see the issue in the CI job that failing here https://github.com/open-telemetry/opentelemetry-rust/actions/runs/7105523258/job/19342898991. It's not really possible to push a fix upstream unless we move the tracing_opentelemetry::OtelData into the current workspace.

I have a forked version that uses this patch, but I noticed that it doesn't fully solve the problem. The trace_id is only set for the root span (see https://github.com/tokio-rs/tracing-opentelemetry/blob/v0.1.x/src/layer.rs#L871-L874), any child span created would be missing the trace_id and not be correlated correctly.

[maximebedard]

I'm getting the CLA situation sorted out with my employer. I'll provide updates ASAP.

[ewoolsey]

Thanks for your hard work on this! Seems to be working well for me.

[lpj145]

What a amazing situation the one's top protocol to monitor services like open telemetry does't have a good integration with tracing, guys, we need to fix this thing... seriously, if you want to enable and add opentelemtry in another languages eg: tech stacks you just need to install it, configure under 3 lines, but in rust, you need todo a entire conversion just to trace works, but not for logs, by the way in the way that open telemtry follow we need to setup 3 connections of grpc just to have the data..... again... amazing...

[hdost]

@maximebedard 👋🏼 Hey I am curious what made you close the PR?

[lalitb]

@maximebedard 👋🏼 Hey I am curious what made you close the PR?

It's summarised here -#1394 (comment)