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

fix(console): reduce decimal digits in UI #402

Merged
merged 5 commits into from
Mar 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
9 changes: 2 additions & 7 deletions tokio-console/src/view/async_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
view::{
self, bold,
table::{self, TableList, TableListState},
DUR_LEN, DUR_PRECISION,
DUR_LEN, DUR_TABLE_PRECISION,
},
};

Expand Down Expand Up @@ -106,12 +106,7 @@ impl TableList<9> for AsyncOpsTable {
let mut polls_width = view::Width::new(Self::WIDTHS[7] as u16);

let dur_cell = |dur: std::time::Duration| -> Cell<'static> {
Cell::from(styles.time_units(format!(
"{:>width$.prec$?}",
dur,
width = DUR_LEN,
prec = DUR_PRECISION,
)))
Cell::from(styles.time_units(dur, DUR_TABLE_PRECISION, Some(DUR_LEN)))
};

let rows = {
Expand Down
5 changes: 3 additions & 2 deletions tokio-console/src/view/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ mod tasks;
pub(crate) use self::styles::{Palette, Styles};
pub(crate) use self::table::SortBy;

const DUR_LEN: usize = 10;
const DUR_LEN: usize = 6;
// This data is only updated every second, so it doesn't make a ton of
// sense to have a lot of precision in timestamps (and this makes sure
// there's room for the unit!)
const DUR_PRECISION: usize = 4;
const DUR_LIST_PRECISION: usize = 2;
const DUR_TABLE_PRECISION: usize = 0;
const TABLE_HIGHLIGHT_SYMBOL: &str = ">> ";

pub struct View {
Expand Down
11 changes: 5 additions & 6 deletions tokio-console/src/view/resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
view::{
self, bold,
table::{self, TableList, TableListState},
DUR_LEN, DUR_PRECISION,
DUR_LEN, DUR_TABLE_PRECISION,
},
};

Expand Down Expand Up @@ -104,12 +104,11 @@ impl TableList<9> for ResourcesTable {
))),
Cell::from(parent_width.update_str(resource.parent_id()).to_owned()),
Cell::from(kind_width.update_str(resource.kind()).to_owned()),
Cell::from(styles.time_units(format!(
"{:>width$.prec$?}",
Cell::from(styles.time_units(
resource.total(now),
width = DUR_LEN,
prec = DUR_PRECISION,
))),
DUR_TABLE_PRECISION,
Some(DUR_LEN),
)),
Cell::from(target_width.update_str(resource.target()).to_owned()),
Cell::from(type_width.update_str(resource.concrete_type()).to_owned()),
Cell::from(resource.type_visibility().render(styles)),
Expand Down
141 changes: 116 additions & 25 deletions tokio-console/src/view/styles.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::config;
use serde::{Deserialize, Serialize};
use std::{borrow::Cow, str::FromStr};
use std::{str::FromStr, time::Duration};
use tui::{
style::{Color, Modifier, Style},
text::Span,
Expand Down Expand Up @@ -32,11 +32,41 @@ pub enum Palette {
All,
}

/// Represents formatted time spans.
///
/// Distinguishing between different units allows appropriate colouring.
enum FormattedDuration {
hawkw marked this conversation as resolved.
Show resolved Hide resolved
/// Days (and no minor unit), e.g. `102d`
Days(String),
/// Days with hours, e.g. `12d03h`
DaysHours(String),
/// Hours with minutes, e.g. `14h32m`
HoursMinutes(String),
/// Minutes with seconds, e.g. `43m02s`
MinutesSeconds(String),
/// The `time::Duration` debug string which uses units ranging from
/// picoseconds (`ps`) to seconds (`s`). May contain decimal digits
/// (e.g. `628.76ms`) or not (e.g. `32ns`)
Debug(String),
}

impl FormattedDuration {
fn into_inner(self) -> String {
match self {
Self::Days(inner) => inner,
Self::DaysHours(inner) => inner,
Self::HoursMinutes(inner) => inner,
Self::MinutesSeconds(inner) => inner,
Self::Debug(inner) => inner,
}
}
}

fn fg_style(color: Color) -> Style {
Style::default().fg(color)
}

// === impl Config ===
// === impl Styles ===

impl Styles {
pub fn from_config(config: config::ViewOptions) -> Self {
Expand Down Expand Up @@ -126,39 +156,100 @@ impl Styles {
}
}

pub fn time_units<'a>(&self, text: impl Into<Cow<'a, str>>) -> Span<'a> {
let mut text = text.into();
if !self.toggles.color_durations() {
return Span::raw(text);
}
/// Creates a span with a formatted duration inside.
///
/// The formatted duration will be colored depending on the palette
/// defined for this `Styles` object.
///
/// If the `width` parameter is `None` then no padding will be
/// added. Otherwise the text in the span will be left-padded to
/// the specified width (right aligned). Passing `Some(0)` is
/// equivalent to `None`.
pub fn time_units<'a>(&self, dur: Duration, prec: usize, width: Option<usize>) -> Span<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

it's probably worth documenting what the width value does here, and what the difference between passing None and Some (and None versus Some(0)) is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Style question here. An Option<usize> seemed like a good idea at the time, but None and Some(0) are equivalent. Do you have any opinion on the merits of just passing a bare usize instead?

Copy link
Member

Choose a reason for hiding this comment

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

You could also use an Option<NonZeroUsize> i suppose (which has the same representation as a single usize, with the actual zero value representing None)...but this is an internal API, so misuse-resistent API design is less important. I definitely prefer None to indicate "no width limit" than 0, though --- when reading the code you might expect "0 width" to mean "no characters", i guess?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. That sounds like a better option (using NonZero). Internal or no, as you said, None to mean no width restriction makes more sense than 0. I'll document the function too, so that it's clear. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, this is a bit ugly because I now need this line when declaring DUR_LEN or I need to be unwraping in a bunch of places because new returns a Option<NonZeroUsize>.

// SAFETY: 6 is non-zero.
const DUR_LEN: NonZeroUsize = unsafe { NonZeroUsize::new_unchecked(6) };

https://github.com/tokio-rs/console/pull/402/files#diff-295fe639b204aaddc6b81e4c68fbb9bc7f2fb3aef43fbca622e99620dfc7debaR22-R23

Thoughts? Should I just go back to the Option<usize> and explain that Some(0) and None are equivalent?

Copy link
Member

Choose a reason for hiding this comment

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

ugh, that's annoying. i would prefer not to put unsafe code in the const. at the callsite, we could call time_units(..., NonZeroUsize::new(DUR_LEN)) but that's more verbose. i think it would be fine to allow Some(0) and None to mean the same thing, or just get rid of the option...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed it back to Option<usize>.

let formatted = self.duration_text(dur, width.unwrap_or(0), prec);

if !self.utf8 {
if let Some(mu_offset) = text.find("µs") {
text.to_mut().replace_range(mu_offset.., "us");
}
if !self.toggles.color_durations() {
return Span::raw(formatted.into_inner());
}

let style = match self.palette {
Palette::NoColors => return Span::raw(text),
Palette::Ansi8 | Palette::Ansi16 => match text.as_ref() {
s if s.ends_with("ps") => fg_style(Color::Blue),
s if s.ends_with("ns") => fg_style(Color::Green),
s if s.ends_with("µs") || s.ends_with("us") => fg_style(Color::Yellow),
s if s.ends_with("ms") => fg_style(Color::Red),
s if s.ends_with('s') => fg_style(Color::Magenta),
Palette::NoColors => return Span::raw(formatted.into_inner()),
Palette::Ansi8 | Palette::Ansi16 => match &formatted {
FormattedDuration::Days(_) => fg_style(Color::Blue),
FormattedDuration::DaysHours(_) => fg_style(Color::Blue),
FormattedDuration::HoursMinutes(_) => fg_style(Color::Cyan),
FormattedDuration::MinutesSeconds(_) => fg_style(Color::Green),
FormattedDuration::Debug(s) if s.ends_with("ps") => fg_style(Color::Gray),
FormattedDuration::Debug(s) if s.ends_with("ns") => fg_style(Color::Gray),
FormattedDuration::Debug(s) if s.ends_with("µs") || s.ends_with("us") => {
fg_style(Color::Magenta)
}
FormattedDuration::Debug(s) if s.ends_with("ms") => fg_style(Color::Red),
FormattedDuration::Debug(s) if s.ends_with('s') => fg_style(Color::Yellow),
_ => Style::default(),
},
Palette::Ansi256 | Palette::All => match text.as_ref() {
s if s.ends_with("ps") => fg_style(Color::Indexed(40)), // green 3
s if s.ends_with("ns") => fg_style(Color::Indexed(41)), // spring green 3
s if s.ends_with("µs") || s.ends_with("us") => fg_style(Color::Indexed(42)), // spring green 2
s if s.ends_with("ms") => fg_style(Color::Indexed(43)), // cyan 3
s if s.ends_with('s') => fg_style(Color::Indexed(44)), // dark turquoise,
Palette::Ansi256 | Palette::All => match &formatted {
FormattedDuration::Days(_) => fg_style(Color::Indexed(33)), // dodger blue 1
FormattedDuration::DaysHours(_) => fg_style(Color::Indexed(33)), // dodger blue 1
FormattedDuration::HoursMinutes(_) => fg_style(Color::Indexed(39)), // deep sky blue 1
FormattedDuration::MinutesSeconds(_) => fg_style(Color::Indexed(45)), // turquoise 2
FormattedDuration::Debug(s) if s.ends_with("ps") => fg_style(Color::Indexed(40)), // green 3
FormattedDuration::Debug(s) if s.ends_with("ns") => fg_style(Color::Indexed(41)), // spring green 3
FormattedDuration::Debug(s) if s.ends_with("µs") || s.ends_with("us") => {
fg_style(Color::Indexed(42))
} // spring green 2
FormattedDuration::Debug(s) if s.ends_with("ms") => fg_style(Color::Indexed(43)), // cyan 3
FormattedDuration::Debug(s) if s.ends_with('s') => fg_style(Color::Indexed(44)), // dark turquoise,
_ => Style::default(),
},
};

Span::styled(text, style)
Span::styled(formatted.into_inner(), style)
}

fn duration_text(&self, dur: Duration, width: usize, prec: usize) -> FormattedDuration {
let secs = dur.as_secs();

if secs >= 60 * 60 * 24 * 100 {
let days = secs / (60 * 60 * 24);
FormattedDuration::Days(format!("{days:>width$}d", days = days, width = width))
} else if secs >= 60 * 60 * 24 {
let hours = secs / (60 * 60);
FormattedDuration::DaysHours(format!(
"{days:>leading_width$}d{hours:02.0}h",
days = hours / 24,
hours = hours % 24,
// Subtract the known 4 characters that trail the days value.
leading_width = width.saturating_sub(4),
))
} else if secs >= 60 * 60 {
let mins = secs / 60;
FormattedDuration::HoursMinutes(format!(
"{hours:>leading_width$}h{minutes:02.0}m",
hours = mins / 60,
minutes = mins % 60,
// Subtract the known 4 characters that trail the hours value.
leading_width = width.saturating_sub(4),
))
} else if secs >= 60 {
FormattedDuration::MinutesSeconds(format!(
"{minutes:>leading_width$}m{seconds:02.0}s",
minutes = secs / 60,
seconds = secs % 60,
// Subtract the known 4 characters that trail the minutes value.
leading_width = width.saturating_sub(4),
))
} else {
let mut text = format!("{:>width$.prec$?}", dur, width = width, prec = prec);

if !self.utf8 {
if let Some(mu_offset) = text.find("µs") {
text.replace_range(mu_offset.., "us");
}
}

FormattedDuration::Debug(text)
}
}

pub fn terminated(&self) -> Style {
Expand Down
31 changes: 15 additions & 16 deletions tokio-console/src/view/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
view::{
self, bold,
mini_histogram::{HistogramMetadata, MiniHistogram},
DUR_LIST_PRECISION,
},
};
use std::{
Expand Down Expand Up @@ -329,29 +330,27 @@ impl Details {
fn make_percentiles_widget(&self, styles: &view::Styles) -> Text<'static> {
let mut text = Text::default();
let histogram = self.poll_times_histogram();
let percentiles =
histogram
.iter()
.flat_map(|&DurationHistogram { ref histogram, .. }| {
let pairs = [10f64, 25f64, 50f64, 75f64, 90f64, 95f64, 99f64]
.iter()
.map(move |i| (*i, histogram.value_at_percentile(*i)));
pairs.map(|pair| {
Spans::from(vec![
bold(format!("p{:>2}: ", pair.0)),
dur(styles, Duration::from_nanos(pair.1)),
])
})
});
let percentiles = histogram
.iter()
.flat_map(|&DurationHistogram { histogram, .. }| {
let pairs = [10f64, 25f64, 50f64, 75f64, 90f64, 95f64, 99f64]
.iter()
.map(move |i| (*i, histogram.value_at_percentile(*i)));
pairs.map(|pair| {
Spans::from(vec![
bold(format!("p{:>2}: ", pair.0)),
dur(styles, Duration::from_nanos(pair.1)),
])
})
});
text.extend(percentiles);
text
}
}

fn dur(styles: &view::Styles, dur: std::time::Duration) -> Span<'static> {
const DUR_PRECISION: usize = 4;
// TODO(eliza): can we not have to use `format!` to make a string here? is
// there a way to just give TUI a `fmt::Debug` implementation, or does it
// have to be given a string in order to do layout stuff?
styles.time_units(format!("{:.prec$?}", dur, prec = DUR_PRECISION))
styles.time_units(dur, DUR_LIST_PRECISION, None)
}
9 changes: 2 additions & 7 deletions tokio-console/src/view/tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
view::{
self, bold,
table::{self, TableList, TableListState},
DUR_LEN, DUR_PRECISION,
DUR_LEN, DUR_TABLE_PRECISION,
},
};
use tui::{
Expand Down Expand Up @@ -68,12 +68,7 @@ impl TableList<11> for TasksTable {
.sort(now, &mut table_list_state.sorted_items);

let dur_cell = |dur: std::time::Duration| -> Cell<'static> {
Cell::from(styles.time_units(format!(
"{:>width$.prec$?}",
dur,
width = DUR_LEN,
prec = DUR_PRECISION,
)))
Cell::from(styles.time_units(dur, DUR_TABLE_PRECISION, Some(DUR_LEN)))
};

// Start out wide enough to display the column headers...
Expand Down