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

feat(console): use tokio task ids in task views #403

Merged
merged 8 commits into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion console-api/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ impl From<&dyn std::fmt::Debug> for field::Value {
// or vice versa. However, this is unavoidable here, because `prost` generates
// a struct with `#[derive(PartialEq)]`, but we cannot add`#[derive(Hash)]` to the
// generated code.
#[allow(clippy::derive_hash_xor_eq)]
#[allow(clippy::derived_hash_with_manual_eq)]
impl Hash for field::Name {
fn hash<H: Hasher>(&self, state: &mut H) {
match self {
Expand Down
1 change: 1 addition & 0 deletions tokio-console/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ impl Metadata {
impl Field {
const SPAWN_LOCATION: &'static str = "spawn.location";
const NAME: &'static str = "task.name";
const TASK_ID: &'static str = "task.id";

/// Converts a wire-format `Field` into an internal `Field` representation,
/// using the provided `Metadata` for the task span that the field came
Expand Down
43 changes: 38 additions & 5 deletions tokio-console/src/state/tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
histogram::DurationHistogram,
pb_duration,
store::{self, Id, SpanId, Store},
Field, Metadata, Visibility,
Field, FieldValue, Metadata, Visibility,
},
util::Percentage,
view,
Expand Down Expand Up @@ -58,13 +58,29 @@ pub(crate) enum TaskState {

pub(crate) type TaskRef = store::Ref<Task>;

/// The Id for a Tokio task.
///
/// This should be equivalent to [`tokio::task::Id`], which can't be
/// used because it's not possible to construct outside the `tokio`
/// crate.
///
/// Within the context of `tokio-console`, we don't depend on it
/// being the same as Tokio's own type, as the task id is recorded
/// as a `u64` in tracing and then sent via the wire protocol as such.
pub(crate) type TaskId = u64;

#[derive(Debug)]
pub(crate) struct Task {
/// The task's pretty (console-generated, sequential) task ID.
///
/// This is NOT the `tracing::span::Id` for the task's tracing span on the
/// remote.
id: Id<Task>,
/// The `tokio::task::Id` in the remote tokio runtime.
///
/// This Id may not be unique if there are multiple runtimes in the
/// remote process.
task_id: Option<TaskId>,
/// The `tracing::span::Id` on the remote process for this task's span.
///
/// This is used when requesting a task details stream.
Expand Down Expand Up @@ -147,6 +163,7 @@ impl TasksState {
}
};
let mut name = None;
let mut task_id = None;
let mut fields = task
.fields
.drain(..)
Expand All @@ -157,6 +174,13 @@ impl TasksState {
name = Some(strings.string(field.value.to_string()));
return None;
}
if &*field.name == Field::TASK_ID {
task_id = match field.value {
FieldValue::U64(id) => Some(id as TaskId),
_ => None,
};
return None;
}
Some(field)
})
.collect::<Vec<_>>();
Expand All @@ -170,14 +194,17 @@ impl TasksState {
// remap the server's ID to a pretty, sequential task ID
let id = ids.id_for(span_id);

let short_desc = strings.string(match name.as_ref() {
Some(name) => format!("{} ({})", id, name),
None => format!("{}", id),
let short_desc = strings.string(match (task_id, name.as_ref()) {
(Some(task_id), Some(name)) => format!("{} ({})", task_id, name),
hds marked this conversation as resolved.
Show resolved Hide resolved
(Some(task_id), None) => format!("{}", task_id),
hds marked this conversation as resolved.
Show resolved Hide resolved
(None, Some(name)) => format!("({})", name),
hds marked this conversation as resolved.
Show resolved Hide resolved
(None, None) => "".to_owned(),
});

let mut task = Task {
name,
id,
task_id,
span_id,
short_desc,
formatted_fields,
Expand Down Expand Up @@ -241,6 +268,10 @@ impl Task {
self.id
}

pub(crate) fn task_id(&self) -> Option<TaskId> {
self.task_id
}

pub(crate) fn span_id(&self) -> SpanId {
self.span_id
}
Expand Down Expand Up @@ -426,7 +457,9 @@ impl Default for SortBy {
impl SortBy {
pub fn sort(&self, now: SystemTime, tasks: &mut [Weak<RefCell<Task>>]) {
match self {
Self::Tid => tasks.sort_unstable_by_key(|task| task.upgrade().map(|t| t.borrow().id)),
Self::Tid => {
tasks.sort_unstable_by_key(|task| task.upgrade().map(|t| t.borrow().task_id))
}
Self::Name => {
tasks.sort_unstable_by_key(|task| task.upgrade().map(|t| t.borrow().name.clone()))
}
Expand Down
2 changes: 1 addition & 1 deletion tokio-console/src/view/tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl TableList<11> for TasksTable {
warnings,
Cell::from(id_width.update_str(format!(
"{:>width$}",
task.id(),
task.task_id().map(|id| id.to_string()).unwrap_or_default(),
Copy link
Member

Choose a reason for hiding this comment

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

since the task ID is not going to change over the lifetime of the task, and we have to format an ID for each task every time we draw the task list, i wonder if we want to cache a string representation of the task's ID as soon as we receive a task --- we could have an id_str field on the Task struct, which would allow us to avoid calling to_string() on the ID for every task every time we render the tasks list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to check the docs, but do we really have to format all rows in a table every time it's opened? Do we not only format the visible rows?

Copy link
Member

Choose a reason for hiding this comment

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

er, that's what i meant. the point is that this work is performed every time we redraw the table.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, got it. I understand now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hawkw I've made some changes to store a string of the id. Please let me know if this is what you had in mind.

width = id_width.chars() as usize
))),
Cell::from(task.state().render(styles)),
Expand Down