console: bold field names #35

zaharidichev · 2021-05-27T17:50:06Z

This PR delays the formatting of the fields and instead
stores them on the Task struct. This allows for nicer formatting
options. Currently we just bold the field names in the task tables view.

fix #33

Signed-off-by: Zahari Dichev zaharidichev@gmail.com

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

hawkw

overall, this looks pretty good, although I had some suggestions you may want to address.

a number of these are things we could also change in follow-up PRs if you prefer --- feel free to merge this now if you'd rather address some of those suggestions in a follow-up, or let me know once you've updated this branch?

hawkw · 2021-05-27T18:08:24Z

console-api/src/common.rs

-impl fmt::Display for field::Value {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            field::Value::BoolVal(v) => fmt::Display::fmt(v, f)?,
-            field::Value::StrVal(v) => fmt::Display::fmt(v, f)?,
-            field::Value::U64Val(v) => fmt::Display::fmt(v, f)?,
-            field::Value::DebugVal(v) => fmt::Display::fmt(v, f)?,
-            field::Value::I64Val(v) => fmt::Display::fmt(v, f)?,
-        }
-
-        Ok(())
-    }
-}
-
-impl fmt::Display for Field {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let name_val = (self.name.as_ref(), self.value.as_ref());
-        if let (Some(field::Name::StrName(name)), Some(val)) = name_val {
-            write!(f, "{}={}", name, val)?;
-        }
-
-        Ok(())
-    }
-}


I think it's probably worth keeping these in the console-api crate, even if we're not currently using them in the console, but TIOLI.

hawkw · 2021-05-27T18:12:39Z

console/src/tasks.rs

