view: support expanding a cgroup's processes in the cgroup view - #8284
view: support expanding a cgroup's processes in the cgroup view#8284JamesOlaitan wants to merge 3 commits into
Conversation
Pressing 'e' on the selected cgroup toggles its member processes as indented child rows, rendered in blue to distinguish them from cgroup rows. Process rows show comm, pid, state, cpu usage, resident set size and the command line. Collapsing a cgroup also hides its process rows. Process rows follow the view's sort order when it maps to a process field (cpu usage/user/system, memory total, io read/write/rw), and fall back to pid order otherwise. Fixes facebookincubator#8238
meta-cla
Bot
added
the
CLA Signed
|
Hi @JamesOlaitan , thanks for the PR. How are you determining which process columns to display here? It looks a little odd that the columns have no relationship to the titles at the top (obviously so because the title is for the cgroup tab). I don't know how I want it to work, but I feel like we need to somehow make it obvious what the per-process columns are when this feature is used. |
The cgroup view's column titles describe cgroup fields, so the process rows shown for an expanded cgroup had no visible column labels. Insert a title row (Comm, Pid, State, CPU, RSS, Cmdline) directly above the process rows, aligned with them and rendered in the default color so blue stays reserved for the processes themselves.
@dschatzberg First off, thanks for taking a look! I chose a fixed set (comm, pid, state, CPU usage, resident memory, cmdline) so the command line is visible without horizontal scrolling, since seeing what is running was what the original issue was after. To make the per-process column obvious, I've added a title row above the processes whenever a cgroup is expanded. Here's how it now looks (I've also updated the image in the PR description): 
|
dschatzberg
left a comment
dschatzberg
left a comment
There was a problem hiding this comment.
Looks pretty good to me. Check out the comments inline
| StyledString::styled( | ||
| line.source(), | ||
| cursive::theme::Color::Light(cursive::theme::BaseColor::Blue), | ||
| ) |
There was a problem hiding this comment.
I don't like this restyling since it overrides the rendering of the model we would otherwise have (e.g. high CPU might show as red). Do we need it visually? Can we just not use the tree indent prefix for the processes (since we now have a separate header) and rely on indentation to show?
There was a problem hiding this comment.
Fair point about hiding the field styling. I dropped the whole-row restyle and the tree prefix. So, stat fields now render normally (a process over the CPU threshold shows red again), and only the comm column keeps the blue tint, so process rows are still easy to pick out on a busy tree (the updated image is in the description). I can drop that too, though, if you feel no color is better (or if it makes no difference when scanning).
| output.push(( | ||
| self.get_process_line(process, depth, offset), | ||
| cgroup.data.full_path.clone(), | ||
| )); |
There was a problem hiding this comment.
I'm a little concerned that its possible for this to be much wider than the cgroup tab view it lives in (you can test this by just adding a ton of fields to the process expansion). I think we might need to make the cgroupview width dynamic based on if processes are expanded?
There was a problem hiding this comment.
The process columns are a hard-coded list in the code. So, a process row can only ever get so wide (about 148 characters with the default name width, which is narrower than the cgroup rows on the General tab).
When a row is wider than the window, it just gets cut off at the edge, same as any wide cgroup row, and there are already two ways to see the hidden part. First, the whole table (column titles plus every row) sits inside one sideways-scrolling box, so scrolling moves titles and rows together and they stay lined up. Second, the Left/Right keys page through columns, and the same "skip the first N columns" count is applied to cgroup rows and process rows, so both kinds of rows always shift together instead of drifting out of sync.
To make sure, I tried it in a terminal on the cgroup view's CPU tab (the tab whose stat columns are the narrowest, so the process rows stick out past the cgroup rows the most). The rows that were too wide just got cut off at the window edge like any other long row, and paging with Left/Right brought the hidden columns into view.
I'd agree that if the process columns ever become user-configurable, the width stops being bounded and dynamic sizing would be worth building then.
Drop the Option around the process index since an empty map already means no expansions. Store the header's name column as a ViewConfig built from the process name item instead of a second ViewItem that never queries a model. Color only the process name blue and leave the stat fields to their own rendering, so field highlights like red cpu show through, and drop the tree prefix on process rows since the header row already sets them apart.
2 participants
Fixes #8238
In the cgroup view, pressing
eon the selected cgroup now displays its member processes as indented child rows, so you can see what is actually running in a cgroup without zooming away from the tree. The rows are rendered in blue to stand out from cgroup rows and show comm, pid, state, CPU usage, resident memory, and the full command line, which is usually enough to tell at a glance what a busy scope is doing.A few notes:
z, below understands you mean "show me the processes of the cgroup this thing lives in" and takes you there. Same idea forEnter/=/e: they act as if you had pressed them on the cgroup that owns the process.yesappears abovebashin the screenshot). Some sorts only make sense for cgroups and have no process version. In those cases, the processes are listed in ascending order of their pid.The process columns intentionally do not line up with the cgroup column titles above them (the fields differ, so they cannot). Keeping the rows compact and colored seemed better than mirroring the full process view, where the command line ends up far off-screen.
Testing: the row generation this feature touches lives in
below/view/src/cgroup_tabs.rs, which previously had no unit tests. This PR adds seven, covering expansion rendering, exact-match scoping, collapse interaction, root path normalization, sort mapping, filter interaction, and the toggle guard.