Skip to content

Commit bdcca4c

Browse files
antonmrygeoffreyclaude
authored andcommitted
fix(instrumented_exec): scope recorder reuse to current parent span
Use the current tracing span id alongside the task context as the execution identity, so concurrent executions sharing a task context but running under different parent spans get independent recorder state.
1 parent c3c18a8 commit bdcca4c

1 file changed

Lines changed: 57 additions & 4 deletions

File tree

datafusion-tracing/src/instrumented_exec.rs

Lines changed: 57 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ use std::{
5959
},
6060
task::{Context, Poll},
6161
};
62-
use tracing::{Span, field};
62+
use tracing::{Id, Span, field};
6363
use tracing_futures::Instrument;
6464

6565
/// Type alias for a function that creates a tracing span.
@@ -140,9 +140,11 @@ impl InstrumentedExec {
140140
context: Arc<TaskContext>,
141141
partition: usize,
142142
) -> Arc<ExecutionRecorders> {
143+
let parent_span_id = Span::current().id();
144+
143145
let mut groups = self.recorders.lock().unwrap();
144146
for recorders in groups.iter() {
145-
if recorders.is_same_context(&context)
147+
if recorders.is_same_execution(parent_span_id.as_ref(), &context)
146148
&& recorders.try_reserve_partition(partition)
147149
{
148150
return recorders.clone();
@@ -159,6 +161,7 @@ impl InstrumentedExec {
159161
});
160162
let recorders = Arc::new(ExecutionRecorders::new(
161163
Arc::downgrade(&self.recorders),
164+
parent_span_id,
162165
context,
163166
partition,
164167
NodeRecorder::new(self.inner.clone(), span.clone()),
@@ -451,6 +454,7 @@ impl ExecutionPlan for InstrumentedExec {
451454

452455
struct ExecutionRecorders {
453456
slot: Weak<Mutex<Vec<Arc<ExecutionRecorders>>>>,
457+
parent_span_id: Option<Id>,
454458
context: Arc<TaskContext>,
455459
seen_partitions: Mutex<HashSet<usize>>,
456460
active_streams: AtomicUsize,
@@ -462,6 +466,7 @@ struct ExecutionRecorders {
462466
impl ExecutionRecorders {
463467
fn new(
464468
slot: Weak<Mutex<Vec<Arc<ExecutionRecorders>>>>,
469+
parent_span_id: Option<Id>,
465470
context: Arc<TaskContext>,
466471
partition: usize,
467472
node_recorder: NodeRecorder,
@@ -470,6 +475,7 @@ impl ExecutionRecorders {
470475
) -> Self {
471476
Self {
472477
slot,
478+
parent_span_id,
473479
context,
474480
seen_partitions: Mutex::new(HashSet::from([partition])),
475481
active_streams: AtomicUsize::new(1),
@@ -483,8 +489,16 @@ impl ExecutionRecorders {
483489
self.node_recorder.span()
484490
}
485491

486-
fn is_same_context(&self, context: &Arc<TaskContext>) -> bool {
487-
Arc::ptr_eq(&self.context, context)
492+
fn is_same_execution(
493+
&self,
494+
parent_span_id: Option<&Id>,
495+
context: &Arc<TaskContext>,
496+
) -> bool {
497+
match (&self.parent_span_id, parent_span_id) {
498+
(Some(a), Some(b)) => a == b && Arc::ptr_eq(&self.context, context),
499+
(None, None) => Arc::ptr_eq(&self.context, context),
500+
_ => false,
501+
}
488502
}
489503

490504
fn try_reserve_partition(&self, partition: usize) -> bool {
@@ -1008,6 +1022,45 @@ mod tests {
10081022
);
10091023
}
10101024

1025+
/// The current parent span is a stronger execution identity than the task
1026+
/// context. Concurrent executions with the same task context but different
1027+
/// parent spans must not share recorder state.
1028+
#[tokio::test]
1029+
async fn different_parent_spans_get_fresh_spans() {
1030+
let capture = SpanCapture::default();
1031+
let _guard = tracing::subscriber::set_default(
1032+
tracing_subscriber::registry()
1033+
.with(tracing::level_filters::LevelFilter::INFO)
1034+
.with(capture.clone()),
1035+
);
1036+
1037+
let plan = two_partition_plan();
1038+
let task_ctx = Arc::new(TaskContext::default());
1039+
let parent_1 = tracing::info_span!("parent_1");
1040+
let parent_2 = tracing::info_span!("parent_2");
1041+
let streams = vec![
1042+
parent_1.in_scope(|| plan.execute(0, task_ctx.clone()).unwrap()),
1043+
parent_2.in_scope(|| plan.execute(1, task_ctx).unwrap()),
1044+
];
1045+
1046+
for mut stream in streams {
1047+
while let Some(batch) = stream.next().await {
1048+
batch.unwrap();
1049+
}
1050+
}
1051+
1052+
assert_eq!(
1053+
capture.opened("InstrumentedExec"),
1054+
2,
1055+
"different parent spans should create independent spans"
1056+
);
1057+
assert_eq!(
1058+
capture.closed("InstrumentedExec"),
1059+
2,
1060+
"each parent-span execution should close its own span"
1061+
);
1062+
}
1063+
10111064
/// `ExecutionPlan::execute` may legally be called for only a subset of
10121065
/// partitions. Recorder lifetime must not wait for partitions that were
10131066
/// never executed.

0 commit comments

Comments
 (0)