Skip to content

Commit 527b4f6

Browse files
authored
mock: correct contextual/explicit parent assertions (#3004)
## Motivation When recording the parent of an event or span, the `MockCollector` treats an explicit parent of `None` (i.e. an event or span that is an explicit root) in the same way as if there is no explicit root. This leads to it picking up the contextual parent or treating the event or span as a contextual root. ## Solution This change refactors the recording of the parent to use `is_contextual` to distinguish whether or not an explicit parent has been specified. The actual parent is also written into an `Ancestry` enum so that the expected and actual values can be compared in a more explicit way. Additionally, the `Ancestry` struct has been moved into its own module and the check behavior has been fixed. The error message has also been unified across all cases. Another problem with the previous API is that the two methods `with_contextual_parent` and `with_explicit_parent` are actually mutually exclusive, a span or event cannot be both of them. It is also a (small) mental leap for the user to go from `with_*_parent(None)` to understanding that this means that a span or event is a root (either contextual or explicit). As such, the API has been reworked into a single method `with_ancestry`, which takes an enum with the following four variants: * `HasExplicitParent(String)` (parent span name) * `IsExplicitRoot` * `HasContextualParent(String)` (parent span name) * `IsContextualRoot` To make the interface as useable as possible, helper functions have been defined in the `expect` module which can be used to create the enum variants. Specifically, these take `Into<String>` parameter for the span name. Given the number of different cases involved in checking ancestry, separate integration tests have been added to `tracing-mock` specifically for testing all the positive and negative cases when asserting on the ancestry of events and spans. There were two tests in `tracing-attributes` which specified both an explicit and a contextual parent. This behavior was never intended to work as all events and spans are either contextual or not. The tests have been corrected to only expect one of the two. Fixes: #2440
1 parent acf92ab commit 527b4f6

File tree

14 files changed

+1110
-378
lines changed

14 files changed

+1110
-378
lines changed

tracing-attributes/tests/parents.rs

+6-19
Original file line numberDiff line numberDiff line change
@@ -21,23 +21,16 @@ fn default_parent_test() {
2121
.new_span(
2222
contextual_parent
2323
.clone()
24-
.with_contextual_parent(None)
25-
.with_explicit_parent(None),
26-
)
27-
.new_span(
28-
child
29-
.clone()
30-
.with_contextual_parent(Some("contextual_parent"))
31-
.with_explicit_parent(None),
24+
.with_ancestry(expect::is_contextual_root()),
3225
)
26+
.new_span(child.clone().with_ancestry(expect::is_contextual_root()))
3327
.enter(child.clone())
3428
.exit(child.clone())
3529
.enter(contextual_parent.clone())
3630
.new_span(
3731
child
3832
.clone()
39-
.with_contextual_parent(Some("contextual_parent"))
40-
.with_explicit_parent(None),
33+
.with_ancestry(expect::has_contextual_parent("contextual_parent")),
4134
)
4235
.enter(child.clone())
4336
.exit(child)
@@ -68,20 +61,14 @@ fn explicit_parent_test() {
6861
.new_span(
6962
contextual_parent
7063
.clone()
71-
.with_contextual_parent(None)
72-
.with_explicit_parent(None),
73-
)
74-
.new_span(
75-
explicit_parent
76-
.with_contextual_parent(None)
77-
.with_explicit_parent(None),
64+
.with_ancestry(expect::is_contextual_root()),
7865
)
66+
.new_span(explicit_parent.with_ancestry(expect::is_contextual_root()))
7967
.enter(contextual_parent.clone())
8068
.new_span(
8169
child
8270
.clone()
83-
.with_contextual_parent(Some("contextual_parent"))
84-
.with_explicit_parent(Some("explicit_parent")),
71+
.with_ancestry(expect::has_explicit_parent("explicit_parent")),
8572
)
8673
.enter(child.clone())
8774
.exit(child)

tracing-futures/tests/std_future.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ fn span_on_drop() {
7171
.enter(expect::span().named("foo"))
7272
.event(
7373
expect::event()
74-
.with_contextual_parent(Some("foo"))
74+
.with_ancestry(expect::has_contextual_parent("foo"))
7575
.at_level(Level::INFO),
7676
)
7777
.exit(expect::span().named("foo"))
@@ -81,7 +81,7 @@ fn span_on_drop() {
8181
.enter(expect::span().named("bar"))
8282
.event(
8383
expect::event()
84-
.with_contextual_parent(Some("bar"))
84+
.with_ancestry(expect::has_contextual_parent("bar"))
8585
.at_level(Level::INFO),
8686
)
8787
.exit(expect::span().named("bar"))

tracing-mock/src/ancestry.rs

+148
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
//! Define the ancestry of an event or span.
2+
//!
3+
//! See the documentation on the [`Ancestry`] enum for further details.
4+
5+
use tracing_core::{
6+
span::{self, Attributes},
7+
Event,
8+
};
9+
10+
/// The ancestry of an event or span.
11+
///
12+
/// An event or span can have an explicitly assigned parent, or be an explicit root. Otherwise,
13+
/// an event or span may have a contextually assigned parent or in the final case will be a
14+
/// contextual root.
15+
#[derive(Debug, Eq, PartialEq)]
16+
pub enum Ancestry {
17+
/// The event or span has an explicitly assigned parent (created with `parent: span_id`) with
18+
/// the specified name.
19+
HasExplicitParent(String),
20+
/// The event or span is an explicitly defined root. It was created with `parent: None` and
21+
/// has no parent.
22+
IsExplicitRoot,
23+
/// The event or span has a contextually assigned parent with the specified name. It has no
24+
/// explicitly assigned parent, nor has it been explicitly defined as a root (it was created
25+
/// without the `parent:` directive). There was a span in context when this event or span was
26+
/// created.
27+
HasContextualParent(String),
28+
/// The event or span is a contextual root. It has no explicitly assigned parent, nor has it
29+
/// been explicitly defined as a root (it was created without the `parent:` directive).
30+
/// Additionally, no span was in context when this event or span was created.
31+
IsContextualRoot,
32+
}
33+
34+
impl Ancestry {
35+
#[track_caller]
36+
pub(crate) fn check(
37+
&self,
38+
actual_ancestry: &Ancestry,
39+
ctx: impl std::fmt::Display,
40+
collector_name: &str,
41+
) {
42+
let expected_description = |ancestry: &Ancestry| match ancestry {
43+
Self::IsExplicitRoot => "be an explicit root".to_string(),
44+
Self::HasExplicitParent(name) => format!("have an explicit parent with name='{name}'"),
45+
Self::IsContextualRoot => "be a contextual root".to_string(),
46+
Self::HasContextualParent(name) => {
47+
format!("have a contextual parent with name='{name}'")
48+
}
49+
};
50+
51+
let actual_description = |ancestry: &Ancestry| match ancestry {
52+
Self::IsExplicitRoot => "was actually an explicit root".to_string(),
53+
Self::HasExplicitParent(name) => {
54+
format!("actually has an explicit parent with name='{name}'")
55+
}
56+
Self::IsContextualRoot => "was actually a contextual root".to_string(),
57+
Self::HasContextualParent(name) => {
58+
format!("actually has a contextual parent with name='{name}'")
59+
}
60+
};
61+
62+
assert_eq!(
63+
self,
64+
actual_ancestry,
65+
"[{collector_name}] expected {ctx} to {expected_description}, but {actual_description}",
66+
expected_description = expected_description(self),
67+
actual_description = actual_description(actual_ancestry)
68+
);
69+
}
70+
}
71+
72+
pub(crate) trait HasAncestry {
73+
fn is_contextual(&self) -> bool;
74+
75+
fn is_root(&self) -> bool;
76+
77+
fn parent(&self) -> Option<&span::Id>;
78+
}
79+
80+
impl HasAncestry for &Event<'_> {
81+
fn is_contextual(&self) -> bool {
82+
(self as &Event<'_>).is_contextual()
83+
}
84+
85+
fn is_root(&self) -> bool {
86+
(self as &Event<'_>).is_root()
87+
}
88+
89+
fn parent(&self) -> Option<&span::Id> {
90+
(self as &Event<'_>).parent()
91+
}
92+
}
93+
94+
impl HasAncestry for &Attributes<'_> {
95+
fn is_contextual(&self) -> bool {
96+
(self as &Attributes<'_>).is_contextual()
97+
}
98+
99+
fn is_root(&self) -> bool {
100+
(self as &Attributes<'_>).is_root()
101+
}
102+
103+
fn parent(&self) -> Option<&span::Id> {
104+
(self as &Attributes<'_>).parent()
105+
}
106+
}
107+
108+
/// Determines the ancestry of an actual span or event.
109+
///
110+
/// The rules for determining the ancestry are as follows:
111+
///
112+
/// +------------+--------------+-----------------+---------------------+
113+
/// | Contextual | Current Span | Explicit Parent | Ancestry |
114+
/// +------------+--------------+-----------------+---------------------+
115+
/// | Yes | Yes | - | HasContextualParent |
116+
/// | Yes | No | - | IsContextualRoot |
117+
/// | No | - | Yes | HasExplicitParent |
118+
/// | No | - | No | IsExplicitRoot |
119+
/// +------------+--------------+-----------------+---------------------+
120+
pub(crate) fn get_ancestry(
121+
item: impl HasAncestry,
122+
lookup_current: impl FnOnce() -> Option<span::Id>,
123+
span_name: impl FnOnce(&span::Id) -> Option<&str>,
124+
) -> Ancestry {
125+
if item.is_contextual() {
126+
if let Some(parent_id) = lookup_current() {
127+
let contextual_parent_name = span_name(&parent_id).expect(
128+
"tracing-mock: contextual parent cannot \
129+
be looked up by ID. Was it recorded correctly?",
130+
);
131+
Ancestry::HasContextualParent(contextual_parent_name.to_string())
132+
} else {
133+
Ancestry::IsContextualRoot
134+
}
135+
} else if item.is_root() {
136+
Ancestry::IsExplicitRoot
137+
} else {
138+
let parent_id = item.parent().expect(
139+
"tracing-mock: is_contextual=false is_root=false \
140+
but no explicit parent found. This is a bug!",
141+
);
142+
let explicit_parent_name = span_name(parent_id).expect(
143+
"tracing-mock: explicit parent cannot be looked \
144+
up by ID. Is the provided Span ID valid: {parent_id}",
145+
);
146+
Ancestry::HasExplicitParent(explicit_parent_name.to_string())
147+
}
148+
}

tracing-mock/src/collector.rs

+36-17
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@
138138
//! [`Collect`]: trait@tracing::Collect
139139
//! [`MockCollector`]: struct@crate::collector::MockCollector
140140
use crate::{
141+
ancestry::get_ancestry,
141142
event::ExpectedEvent,
142143
expect::Expect,
143144
field::ExpectedFields,
@@ -1039,16 +1040,20 @@ where
10391040
)
10401041
}
10411042
}
1042-
let get_parent_name = || {
1043-
let stack = self.current.lock().unwrap();
1044-
let spans = self.spans.lock().unwrap();
1045-
event
1046-
.parent()
1047-
.and_then(|id| spans.get(id))
1048-
.or_else(|| stack.last().and_then(|id| spans.get(id)))
1049-
.map(|s| s.name.to_string())
1043+
let event_get_ancestry = || {
1044+
get_ancestry(
1045+
event,
1046+
|| self.lookup_current(),
1047+
|span_id| {
1048+
self.spans
1049+
.lock()
1050+
.unwrap()
1051+
.get(span_id)
1052+
.map(|span| span.name)
1053+
},
1054+
)
10501055
};
1051-
expected.check(event, get_parent_name, &self.name);
1056+
expected.check(event, event_get_ancestry, &self.name);
10521057
}
10531058
Some(ex) => ex.bad(&self.name, format_args!("observed event {:#?}", event)),
10541059
}
@@ -1108,14 +1113,18 @@ where
11081113
if let Some(expected_id) = &expected.span.id {
11091114
expected_id.set(id.into_u64()).unwrap();
11101115
}
1111-
let get_parent_name = || {
1112-
let stack = self.current.lock().unwrap();
1113-
span.parent()
1114-
.and_then(|id| spans.get(id))
1115-
.or_else(|| stack.last().and_then(|id| spans.get(id)))
1116-
.map(|s| s.name.to_string())
1117-
};
1118-
expected.check(span, get_parent_name, &self.name);
1116+
1117+
expected.check(
1118+
span,
1119+
|| {
1120+
get_ancestry(
1121+
span,
1122+
|| self.lookup_current(),
1123+
|span_id| spans.get(span_id).map(|span| span.name),
1124+
)
1125+
},
1126+
&self.name,
1127+
);
11191128
}
11201129
}
11211130
spans.insert(
@@ -1265,6 +1274,16 @@ where
12651274
}
12661275
}
12671276

1277+
impl<F> Running<F>
1278+
where
1279+
F: Fn(&Metadata<'_>) -> bool,
1280+
{
1281+
fn lookup_current(&self) -> Option<span::Id> {
1282+
let stack = self.current.lock().unwrap();
1283+
stack.last().cloned()
1284+
}
1285+
}
1286+
12681287
impl MockHandle {
12691288
#[cfg(feature = "tracing-subscriber")]
12701289
pub(crate) fn new(expected: Arc<Mutex<VecDeque<Expect>>>, name: String) -> Self {

0 commit comments

Comments
 (0)