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 #2812

Closed
wants to merge 2 commits into from

Conversation

hds
Copy link
Contributor

@hds hds commented Nov 20, 2023

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 a Parent enum so that the expected
and actual values can be compared in a more explicit way.

Additionally, the Parent 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.

Given the number of different cases involved in checking parents,
separate integration tests have been added to tracing-mock
specifically for testing all the positive and negative cases when
asserting on the parents 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 requested review from hawkw, davidbarsky and a team as code owners November 20, 2023 13:05
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 a `Parent` enum so that the expected
and actual values can be compared in a more explicit way.

Additionally, the `Parent` 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.

Given the number of different cases involved in checking parents,
separate integration tests have been added to `tracing-mock`
specifically for testing all the positive and negative cases when
asserting on the parents 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-fix-expect-parent branch from dd2c874 to 10fd046 Compare November 20, 2023 14:23
@hawkw
Copy link
Member

hawkw commented Nov 20, 2023

The clippy failure is unrelated to this change, so don't worry about that.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me, thank you for fixing this!

i had some suggestions for possible API improvements, but they're not blockers for this PR.

Comment on lines -24 to -25
.with_contextual_parent(None)
.with_explicit_parent(None),
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?

Comment on lines +965 to +988
let get_parent = || {
if span.is_contextual() {
if let Some(parent) = cx.lookup_current() {
let contextual_parent = cx.span(&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 = cx.span(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())
}
Copy link
Member

Choose a reason for hiding this comment

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

this code is kinda duplicated here and in collector.rs, although not quite...i wonder if some of it can be factored out? idk. not a blocker.

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 tried to refactor this a couple of times and didn't come up with anything that wasn't more verbose than this. On top of that, the indirection required to handle different span lookups and context fetching tends to make it all longer as well.

I'd leave tis as is for now if that's OK.

@hds hds marked this pull request as draft July 29, 2024 21:43
@hds
Copy link
Contributor Author

hds commented Jul 29, 2024

Marked this PR as a draft as it has been superceded by #3004.

@hds
Copy link
Contributor Author

hds commented Aug 5, 2024

Closing this one in favor of #3004 which has now been merged.

@hds hds closed this Aug 5, 2024
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