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

tracing: instrument task wakers #3836

Merged
merged 3 commits into from
Jun 8, 2021
Merged

tracing: instrument task wakers #3836

merged 3 commits into from
Jun 8, 2021

Conversation

seanmonstar
Copy link
Member

Motivation

In support of tokio-rs/console#37, we want to understand when a specific task's waker has been interacted with, such as when it is awoken, or if it's forgotten (not cloned), etc.

Solution

When the tracing feature is enabled, a super trait of Future (InstrumentedFuture) is implemented for Instrumented<F> that allows grabbing the task's ID (well, its span ID), and stores that in the raw task trailer. The waker vtable then emits events and includes that ID.

@seanmonstar seanmonstar requested a review from hawkw June 3, 2021 18:07

cfg_trace! {
mod trace;
pub(crate) use trace::InstrumentedFuture as Future;
Copy link
Member Author

Choose a reason for hiding this comment

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

This rename was mostly for convenience in all the other modules. But it could be that people find it too confusing, in which case I'll ditch the rename and just make the name change in all the other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change the public API of spawn methods to use a different trait, or is it only internal? I can't really tell.

Copy link
Member Author

Choose a reason for hiding this comment

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

No it doesn't. And I checked in a playground, since the trait is pub(crate) the compiler will error if the bounds were used on a pub function.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-tracing Tracing support in Tokio labels Jun 3, 2021
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.

this is great, thanks for working on this! i commented on a few things that might be worth considering, but this looks good to me as-is!

tokio/src/runtime/task/waker.rs Show resolved Hide resolved
tokio/src/runtime/task/waker.rs Show resolved Hide resolved
tokio/src/runtime/task/waker.rs Outdated Show resolved Hide resolved
tokio/src/runtime/task/core.rs Outdated Show resolved Hide resolved
tokio/src/future/trace.rs Show resolved Hide resolved
if let Some(id) = $harness.id() {
tracing::trace!(
target: "tokio::task",
op = %$op,
Copy link
Member

Choose a reason for hiding this comment

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

i can see why this is a potential use-case for custom event names...we should definitely consider adding the option to override event names upstream, so that we can use custom names here rather than a field value...but, again, this is fine for now.

tokio/src/runtime/task/harness.rs Outdated Show resolved Hide resolved
cfg_trace! {
macro_rules! trace {
($harness:expr, $op:expr) => {
if let Some(id) = $harness.id() {
Copy link
Member

Choose a reason for hiding this comment

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

it looks like the intention here is to avoid emitting the event if the task is not being tracked? i wonder if we actually want to do this inside of the tracing event macro, to handle cases where the event is disabled by one of tracing's filtering fast paths. e.g., if the user compiles tracing with one of the "static_max_level_xxx" feature flags, the event macro will be completely empty and won't compile to any code, but we'll still dereference the trailer and check if it has a task ID.

this is probably an unnecessary microoptimzation, though, and we could do it later --- moving it inside the macro will be much easier when tokio-rs/tracing#1393 is published, anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I'd really want to be to check tracing if the event would be emitted, and check if the ID is Some, in the same if statement. I don't want to emit an event if we're not tracking the task either.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

looks fine to me! thanks.

@seanmonstar seanmonstar merged commit e7d74b3 into master Jun 8, 2021
@seanmonstar seanmonstar deleted the tracing-wakers branch June 8, 2021 21:50
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.

4 participants