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

task: fix spawn_local source location for console #5984

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

hds
Copy link
Contributor

@hds hds commented Sep 5, 2023

Motivation

The location of a spawned task, as shown in tokio console, is taken from
the location set on the tracing span that instruments the task. For this
location to work, there must be unbroken chain of functions instrumented
with #[track_caller].

For task::spawn_local, there was a break in this chain and so the
span contained the location of an internal function in tokio.

Solution

This change adds the missing #[track_caller] attribute. It has been
tested locally as automated tests would really need tracing-mock to be
published so we can use it in the tokio tests.

The location of a spawned task, as shown in tokio console, is taken from
the location set on the tracing span that instruments the task. For this
location to work, there must be unbroken chain of functions instrumented
with `#[track_caller]`.

For `task::spawn_local`, there was a break in this chain and so the
span contained the location of an internal function in tokio.

This change adds the missing `#[track_caller]` attribute. It has been
tested locally as automated tests would really need `tracing-mock` to be
published so we can use it in the tokio tests.
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-tracing Tracing support in Tokio labels Sep 5, 2023
@Darksonn
Copy link
Contributor

Darksonn commented Sep 5, 2023

We have various tests for #[track_caller]. Can you add another one for this method?

@hds
Copy link
Contributor Author

hds commented Sep 5, 2023

Those tests all involve something panicking, which isn't the case here.

For this method we would need to check that the recorded span contains the correct value. This is something that we could do with tracing-mock, but since it hasn't yet been published to crates.io, it's really hard to use it to test tokio (we would need a test crate that isn't part of the workspace so that we can override tracing to use the one from the v0.1.x branch).

Also I don't know how to make this function panic (in theory I could by calling local_set.spawn_local() from a different thread than the one that it was created on, but LocalSet isn't Sync so I don't know how I'd manage that.

What I will do, is add tests for this in console-subscriber, once tokio-rs/console#452 goes in, this won't prevent merging a breaking change in tokio, but it will let us know after the fact.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Fair enough. I'm happy to take this without a test in Tokio.

@Darksonn Darksonn merged commit aad1892 into master Sep 6, 2023
75 checks passed
@Darksonn Darksonn deleted the hds/spawn-named-track-caller branch September 6, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-tracing Tracing support in Tokio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants