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

Jump to bottom

mock: MockCollector can't differentiate between contextual and explicit roots #2440

Open
hds opened this issue Jan 10, 2023 · 0 comments · May be fixed by #2812
Open

mock: MockCollector can't differentiate between contextual and explicit roots #2440

hds opened this issue Jan 10, 2023 · 0 comments · May be fixed by #2812

Comments

@hds
Copy link
Contributor

hds commented Jan 10, 2023

Bug Report

Version

master branch, tracing-mock isn't released yet.

Probably also happens on v0.1.x branch, I haven't checked, but that code is the same.

Crates

tracing-mock

Description

Expecting an explicit root or a contextual root will accept either indescriminately. The following test should panic, but it doesn't.

#[test]
fn explicit_vs_contextual_root() {
    use tracing_mock::{collector, expect};

    let (collector, handle) = collector::mock()
        .new_span(expect::span().with_contextual_parent(None))
        .new_span(expect::span().with_explicit_parent(None))
        .new_span(expect::span().with_contextual_parent(None))
        .new_span(expect::span().with_explicit_parent(None))
        .run_with_handle();

    tracing::collect::with_default(collector, || {
        tracing::info_span!("contextual_root");
        tracing::info_span!(parent: None, "explicit_root");
        tracing::info_span!(parent: None, "explicit_root");
        tracing::info_span!("contextual_root");
    });

    handle.assert_finished();
}
@hds hds mentioned this issue Nov 14, 2023
Merged
hds added a commit to tokio-rs/tokio that referenced this issue Nov 18, 2023
@hds
task: make task span explicit root
571d01c 
In Tokio, tasks are optionally instrumented with tracing spans to allow
analysis of the runtime behavior to be performed with tools like
tokio-console.

The span that is created for each task gets currently follows the
default tracing behavior and has a contextual parent attached to it
based on the span that is actual when `tokio::spawn` or similar is
called.

However, in tracing, a span will remain "alive" until all its children
spans are closed. This doesn't match how spawned tasks work. A task may
outlive the context in which is was spawned (and frequently does). This
causes tasks which spawn other - longer living - tasks to appear in
`tokio-console` as having lost their waker when instead they should be
shown as completed (tokio-rs/console#345). It can also cause undesired
behavior for unrelated tracing spans if a subscriber is receiving both
the other spans as well as Tokio's instrumentation.

To fix this mismatch in behavior, the task span has `parent: None` set
on it, making it an explicit root - it has no parent. The same was
already done for all spans representing resources in #6107. This change
is made within the scope of #5792.

Due to a defect in the currently available `tracing-mock` crate, it is
not possible to test this change at a tracing level
(tokio-rs/tracing#2440). Instead, a test for the `console-subscriber`
has been written which shows that this change fixes the defect as
observed in `tokio-console` (tokio-rs/console#490).
@hds hds mentioned this issue Nov 18, 2023
Merged
hds added a commit to tokio-rs/tokio that referenced this issue Nov 19, 2023
@hds
task: make task span explicit root (#6158)
7a30504 
In Tokio, tasks are optionally instrumented with tracing spans to allow
analysis of the runtime behavior to be performed with tools like
tokio-console.

The span that is created for each task gets currently follows the
default tracing behavior and has a contextual parent attached to it
based on the span that is actual when `tokio::spawn` or similar is
called.

However, in tracing, a span will remain "alive" until all its children
spans are closed. This doesn't match how spawned tasks work. A task may
outlive the context in which is was spawned (and frequently does). This
causes tasks which spawn other - longer living - tasks to appear in
`tokio-console` as having lost their waker when instead they should be
shown as completed (tokio-rs/console#345). It can also cause undesired
behavior for unrelated tracing spans if a subscriber is receiving both
the other spans as well as Tokio's instrumentation.

To fix this mismatch in behavior, the task span has `parent: None` set
on it, making it an explicit root - it has no parent. The same was
already done for all spans representing resources in #6107. This change
is made within the scope of #5792.

Due to a defect in the currently available `tracing-mock` crate, it is
not possible to test this change at a tracing level
(tokio-rs/tracing#2440). Instead, a test for the `console-subscriber`
has been written which shows that this change fixes the defect as
observed in `tokio-console` (tokio-rs/console#490).
hds added a commit that referenced this issue Nov 20, 2023
@hds
mock: correct contextual/explicit parent assertions
a722e89 
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.

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 issue Nov 20, 2023
@hds
mock: correct contextual/explicit parent assertions
717b791 
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 added a commit that referenced this issue Nov 20, 2023
@hds
mock: correct contextual/explicit parent assertions
f9f9496 
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 added a commit that referenced this issue Nov 20, 2023
@hds
mock: correct contextual/explicit parent assertions
dd2c874 
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 linked a pull request Nov 20, 2023 that will close this issue
Open
hds added a commit that referenced this issue Nov 20, 2023
@hds
mock: correct contextual/explicit parent assertions
10fd046 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

mock: correct contextual/explicit parent assertions tokio-rs/tracing
1 participant
@hds