fix(console): fix column sorting in resources tab (#488)

In the Resources tab, columns names were being associates to the wrong
sorting keys, and some column names were just missing from the `SortBy`
implementation.

All columns have been associated to the corresponding sorting keys, and
for the `Location` column, a structured type has been stored in the
resource object, in order to sort lexicographically on the path and with
conventional numeric ordering on row and column. Also, sorting for
attributes as been implemented by sorting lexicographically on the first
attribute of each resource row, even if that is probably sub-optimal

Author: Matteo Biggio

tokio-console/src/state/mod.rs

@@ -522,19 +522,54 @@ fn truncate_registry_path(s: String) -> String {
     };
 }
 
-fn format_location(loc: Option<proto::Location>) -> String {
-    loc.map(|mut l| {
-        if let Some(file) = l.file.take() {
-            let truncated = truncate_registry_path(file);
-            l.file = Some(truncated);
-        }
-        l.to_string()
-    })
-    .unwrap_or_else(|| "<unknown location>".to_string())
-}
-
 fn pb_duration(dur: prost_types::Duration) -> Duration {
     let secs = u64::try_from(dur.seconds).expect("duration should not be negative!");
     let nanos = u64::try_from(dur.nanos).expect("duration should not be negative!");
     Duration::from_secs(secs) + Duration::from_nanos(nanos)
 }
+
+#[derive(Debug, Default, Clone, Eq, PartialEq, Ord, PartialOrd)]
+pub(crate) struct Location {
+    path: Option<String>,
+    line: Option<u32>,
+    column: Option<u32>,
+}
+
+impl fmt::Display for Location {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self.path.as_ref() {
+            Some(path) => f.write_str(path)?,
+            // If there's no path, then printing the line and
+            // column makes no sense...
+            None => return f.write_str("<unknown location>"),
+        };
+
+        if let Some(line) = self.line {
+            write!(f, ":{}", line)?;
+
+            // Printing the column only makes sense if there's a line...
+            if let Some(column) = self.column {
+                write!(f, ":{}", column)?;
+            }
+        }
+
+        Ok(())
+    }
+}
+
+impl From<proto::Location> for Location {
+    fn from(value: proto::Location) -> Self {
+        let path = match (value.module_path, value.file) {
+            // Module paths take precedence because they're shorter...
+            (Some(module), _) => Some(module),
+            (None, Some(file)) => Some(truncate_registry_path(file)),
+            (None, None) => None,
+        };
+
+        Self {
+            path,
+            column: value.column,
+            line: value.line,
+        }
+    }
+}

tokio-console/src/state/resources.rs

@@ -1,6 +1,5 @@
 use crate::intern::{self, InternedStr};
 use crate::state::{
-    format_location,
     store::{self, Id, SpanId, Store},
     Attribute, Field, Metadata, Visibility,
 };
@@ -14,6 +13,8 @@ use std::{
     time::{Duration, SystemTime},
 };
 
