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 more resources #4302

Merged
merged 9 commits into from Dec 14, 2021
Merged

Conversation

zaharidichev
Copy link
Contributor

This PR adds instrumentation to more resources from the sync package.
Related: tokio-rs/console#188

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync M-tracing Tracing support in Tokio labels Dec 7, 2021
Comment on lines 188 to 207
cfg_trace! {
return Self {
permits: AtomicUsize::new(permits << Self::PERMIT_SHIFT),
waiters: Mutex::const_new(Waitlist {
queue: LinkedList::new(),
closed: false,
}),
resource_span: tracing::Span::none(),
};
}

cfg_not_trace! {
return Self {
permits: AtomicUsize::new(permits << Self::PERMIT_SHIFT),
waiters: Mutex::const_new(Waitlist {
queue: LinkedList::new(),
closed: false,
}),
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely there must be a better way...

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 would like to believe so. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you do this?

return Self {
    permits: AtomicUsize::new(permits << Self::PERMIT_SHIFT),
    waiters: Mutex::const_new(Waitlist {
        queue: LinkedList::new(),
        closed: false,
    }),
    #[cfg(tracing)]
    resource_span: tracing::Span::none(),
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot, I have changed it to use this approach in other places as well

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Dec 10, 2021
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
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.

Thanks for this. Looks good to merge.

As a general comment, there probably are opportunities to clean up the code and avoid all the cfg attributes in the main source. We can worry about that in a follow up PR. I'm happy to work with you on figuring out strategies to clean it up.

The change here will only apply when the unstable flag is enabled, so lets move forward and cleanup incrementally.

@carllerche carllerche merged commit 4e3268d into master Dec 14, 2021
@carllerche carllerche deleted the zd/more-instrumentation branch December 14, 2021 22:04
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-sync Module: tokio/sync M-tracing Tracing support in Tokio R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants