Skip to content

Commit

Permalink
Respond to comments by @jorajeev
Browse files Browse the repository at this point in the history
- current_task -> parent_task_id
- span -> top_level_span
- parent_id -> parent_span_id
- #[ignore] tracing tests
- move test_tracing_nested_spans up
  • Loading branch information
sarsko committed Dec 11, 2023
1 parent a7f5688 commit c4c6517
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 26 deletions.
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ pub struct Config {
/// `tracing_subscriber::fmt`, appends to the span on calls to `record()` (instead of
/// overwriting), which results in traces which are hard to read if the task is scheduled more
/// than a few times.
/// Thus: set `record_steps_in_span` to `true` if you want this behaviour, or if you are using
/// Thus: set `record_steps_in_span` to `true` if you want "append behavior", or if you are using
/// a `Subscriber` which overwrites on calls to `record()` and want to display the current step
/// count.
pub record_steps_in_span: bool,
Expand Down
17 changes: 9 additions & 8 deletions src/runtime/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ impl Execution {
}
state.current_mut().span = current_span;

if let Some(span_id) = state.span.id().as_ref() {
if let Some(span_id) = state.top_level_span.id().as_ref() {
subscriber.enter(span_id)
}
});
Expand Down Expand Up @@ -241,7 +241,7 @@ pub(crate) struct ExecutionState {
has_cleaned_up: bool,

// The `Span` which the `ExecutionState` was created under. Will be the parent of all `Task` `Span`s
pub(crate) span: Span,
pub(crate) top_level_span: Span,
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
Expand Down Expand Up @@ -279,7 +279,7 @@ impl ExecutionState {
current_schedule: initial_schedule,
#[cfg(debug_assertions)]
has_cleaned_up: false,
span: tracing::Span::current(),
top_level_span: tracing::Span::current(),
}
}

Expand Down Expand Up @@ -327,7 +327,7 @@ impl ExecutionState {
{
Self::with(|state| {
let schedule_len = state.current_schedule.len();
let parent_id = state.span.id();
let parent_span_id = state.top_level_span.id();

let task_id = TaskId(state.tasks.len());
let tag = state.get_tag_or_default_for_current_task();
Expand All @@ -340,7 +340,7 @@ impl ExecutionState {
task_id,
name,
clock.clone(),
parent_id,
parent_span_id,
schedule_len,
tag,
state.try_current().map(|t| t.id()),
Expand All @@ -361,7 +361,7 @@ impl ExecutionState {
F: FnOnce() + Send + 'static,
{
Self::with(|state| {
let parent_id = state.span.id();
let parent_span_id = state.top_level_span.id();
let task_id = TaskId(state.tasks.len());
let tag = state.get_tag_or_default_for_current_task();
let clock = if let Some(ref mut clock) = initial_clock {
Expand All @@ -381,7 +381,7 @@ impl ExecutionState {
task_id,
name,
clock,
parent_id,
parent_span_id,
schedule_len,
tag,
state.try_current().map(|t| t.id()),
Expand Down Expand Up @@ -638,7 +638,8 @@ impl ExecutionState {
}
}

self.span.in_scope(|| trace!(?runnable, next_task=?self.next_task));
self.top_level_span
.in_scope(|| trace!(?runnable, next_task=?self.next_task));

Ok(())
}
Expand Down
24 changes: 12 additions & 12 deletions src/runtime/task/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ impl Task {
id: TaskId,
name: Option<String>,
clock: VectorClock,
parent_id: Option<tracing::span::Id>,
parent_span_id: Option<tracing::span::Id>,
schedule_len: usize,
tag: Option<Arc<dyn Tag>>,
current_task: Option<TaskId>,
parent_task_id: Option<TaskId>,
) -> Self
where
F: FnOnce() + Send + 'static,
Expand All @@ -123,7 +123,7 @@ impl Task {
let waker = make_waker(id);
let continuation = Rc::new(RefCell::new(continuation));

let step_span = info_span!(parent: parent_id.clone(), "step", task = id.0, i = field::Empty);
let step_span = info_span!(parent: parent_span_id.clone(), "step", task = id.0, i = field::Empty);
let span = step_span.clone();

let mut task = Self {
Expand All @@ -147,7 +147,7 @@ impl Task {
task.set_tag(tag);
}

info_span!(parent: parent_id, "new_task", parent = ?current_task, i = schedule_len)
info_span!(parent: parent_span_id, "new_task", parent = ?parent_task_id, i = schedule_len)
.in_scope(|| event!(Level::INFO, "created task: {:?}", task.id));

task
Expand All @@ -160,10 +160,10 @@ impl Task {
id: TaskId,
name: Option<String>,
clock: VectorClock,
parent_id: Option<tracing::span::Id>,
parent_span_id: Option<tracing::span::Id>,
schedule_len: usize,
tag: Option<Arc<dyn Tag>>,
current_task: Option<TaskId>,
parent_task_id: Option<TaskId>,
) -> Self
where
F: FnOnce() + Send + 'static,
Expand All @@ -174,10 +174,10 @@ impl Task {
id,
name,
clock,
parent_id,
parent_span_id,
schedule_len,
tag,
current_task,
parent_task_id,
)
}

Expand All @@ -188,10 +188,10 @@ impl Task {
id: TaskId,
name: Option<String>,
clock: VectorClock,
parent_id: Option<tracing::span::Id>,
parent_span_id: Option<tracing::span::Id>,
schedule_len: usize,
tag: Option<Arc<dyn Tag>>,
current_task: Option<TaskId>,
parent_task_id: Option<TaskId>,
) -> Self
where
F: Future<Output = ()> + Send + 'static,
Expand All @@ -211,10 +211,10 @@ impl Task {
id,
name,
clock,
parent_id,
parent_span_id,
schedule_len,
tag,
current_task,
parent_task_id,
)
}

Expand Down
12 changes: 7 additions & 5 deletions tests/basic/tracing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ fn tracing_nested_spans() {
}
}

#[ignore]
#[test]
fn test_tracing_nested_spans() {
check_random(tracing_nested_spans, 10);
}

fn tracing_nested_spans_panic_mod_5(number: usize) {
let lock = Arc::new(Mutex::new(0));
let threads: Vec<_> = (0..3)
Expand Down Expand Up @@ -61,17 +67,13 @@ fn tracing_nested_spans_panic_mod_5(number: usize) {
}
}

#[test]
fn test_tracing_nested_spans() {
check_random(tracing_nested_spans, 10);
}

// Test to check that spans don't stack on panic and that minimization works as it should
proptest! {
#![proptest_config(
Config { cases: 1000, failure_persistence: None, .. Config::default() }
)]
#[should_panic]
#[ignore]
#[test]
fn test_stacks_cleaned_on_panic(i: usize) {
check_random(move || {
Expand Down

0 comments on commit c4c6517

Please sign in to comment.