+        if let Some(new_metada) = update.new_metadata {
+            let metas: HashMap<_, Metadata> = new_metada


nit: i'd prefer either

Suggested change

if let Some(new_metada) = update.new_metadata {

let metas: HashMap<_, Metadata> = new_metada

if let Some(new_metadata) = update.new_metadata {

let metas: HashMap<_, Metadata> = new_metadata

or

Suggested change

if let Some(new_metada) = update.new_metadata {

let metas: HashMap<_, Metadata> = new_metada

if let Some(new_meta) = update.new_metadata {

let metas: HashMap<_, Metadata> = new_meta

as i feel like metadata is rarely, if ever, abbreviated as "metada"...

hawkw · 2021-05-27T18:15:37Z

console/src/tasks.rs

+            let metas: HashMap<_, Metadata> = new_metada
+                .metadata
+                .into_iter()
+                .map(|meta| {
+                    let id = meta.id.expect("no id").id;
+                    let metadata = meta.metadata.expect("no metadata");
+                    (id, metadata.into())
+                })
+                .collect();
+            self.metas.extend(metas);


nit: the extend method takes a T: IntoIterator, and all types implementing Iterator also implement IntoIterator. so, we don't need to call collect here and could skip allocating an intermediate hashmap --- making the code slightly simpler and removing an unnecessary allocation.

Suggested change

let metas: HashMap<_, Metadata> = new_metada

.metadata

.into_iter()

.map(|meta| {

let id = meta.id.expect("no id").id;

let metadata = meta.metadata.expect("no metadata");

(id, metadata.into())

})

.collect();

self.metas.extend(metas);

let metas = new_metada

.metadata

.into_iter()

.map(|meta| {

let id = meta.id.expect("no id").id;

let metadata = meta.metadata.expect("no metadata");

(id, metadata.into())

});

self.metas.extend(metas);

hawkw · 2021-05-27T18:18:07Z

console/src/tasks.rs

+                            metas
+                                .get(&meta_id.id)
+                                .and_then(|meta| meta.field_names.get(*idx as usize))
+                                .cloned()


it might be worth representing the metadata's field names as Arc<str>s, so that they are cheap to clone --- once the subscriber populates the metadata field names, we can expect most spans to have field names stored as indices rather than strings, so we could significantly reduce the number of strings the console allocates if we only stored a single copy of those strings in the metadata, and cloned an Arc into the task's fields.

it would be fine to do this in a follow-up PR, though --- this is fine for now.

hawkw · 2021-05-27T18:21:22Z

console/src/tasks.rs

                .iter()
-                .fold(String::new(), |mut res, f| {
-                    write!(&mut res, "{} ", f).unwrap();
-                    res
+                .filter_map(|f| {
+                    let field_name = f.name.as_ref().expect("no name");
+                    let name = match field_name {
+                        proto::field::Name::StrName(n) => Some(n.clone()),
+                        proto::field::Name::NameIdx(idx) => {
+                            let meta_id = f.metadata_id.as_ref().expect("no id");
+                            metas
+                                .get(&meta_id.id)
+                                .and_then(|meta| meta.field_names.get(*idx as usize))
+                                .cloned()
+                        }
+                    };
+                    let value = f.value.as_ref().expect("no value").clone().into();
+                    name.map(|name| Field { name, value })


we never reuse the protobuf Task's fields, so we shouldn't have to clone them here. if we moved the fields by value, we could write something like this instead:

let fields = task .fields .drain(..) .filter_map(|f| { let field_name = f.name.expect("no name"); let name = match field_name { proto::field::Name::StrName(n) => Some(n), proto::field::Name::NameIdx(idx) => { let meta_id = f.metadata_id.as_ref().expect("no id"); metas .get(&meta_id.id) .and_then(|meta| meta.field_names.get(*idx as usize)) .cloned() } }; let value = f.value.expect("no value").into(); name.map(|name| Field { name, value }) }) .collect();

hawkw · 2021-05-27T18:23:57Z

console/src/tasks.rs

+            let fields = Spans::from(task.fields().iter().fold(Vec::default(), |mut acc, f| {
+                acc.extend(vec![
+                    Span::styled(
+                        f.name.clone(),
+                        Style::default().add_modifier(Modifier::BOLD),
+                    ),
+                    Span::from("="),
+                    Span::from(format!("{} ", f.value)),
+                ]);
+                acc
+            }));


hmm, this change is actually a performance hit, because we are now formatting the fields again every time we update the view, rather than just once.

it might be worth having the Task struct store the formatted representation as well as the structured representation (which we would use for things like sorting/filtering by fields). we could have some logic for determining when to re-format the fields if the task span records new fields (which currently won't happen, but we may add in the future).

again, we don't have to do that in this PR, but it might be nice to do in the future as an optimization?

hawkw · 2021-05-27T18:24:43Z

console/src/view/task.rs

+        let fields: String = task
+            .fields()
+            .iter()
+            .fold(String::new(), |mut res, f| {
+                write!(&mut res, "{}={} ", f.name, f.value).unwrap();
+                res
+            })
+            .trim_end()
+            .into();


it would be nice to do the same formatting here as we do in the tasks list view? but again, we can add that in a follow-up if you prefer.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev · 2021-05-31T13:27:07Z

console/src/tasks.rs

@@ -39,12 +41,19 @@ enum SortBy {
 pub(crate) struct Task {
    id: u64,
    id_hex: String,
-    fields: String,
+    fields: Vec<Field>,
+    formatted_fields: Vec<Span<'static>>,


is that 'static correct.. 😕

console/src/tasks.rs

bold field names

86d1a37

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

hawkw approved these changes May 27, 2021

View reviewed changes

hawkw and others added 2 commits May 27, 2021 11:36

Merge branch 'main' into zd/fields-on-task-struct

f1dc5bc

feedback

81771ad

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev commented May 31, 2021

View reviewed changes

hawkw approved these changes Jun 11, 2021

View reviewed changes

console/src/tasks.rs Outdated Show resolved Hide resolved

console/src/tasks.rs Outdated Show resolved Hide resolved

console/src/tasks.rs Outdated Show resolved Hide resolved

hawkw added 2 commits June 14, 2021 11:00

Apply suggestions from code review

c4e863a

Merge branch 'main' into zd/fields-on-task-struct

901d60e

hawkw merged commit 8e36e17 into tokio-rs:main Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console: bold field names #35

console: bold field names #35

zaharidichev commented May 27, 2021

hawkw left a comment

hawkw May 27, 2021

hawkw May 27, 2021

hawkw May 27, 2021

hawkw May 27, 2021

hawkw May 27, 2021

hawkw May 27, 2021

hawkw May 27, 2021

zaharidichev May 31, 2021

		if let Some(new_metada) = update.new_metadata {
		let metas: HashMap<_, Metadata> = new_metada

console: bold field names #35

console: bold field names #35

Conversation

zaharidichev commented May 27, 2021

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment