fix(console): don't make details requests with rewritten IDs #251

hawkw · 2022-01-06T21:33:16Z

PR #244 moved the rewriting of tracing span IDs to sequential
low-number IDs from the console-subscriber crate to the console CLI.
However, this introduced a bug with task details, because that PR didn't
change the part of the console that makes watch_details RPCs to use
the tracing span ID recieved from the remote --- it continued to use
the Task and Resource structs' id fields. These are now different
from the tracing ID recieved from the remote, because they're
rewritten in the console CLI. This means that watch_details RPCs would
generally recieve an error, or (in the off-chance that a rewritten ID
collides with a low-numbered tracing ID) the details for anoter task
or resource.

This branch fixes the bug by changing the Task and Resource structs
to store both the span ID received from the remote and the rewritten
pretty ID. This way, when we make watch_details RPC calls, we do it
with the span ID we received from the remote process.

I also changed some naming to make the distinction between rewritten IDs
and span IDs clearer.

PR #244 moved the rewriting of `tracing` span IDs to sequential low-number IDs from the `console-subscriber` crate to the console CLI. However, this introduced a bug with task details, because that PR didn't change the part of the console that makes `watch_details` RPCs to use the `tracing` span ID recieved from the remote --- it continued to use the `Task` and `Resource` structs' `id` fields. These are now different from the `tracing` ID recieved from the remote, because they're rewritten in the console CLI. This means that `watch_details` RPCs would generally recieve an error, or (in the off-chance that a rewritten ID collides with a low-numbered `tracing` ID) the details for anoter task or resource. This branch fixes the bug by changing the `Task` and `Resource` structs to store both the span ID received from the remote _and_ the rewritten pretty ID. This way, when we make `watch_details` RPC calls, we do it with the span ID we received from the remote process. I also changed some naming to make the distinction between rewritten IDs and span IDs clearer.

zaharidichev

LGTM

zaharidichev · 2022-01-07T13:24:28Z

console/src/state/resources.rs

+    /// The resource's pretty (console-generated, sequential) ID.
+    ///
+    /// This is NOT the `tracing::span::Id` for the resource's `tracing` span on the
+    /// remote.
+    num: u64,
+    /// The `tracing::span::Id` on the remote process for this resource's span.
+    ///
+    /// This is used when requesting a resource details stream.
+    span_id: u64,


maybe we can rename it to just pretty_id or display_id?

i'm also fine with a name like that; i went with num because they're sequential numbers, but I'd be happy to change it.

zaharidichev

LGTM

hawkw · 2022-01-07T17:18:20Z

A follow-up that I didn't address in this branch, but that we probably ought to add, is something that's displayed in the UI when we can't get details for a task. Currently, when a watch_details RPC returns an error, we log it...but logs from the console aren't visible in the UI and can only be viewed if the user redirects them to a file. So, we probably want to add something in the UI showing that there was an error.

<a name=""></a> ## (2022-01-18) #### Features * feature-flag `tracing-journald` dependency (#250) ([24f25db](24f25db)) * add vi style keybinds for tables (#223) ([1845c99](1845c99)) #### Bug Fixes * fix task lookup in async ops view (#257) ([9a50b63](9a50b63)) * don't make details requests with rewritten IDs (#251) ([4ec26a8](4ec26a8)) * fix build error with journald enabled ([a931b7e](a931b7e)) * increase default event buffer capacity a bit (#235) ([0cf0aee](0cf0aee)) * wrap controls line when the terminal is too narrow (#231) ([ef41507](ef41507)) * don't enable crossterm mouse capture (#222) ([e020d66](e020d66), closes [#167](167))

The console TUI's state model deals with two different types of IDs: - the actual `tracing` span IDs for an object (task, resource, or async op), which are *not* assigned sequentially, and may be reused over the program's lifetime, and - sequential "pretty" IDs assigned by the console, which are mapped to span IDs when an object is sent to the console. These are assigned based on separate counters for each type of object (so there can be both a task 1 and a resource 1, for example). Remote span IDs are mapped to rewritten sequential IDs by the `Id` type, which stores a map of span IDs to sequential IDs, and generates new sequential IDs when a new span ID is recorded. The `Ids::id_for` method takes a span ID and returns the corresponding sequential ID, and this must be called before looking up or inserting an object by its remote span ID. Currently, *all* of these IDs are represented as `u64`s. This is unfortunate, as it makes it very easy to accidentally introduce bugs where the wrong kind of ID is used. For example, it's possible to accidentally look up a task in the map of active tasks based on its span ID on the remote application, which is likely to return `None` even if there is a task with that span ID. PR #251 fixed one such ID-confusion issue (where `WatchDetails` RPCs were performed with local, rewritten task IDs rather than the remote's span ID for that task). However, it would be better if we could just *not have* this class of issue. This branch refactors the console's `state` module to make ID confusion issues much harder to write. This is accomplished by adding an `Id` newtype to represent our rewritten sequential IDs. `Ids::id_for` now still takes a `u64` (as the remote span IDs are represented as `u64`s in protobuf, so there's nothing we can do), but it returns an `Id` newtype. Looking up or inserting objects in a state map now takes the `Id` newtype. This ensures that all lookups are performed with sequential IDs at the type level, and the only way to get a sequential ID is to ask the `Ids` map for one. Additionally, the `Id` type has a type parameter for the type of object it identifies. This prevents additional issues where we might look up the ID of a task in the map of resources, or similar.

hawkw requested review from seanmonstar and zaharidichev January 6, 2022 21:33

zaharidichev approved these changes Jan 7, 2022

View reviewed changes

hawkw mentioned this pull request Jan 7, 2022

console: surface WatchDetails errors in the UI #253

Open

hawkw merged commit 4ec26a8 into main Jan 7, 2022

hawkw deleted the eliza/fix-details branch January 7, 2022 17:24

hawkw mentioned this pull request Jan 18, 2022

console: prepare to release v0.1.1 #261

Merged

hawkw mentioned this pull request Jan 19, 2022

Histogram empty with tokio-console 0.1.1 #262

Closed

hawkw mentioned this pull request Jul 5, 2022

refac(console): use a newtype to separate ID types #358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(console): don't make details requests with rewritten IDs #251

fix(console): don't make details requests with rewritten IDs #251

hawkw commented Jan 6, 2022

zaharidichev left a comment

zaharidichev Jan 7, 2022

hawkw Jan 7, 2022

zaharidichev left a comment

hawkw commented Jan 7, 2022

fix(console): don't make details requests with rewritten IDs #251

fix(console): don't make details requests with rewritten IDs #251

Conversation

hawkw commented Jan 6, 2022

zaharidichev left a comment

Choose a reason for hiding this comment

zaharidichev Jan 7, 2022

Choose a reason for hiding this comment

hawkw Jan 7, 2022

Choose a reason for hiding this comment

zaharidichev left a comment

Choose a reason for hiding this comment

hawkw commented Jan 7, 2022