OpenTelemetrySpanExt::set_parent does not update trace ID in OtelData

[mladedav]

Bug Report

Version

├── tracing v0.1.37 (*)
├── tracing-opentelemetry v0.18.0
│   ├── tracing v0.1.37 (*)
│   ├── tracing-core v0.1.31 (*)
│   ├── tracing-log v0.1.3
│   │   └── tracing-core v0.1.31 (*)
│   └── tracing-subscriber v0.3.17
│       ├── tracing v0.1.37 (*)
│       ├── tracing-core v0.1.31 (*)
│       ├── tracing-log v0.1.3 (*)
│       └── tracing-serde v0.1.3
│           └── tracing-core v0.1.31 (*)
├── tracing-serde v0.1.3 (*)
└── tracing-subscriber v0.3.17 (*)

Platform

Linux DESKTOP 5.4.72-microsoft-standard-WSL2 #1 SMP Wed Oct 28 23:40:43 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Description

I used this code to create a span and manually change the parent as if I had a remote context to put there:

        let span = tracing::trace_span!("Remote trace");
        span.set_parent(opentelemetry::Context::new().with_remote_span_context(
            opentelemetry::trace::SpanContext::new(
                TraceId::from([0x11; 16]),
                SpanId::from([0x22; 8]),
                TraceFlags::SAMPLED,
                true,
                TraceState::default(),
            ),
        ));

I have noticed that the OtelData associated with the span has different trace IDs in builder and parent_cx. That is because the span was created with no active span and OpenTelemetryLayer::on_new_span created a trace ID. But when a parent context is added, I think this trace ID should be deleted as it should not be used. All other spans with parents also have None as their trace ID in the builder.

I do not believe the ID is actually used anywhere, but I think it would be safer to remove it because that would make it harder to misuse this. Unless that would break some guarantee, e.g. that the builder should never change.

[jtescher]

The trace id stored in the builder is primarily used to maintain a consistent trace id for the span's children when no parent context is set (for example a new root tracing span that has not yet ended and has no external parent). See example usage here which maintains a stable trace state for child spans:

tracing-opentelemetry/src/tracer.rs

Lines 119 to 136 in 5ce1b10

	fn current_trace_state(
	builder: &SpanBuilder,
	parent_cx: &OtelContext,
	provider: &TracerProvider,
	) -> (TraceId, TraceFlags) {
	if parent_cx.has_active_span() {
	let span = parent_cx.span();
	let sc = span.span_context();
	(sc.trace_id(), sc.trace_flags())
	} else {
	(
	builder
	.trace_id
	.unwrap_or_else(|| provider.config().id_generator.new_trace_id()),
	Default::default(),
	)
	}
	}

It is ignored if there is a parent span context, so it shouldn't cause issues in practice, but it should be safe to assign it to the new parent span context's trace id, or to None, when assigning it here if you want to propose the change:

tracing-opentelemetry/src/span_ext.rs

Lines 120 to 131 in 5ce1b10

	fn set_parent(&self, cx: Context) {
	let mut cx = Some(cx);
	self.with_subscriber(move |(id, subscriber)| {
	if let Some(get_context) = subscriber.downcast_ref::<WithContext>() {
	get_context.with_context(subscriber, id, move |data, _tracer| {
	if let Some(cx) = cx.take() {
	data.parent_cx = cx;
	}
	});
	}
	});
	}