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

tracing-opentelemetry: Add opentelemetry::Context propagation #2345

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mkatychev
Copy link

@mkatychev mkatychev commented Oct 11, 2022

Motivation

After some discussion as to the motivation of this PR: open-telemetry/opentelemetry-rust#893

an alternative solution was proposed for allowing spans to implicitly propagate otel context:

Allow spans to propagate and drop otel context IDs when spans are entered and exited

Solution

Add otel::Context to OpenTelemetrySubscriber for on_enter and on_exit

@mkatychev mkatychev changed the title Otex cx guards tracing-opentelemetry: Add opentelemetry::Context propagation Oct 12, 2022
Comment on lines +1247 to +1250
assert_eq!(
OtelContext::current().span().span_context().trace_id(),
trace_id
);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inner_span is not holding a parent, this currently evaluates to None:

if let Some(parent) = attrs.parent() {

@mkatychev
Copy link
Author

@jtescher any thoughts as to why the test is preventing a Span::child_of to be built on macro invocation?

let meta = CALLSITE.metadata();
// span with contextual parent
$crate::Span::new(
meta,
&$crate::valueset!(meta.fields(), $($fields)*),
)

Am I missing something in the test harness?

let _outer_g = outer_span.enter();
let inner_span = tracing::debug_span!("inner");
dbg!(OtelContext::current().span().span_context().trace_id());
dbg!(OtelContext::current().has_active_span(),);
dbg!(outer_span.context().has_active_span(),);
dbg!(outer_span.context().span().span_context().trace_id());
// let inner_span = tracing::debug_span!("inner");
dbg!(tracing::Span::current().id().unwrap());
assert_eq!(
OtelContext::current().span().span_context().trace_id(),
trace_id
);
let _inner_g = inner_span.enter();
assert_eq!(
OtelContext::current().span().span_context().trace_id(),
trace_id
);
dbg!(tracing::Span::current().id().unwrap());
});
// let recorded_trace_id =

These are the logs:

[tracing-opentelemetry/src/subscriber.rs:681] id = Id(
    2,
)
[tracing-opentelemetry/src/subscriber.rs:682] attrs = Attributes {
    metadata: Metadata {
        name: "inner",
        target: "tracing_opentelemetry::subscriber::tests",
        level: Level(
            Debug,
        ),
        module_path: "tracing_opentelemetry::subscriber::tests",
        location: tracing-opentelemetry/src/subscriber.rs:1246,
        fields: {},
        callsite: Identifier(0x10518c330),
        kind: Kind(SPAN),
    },
    values: ValueSet {
        callsite: Identifier(0x10518c330),
    },
    parent: Current,
}
[tracing-opentelemetry/src/subscriber.rs:590] "parent" = "parent"
[tracing-opentelemetry/src/subscriber.rs:592] attrs.parent().is_some() = false
on_enter attaching trace id: 00000000000000000000000000000000
thread 'subscriber::tests::trace_id_propagates_to_current_context' panicked at 'assertion failed: `(left == right)`
  left: `00000000000000000000000000000000`,
 right: `0000000000000000000000000000002a`', tracing-opentelemetry/src/subscriber.rs:1247:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
on_exit dropping trace id: 00000000000000000000000000000000
on_exit dropping trace id: 0000000000000000000000000000002a
test subscriber::tests::trace_id_propagates_to_current_context ... FAILED

@jtescher
Copy link
Collaborator

Hm this approach may actually have to be re-assessed, dropping the otel context guards will replace the previously active context so the order dependence of entering and exiting will likely cause issues.

@mkatychev
Copy link
Author

mkatychev commented Oct 16, 2022

Hm this approach may actually have to be re-assessed, dropping the otel context guards will replace the previously active context so the order dependence of entering and exiting will likely cause issues.

Would having access to the previous_cx field on ContextGuard help this? I assume your concern is that dropping a context guard does not always follow a FILO pattern.

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

Successfully merging this pull request may close these issues.

2 participants