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

[hds]

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]

@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 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.

[hawkw]

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.

[hawkw]

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?

[hds]

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?

[hawkw]

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

[hds]

OK, got it. I understand now.

[hds]

@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]

+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.

[hawkw]

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

Suggested change

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

[hds]

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.

[hawkw]

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