feat(console): display warnings in task details *and* list (console-r…

…s#123)

The current method of displaying warnings is as list of all individual
tasks with warnings on the task list screen. As discussed in the
comments on PR console-rs#113, this may not be the ideal approach when there are a
very large number of tasks with warnings.

This branch changes this behavior so that the details of a particular
warning for a given task is shown only on the task details screen. On
the task list screen, we instead list the different _categories_ of
warnings that were detected, along with the number of tasks with that
warning. This means that when a large number of tasks have warnings, we
will only use a number of lines equal to the number of different
categories of warning that were detected, rather than the number of
individual tasks that have that warning.

Each individual task that has warnings shows a warning icon and count in
a new column in the task list table. This makes it easy for the user to
find the tasks that have warnings and get details on them, including
sorting by the number of warnings.

Implementing this required some refactoring of how warnings are
implemented. This includes:

* Changing the `Warn` trait to have separate methods for detecting a
  warning, formatting the warning for an individual task instance, and
  summarizing the warning for the list of detected warning types
* Introducing a new `Linter` struct that wraps a `dyn Warning` in an
  `Rc` and clones it into tasks that have lints. This allows the task
  details screen to determine how to format the lint when it is needed.
  It also allows tracking the total number of tasks that have a given
  warning, by using the `Rc`'s reference count.

## Screenshots

To demonstrate how this design saves screen real estate when there are
many tasks with warnings, I modified the `burn` example to spawn
several burn tasks rather than just one.

Before, we spent several lines on warnings (one for each task):
![warn_old](https://user-images.githubusercontent.com/2796466/132567589-862d1f82-1b8a-481e-9ce0-fc0798319c8a.png)

After, we only need one line:
![warnings1](https://user-images.githubusercontent.com/2796466/132567656-2c1473fb-22a2-45bb-99b1-c23cce4d86dd.png)

The detailed warning text for the individual tasks are displayed on the
task details view:
![warnings1_details](https://user-images.githubusercontent.com/2796466/132567713-8e1162f4-f989-488b-b347-f8837ba67d65.png)

And, it still looks okay in ASCII-only mode:
![warnings1_ascii-only](https://user-images.githubusercontent.com/2796466/132567772-9d6ed35d-254d-4b9e-bf6e-eef1819c211c.png)
![warnings1_details_ascii-only](https://user-images.githubusercontent.com/2796466/132567783-a88e4730-0a0d-4240-a285-a99bc2ff1047.png)

Signed-off-by: Eliza Weisman <[email protected]>

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

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<u64, Rc<RefCell<Task>>>,
     metas: HashMap<u64, Metadata>,
+    linters: Vec<Linter<Task>>,
     last_updated_at: Option<SystemTime>,
     new_tasks: Vec<TaskRef>,
     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<str>,
     name: Option<Arc<str>>,
+    /// Currently active warnings for this task.
+    warnings: Vec<Linter<Task>>,
 }
 
 #[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<Item = Linter<Task>>) -> Self {
+        self.linters.extend(linters.into_iter());
+        self
+    }
+
     pub(crate) fn last_updated_at(&self) -> Option<SystemTime> {
         self.last_updated_at
     }
@@ -202,19 +211,29 @@ impl State {
                 stats,
                 completed_for: 0,
                 target: meta.target.clone(),
+                warnings: Vec::new(),
             };
             task.update();
             let task = Rc::new(RefCell::new(task));
             new_list.push(Rc::downgrade(&task));
             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<Item = &Linter<Task>> {
+        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<Task>] {
+        &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<usize> 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),

console/src/view/mod.rs

@@ -137,12 +137,14 @@ impl Width {
     }
 
     pub(crate) fn update_str<S: AsRef<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 {

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<Color> {
         use Palette::*;
         match (self.palette, color) {

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