diff --git a/console/src/main.rs b/console/src/main.rs index 162ab3d0..862849e0 100644 --- a/console/src/main.rs +++ b/console/src/main.rs @@ -44,7 +44,12 @@ async fn main() -> color_eyre::Result<()> { // A channel to send the task details update stream (no need to keep outdated details in the memory) let (details_tx, mut details_rx) = mpsc::channel::(2); - let mut tasks = State::default(); + let mut tasks = State::default() + // TODO(eliza): allow configuring the list of linters via the + // CLI/possibly a config file? + .with_linters(vec![warnings::Linter::new( + warnings::SelfWakePercent::default(), + )]); let mut input = input::EventStream::new(); let mut view = view::View::new(styles); diff --git a/console/src/tasks.rs b/console/src/tasks.rs index 3fbf5474..ea970a93 100644 --- a/console/src/tasks.rs +++ b/console/src/tasks.rs @@ -1,4 +1,4 @@ -use crate::{util::Percentage, view}; +use crate::{util::Percentage, view, warnings::Linter}; use console_api as proto; use hdrhistogram::Histogram; use std::{ @@ -20,6 +20,7 @@ use tui::{ pub(crate) struct State { tasks: HashMap>>, metas: HashMap, + linters: Vec>, last_updated_at: Option, new_tasks: Vec, current_task_details: DetailsRef, @@ -35,13 +36,14 @@ enum Temporality { #[derive(Debug, Copy, Clone)] #[repr(usize)] pub(crate) enum SortBy { - Tid = 0, - State = 1, - Name = 2, - Total = 3, - Busy = 4, - Idle = 5, - Polls = 6, + Warns = 0, + Tid = 1, + State = 2, + Name = 3, + Total = 4, + Busy = 5, + Idle = 6, + Polls = 7, } #[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] @@ -63,6 +65,8 @@ pub(crate) struct Task { completed_for: usize, target: Arc, name: Option>, + /// Currently active warnings for this task. + warnings: Vec>, } #[derive(Debug, Default)] @@ -123,6 +127,11 @@ pub(crate) enum FieldValue { impl State { const RETAIN_COMPLETED_FOR: usize = 6; + pub(crate) fn with_linters(mut self, linters: impl IntoIterator>) -> Self { + self.linters.extend(linters.into_iter()); + self + } + pub(crate) fn last_updated_at(&self) -> Option { self.last_updated_at } @@ -202,6 +211,7 @@ impl State { stats, completed_for: 0, target: meta.target.clone(), + warnings: Vec::new(), }; task.update(); let task = Rc::new(RefCell::new(task)); @@ -209,12 +219,21 @@ impl State { Some((id, task)) }); self.tasks.extend(new_tasks); - + let linters = &self.linters; for (id, stats) in stats_update { if let Some(task) = self.tasks.get_mut(&id) { - let mut t = task.borrow_mut(); - t.stats = stats.into(); - t.update(); + let mut task = task.borrow_mut(); + tracing::trace!(?task, "processing stats update for"); + task.warnings.clear(); + for lint in linters { + tracing::debug!(?lint, ?task, "checking..."); + if let Some(lint) = lint.check(&*task) { + tracing::info!(?lint, ?task, "found a warning!"); + task.warnings.push(lint) + } + } + task.stats = stats.into(); + task.update(); } } } @@ -272,6 +291,10 @@ impl State { pub(crate) fn is_paused(&self) -> bool { matches!(self.temporality, Temporality::Paused) } + + pub(crate) fn warnings(&self) -> impl Iterator> { + self.linters.iter().filter(|linter| linter.count() > 0) + } } impl Task { @@ -384,6 +407,10 @@ impl Task { self.self_wakes().percent_of(self.wakes()) } + pub(crate) fn warnings(&self) -> &[Linter] { + &self.warnings[..] + } + fn update(&mut self) { let completed = self.stats.total.is_some() && self.completed_for == 0; if completed { @@ -481,6 +508,8 @@ impl SortBy { Self::State => { tasks.sort_unstable_by_key(|task| task.upgrade().map(|t| t.borrow().state())) } + Self::Warns => tasks + .sort_unstable_by_key(|task| task.upgrade().map(|t| t.borrow().warnings().len())), Self::Total => { tasks.sort_unstable_by_key(|task| task.upgrade().map(|t| t.borrow().total(now))) } @@ -503,6 +532,7 @@ impl TryFrom for SortBy { match idx { idx if idx == Self::Tid as usize => Ok(Self::Tid), idx if idx == Self::State as usize => Ok(Self::State), + idx if idx == Self::Warns as usize => Ok(Self::Warns), idx if idx == Self::Name as usize => Ok(Self::Name), idx if idx == Self::Total as usize => Ok(Self::Total), idx if idx == Self::Busy as usize => Ok(Self::Busy), diff --git a/console/src/view/mod.rs b/console/src/view/mod.rs index c23c24ab..311795ba 100644 --- a/console/src/view/mod.rs +++ b/console/src/view/mod.rs @@ -137,12 +137,14 @@ impl Width { } pub(crate) fn update_str>(&mut self, s: S) -> S { - let len = s.as_ref().len(); + self.update_len(s.as_ref().len()); + s + } + pub(crate) fn update_len(&mut self, len: usize) { let max = cmp::max(self.curr as usize, len); // Cap since a string could be stupid-long and not fit in a u16. // 100 is arbitrarily chosen, to keep the UI sane. self.curr = cmp::min(max, 100) as u16; - s } pub(crate) fn constraint(&self) -> layout::Constraint { diff --git a/console/src/view/styles.rs b/console/src/view/styles.rs index 7c65fb47..9d861eb6 100644 --- a/console/src/view/styles.rs +++ b/console/src/view/styles.rs @@ -113,6 +113,20 @@ impl Styles { } } + pub fn warning_wide(&self) -> Span<'static> { + Span::styled( + self.if_utf8("\u{26A0} ", "/!\\ "), + self.fg(Color::LightYellow).add_modifier(Modifier::BOLD), + ) + } + + pub fn warning_narrow(&self) -> Span<'static> { + Span::styled( + self.if_utf8("\u{26A0} ", "! "), + self.fg(Color::LightYellow).add_modifier(Modifier::BOLD), + ) + } + pub fn color(&self, color: Color) -> Option { use Palette::*; match (self.palette, color) { diff --git a/console/src/view/task.rs b/console/src/view/task.rs index ac102073..be43d806 100644 --- a/console/src/view/task.rs +++ b/console/src/view/task.rs @@ -14,7 +14,7 @@ use std::{ use tui::{ layout::{self, Layout}, text::{Span, Spans, Text}, - widgets::{Block, Paragraph}, + widgets::{Block, List, ListItem, Paragraph}, }; pub(crate) struct TaskView { @@ -50,20 +50,60 @@ impl TaskView { .as_ref() .filter(|details| details.task_id() == task.id()); - let chunks = Layout::default() - .direction(layout::Direction::Vertical) - .constraints( - [ - layout::Constraint::Length(1), - layout::Constraint::Length(8), - layout::Constraint::Length(9), - layout::Constraint::Percentage(60), - ] - .as_ref(), - ) - .split(area); + let warnings: Vec<_> = task + .warnings() + .iter() + .map(|linter| { + ListItem::new(Text::from(Spans::from(vec![ + styles.warning_wide(), + // TODO(eliza): it would be nice to handle singular vs plural... + Span::from(linter.format(task)), + ]))) + }) + .collect(); + + let (controls_area, stats_area, poll_dur_area, fields_area, warnings_area) = + if warnings.is_empty() { + let chunks = Layout::default() + .direction(layout::Direction::Vertical) + .constraints( + [ + // controls + layout::Constraint::Length(1), + // task stats + layout::Constraint::Length(8), + // poll duration + layout::Constraint::Length(9), + // fields + layout::Constraint::Percentage(60), + ] + .as_ref(), + ) + .split(area); + (chunks[0], chunks[1], chunks[2], chunks[3], None) + } else { + let chunks = Layout::default() + .direction(layout::Direction::Vertical) + .constraints( + [ + // controls + layout::Constraint::Length(1), + // warnings (add 2 for top and bottom borders) + layout::Constraint::Length(warnings.len() as u16 + 2), + // task stats + layout::Constraint::Length(8), + // poll duration + layout::Constraint::Length(9), + // fields + layout::Constraint::Percentage(60), + ] + .as_ref(), + ) + .split(area); + + (chunks[0], chunks[2], chunks[3], chunks[4], Some(chunks[1])) + }; - let controls_area = chunks[0]; let stats_area = Layout::default() .direction(layout::Direction::Horizontal) .constraints( @@ -73,11 +113,11 @@ impl TaskView { ] .as_ref(), ) - .split(chunks[1]); + .split(stats_area); // Only split the histogram area in half if we're also drawing a // sparkline (which requires UTF-8 characters). - let histogram_area = if styles.utf8 { + let poll_dur_area = if styles.utf8 { Layout::default() .direction(layout::Direction::Horizontal) .constraints( @@ -88,14 +128,12 @@ impl TaskView { ] .as_ref(), ) - .split(chunks[2]) + .split(poll_dur_area) } else { - vec![chunks[2]] + vec![poll_dur_area] }; - let percentiles_area = histogram_area[0]; - - let fields_area = chunks[3]; + let percentiles_area = poll_dur_area[0]; let controls = Spans::from(vec![ Span::raw("controls: "), @@ -173,7 +211,7 @@ impl TaskView { // If UTF-8 is disabled we can't draw the histogram sparklne. if styles.utf8 { - let sparkline_area = histogram_area[1]; + let sparkline_area = poll_dur_area[1]; // Bit of a deadlock: We cannot know the highest bucket value without determining the number of buckets, // and we cannot determine the number of buckets without knowing the width of the chart area which depends on @@ -195,6 +233,11 @@ impl TaskView { frame.render_widget(histogram_sparkline, sparkline_area); } + if let Some(warnings_area) = warnings_area { + let warnings = List::new(warnings).block(styles.border_block().title("Warnings")); + frame.render_widget(warnings, warnings_area); + } + let task_widget = Paragraph::new(metrics).block(styles.border_block().title("Task")); let wakers_widget = Paragraph::new(waker_stats).block(styles.border_block().title("Waker")); let fields_widget = Paragraph::new(fields).block(styles.border_block().title("Fields")); diff --git a/console/src/view/tasks.rs b/console/src/view/tasks.rs index 7674b71a..f3ebc150 100644 --- a/console/src/view/tasks.rs +++ b/console/src/view/tasks.rs @@ -1,22 +1,18 @@ use crate::{ input, - tasks::{self, Task, TaskRef, TaskState}, + tasks::{self, TaskRef, TaskState}, view::{self, bold}, - warnings, }; use std::convert::TryFrom; use tui::{ layout, style::{self, Color, Style}, - text::{self, Span, Spans}, - widgets::{Cell, Paragraph, Row, Table, TableState}, + text::{self, Span, Spans, Text}, + widgets::{self, Cell, ListItem, Paragraph, Row, Table, TableState}, }; #[derive(Debug)] pub(crate) struct List { - /// A list of linters (implementing the [`warnings::Warn`] trait) used to generate - /// warnings. - linters: Vec>>, sorted_tasks: Vec, sort_by: tasks::SortBy, table_state: TableState, @@ -26,7 +22,7 @@ pub(crate) struct List { impl List { const HEADER: &'static [&'static str] = &[ - "ID", "State", "Name", "Total", "Busy", "Idle", "Polls", "Target", "Fields", + "Warn", "ID", "State", "Name", "Total", "Busy", "Idle", "Polls", "Target", "Fields", ]; pub(crate) fn len(&self) -> usize { @@ -81,7 +77,7 @@ impl List { area: layout::Rect, state: &mut tasks::State, ) { - const STATE_LEN: u16 = List::HEADER[1].len() as u16; + const STATE_LEN: u16 = List::HEADER[2].len() as u16; const DUR_LEN: usize = 10; // 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 @@ -109,49 +105,45 @@ impl List { }; // Start out wide enough to display the column headers... - let mut id_width = view::Width::new(Self::HEADER[0].len() as u16); - let mut name_width = view::Width::new(Self::HEADER[2].len() as u16); - let mut target_width = view::Width::new(Self::HEADER[7].len() as u16); + let mut warn_width = view::Width::new(Self::HEADER[0].len() as u16); + let mut id_width = view::Width::new(Self::HEADER[1].len() as u16); + let mut name_width = view::Width::new(Self::HEADER[3].len() as u16); + let mut target_width = view::Width::new(Self::HEADER[8].len() as u16); let mut num_idle = 0; let mut num_running = 0; - let mut warnings = Vec::new(); let rows = { let id_width = &mut id_width; let target_width = &mut target_width; let name_width = &mut name_width; + let warn_width = &mut warn_width; let num_running = &mut num_running; let num_idle = &mut num_idle; - let warnings = &mut warnings; - let linters = &self.linters; self.sorted_tasks.iter().filter_map(move |task| { let task = task.upgrade()?; let task = task.borrow(); let state = task.state(); - warnings.extend(linters.iter().filter_map(|warning| { - let warning = warning.check(&*task)?; - let task = if let Some(name) = task.name() { - Span::from(format!("Task '{}' (ID {}) ", name, task.id())) - } else { - Span::from(format!("Task ID {} ", task.id())) - }; - Some(Spans::from(vec![ - Span::styled( - styles.if_utf8("\u{26A0} ", "/!\\ "), - styles.fg(Color::LightYellow), - ), - task, - Span::from(warning), - ])) - })); + // Count task states match state { TaskState::Running => *num_running += 1, TaskState::Idle => *num_idle += 1, _ => {} }; + let n_warnings = task.warnings().len(); + let warnings = if n_warnings > 0 { + let n_warnings = n_warnings.to_string(); + warn_width.update_len(n_warnings.len() + 2); // add 2 for the warning icon + whitespace + Cell::from(Spans::from(vec![ + styles.warning_narrow(), + Span::from(n_warnings), + ])) + } else { + Cell::from("") + }; let mut row = Row::new(vec![ + warnings, Cell::from(id_width.update_str(format!( "{:>width$}", task.id(), @@ -225,6 +217,17 @@ impl List { + POLLS_LEN as u16 + target_width.chars(); */ + let warnings = state + .warnings() + .map(|warning| { + ListItem::new(Text::from(Spans::from(vec![ + styles.warning_wide(), + // TODO(eliza): it would be nice to handle singular vs plural... + Span::from(format!("{} {}", warning.count(), warning.summary())), + ]))) + }) + .collect::>(); + let layout = layout::Layout::default() .direction(layout::Direction::Vertical) .margin(0); @@ -261,6 +264,7 @@ impl List { // See https://github.com/fdehau/tui-rs/issues/525 let fields_width = layout::Constraint::Percentage(100); let widths = &[ + warn_width.constraint(), id_width.constraint(), layout::Constraint::Length(STATE_LEN), name_width.constraint(), @@ -298,11 +302,10 @@ impl List { frame.render_widget(Paragraph::new(controls), controls_area); if let Some(area) = warnings_area { - let block = styles.border_block().title(Spans::from(vec![ - bold("Warnings"), - Span::from(format!(" ({})", warnings.len())), - ])); - frame.render_widget(Paragraph::new(warnings).block(block), area); + let block = styles + .border_block() + .title(Spans::from(vec![bold("Warnings")])); + frame.render_widget(widgets::List::new(warnings).block(block), area); } self.sorted_tasks.retain(|t| t.upgrade().is_some()); @@ -365,12 +368,12 @@ impl List { impl Default for List { fn default() -> Self { - Self { - linters: vec![Box::new(warnings::SelfWakePercent::default())], - sorted_tasks: Vec::new(), - sort_by: tasks::SortBy::default(), - table_state: TableState::default(), - selected_column: 0, + let sort_by = tasks::SortBy::default(); + List { + sorted_tasks: Default::default(), + sort_by, + table_state: Default::default(), + selected_column: sort_by as usize, sort_descending: false, } } diff --git a/console/src/warnings.rs b/console/src/warnings.rs index 972b80ca..5de86c66 100644 --- a/console/src/warnings.rs +++ b/console/src/warnings.rs @@ -1,18 +1,111 @@ use crate::tasks::Task; +use std::{fmt::Debug, rc::Rc}; -pub trait Warn: std::fmt::Debug { - fn check(&self, val: &T) -> Option; +/// A warning for a particular type of monitored entity (e.g. task or resource). +/// +/// This trait implements the logic for detecting a particular warning, and +/// generating a warning message describing it. The [`Linter`] type wraps an +/// instance of this trait to track active instances of the warning. +pub trait Warn: Debug { + /// Returns `true` if the warning applies to `val`. + fn check(&self, val: &T) -> bool; + + /// Formats a description of the warning detected for a *specific* `val`. + /// + /// This may include dynamically formatted content specific to `val`, such + /// as the specific numeric value that was over the line for detecting the + /// warning. + /// + /// This should be a complete sentence describing the warning. For example, + /// for the [`SelfWakePercent`] warning, this returns a string like: + /// + /// > "This task has woken itself for more than 50% of its total wakeups (86%)" + fn format(&self, val: &T) -> String; + + /// Returns a string summarizing the warning *in general*, suitable for + /// displaying in a list of all detected warnings. + /// + /// The list entry will begin with a count of the number of monitored + /// entities for which the warning was detected. Therefore, this should be a + /// sentence fragment suitable to follow a count. For example, for the + /// [`SelfWakePercent`] warning, this method will return a string like + /// + /// > "tasks have woken themselves more than 50% of the time" + /// + /// so that the warnings list can read + /// + /// > "45 tasks have woken themselves more than 50% of the time" + /// + /// If the warning is configurable (for example, [`SelfWakePercent`] allows + /// customizing the percentage used to detect the warning), this string may + /// be formatted dynamically for the *linter*, but not for the individual + /// instances of the lint that were detected. + // + // TODO(eliza): it would be nice if we had separate plural and singular + // versions of this, like "56 tasks have..." vs "1 task has...". + fn summary(&self) -> &str; +} + +#[derive(Debug)] +pub(crate) struct Linter(Rc>); + +impl Linter { + pub(crate) fn new(warning: W) -> Self + where + W: Warn + 'static, + { + Self(Rc::new(warning)) + } + + /// Checks if the warning applies to a particular entity, returning a clone + /// of `Self` if it does. + /// + /// The cloned instance of `Self` should be held by the entity that + /// generated the warning, so that it can be formatted. Holding the clone of + /// `Self` will increment the warning count for that entity. + pub(crate) fn check(&self, val: &T) -> Option { + if self.0.check(val) { + Some(Self(self.0.clone())) + } else { + None + } + } + + /// Returns the number of monitored entities that currently have this warning. + pub(crate) fn count(&self) -> usize { + Rc::strong_count(&self.0) - 1 + } + + pub(crate) fn format(&self, val: &T) -> String { + debug_assert!( + self.0.check(val), + "tried to format a warning for a {} that did not have that warning!", + std::any::type_name::() + ); + self.0.format(val) + } + + pub(crate) fn summary(&self) -> &str { + self.0.summary() + } } #[derive(Clone, Debug)] pub(crate) struct SelfWakePercent { min_percent: u64, + description: String, } impl SelfWakePercent { pub(crate) const DEFAULT_PERCENT: u64 = 50; pub(crate) fn new(min_percent: u64) -> Self { - Self { min_percent } + Self { + min_percent, + description: format!( + "tasks have woken themselves over {}% of the time", + min_percent + ), + } } } @@ -23,15 +116,20 @@ impl Default for SelfWakePercent { } impl Warn for SelfWakePercent { - fn check(&self, task: &Task) -> Option { + fn summary(&self) -> &str { + self.description.as_str() + } + + fn check(&self, task: &Task) -> bool { let self_wakes = task.self_wake_percent(); - if self_wakes > self.min_percent { - return Some(format!( - "has woken itself for more than {}% of its total wakeups ({}%)", - self.min_percent, self_wakes - )); - } + self_wakes > self.min_percent + } - None + fn format(&self, task: &Task) -> String { + let self_wakes = task.self_wake_percent(); + format!( + "This task has woken itself for more than {}% of its total wakeups ({}%)", + self.min_percent, self_wakes + ) } }