Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console: bold field names #35

Merged
merged 5 commits into from
Jun 14, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions console-api/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,31 +62,6 @@ impl<'a> From<&'a std::panic::Location<'a>> for Location {
}
}

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(())
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


impl fmt::Display for Location {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match (self.module_path.as_ref(), self.file.as_ref()) {
Expand Down
118 changes: 107 additions & 11 deletions console/src/tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,20 @@ use std::{
cell::RefCell,
collections::HashMap,
convert::TryFrom,
fmt::Write,
fmt,
rc::{Rc, Weak},
time::{Duration, SystemTime},
};
use tui::{
layout,
style::{self, Style},
text,
style::{self, Modifier, Style},
text::{self, Span, Spans},
widgets::{Block, Cell, Row, Table, TableState},
};
#[derive(Debug)]
pub(crate) struct State {
tasks: HashMap<u64, Rc<RefCell<Task>>>,
metas: HashMap<u64, Metadata>,
sorted_tasks: Vec<Weak<RefCell<Task>>>,
sort_by: SortBy,
table_state: TableState,
Expand All @@ -39,12 +40,18 @@ enum SortBy {
pub(crate) struct Task {
id: u64,
id_hex: String,
fields: String,
fields: Vec<Field>,
kind: &'static str,
stats: Stats,
completed_for: usize,
}

#[derive(Debug)]
pub(crate) struct Metadata {
field_names: Vec<String>,
//TODO: add more metadata as needed
}

#[derive(Debug)]
struct Stats {
polls: u64,
Expand All @@ -54,6 +61,21 @@ struct Stats {
total: Option<Duration>,
}

#[derive(Debug)]
pub(crate) struct Field {
pub(crate) name: String,
pub(crate) value: FieldValue,
}

#[derive(Debug)]
pub(crate) enum FieldValue {
Bool(bool),
Str(String),
U64(u64),
I64(i64),
Debug(String),
}

impl State {
// How many updates to retain completed tasks for
const RETAIN_COMPLETED_FOR: usize = 6;
Expand Down Expand Up @@ -114,8 +136,23 @@ impl State {
if let Some(now) = update.now {
self.last_updated_at = Some(now.into());
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"...

.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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

}

let mut stats_update = update.stats_update;
let sorted = &mut self.sorted_tasks;
let metas = &mut self.metas;
let new_tasks = update.new_tasks.into_iter().filter_map(|task| {
if task.id.is_none() {
tracing::warn!(?task, "skipping task with no id");
Expand All @@ -127,12 +164,23 @@ impl State {
let fields = task
.fields
.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");
hawkw marked this conversation as resolved.
Show resolved Hide resolved
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");
hawkw marked this conversation as resolved.
Show resolved Hide resolved
metas
.get(&meta_id.id)
.and_then(|meta| meta.field_names.get(*idx as usize))
.cloned()
Comment on lines +172 to +175
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
};
let value = f.value.as_ref().expect("no value").clone().into();
name.map(|name| Field { name, value })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

})
.trim_end()
.into();
.collect();

let id = task.id?.id;
let stats = stats_update.remove(&id)?.into();
let mut task = Task {
Expand Down Expand Up @@ -182,6 +230,19 @@ impl State {
let rows = self.sorted_tasks.iter().filter_map(|task| {
let task = task.upgrade()?;
let task = task.borrow();

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
}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


let mut row = Row::new(vec![
Cell::from(task.id_hex.to_string()),
// TODO(eliza): is there a way to write a `fmt::Debug` impl
Expand All @@ -206,7 +267,7 @@ impl State {
prec = DUR_PRECISION,
)),
Cell::from(format!("{:>width$}", task.stats.polls, width = POLLS_LEN)),
Cell::from(task.fields.to_string()),
Cell::from(fields),
]);
if task.completed_for > 0 {
row = row.style(Style::default().add_modifier(style::Modifier::DIM));
Expand Down Expand Up @@ -330,6 +391,7 @@ impl Default for State {
fn default() -> Self {
Self {
tasks: Default::default(),
metas: Default::default(),
sorted_tasks: Default::default(),
sort_by: Default::default(),
selected_column: SortBy::default() as usize,
Expand All @@ -345,7 +407,7 @@ impl Task {
&self.id_hex
}

pub(crate) fn fields(&self) -> &str {
pub(crate) fn fields(&self) -> &Vec<Field> {
&self.fields
}

Expand Down Expand Up @@ -397,6 +459,26 @@ impl From<proto::tasks::Stats> for Stats {
}
}

impl From<proto::Metadata> for Metadata {
fn from(pb: proto::Metadata) -> Self {
Self {
field_names: pb.field_names,
}
}
}

impl From<proto::field::Value> for FieldValue {
fn from(pb: proto::field::Value) -> Self {
match pb {
proto::field::Value::BoolVal(v) => Self::Bool(v),
proto::field::Value::StrVal(v) => Self::Str(v),
proto::field::Value::I64Val(v) => Self::I64(v),
proto::field::Value::U64Val(v) => Self::U64(v),
proto::field::Value::DebugVal(v) => Self::Debug(v),
}
}
}

impl Default for SortBy {
fn default() -> Self {
Self::Total
Expand Down Expand Up @@ -437,3 +519,17 @@ impl TryFrom<usize> for SortBy {
}
}
}

impl fmt::Display for FieldValue {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
FieldValue::Bool(v) => fmt::Display::fmt(v, f)?,
FieldValue::Str(v) => fmt::Display::fmt(v, f)?,
FieldValue::U64(v) => fmt::Display::fmt(v, f)?,
FieldValue::Debug(v) => fmt::Display::fmt(v, f)?,
FieldValue::I64(v) => fmt::Display::fmt(v, f)?,
}

Ok(())
}
}
14 changes: 12 additions & 2 deletions console/src/view/task.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::{input, tasks::Task};
use std::{cell::RefCell, rc::Rc, time::SystemTime};
use std::{cell::RefCell, fmt::Write, rc::Rc, time::SystemTime};
use tui::{
layout,
style::{self, Style},
Expand Down Expand Up @@ -35,6 +35,16 @@ impl TaskView {
let task = &*self.task.borrow();
const DUR_PRECISION: usize = 4;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


let attrs = Spans::from(vec![
Span::styled("ID: ", Style::default().add_modifier(style::Modifier::BOLD)),
Span::raw(task.id_hex()),
Expand All @@ -43,7 +53,7 @@ impl TaskView {
"Fields: ",
Style::default().add_modifier(style::Modifier::BOLD),
),
Span::raw(task.fields()),
Span::raw(fields),
]);

let metrics = Spans::from(vec![
Expand Down