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

hawkw · web-flow · commit 4ec26a8d5356 · 2022-01-07T09:24:30.000-08:00
## Motivation 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. ## Solution 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.
diff --git a/console/src/main.rs b/console/src/main.rs
@@ -81,6 +81,7 @@ async fn main() -> color_eyre::Result<()> {
                 let _ = update_tx.send(update_kind);
                 match update_kind {
                     UpdateKind::SelectTask(task_id) => {
+                        tracing::info!(task_id, "starting details watch");
                         match conn.watch_details(task_id).await {
                             Ok(stream) => {
                                 tokio::spawn(watch_details_stream(task_id, stream, update_rx.clone(), details_tx.clone()));
diff --git a/console/src/state/async_ops.rs b/console/src/state/async_ops.rs
@@ -34,7 +34,7 @@ pub(crate) enum SortBy {
 
 #[derive(Debug)]
 pub(crate) struct AsyncOp {
-    id: u64,
+    num: u64,
     parent_id: InternedStr,
     resource_id: u64,
     meta_id: u64,
@@ -69,7 +69,7 @@ impl Default for SortBy {
 impl SortBy {
     pub fn sort(&self, now: SystemTime, ops: &mut Vec<Weak<RefCell<AsyncOp>>>) {
         match self {
-            Self::Aid => ops.sort_unstable_by_key(|ao| ao.upgrade().map(|a| a.borrow().id)),
+            Self::Aid => ops.sort_unstable_by_key(|ao| ao.upgrade().map(|a| a.borrow().num)),
             Self::Task => ops.sort_unstable_by_key(|ao| ao.upgrade().map(|a| a.borrow().task_id())),
             Self::Source => {
                 ops.sort_unstable_by_key(|ao| ao.upgrade().map(|a| a.borrow().source.clone()))
@@ -154,10 +154,11 @@ impl AsyncOpsState {
                 }
             };
 
-            let id = async_op.id?.id;
-            let stats = AsyncOpStats::from_proto(stats_update.remove(&id)?, meta, styles, strings);
+            let span_id = async_op.id?.id;
+            let stats =
+                AsyncOpStats::from_proto(stats_update.remove(&span_id)?, meta, styles, strings);
 
-            let id = self.ids.id_for(id);
+            let num = self.ids.id_for(span_id);
             let resource_id = resource_ids.id_for(async_op.resource_id?.id);
             let parent_id = match async_op.parent_async_op_id {
                 Some(id) => strings.string(format!("{}", self.ids.id_for(id.id))),
@@ -167,7 +168,7 @@ impl AsyncOpsState {
             let source = strings.string(async_op.source);
 
             let async_op = AsyncOp {
-                id,
+                num,
                 parent_id,
                 resource_id,
                 meta_id,
@@ -176,14 +177,14 @@ impl AsyncOpsState {
             };
             let async_op = Rc::new(RefCell::new(async_op));
             new_list.push(Rc::downgrade(&async_op));
-            Some((id, async_op))
+            Some((num, async_op))
         });
 
         self.async_ops.extend(new_async_ops);
 
-        for (id, stats) in stats_update {
-            let id = self.ids.id_for(id);
-            if let Some(async_op) = self.async_ops.get_mut(&id) {
+        for (span_id, stats) in stats_update {
+            let num = self.ids.id_for(span_id);
+            if let Some(async_op) = self.async_ops.get_mut(&num) {
                 let mut async_op = async_op.borrow_mut();
                 if let Some(meta) = metas.get(&async_op.meta_id) {
                     async_op.stats = AsyncOpStats::from_proto(stats, meta, styles, strings);
@@ -210,7 +211,7 @@ impl AsyncOpsState {
 
 impl AsyncOp {
     pub(crate) fn id(&self) -> u64 {
-        self.id
+        self.num
     }
 
     pub(crate) fn parent_id(&self) -> &str {
diff --git a/console/src/state/resources.rs b/console/src/state/resources.rs
@@ -36,7 +36,15 @@ pub(crate) enum SortBy {
 
 #[derive(Debug)]
 pub(crate) struct Resource {
-    id: u64,
+    /// 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,
     id_str: InternedStr,
     parent: InternedStr,
     parent_id: InternedStr,
@@ -68,9 +76,8 @@ impl Default for SortBy {
 impl SortBy {
     pub fn sort(&self, now: SystemTime, resources: &mut Vec<Weak<RefCell<Resource>>>) {
         match self {
-            Self::Rid => {
-                resources.sort_unstable_by_key(|resource| resource.upgrade().map(|r| r.borrow().id))
-            }
+            Self::Rid => resources
+                .sort_unstable_by_key(|resource| resource.upgrade().map(|r| r.borrow().num)),
             Self::Kind => resources.sort_unstable_by_key(|resource| {
                 resource.upgrade().map(|r| r.borrow().kind.clone())
             }),
@@ -166,10 +173,11 @@ impl ResourcesState {
                 }
             };
 
-            let id = resource.id?.id;
-            let stats = ResourceStats::from_proto(stats_update.remove(&id)?, meta, styles, strings);
+            let span_id = resource.id?.id;
+            let stats =
+                ResourceStats::from_proto(stats_update.remove(&span_id)?, meta, styles, strings);
 
-            let id = self.ids.id_for(id);
+            let num = self.ids.id_for(span_id);
             let parent_id = resource.parent_resource_id.map(|id| self.ids.id_for(id.id));
 
             let parent = strings.string(match parent_id {
@@ -199,8 +207,9 @@ impl ResourcesState {
             };
 
             let resource = Resource {
-                id,
-                id_str: strings.string(id.to_string()),
+                num,
+                span_id,
+                id_str: strings.string(num.to_string()),
                 parent,
                 parent_id,
                 kind,
@@ -213,14 +222,14 @@ impl ResourcesState {
             };
             let resource = Rc::new(RefCell::new(resource));
             new_list.push(Rc::downgrade(&resource));
-            Some((id, resource))
+            Some((num, resource))
         });
 
         self.resources.extend(new_resources);
 
-        for (id, stats) in stats_update {
-            let id = self.ids.id_for(id);
-            if let Some(resource) = self.resources.get_mut(&id) {
+        for (span_id, stats) in stats_update {
+            let num = self.ids.id_for(span_id);
+            if let Some(resource) = self.resources.get_mut(&num) {
                 let mut r = resource.borrow_mut();
                 if let Some(meta) = metas.get(&r.meta_id) {
                     r.stats = ResourceStats::from_proto(stats, meta, styles, strings);
@@ -247,7 +256,11 @@ impl ResourcesState {
 
 impl Resource {
     pub(crate) fn id(&self) -> u64 {
-        self.id
+        self.num
+    }
+
+    pub(crate) fn span_id(&self) -> u64 {
+        self.span_id
     }
 
     pub(crate) fn id_str(&self) -> &str {
diff --git a/console/src/state/tasks.rs b/console/src/state/tasks.rs
@@ -56,7 +56,15 @@ pub(crate) type TaskRef = Weak<RefCell<Task>>;
 
 #[derive(Debug)]
 pub(crate) struct Task {
-    id: u64,
+    /// 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.
+    num: u64,
+    /// The `tracing::span::Id` on the remote process for this task's span.
+    ///
+    /// This is used when requesting a task details stream.
+    span_id: u64,
     short_desc: InternedStr,
     formatted_fields: Vec<Vec<Span<'static>>>,
     stats: TaskStats,
@@ -150,22 +158,23 @@ impl TasksState {
                 .collect::<Vec<_>>();
 
             let formatted_fields = Field::make_formatted(styles, &mut fields);
-            let id = task.id?;
+            let span_id = task.id?.id;
 
-            let stats = stats_update.remove(&id.id)?.into();
+            let stats = stats_update.remove(&span_id)?.into();
             let location = format_location(task.location);
 
             // remap the server's ID to a pretty, sequential task ID
-            let id = self.ids.id_for(id.id);
+            let num = self.ids.id_for(span_id);
 
             let short_desc = strings.string(match name.as_ref() {
-                Some(name) => format!("{} ({})", id, name),
-                None => format!("{}", id),
+                Some(name) => format!("{} ({})", num, name),
+                None => format!("{}", num),
             });
 
             let mut task = Task {
                 name,
-                id,
+                num,
+                span_id,
                 short_desc,
                 formatted_fields,
                 stats,
@@ -176,12 +185,12 @@ impl TasksState {
             task.lint(linters);
             let task = Rc::new(RefCell::new(task));
             new_list.push(Rc::downgrade(&task));
-            Some((id, task))
+            Some((num, task))
         });
         self.tasks.extend(new_tasks);
-        for (id, stats) in stats_update {
-            let id = self.ids.id_for(id);
-            if let Some(task) = self.tasks.get_mut(&id) {
+        for (span_id, stats) in stats_update {
+            let num = self.ids.id_for(span_id);
+            if let Some(task) = self.tasks.get_mut(&num) {
                 let mut task = task.borrow_mut();
                 tracing::trace!(?task, "processing stats update for");
                 task.stats = stats.into();
@@ -225,7 +234,11 @@ impl Details {
 
 impl Task {
     pub(crate) fn id(&self) -> u64 {
-        self.id
+        self.num
+    }
+
+    pub(crate) fn span_id(&self) -> u64 {
+        self.span_id
     }
 
     pub(crate) fn target(&self) -> &str {
@@ -407,7 +420,7 @@ impl Default for SortBy {
 impl SortBy {
     pub fn sort(&self, now: SystemTime, tasks: &mut Vec<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().num)),
             Self::Name => {
                 tasks.sort_unstable_by_key(|task| task.upgrade().map(|t| t.borrow().name.clone()))
             }
diff --git a/console/src/view/mod.rs b/console/src/view/mod.rs
@@ -103,7 +103,7 @@ impl View {
                 match event {
                     key!(Enter) => {
                         if let Some(task) = self.tasks_list.selected_item().upgrade() {
-                            update_kind = UpdateKind::SelectTask(task.borrow().id());
+                            update_kind = UpdateKind::SelectTask(task.borrow().span_id());
                             self.state = TaskInstance(self::task::TaskView::new(
                                 task,
                                 state.task_details_ref(),
@@ -123,7 +123,7 @@ impl View {
                 match event {
                     key!(Enter) => {
                         if let Some(res) = self.resources_list.selected_item().upgrade() {
-                            update_kind = UpdateKind::SelectResource(res.borrow().id());
+                            update_kind = UpdateKind::SelectResource(res.borrow().span_id());
                             self.state = ResourceInstance(self::resource::ResourceView::new(res));
                         }
                     }
