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

Regression with 0.20 regarding remote contexts #54

Closed
valkum opened this issue Aug 21, 2023 · 2 comments
Closed

Regression with 0.20 regarding remote contexts #54

valkum opened this issue Aug 21, 2023 · 2 comments

Comments

@valkum
Copy link
Contributor

valkum commented Aug 21, 2023

Bug Report

We see a regression after updating to v0.20 regarding remote context and how traceIds are propagated to children.

Version

tracing v0.1.37
tracing-futures v0.2.5
tracing-subscriber v0.3.17
tracing-core v0.1.31
tracing-log v0.1.3
tracing-attributes v0.1.26
tracing-serde v0.1.3
tracing-opentelemetry v0.20.0

Platform

Description

We have an instrumented function (fn handler) in which we call OpenTelemetrySpanExt::set_parent.
Previously, we used something like this:

let context = Context::default();
let context = TraceContextPropagator::new().extract_with_context(&context, &carrier);
Span::current().set_parent(context);

With v0.20 this creates two traces because all spans created after setting the context will get a different traceId than the
span created for handler.

The example of set_parent uses TextMapPropagator::extract(&carrier). extract just calls Context::currenty() and then calls extract_with_context. If we change our code to use something equivalent:

let context = Span::current().context();
let context = TraceContextPropagator::new().extract_with_context(&context, &carrier);
Span::current().set_parent(context);

we miss a common root span according to Jaeger.

Note that the root span is entered at the time of calling Span::current().context().

I think the regression was caused by #26.

If this is some error at our side, it should be better documented for set_parent. If not, and the PR is also not the cause, I assume we hit a regression in opentelemetry.
We are willing to create a PR once we know which path should be taken.

@valkum
Copy link
Contributor Author

valkum commented Aug 21, 2023

I read the description of #45 more thoroughly, and it seems this is a duplicate of #45. Feel free to close this or the other one (depending on which one you think contains more context to fix this)

jtescher pushed a commit that referenced this issue Aug 23, 2023
## Motivation

Fix following issues
* #54
* #45

## Solution

* Revert https://github.com/tokio-rs/tracing-opentelemetry/pull/26/files
* When an invalid(empty) Context is extracted from the propagator and
passed to OpenTelemetrySpanExt::set_parent(), if none is set for
trace_id, root and child trace id will not match
* I can confirm that this change works in
[reproduction](https://github.com/notheotherben/tracing-opentelemetry-traceid-repro)
that @notheotherben created
* add regression test case which propagate empty context
@jtescher
Copy link
Collaborator

Resolved in #55

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

2 participants