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

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

merged 8 commits into from
Mar 28, 2023

Conversation

hds
Copy link
Collaborator

@hds hds commented Mar 13, 2023

Tokio Console generates its own sequential Id for internal tracking and
indexing of objects (tasks, resources, etc.). However, this Id will be
recreated if Console is restarted.

In order to provide more useful information to the user, the task Id
generated by Tokio can be used in the task list and task details screens
instead. If used in this way, the ID field in the task list and task
detail views will be stable across restarts of Console (assuming the
monitored application is not restarted).

This also frees up horizontal space, as the task.id attribute
doesn't need to be displayed separately.

The disadvantage of using Tokio's task Id is that it is not guaranteed
to be present by the type system.

To avoid problems with missing task Ids, the Tokio task Id is store in
addition to the span::Id (used to communicate with the
console-subscriber) and the sequential console Id (used in the
store). If a task is missing the task.id field for whatever reason
it will still appear, but with an empty ID. If multiple runtimes are
active, then duplicate ID values will appear.

Fixes: #385

Tokio Console generates its own sequential Id for internal tracking and
indexing of objects (tasks, resources, etc.). However, this Id will be
recreated if Console is restarted.

In order to provide more useful information to the user, the task Id
generated by Tokio can be used in the task list and task details screens
instead. If used in this way, the ID field in the task list and task
detail views will be stable across restarts of Console (assuming the
monitored application is not restarted).

This also frees up horizontal space, as the `task.id` attribute
doesn't need to be displayed separately.

The disadvantage of using Tokio's task Id is that it is not guaranteed
to be present by the type system.

To avoid problems with missing task Ids, the Tokio task Id is store in
addition to the `span::Id` (used to communicate with the
`console-subscriber`) and the sequential console `Id` (used in the
`store`). If a task is missing the `task.id` field for whatever reason
it will still appear, but with an empty ID. If multiple runtimes are
active, then duplicate ID values will appear.

Fixes: #385
@hds hds requested a review from a team as a code owner March 13, 2023 22:13
@hds
Copy link
Collaborator Author

hds commented Mar 13, 2023

@hawkw I'm a bit torn about this approach. Adding "yet another Id" seems a bit hacky, but replacing either the span::Id or the sequential console Id which are both shared amongst all object types also seemed like a bad idea.

