From 4ec26a8d535674c7906d6e42c2633d4f24a524a5 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 7 Jan 2022 09:24:30 -0800 Subject: [PATCH] fix(console): don't make details requests with rewritten IDs (#251) ## 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. --- console/src/main.rs | 1 + console/src/state/async_ops.rs | 23 ++++++++++--------- console/src/state/resources.rs | 41 ++++++++++++++++++++++------------ console/src/state/tasks.rs | 39 +++++++++++++++++++++----------- console/src/view/mod.rs | 4 ++-- 5 files changed, 68 insertions(+), 40 deletions(-) diff --git a/console/src/main.rs b/console/src/main.rs index 8037f55d9..e41241d7c 100644 --- 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 index 58da6b7ba..ee2582bee 100644 --- 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>>) { 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 index e0fcee495..f5d8a959b 100644 --- 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>>) { 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 index 0c4839baf..c19e091d3 100644 --- a/console/src/state/tasks.rs +++ b/console/src/state/tasks.rs @@ -56,7 +56,15 @@ pub(crate) type TaskRef = Weak>; #[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>>, stats: TaskStats, @@ -150,22 +158,23 @@ impl TasksState { .collect::>(); 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>>) { 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 index 7a0573874..aa82e9e07 100644 --- 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)); } }