+use super::Location;
+
 #[derive(Default, Debug)]
 pub(crate) struct ResourcesState {
     resources: Store<Resource>,
@@ -29,11 +30,15 @@ pub(crate) enum TypeVisibility {
 #[derive(Debug, Copy, Clone)]
 #[repr(usize)]
 pub(crate) enum SortBy {
-    Rid = 0,
-    Kind = 1,
-    ConcreteType = 2,
-    Target = 3,
-    Total = 4,
+    Id = 0,
+    ParentId = 1,
+    Kind = 2,
+    Total = 3,
+    Target = 4,
+    ConcreteType = 5,
+    Visibility = 6,
+    Location = 7,
+    Attributes = 8,
 }
 
 #[derive(Debug)]
@@ -55,7 +60,7 @@ pub(crate) struct Resource {
     stats: ResourceStats,
     target: InternedStr,
     concrete_type: InternedStr,
-    location: String,
+    location: Location,
     visibility: TypeVisibility,
 }
 
@@ -71,27 +76,50 @@ struct ResourceStats {
 
 impl Default for SortBy {
     fn default() -> Self {
-        Self::Rid
+        Self::Id
     }
 }
 
 impl SortBy {
     pub fn sort(&self, now: SystemTime, resources: &mut [ResourceRef]) {
         match self {
-            Self::Rid => {
+            Self::Id => {
                 resources.sort_unstable_by_key(|resource| resource.upgrade().map(|r| r.borrow().id))
             }
+            Self::ParentId => resources.sort_unstable_by_key(|resource| {
+                resource.upgrade().map(|r| r.borrow().parent_id.clone())
+            }),
             Self::Kind => resources.sort_unstable_by_key(|resource| {
                 resource.upgrade().map(|r| r.borrow().kind.clone())
             }),
+            Self::Total => resources
+                .sort_unstable_by_key(|resource| resource.upgrade().map(|r| r.borrow().total(now))),
+            Self::Target => resources.sort_unstable_by_key(|resource| {
+                resource.upgrade().map(|r| r.borrow().target.clone())
+            }),
             Self::ConcreteType => resources.sort_unstable_by_key(|resource| {
                 resource.upgrade().map(|r| r.borrow().concrete_type.clone())
             }),
-            Self::Target => resources.sort_unstable_by_key(|resource| {
-                resource.upgrade().map(|r| r.borrow().target.clone())
+            Self::Visibility => resources
+                .sort_unstable_by_key(|resource| resource.upgrade().map(|r| r.borrow().visibility)),
+            Self::Location => resources.sort_unstable_by_key(|resource| {
+                resource.upgrade().map(|r| r.borrow().location.clone())
+            }),
+            Self::Attributes => resources.sort_unstable_by_key(|resource| {
+                resource.upgrade().map(|r| {
+                    // FIXME - we are taking only the first span as sorting key here,
+                    // and we are sorting each attribute as a String.
+                    // Instead, attributes should probably be parsed and sorted according to their actual values,
+                    // not just as strings.
+                    r.borrow()
+                        .formatted_attributes()
+                        .iter()
+                        .flatten()
+                        .next()
+                        .cloned()
+                        .map(|s| s.content)
+                })
             }),
-            Self::Total => resources
-                .sort_unstable_by_key(|resource| resource.upgrade().map(|r| r.borrow().total(now))),
         }
     }
 }
@@ -100,11 +128,15 @@ impl TryFrom<usize> for SortBy {
     type Error = ();
     fn try_from(idx: usize) -> Result<Self, Self::Error> {
         match idx {
-            idx if idx == Self::Rid as usize => Ok(Self::Rid),
+            idx if idx == Self::Id as usize => Ok(Self::Id),
+            idx if idx == Self::ParentId as usize => Ok(Self::ParentId),
             idx if idx == Self::Kind as usize => Ok(Self::Kind),
-            idx if idx == Self::ConcreteType as usize => Ok(Self::ConcreteType),
-            idx if idx == Self::Target as usize => Ok(Self::Target),
             idx if idx == Self::Total as usize => Ok(Self::Total),
+            idx if idx == Self::Target as usize => Ok(Self::Target),
+            idx if idx == Self::ConcreteType as usize => Ok(Self::ConcreteType),
+            idx if idx == Self::Visibility as usize => Ok(Self::Visibility),
+            idx if idx == Self::Location as usize => Ok(Self::Location),
+            idx if idx == Self::Attributes as usize => Ok(Self::Attributes),
             _ => Err(()),
         }
     }
@@ -202,7 +234,7 @@ impl ResourcesState {
                         .unwrap_or_else(|| "n/a".to_string()),
                 );
 
-                let location = format_location(resource.location);
+                let location = resource.location.map(|l| l.into()).unwrap_or_default();
                 let visibility = if resource.is_internal {
                     TypeVisibility::Internal
                 } else {
@@ -309,8 +341,8 @@ impl Resource {
         self.stats.total.is_some()
     }
 
-    pub(crate) fn location(&self) -> &str {
-        &self.location
+    pub(crate) fn location(&self) -> String {
+        self.location.to_string()
     }
 }
 

tokio-console/src/state/tasks.rs

@@ -1,7 +1,6 @@
 use crate::{
     intern::{self, InternedStr},
     state::{
-        format_location,
         histogram::DurationHistogram,
         pb_duration,
         store::{self, Id, SpanId, Store},
@@ -21,6 +20,8 @@ use std::{
     time::{Duration, SystemTime},
 };
 
+use super::Location;
+
 #[derive(Default, Debug)]
 pub(crate) struct TasksState {
     tasks: Store<Task>,
@@ -101,7 +102,7 @@ pub(crate) struct Task {
     /// Currently active warnings for this task.
     warnings: Vec<Linter<Task>>,
     /// The source file and line number the task was spawned from
-    location: String,
+    location: Location,
     /// The kind of task, currently one of task, blocking, block_on, local
     kind: InternedStr,
 }
@@ -217,7 +218,7 @@ impl TasksState {
                 let span_id = task.id?.id;
 
                 let stats = stats_update.remove(&span_id)?.into();
-                let location = format_location(task.location);
+                let location = task.location.map(|l| l.into()).unwrap_or_default();
 
                 // remap the server's ID to a pretty, sequential task ID
                 let id = ids.id_for(span_id);
@@ -496,8 +497,8 @@ impl Task {
         }
     }
 
-    pub(crate) fn location(&self) -> &str {
-        &self.location
+    pub(crate) fn location(&self) -> String {
+        self.location.to_string()
     }
 }