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

feat(console): dynamically size task details field list #352

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented May 23, 2022

Motivation

Currently, the task details view always gives the list of fields 50% of
the vertical space. This isn't a particularly effective use of space
when a task has only a small number of fields --- we end up wasting a
lot of the vertical real estate that could be used for something nicer.

Solution

This branch changes the layout of the task details view to give the list
of fields a Min layout constraint with the length of the list of
fields plus the border width. This way, when the list of fields is
short, we don't waste a bunch of vertical space on blank lines.

Now, we have a bunch more vertical resolution for drawing a nicer
histogram.

Screenshots

Before:
image

After:
image

## Motivation

Currently, the task details view always gives the list of fields 50% of
the vertical space. This isn't a particularly effective use of space
when a task has only a small number of fields --- we end up wasting a
lot of the vertical real estate that could be used for something nicer.

## Solution

This branch changes the layout of the task details view to give the list
of fields a `Min` layout constraint with the length of the list of
fields plus the border width. This way, when the list of fields is
short, we don't waste a bunch of vertical space on blank lines.
@hawkw hawkw requested a review from a team as a code owner May 23, 2022 20:22
@hawkw
Copy link
Member Author

hawkw commented May 23, 2022

Maybe the histogram looks too stretched out in the second case and we should give it a percent constraint additionally...would love to hear if anyone has thoughts on this.

hawkw and others added 27 commits May 23, 2022 13:24
This branch updates the console crates' dependency on `tonic` to v0.8.0
and `prost` to 0.11.0.

In addition, I've added a [`cargo xtask`][xtask] command for manually
regenerating the generated protobuf bindings.

This is necessary as the current approach, regenerating the bindings in
an integration test, does not work when the protos fail to compile
(which they do after the Tonic update). Since running the crate's tests
requires compiling the crate, if the proto bindings don't compile, we
can't re-run the test.

