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

a sketch for how task naming might work for tracing #3871

Closed
wants to merge 2 commits into from
Closed

a sketch for how task naming might work for tracing #3871

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29caf61
a sketch for how task naming might work for tracing
jbr Jun 18, 2021
2a821b8
accept &str instead of Into<Cow<'static, str>>
jbr Jun 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion tokio/src/runtime/handle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl Handle {
F::Output: Send + 'static,
{
#[cfg(all(tokio_unstable, feature = "tracing"))]
let future = crate::util::trace::task(future, "task");
let future = crate::util::trace::task(future, "task", None);
self.spawner.spawn(future)
}

Expand Down
4 changes: 2 additions & 2 deletions tokio/src/task/local.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ cfg_rt! {
F: Future + 'static,
F::Output: 'static,
{
let future = crate::util::trace::task(future, "local");
let future = crate::util::trace::task(future, "local", None);
CURRENT.with(|maybe_cx| {
let cx = maybe_cx
.expect("`spawn_local` called from outside of a `task::LocalSet`");
Expand Down Expand Up @@ -381,7 +381,7 @@ impl LocalSet {
F: Future + 'static,
F::Output: 'static,
{
let future = crate::util::trace::task(future, "local");
let future = crate::util::trace::task(future, "local", None);
let (task, handle) = unsafe { task::joinable_local(future) };
self.context.tasks.borrow_mut().queue.push_back(task);
self.context.shared.waker.wake();
Expand Down
4 changes: 4 additions & 0 deletions tokio/src/task/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,4 +299,8 @@ cfg_rt! {

mod unconstrained;
pub use unconstrained::{unconstrained, Unconstrained};

cfg_trace! {
pub use spawn::spawn_named;
}
}
16 changes: 15 additions & 1 deletion tokio/src/task/spawn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,21 @@ cfg_rt! {
{
let spawn_handle = runtime::context::spawn_handle()
.expect(CONTEXT_MISSING_ERROR);
let task = crate::util::trace::task(task, "task");
let task = crate::util::trace::task(task, "task", None);
spawn_handle.spawn(task)
}

cfg_trace! {
#[cfg_attr(tokio_track_caller, track_caller)]
pub fn spawn_named<T>(name: impl Into<std::borrow::Cow<'static, str>>, task: T) -> JoinHandle<T::Output>
Copy link
Member

Choose a reason for hiding this comment

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

Couple of thoughts.

First, is there a reason to avoid the builder patter? Generally, we try to match the std API and specifying a name matches up w/ the thread::Builder pattern. Also, using a builder could allow us to add Builder::spawn_blocking as a separate API.

Then, what was the motivation to use impl Into<std::borrow::Cow<'static, str>>? Looking at the code, it seems like &str should work?

Copy link
Member

Choose a reason for hiding this comment

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

To elaborate a bit, we can deviate from copying std APIs and patterns, but we should have a good reason to do so.

where
T: Future + Send + 'static,
T::Output: Send + 'static,
{
let spawn_handle = runtime::context::spawn_handle()
.expect(CONTEXT_MISSING_ERROR);
let task = crate::util::trace::task(task, "task", Some(name.into()));
spawn_handle.spawn(task)
}
}
}
6 changes: 4 additions & 2 deletions tokio/src/util/trace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ cfg_trace! {

#[inline]
#[cfg_attr(tokio_track_caller, track_caller)]
pub(crate) fn task<F>(task: F, kind: &'static str) -> Instrumented<F> {
pub(crate) fn task<F>(task: F, kind: &'static str, name: Option<std::borrow::Cow<'static, str>>) -> Instrumented<F> {
use tracing::instrument::Instrument;
#[cfg(tokio_track_caller)]
let location = std::panic::Location::caller();
Expand All @@ -14,12 +14,14 @@ cfg_trace! {
"task",
%kind,
spawn.location = %format_args!("{}:{}:{}", location.file(), location.line(), location.column()),
task.name = %name.unwrap_or_default()
);
#[cfg(not(tokio_track_caller))]
let span = tracing::trace_span!(
target: "tokio::task",
"task",
%kind,
task.name = %name.unwrap_or_default()
);
task.instrument(span)
}
Expand All @@ -29,7 +31,7 @@ cfg_trace! {
cfg_not_trace! {
cfg_rt! {
#[inline]
pub(crate) fn task<F>(task: F, _: &'static str) -> F {
pub(crate) fn task<F>(task: F, _: &'static str, _name: Option<std::borrow::Cow<'static, str>>) -> F {
// nop
task
}
Expand Down