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.

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

Merged
merged 5 commits into from
Aug 5, 2024

Conversation

hds
Copy link
Contributor

@hds hds commented Jun 11, 2024

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

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() methods
with 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 the MockSubscriber
structs.

@hds hds requested review from hawkw, davidbarsky and a team as code owners June 11, 2024 14:08
@hds hds force-pushed the hds/mock-refactor-with-ancestry branch 2 times, most recently from 53c0a5e to 4c0f243 Compare June 11, 2024 14:19
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
@hds hds force-pushed the hds/mock-refactor-with-ancestry branch from 4c0f243 to 2885c46 Compare June 21, 2024 10:49
Comment on lines 23 to 24
/// The event or span has a contextually assigned parent with the specified name. Additionally,
/// it has no explicitly assigned parent.
Copy link
Member

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..?

Copy link
Contributor Author

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.

Comment on lines +109 to +116
/// +------------+--------------+-----------------+---------------------+
/// | Contextual | Current Span | Explicit Parent | Ancestry |
/// +------------+--------------+-----------------+---------------------+
/// | Yes | Yes | - | HasContextualParent |
/// | Yes | No | - | IsContextualRoot |
/// | No | - | Yes | HasExplicitParent |
/// | No | - | No | IsExplicitRoot |
/// +------------+--------------+-----------------+---------------------+
Copy link
Member

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!

@hds hds force-pushed the hds/mock-refactor-with-ancestry branch from ad70172 to dbdf5f0 Compare August 5, 2024 14:25
@hds hds merged commit 527b4f6 into master Aug 5, 2024
56 checks passed
@hds hds deleted the hds/mock-refactor-with-ancestry branch August 5, 2024 17:03
hds added a commit that referenced this pull request Nov 5, 2024
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
hds added a commit that referenced this pull request Nov 7, 2024
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
hds added a commit that referenced this pull request Nov 8, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mock: MockCollector can't differentiate between contextual and explicit roots
2 participants