[xtask]: https://github.com/matklad/cargo-xtask
This changes the console CLI's error handling so that GitHub issues are
only suggested for panics, not for recoverable errors (such as "no
config file found", "couldn't connect to remote host", etc).
This way, if running a subcommand panics/errors, we still get nice
`color-eyre` reports.
This commit adds `color_eyre`'s default backtrace frame filters, so we
skip stuff that's not relevant.
This adds a dump of all the console's config options to the issue
metadata for GitHub issues generated using `color_eyre`'s GitHub issue
generation on panics.

<details>

<summary>Example issue Markdown:</summary>

## Error
```
lol
```

## Metadata
|key|value|
|--|--|
|**version**|0.1.6|
|**config.subcmd**|`None`|
|**config.target_addr**|`Some(http://127.0.0.1:6669/)`|
|**config.env_filter**|`None`|
|**config.log_directory**|`Some("/tmp/tokio-console/logs")`|
|**config.retain_for**|`None`|
|**config.view_options.no_colors**|`false`|
|**config.view_options.lang**|`Some("en_US.UTF-8")`|
|**config.view_options.ascii_only**|`Some(false)`|
|**config.view_options.truecolor**|`Some(true)`|
|**config.view_options.palette**|`Some(All)`|
|**config.view_options.toggles.color_durations**|`Some(false)`|
|**config.view_options.toggles.color_terminated**|`Some(false)`|
|**location**|tokio-console/src/main.rs:36:5|

</summary>
<a name="0.4.0"></a>
## 0.4.0 (2022-08-10)

#### Breaking Changes

*  Update `tonic` to `0.8` (#364) ([40e2f6f](40e2f6f))

#### Features

*  Update `tonic` to `0.8` (#364) ([40e2f6f](40e2f6f))
<a name="0.1.7"></a>
## 0.1.7 (2022-08-10)

#### Features

*  Update `tonic` to `0.8` (#364) ([40e2f6f](40e2f6f))
*  Update `console-api` to `0.4` (#364) ([40e2f6f](40e2f6f))
<a name="0.1.7"></a>
## 0.1.7 (2022-08-10)

#### Features

*  include config options in autogenerated issues (#365)
   ([fcb54df](fcb54df))
*  filter out boring frames in backtraces (#365) ([523a44a](523a44a))
*  init error handling before subcmds (#365) ([6646568](6646568))
*  only suggest opening issues for panics (#365) ([23cb6bf](23cb6bf))
*  update `tonic` to `0.8` (#364) ([40e2f6f](40e2f6f))
*  update `console-api` to `0.4` (#364) ([40e2f6f](40e2f6f))
Needed for #374.

This configures clippy to ignore most of the generated code in
`console-api`.
Due to a change in the unstable task builder APIs, this no longer
compiles with the latest version of Tokio. Fortunately, it's a simple
fix.
<a name="0.1.7"></a>
## 0.1.8 (2022-09-04)

#### Bug Fixes

*  fix build on tokio 1.21.0 (#374) ([0106407](0106407))
Fixes: #372

`--ascii-only true` shows ascii
`--ascii-only false` shows emojis

if `--ascii-only` is not passed, default value will be true, shows emojis
Currently, the documented `cargo-run` in README no longer works, as now
there are two binaries:

```console
$ cargo run
error: `cargo run` could not determine which binary to run. Use the `--bin` option t
o specify a binary, or the `default-run` manifest key.
available binaries: tokio-console, xtask
```

This branch declares the `tokio-console` binary as the [`default-run`]
value in the `Cargo.toml`, so it is now run by default by `cargo run`.

Alternatively, we can just update the README line with `cargo run --bin
tokio-console`

[`default-run`]:
    (https://doc.rust-lang.org/cargo/reference/manifest.html#the-default-run-field)
Found via these commands:

    codespell .
    markdownlint *.md --disable MD013
Currently, the help text for the `r` and `t` keystrokes to switch to the
Resources and Tasks views is shown on the Task Details and Resource
Details views, as well as on the Task List and Resource List views.

See this screenshot for an example:
![image](https://user-images.githubusercontent.com/2345750/202058962-4a8f062c-78fd-47eb-a3b1-5de470976aa8.png)

However, the keystrokes are not actually handled while the console is
showing a details view.

This branch changes the console to handle the `r` and `t` keystrokes on
all views. This also simplifies the keyboard input code somewhat.
Build and add to the released binaries for `aarch64-unknown-linux-gnu`,
`aarch64-apple-darwin` and `aarch64-pc-windows-msvc`
Hey I noticed this out of date in my `Cargo.lock`, and since I didn't
see any PRs/issues open doing so already, figured I'd open one!
Signed-off-by: hi-rustin <[email protected]>

Signed-off-by: hi-rustin <[email protected]>
The durations in the tokio-console UI are shown with a unit, so the
digits after the decimal separator are generally not relevant. Since
space is at a premium in a terminal UI, it makes sense to cut down where
possible.

This change removes all digits after the decimal separator in the tasks
table view. In the task detail view, 2 decimal places are kept.

Additionally, 4 new duration formats are added to represent
minutes-with-secondsi, hours-with-minutes, days-with-hours and days.
These are only applied once the leading unit is greater than zero.

For example, 59 seconds will be shown as seconds: `99s` and then 60
seconds will be shown as minutes-with-seconds: `1m00s`. New colors have
been chosen for each format.

NOTE: I had to make a small unrelated change in
`task::Details::make_percentiles_widget` to make clippy happy.

Fixes: #224
The `color-eyre` crate was previously on version 0.5. This version
depends on `tracing-subscriber` 0.2, meaning that we had 2 different
veresions of `tracing-subscriber` in our dependency tree.

This change updates `color-eyre` to 0.6, which depends on
`tracing-subscriber` 0.3, the same as `console-subscriber`.
hds and others added 5 commits March 28, 2023 16:27
Tokio Console generates its own sequential Id for internal tracking and
indexing of objects (tasks, resources, etc.). However, this Id will be
recreated if Console is restarted.

In order to provide more useful information to the user, the task Id
generated by Tokio can be used in the task list and task details screens
instead. If used in this way, the ID field in the task list and task
detail views will be stable across restarts of Console (assuming the
monitored application is not restarted).

This also frees up horizontal space, as the `task.id` attribute
doesn't need to be displayed separately.

The disadvantage of using Tokio's task Id is that it is not guaranteed
to be present by the type system.

To avoid problems with missing task Ids, the Tokio task Id is store in
addition to the `span::Id` (used to communicate with the
`console-subscriber`) and the sequential console `Id` (used in the
`store`). If a task is missing the `task.id` field for whatever reason
it will still appear, but with an empty ID. If multiple runtimes are
active, then duplicate ID values will appear.

Fixes: #385

Co-authored-by: Eliza Weisman <[email protected]>
Add support for console connections that use Unix domain sockets rather
than TCP.

Closes #296.

Co-authored-by: Eliza Weisman <[email protected]>
The Console API specifies sending task busy duration only for completed
polls, it doesn't include the time spent in the current poll if the task
is active.

Tokio Console then calculates the busy time including the time spent in
the current poll - based on the last poll start and poll end times sent
by the Console Subscriber.

However, there was an error in the logic which determined when a task is
being polled for the purpose of calculating the busy time. The logic
only considered the first poll, when there was no recorded end poll time
at all.

This change adapts the logic so that it also considers the case where
the last recorded poll start is later than the last recorded poll end.
This indicates that the task is being polled.

Co-authored-by: Eliza Weisman <[email protected]>
There are 2 widgets which display the poll times for a task in the
detail view. The poll times percentiles are always displayed and if
UTF-8 is available, then a sparkline histogram is also shown to the
right.

The logic for displaying these two widgets is quite long and is
currently interspersed within the `render` function for the task detail
view plus helper functions. Additionally, it is not easy to add a second
set of widgets showing the time between waking and being polled for a
task which is planned for #409.

This change factors out that logic into separate widgets.

There was already a separate widget `MiniHistogram`. Some of the logic
that was previously in the task detail view has been moved here.

A new widget `Percentiles` has been added to encapsulate the logic for
preparing and displaying the percentiles.

A top level `Durations` widget occupies the entire width of the task
detail view and control the horizontal layout of the `Percentiles` and
`MiniHistogram` widgets. The new widget will also supress the histogram
if there isn't at least enough room to display the legend at the bottom
@hawkw
Copy link
Member Author

hawkw commented Apr 19, 2023

@hds i wonder if it's worth resurrecting this after your changes to add additional histograms to the task details view (#409) merge?

@hds
Copy link
Collaborator

hds commented Apr 20, 2023

@hds i wonder if it's worth resurrecting this after your changes to add additional histograms to the task details view (#409) merge?

@hawkw I think that makes sense. The concerns you raised above are less of an issue with 2 histograms - although we'll still need to improve the constraints so that we don't end up with really, really tall histograms.

I'm happy to look into this after I've finished the scheduled time histogram (and the related docs change). I can pull this branch into my own or if you like you can give me permissions on this branch and I'll work on it here.

@hawkw
Copy link
Member Author

hawkw commented Apr 20, 2023

I'm happy to look into this after I've finished the scheduled time histogram (and the related docs change). I can pull this branch into my own or if you like you can give me permissions on this branch and I'll work on it here.

Let's just get #409 through first, and then I can rebase onto main. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.