We could skip tasks with no tokio task Id (which I think shouldn't happen), the same way we skip tasks without a span::Id or a metadata Id, which would at least guarantee that some Id is present.

As a follow up to this change, I think that it's worth trying to get some per runtime identifier in (unstable) tokio so that we can distinguish tasks from different runtimes, an optional user supplied name would also be nice, but it's neither necessary nor sufficient to properly distinguish tasks. I think there's already an Issue for this, I'll see if I can find it.

@hawkw hawkw self-assigned this Mar 20, 2023
@hawkw
Copy link
Member

hawkw commented Mar 27, 2023

@hawkw I'm a bit torn about this approach. Adding "yet another Id" seems a bit hacky, but replacing either the span::Id or the sequential console Id which are both shared amongst all object types also seemed like a bad idea.

Hmm, I'm not sure if I love having three separate concepts of IDs...the primary reason we added the sequential console-generated IDs was because the raw span IDs are quite long regardless of the number of tasks, because span IDs are generated based on which thread a span was created on (among other things), and are non-sequential and able to be reused by tracing.

The tokio task IDs and console generated sequential IDs have more or less the same characteristics (they are sequential and not reused when a task completes). However, because the console-generated sequential IDs are generated in the tokio-console TUI rather than in the instrumented application, they have some additional issues: they are not persistent across tokio-console sessions (see #385).

Since the tokio-assigned task IDs and the console-assigned task IDs have the same general characteristics, my preference would be to remove the console IDs entirely. This would simplify the implementation significantly, as we would no longer have to track ID remappings in the console, and would solve #385. We would preferentially display the tokio task IDs in the console UI, and fall back to the raw span ID if a task ID is unknown. We may want to have some way of visually distinguishing tokio IDs and span IDs in the UI...

We could skip tasks with no tokio task Id (which I think shouldn't happen), the same way we skip tasks without a span::Id or a metadata Id, which would at least guarantee that some Id is present.

I think we should probably use the span ID if no task ID is present, but clearly indicate that these are different.

As a follow up to this change, I think that it's worth trying to get some per runtime identifier in (unstable) tokio so that we can distinguish tasks from different runtimes, an optional user supplied name would also be nice, but it's neither necessary nor sufficient to properly distinguish tasks. I think there's already an Issue for this, I'll see if I can find it.

+1 for this --- we should definitely add a runtime ID/name notion to Tokio. But for now, I think it's acceptable to permit collisions in the case of multiple runtimes, since they can be resolved by comparing the task's span IDs.

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.

Since this is a relatively small change, I would be open to merging it as-is and then removing the console-generated IDs in a follow-up, or something. Removing the console's sequential IDs would be a larger change that might require some discussion, while this branch is basically a quick fix that will improve UX for now regardless of what we decide to do long term.

tokio-console/src/state/tasks.rs Outdated Show resolved Hide resolved
tokio-console/src/state/tasks.rs Outdated Show resolved Hide resolved
tokio-console/src/state/tasks.rs Outdated Show resolved Hide resolved
@@ -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.

@hds
Copy link
Collaborator Author

hds commented Mar 27, 2023

+1 for this --- we should definitely add a runtime ID/name notion to Tokio. But for now, I think it's acceptable to permit collisions in the case of multiple runtimes, since they can be resolved by comparing the task's span IDs.

Actually it turns out that tokio task ids are globally unique. I had thought (assumed) that they would depend on the runtime, but there's a once_cell atomic counter (or similar) used to generate them. Means that there aren't collisions, but the runtime ids would still be useful.

(None, Some(name)) => format!("({})", name),
(Some(task_id), Some(name)) => format!("{task_id} ({name})"),
(Some(task_id), None) => task_id.to_string(),
(None, Some(name)) => name.to_owned()
Copy link
Member

Choose a reason for hiding this comment

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

missing , here (i think that was my fault, sorry!)

Suggested change
(None, Some(name)) => name.to_owned()
(None, Some(name)) => name.to_owned(),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, rustc caught that one for me, I didn't notice it either. (-;

I needed to use name.as_ref().to_owned() here because name is an InternedStr. Not sure if there's a better pattern for this.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, whoops, yeah, that makes sense. I assumed it was just a string.

@hawkw hawkw enabled auto-merge (squash) March 28, 2023 16:26
@hawkw hawkw merged commit 001fc49 into tokio-rs:main Mar 28, 2023
@hds hds deleted the task-list-ids branch March 28, 2023 16:37
hds pushed a commit that referenced this pull request Aug 18, 2023
…Id (#455)

In #403, the tasks list view was changed to display the `tokio::task::Id` of
the task, instead of the tokio-console generated display ID (which isn't
stable across restarts of the tokio-console itself. However, the task
detail view wasn't updated and was still using the display ID.

This change fixes the ID displayed in the task detail view so that it also
displays the `tokio::task::Id` of the task.
hawkw added a commit that referenced this pull request Sep 29, 2023
Tokio Console generates its own sequential Id for internal tracking and
indexing of objects (tasks, resources, etc.). However, this Id will be
recreated if Console is restarted.

In order to provide more useful information to the user, the task Id
generated by Tokio can be used in the task list and task details screens
instead. If used in this way, the ID field in the task list and task
detail views will be stable across restarts of Console (assuming the
monitored application is not restarted).

This also frees up horizontal space, as the `task.id` attribute
doesn't need to be displayed separately.

The disadvantage of using Tokio's task Id is that it is not guaranteed
to be present by the type system.

To avoid problems with missing task Ids, the Tokio task Id is store in
addition to the `span::Id` (used to communicate with the
`console-subscriber`) and the sequential console `Id` (used in the
`store`). If a task is missing the `task.id` field for whatever reason
it will still appear, but with an empty ID. If multiple runtimes are
active, then duplicate ID values will appear.

Fixes: #385

Co-authored-by: Eliza Weisman <[email protected]>
hawkw pushed a commit that referenced this pull request Sep 29, 2023
…Id (#455)

In #403, the tasks list view was changed to display the `tokio::task::Id` of
the task, instead of the tokio-console generated display ID (which isn't
stable across restarts of the tokio-console itself. However, the task
detail view wasn't updated and was still using the display ID.

This change fixes the ID displayed in the task detail view so that it also
displays the `tokio::task::Id` of the task.
hawkw added a commit that referenced this pull request Sep 29, 2023
# Changelog

All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## tokio-console-v0.1.10 - (2023-09-29)

[7d009f8](https://github.com/tokio-rs/console/commit/7d009f87120ce0c89f5f9c5311f05b6756ca770f)...[7d009f8](https://github.com/tokio-rs/console/commit/7d009f87120ce0c89f5f9c5311f05b6756ca770f)

### <a id = "tokio-console-v0.1.10-breaking"></a>Breaking Changes
- **Update Tonic and Prost dependencies ([#364](#364 ([f9b8e03](https://github.com/tokio-rs/console/commit/f9b8e03bd7ee1d0edb441c94a93a350d5b06ed3b))<br />This commit updates the public dependencies `prost` and `tonic` to
semver-incompatible versions (v0.11.0 and v0.8.0, respectively). This is
a breaking change for users who are integrating the `console-api` protos
with their own `tonic` servers or clients.
- **Update `tonic` to v0.10 and increase MSRV to 1.64 ([#464](#464 ([96e62c8](https://github.com/tokio-rs/console/commit/96e62c83ef959569bb062dc8fee98fa2b2461e8d))<br />This is a breaking change for users of `console-api` and
`console-subscriber`, as it changes the public `tonic` dependency to a
semver-incompatible version. This breaks compatibility with `tonic`
0.9.x and `prost` 0.11.x.

### Added

- [**breaking**](#tokio-console-v0.1.10-breaking) Update Tonic and Prost dependencies ([#364](#364)) ([f9b8e03](f9b8e03))
- Only suggest opening issues for panics ([#365](#365)) ([da2a89c](da2a89c))
- Init error handling before subcmds ([#365](#365)) ([ec66eda](ec66eda))
- Filter out boring frames in backtraces ([#365](#365)) ([95a5e54](95a5e54))
- Include config options in autogenerated issues ([#365](#365)) ([3244a1f](3244a1f))
- Reduce decimal digits in UI ([#402](#402)) ([c13085e](c13085e))
- Use tokio task ids in task views ([#403](#403)) ([f5b06d2](f5b06d2))
- Add support for Unix domain sockets ([#388](#388)) ([a944dbc](a944dbc), closes [#296](#296))
- Add scheduled time per task ([#406](#406)) ([f280df9](f280df9))
- Add task scheduled times histogram ([#409](#409)) ([d92a399](d92a399))
- Update `tonic` to 0.9 ([#420](#420)) ([48af1ee](48af1ee))
- Update MSRV to Rust 1.60.0 ([b18ee47](b18ee47))
- Migrate to `ratatui` and update `crossterm` ([#425](#425)) ([b209dd6](b209dd6))
- Help view modal ([#432](#432)) ([359a4e7](359a4e7))
- Add way to inspect details of task from resource view ([#449](#449)) ([132ed4e](132ed4e), closes [#448](#448))
- Add warning for tasks that never yield ([#439](#439)) ([d05fa9e](d05fa9e))
- [**breaking**](#tokio-console-v0.1.10-breaking) Update `tonic` to v0.10 and increase MSRV to 1.64 ([#464](#464)) ([96e62c8](96e62c8))

### Documented

- Update screenshots in README ([#419](#419)) ([e9bcd67](e9bcd67))
- Revert "update screenshots in README ([#419](#419))" ([993a3d9](993a3d9))
- Update screenshots in README ([#421](#421)) ([8a27f96](8a27f96))
- Add column descriptions for all tables ([#431](#431)) ([e3cf82b](e3cf82b))
- Update MSRV version docs to 1.64 ([#467](#467)) ([94a5a51](94a5a51))

### Fixed

- Fix ascii-only flipped input ([#377](#377)) ([652ac34](652ac34))
- Declare `tokio-console` bin as `default-run` ([#379](#379)) ([9ce60ec](9ce60ec))
- Make `retain_for` default to 6s if not specfied ([#383](#383)) ([0a6012b](0a6012b), fixes [#382](#382))
- Enable view-switching keystrokes on details views ([#387](#387)) ([f417d7a](f417d7a))
- Fix `ViewOptions` default lang' ([#394](#394)) ([a1cf1b8](a1cf1b8), fixes [#393](#393))
- Remove `tracing-subscriber` 0.2 from dependencies ([#404](#404)) ([768534a](768534a))
- Fix calculation of busy time during poll ([#405](#405)) ([e2c536a](e2c536a))
- Remove histogram minimum count ([#424](#424)) ([02cf8a6](02cf8a6))
- Remove trailing space from task/resource location ([#443](#443)) ([90e5918](90e5918))
- Make long locations readable ([#441](#441)) ([9428d7f](9428d7f), closes [#411](#411))
- Fix task detail view Id to display remote tokio::task::Id ([#455](#455)) ([70c3952](70c3952))

Signed-off-by: Eliza Weisman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task IDs not stable
2 participants