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.

Sign up for GitHub

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

mock: correct contextual/explicit parent assertions #2812

Closed
wants to merge 2 commits into from
Closed

mock: correct contextual/explicit parent assertions #2812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10fd046
mock: correct contextual/explicit parent assertions
hds Nov 7, 2023
937e44d
Merge branch 'master' into hds/mock-fix-expect-parent
hds Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 6 additions & 31 deletions tracing-attributes/tests/parents.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,26 +18,15 @@ fn default_parent_test() {
let child = expect::span().named("with_default_parent");

let (collector, handle) = collector::mock()
.new_span(
contextual_parent
.clone()
.with_contextual_parent(None)
.with_explicit_parent(None),
Comment on lines -24 to -25
Copy link
Member

Choose a reason for hiding this comment

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

it seems like the fact that both of these methods can be called on an ExpectedSpan creates some ambiguity in the tracing-mock API surface. what do you think about getting rid of with_contextual_parent() and with_explicit_parent(), and replacing them with a single with_parent() method taking a Parent enum? something like this:

expect::span()
   .with_parent(Parent::Contextual(Some("whatever"))

this way, the user can no longer construct a span that expects both a contextual parent and an explicit parent at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started doing this, but the change is huge. It also has some unfortunate side-efffects (at lesat the way that I implemented it). Because then we'll need:

expect::span()
    .with_parent(Parent::Contextual(Some("whatever".to_owned())))

Which starts to get a bit long. Also, exposing the Parent enum makes everything longer too.

I think that there is room for improvement here, but I would like to look at it in a separate PR so as not to make this one too large. What do you think?

)
.new_span(
child
.clone()
.with_contextual_parent(Some("contextual_parent"))
.with_explicit_parent(None),
)
.new_span(contextual_parent.clone().with_contextual_parent(None))
.new_span(child.clone().with_contextual_parent(None))
.enter(child.clone())
.exit(child.clone())
.enter(contextual_parent.clone())
.new_span(
child
.clone()
.with_contextual_parent(Some("contextual_parent"))
.with_explicit_parent(None),
.with_contextual_parent(Some("contextual_parent")),
)
.enter(child.clone())
.exit(child)
Expand Down Expand Up @@ -65,24 +54,10 @@ fn explicit_parent_test() {
let child = expect::span().named("with_explicit_parent");

let (collector, handle) = collector::mock()
.new_span(
contextual_parent
.clone()
.with_contextual_parent(None)
.with_explicit_parent(None),
)
.new_span(
explicit_parent
.with_contextual_parent(None)
.with_explicit_parent(None),
)
.new_span(contextual_parent.clone().with_contextual_parent(None))
.new_span(explicit_parent.with_contextual_parent(None))
.enter(contextual_parent.clone())
.new_span(
child
.clone()
.with_contextual_parent(Some("contextual_parent"))
.with_explicit_parent(Some("explicit_parent")),
)
.new_span(child.clone().with_explicit_parent(Some("explicit_parent")))
.enter(child.clone())
.exit(child)
.exit(contextual_parent)
Expand Down
70 changes: 55 additions & 15 deletions tracing-mock/src/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ use crate::{
event::ExpectedEvent,
expect::Expect,
field::ExpectedFields,
parent::Parent,
span::{ExpectedSpan, NewSpan},
};
use std::{
Expand Down Expand Up @@ -1034,16 +1035,36 @@ where
)
}
}
let get_parent_name = || {
let stack = self.current.lock().unwrap();

let get_parent = || {
let spans = self.spans.lock().unwrap();
event
.parent()
.and_then(|id| spans.get(id))
.or_else(|| stack.last().and_then(|id| spans.get(id)))
.map(|s| s.name.to_string())

if event.is_contextual() {
let stack = self.current.lock().unwrap();
if let Some(parent_id) = stack.last() {
let contextual_parent = spans.get(parent_id).expect(
"tracing-mock: contextual parent cannot \
be looked up by ID. Was it recorded correctly?",
);
Parent::Contextual(contextual_parent.name.to_string())
} else {
Parent::ContextualRoot
}
} else if event.is_root() {
Parent::ExplicitRoot
} else {
let parent_id = event.parent().expect(
"tracing-mock: is_contextual=false is_root=false \
but no explicit parent found. This is a bug!",
);
let explicit_parent = spans.get(parent_id).expect(
"tracing-mock: explicit parent cannot be looked \
up by ID. Is the provided Span ID valid: {parent_id}",
);
Parent::Explicit(explicit_parent.name.to_string())
}
};
expected.check(event, get_parent_name, &self.name);
expected.check(event, get_parent, &self.name);
}
Some(ex) => ex.bad(&self.name, format_args!("observed event {:#?}", event)),
}
Expand Down Expand Up @@ -1100,14 +1121,33 @@ where
let mut spans = self.spans.lock().unwrap();
if was_expected {
if let Expect::NewSpan(mut expected) = expected.pop_front().unwrap() {
let get_parent_name = || {
let stack = self.current.lock().unwrap();
span.parent()
.and_then(|id| spans.get(id))
.or_else(|| stack.last().and_then(|id| spans.get(id)))
.map(|s| s.name.to_string())
let get_parent = || {
if span.is_contextual() {
let stack = self.current.lock().unwrap();
if let Some(parent_id) = stack.last() {
let contextual_parent = spans.get(parent_id).expect(
"tracing-mock: contextual parent cannot \
be looked up by ID. Was it recorded correctly?",
);
Parent::Contextual(contextual_parent.name.to_string())
} else {
Parent::ContextualRoot
}
} else if span.is_root() {
Parent::ExplicitRoot
} else {
let parent_id = span.parent().expect(
"tracing-mock: is_contextual=false is_root=false \
but no explicit parent found. This is a bug!",
);
let explicit_parent = spans.get(parent_id).expect(
"tracing-mock: explicit parent cannot be looked \
up by ID. Is the provided Span ID valid: {parent_id}",
);
Parent::Explicit(explicit_parent.name.to_string())
}
};
expected.check(span, get_parent_name, &self.name);
expected.check(span, get_parent, &self.name);
}
}
spans.insert(
Expand Down
15 changes: 6 additions & 9 deletions tracing-mock/src/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
//! [`collector`]: mod@crate::collector
//! [`expect::event`]: fn@crate::expect::event
#![allow(missing_docs)]
use super::{expect, field, metadata::ExpectedMetadata, span, Parent};
use super::{expect, field, metadata::ExpectedMetadata, parent::Parent, span};

use std::fmt;

Expand Down Expand Up @@ -303,10 +303,12 @@ impl ExpectedEvent {
/// .with_explicit_parent(None);
///
/// let (collector, handle) = collector::mock()
/// .enter(expect::span())
/// .event(event)
/// .run_with_handle();
///
/// with_default(collector, || {
/// let _guard = tracing::info_span!("contextual parent").entered();
/// tracing::info!(parent: None, field = &"value");
/// });
///
Expand Down Expand Up @@ -557,7 +559,7 @@ impl ExpectedEvent {
pub(crate) fn check(
&mut self,
event: &tracing::Event<'_>,
get_parent_name: impl FnOnce() -> Option<String>,
get_parent: impl FnOnce() -> Parent,
collector_name: &str,
) {
let meta = event.metadata();
Expand All @@ -578,13 +580,8 @@ impl ExpectedEvent {
}

if let Some(ref expected_parent) = self.parent {
let actual_parent = get_parent_name();
expected_parent.check_parent_name(
actual_parent.as_deref(),
event.parent().cloned(),
event.metadata().name(),
collector_name,
)
let actual_parent = get_parent();
expected_parent.check(&actual_parent, event.metadata().name(), collector_name);
}
}
}
Expand Down
86 changes: 1 addition & 85 deletions tracing-mock/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,92 +4,8 @@ pub mod event;
pub mod expect;
pub mod field;
mod metadata;
mod parent;
pub mod span;

#[cfg(feature = "tracing-subscriber")]
pub mod subscriber;

#[derive(Debug, Eq, PartialEq)]
pub enum Parent {
ContextualRoot,
Contextual(String),
ExplicitRoot,
Explicit(String),
}

impl Parent {
pub fn check_parent_name(
&self,
parent_name: Option<&str>,
provided_parent: Option<tracing_core::span::Id>,
ctx: impl std::fmt::Display,
collector_name: &str,
) {
match self {
Parent::ExplicitRoot => {
assert!(
provided_parent.is_none(),
"[{}] expected {} to be an explicit root, but its parent was actually {:?} (name: {:?})",
collector_name,
ctx,
provided_parent,
parent_name,
);
}
Parent::Explicit(expected_parent) => {
assert!(
provided_parent.is_some(),
"[{}] expected {} to have explicit parent {}, but it has no explicit parent",
collector_name,
ctx,
expected_parent,
);
assert_eq!(
Some(expected_parent.as_ref()),
parent_name,
"[{}] expected {} to have explicit parent {}, but its parent was actually {:?} (name: {:?})",
collector_name,
ctx,
expected_parent,
provided_parent,
parent_name,
);
}
Parent::ContextualRoot => {
assert!(
provided_parent.is_none(),
"[{}] expected {} to be a contextual root, but its parent was actually {:?} (name: {:?})",
collector_name,
ctx,
provided_parent,
parent_name,
);
assert!(
parent_name.is_none(),
"[{}] expected {} to be contextual a root, but we were inside span {:?}",
collector_name,
ctx,
parent_name,
);
}
Parent::Contextual(expected_parent) => {
assert!(provided_parent.is_none(),
"[{}] expected {} to have a contextual parent\nbut it has the explicit parent {:?} (name: {:?})",
collector_name,
ctx,
provided_parent,
parent_name,
);
assert_eq!(
Some(expected_parent.as_ref()),
parent_name,
"[{}] expected {} to have contextual parent {:?}, but got {:?}",
collector_name,
ctx,
expected_parent,
parent_name,
);
}
}
}
}
50 changes: 50 additions & 0 deletions tracing-mock/src/parent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/// The parent of an event or span.
///
/// This enum is used to represent the expected and the actual parent of an
/// event or a span.
#[derive(Debug, Eq, PartialEq)]
pub(crate) enum Parent {
/// The event or span is contextually a root - it has no parent.
ContextualRoot,
/// The event or span has a contextually assigned parent, with the specified name.
Contextual(String),
/// The event or span is explicitly a root, it was created with `parent: None`.
ExplicitRoot,
/// The event or span has an explicit parent with the specified name, it was created with
/// `parent: span_id`.
Explicit(String),
}

impl Parent {
#[track_caller]
pub(crate) fn check(
&self,
actual_parent: &Parent,
ctx: impl std::fmt::Display,
collector_name: &str,
) {
let expected_description = |parent: &Parent| match parent {
Self::ExplicitRoot => "be an explicit root".to_string(),
Self::Explicit(name) => format!("have an explicit parent with name='{name}'"),
Self::ContextualRoot => "be a contextual root".to_string(),
Self::Contextual(name) => format!("have a contextual parent with name='{name}'"),
};

let actual_description = |parent: &Parent| match parent {
Self::ExplicitRoot => "was actually an explicit root".to_string(),
Self::Explicit(name) => format!("actually has an explicit parent with name='{name}'"),
Self::ContextualRoot => "was actually a contextual root".to_string(),
Self::Contextual(name) => {
format!("actually has a contextual parent with name='{name}'")
}
};

assert_eq!(
self,
actual_parent,
"[{collector_name}] expected {ctx} to {expected_description}, but {actual_description}",
expected_description = expected_description(self),
actual_description = actual_description(actual_parent)
);
}
}
15 changes: 7 additions & 8 deletions tracing-mock/src/span.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
//! [`expect::span`]: fn@crate::expect::span
#![allow(missing_docs)]
use crate::{
collector::SpanState, expect, field::ExpectedFields, metadata::ExpectedMetadata, Parent,
collector::SpanState, expect, field::ExpectedFields, metadata::ExpectedMetadata, parent::Parent,
};
use std::fmt;

Expand Down Expand Up @@ -699,7 +699,7 @@ impl NewSpan {
pub(crate) fn check(
&mut self,
span: &tracing_core::span::Attributes<'_>,
get_parent_name: impl FnOnce() -> Option<String>,
get_parent: impl FnOnce() -> Parent,
collector_name: &str,
) {
let meta = span.metadata();
Expand All @@ -711,14 +711,13 @@ impl NewSpan {
span.record(&mut checker);
checker.finish();

if let Some(expected_parent) = self.parent.as_ref() {
let actual_parent = get_parent_name();
expected_parent.check_parent_name(
actual_parent.as_deref(),
span.parent().cloned(),
if let Some(ref expected_parent) = self.parent {
let actual_parent = get_parent();
expected_parent.check(
&actual_parent,
format_args!("span `{}`", name),
collector_name,
)
);
}
}
}
Expand Down
Loading
Loading