-
Notifications
You must be signed in to change notification settings - Fork 726
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.
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 #3004
Conversation
53c0a5e
to
4c0f243
Compare
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. 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
4c0f243
to
2885c46
Compare
tracing-mock/src/ancestry.rs
Outdated
/// The event or span has a contextually assigned parent with the specified name. Additionally, | ||
/// it has no explicitly assigned parent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not parsing this sentence right: are you trying to say it has no explicitly defined root..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I want to say that the span or event has no explicitly defined parent (which means the parent:
field in a span or event macro was not used). Instead, it has a parent that was contextually assigned.
Does that make sense? I see how it's not super clear, I can have another go at rewriting it if it's still not making sense.
/// +------------+--------------+-----------------+---------------------+ | ||
/// | Contextual | Current Span | Explicit Parent | Ancestry | | ||
/// +------------+--------------+-----------------+---------------------+ | ||
/// | Yes | Yes | - | HasContextualParent | | ||
/// | Yes | No | - | IsContextualRoot | | ||
/// | No | - | Yes | HasExplicitParent | | ||
/// | No | - | No | IsExplicitRoot | | ||
/// +------------+--------------+-----------------+---------------------+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, this is very helpful!
ad70172
to
dbdf5f0
Compare
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. 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
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. 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
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. 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
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. 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
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 anexplicit 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 theexpected and actual values can be compared in a more explicit way.
Additionally, the
Ancestry
struct has been moved into its own module andthe 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
andwith_explicit_parent
are actuallymutually 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)
tounderstanding 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 enumvariants. Specifically, these take
Into<String>
parameter for the spanname.
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 anexplicit 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
PR details
This PR attempts to address the comments made on #2812.
Specifically to address this comment #2812 (comment)
by replacing
.with_explicit_parent()
and.with_contextual_parent()
methodswith a single
.with_ancestry()
method.To address this comment #2812 (comment)
the logic to pick the ancestry of an actual span or event has been centralised. This
required factoring out the logic to get the current span and to look up a span by ID,
since these are different between the
Running
(MockCollector
) and theMockSubscriber
structs.