Colorize zfs list output #14350

ethan-coe-renner · 2023-01-04T23:09:26Z

Motivation and Context

Add colored output to zfs list to improve readability and allow users to quickly see datasets that are dangerously (performance-wise) full.

Description

This uses the interface introduced in #9340 to add color to zfs list.

If the ZFS_COLOR env variable is set, then output zfs list is colored as follows:

Header row is bold
The AVAIL column is colored based on the percentage still available
- yellow if < 20% available
- red if < 10% available

Screenshots

How Has This Been Tested?

Manually tested.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

ethan-coe-renner · 2023-01-04T23:41:56Z

An idea came up while looking over the PR just before submitting: An alternative way of coloring the AVAIL column could be as a percentage of the total. I.e. if AVAIL is below 20% of the total, it gets colored red, below 40% then yellow, otherwise green. Might this be more useful? I currently color by the order of magnitude, inspiration for this came from this issue: #13168. Any thoughts on which would be more useful?

tonyhutter · 2023-01-05T00:10:07Z

An alternative way of coloring the AVAIL column could be as a percentage of the total

That would be my vote as well. Maybe judging by "80% bad, 95-96% really bad" we should do:

80% full: AVAIL yellow
90% full: AVAIL red

Otherwise just keep AVAIL the default color.

rincebrain · 2023-01-05T07:11:34Z

You might find how I color zfs list interesting, or maybe not, it's very loud and information-dense, and not everyone's cup of tea.

Or, more accurately, in its current iteration.

I would particularly suggest coloring the segments of the dataset names consistently but distinctly, and the non-digit property values likewise, as extremely useful, as well as the every other line contrast to help avoid trouble distinguishing which row and column go together.

You can play with it yourself, if you like, it's just something to pipe zfs list into.

(The sizes as a gradient from 1-1024 and then the suffix as a separate coloring I also find useful, but some people really dislike Many Colors.)

ethan-coe-renner · 2023-01-05T21:52:16Z

Thanks for the input!

@rincebrain thanks for sending your implementation. I like many of the ideas in it, though, as you suggest, as a whole it may be too opinionated to be built directly in.

I would particularly suggest coloring the segments of the dataset names consistently but distinctly, and the non-digit property values likewise, as extremely useful, as well as the every other line contrast to help avoid trouble distinguishing which row and column go together.

These are excellent suggestions, I will add these!
Additionally, doing order of magnitude coloring only on the actual suffix I think better conveys that that's what the color relates to, I'll steal that as well.

@tonyhutter I agree, I think coloring by percentage makes sense in the AVAIL column, thanks for directing me to the "80% bad, 95-96% really bad" idea, I'll use those thresholds.
Thus, I think for that column, the digits should be colored by percentage, and the suffix by order of magnitude. For all other digit columns, only the suffix should be colored.

ethan-coe-renner · 2023-01-10T21:54:37Z

After doing some testing and having a conversation, I’ve come to the conclusion that limiting the amount of color to specifically highlighting problems (or potential problems) is best. This is for two reasons:

Different themes: In order to e.g. highlight every other line alternating, the alternate color should be subtle. However a subtle color on a light theme makes a dark theme unreadable. Additionally, in order to use enough colors to effectively color segments of names, we would have to use ANSI-256 colors, which introduces further potential conflicts with themes.
The precedent set by Colorize zpool status output #9340 is that color is used to highlight errors/problems (in places where these can occur). Adding more color around that lessens the effectiveness of color at highlighting the errors/potential problems.

For these reasons, I've scaled back this PR to just color the AVAIL column based on the percentage full.

cmd/zfs/zfs_main.c

- Bold header row - Color AVAIL column based on percentage of volume available - < 20%: Yellow - < 10%: Red Signed-off-by: Ethan Coe-Renner <coerenner1@llnl.gov>

behlendorf

Just a few small nits. When you refresh this can you make sure to rebase it on the latest commits in the master branch. That should get us a cleaner CI test run.

behlendorf · 2023-02-06T18:47:03Z

cmd/zfs/zfs_main.c

+zfs_list_avail_color(zfs_handle_t *zhp)
+{
+	int used = zfs_prop_get_int(zhp, ZFS_PROP_USED);
+	int available = zfs_prop_get_int(zhp, ZFS_PROP_AVAILABLE);


Suggested change

int available = zfs_prop_get_int(zhp, ZFS_PROP_AVAILABLE);

uint64_t available = zfs_prop_get_int(zhp, ZFS_PROP_AVAILABLE);

zfs_prop_get_int() returns a uint64_t which we want to make sure to use here for both used and available.

behlendorf · 2023-02-06T18:57:20Z

cmd/zfs/zfs_main.c

+{
+	int used = zfs_prop_get_int(zhp, ZFS_PROP_USED);
+	int available = zfs_prop_get_int(zhp, ZFS_PROP_AVAILABLE);
+	int percentage = (int)((double)available / (available + used) * 100);


Suggested change

int percentage = (int)((double)available / (available + used) * 100);

int percentage = (int)((double)available / MAX(available + used, 1) * 100);

Even though it shouldn't happen, we should protect against a division by zero error here.

mcmilk · 2023-02-28T19:53:54Z

When the output of zfs list is redirected to some file, then it will also print those ANSI key escapes ....

Maybe some check with isatty(int fd); could be done before?
You can issie isatty() once and then remeber it with some static variable for later calls.

Edit: libzfs_util.c has a isatty() check - so everything is fine with this. Please ignore this comment.

mcmilk

Can you add the small changes which @behlendorf requested and do a rebase?
The tests should be fine then - I think.

Use a bold header row and colorize the AVAIL column based on the used space percentage of volume. We define these colors: - when > 80%, use yellow - when > 90%, use red Signed-off-by: Ethan Coe-Renner <coerenner1@llnl.gov> Signed-off-by: Tino Reichardt <milky-zfs@mcmilk.de> Closes: openzfs#14350

Use a bold header row and colorize the AVAIL column based on the used space percentage of volume. We define these colors: - when > 80%, use yellow - when > 90%, use red Reviewed-by: WHR <msl0000023508@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ethan Coe-Renner <coerenner1@llnl.gov> Signed-off-by: Tino Reichardt <milky-zfs@mcmilk.de> Closes openzfs#14621 Closes openzfs#14350

ethan-coe-renner marked this pull request as ready for review January 4, 2023 23:10

behlendorf added the Status: Code Review Needed Ready for review and testing label Jan 5, 2023

ethan-coe-renner marked this pull request as draft January 5, 2023 19:40

ethan-coe-renner force-pushed the colorize-zfs-list branch 4 times, most recently from 277fe2d to 9cb07df Compare January 10, 2023 21:11

ethan-coe-renner marked this pull request as ready for review January 10, 2023 22:08

tonyhutter reviewed Jan 10, 2023

View reviewed changes

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved

tonyhutter reviewed Jan 10, 2023

View reviewed changes

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved

tonyhutter reviewed Jan 10, 2023

View reviewed changes

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved

tonyhutter reviewed Jan 10, 2023

View reviewed changes

cmd/zfs/zfs_main.c Outdated Show resolved Hide resolved

ethan-coe-renner force-pushed the colorize-zfs-list branch from 9cb07df to a56bb2b Compare January 12, 2023 20:15

tonyhutter reviewed Jan 12, 2023

View reviewed changes

cmd/zfs/zfs_main.c Show resolved Hide resolved

ethan-coe-renner force-pushed the colorize-zfs-list branch 2 times, most recently from 0d064c0 to 337320c Compare January 30, 2023 20:13

Add colored output to zfs list

bc1ba01

- Bold header row - Color AVAIL column based on percentage of volume available - < 20%: Yellow - < 10%: Red Signed-off-by: Ethan Coe-Renner <coerenner1@llnl.gov>

ethan-coe-renner force-pushed the colorize-zfs-list branch from 337320c to bc1ba01 Compare February 3, 2023 21:27

behlendorf approved these changes Feb 6, 2023

View reviewed changes

mcmilk approved these changes Mar 5, 2023

View reviewed changes

mcmilk mentioned this pull request Mar 14, 2023

Colorize patchset #14621

Closed

13 tasks

behlendorf closed this in 2bd0490 Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorize zfs list output #14350

Colorize zfs list output #14350

ethan-coe-renner commented Jan 4, 2023 •

edited

Loading

ethan-coe-renner commented Jan 4, 2023

tonyhutter commented Jan 5, 2023 •

edited

Loading

rincebrain commented Jan 5, 2023 •

edited

Loading

ethan-coe-renner commented Jan 5, 2023

ethan-coe-renner commented Jan 10, 2023

behlendorf left a comment

behlendorf Feb 6, 2023

behlendorf Feb 6, 2023

mcmilk commented Feb 28, 2023 •

edited

Loading

mcmilk left a comment

	int available = zfs_prop_get_int(zhp, ZFS_PROP_AVAILABLE);
	uint64_t available = zfs_prop_get_int(zhp, ZFS_PROP_AVAILABLE);

	int percentage = (int)((double)available / (available + used) * 100);
	int percentage = (int)((double)available / MAX(available + used, 1) * 100);

Colorize zfs list output #14350

Colorize zfs list output #14350

Conversation

ethan-coe-renner commented Jan 4, 2023 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ethan-coe-renner commented Jan 4, 2023

tonyhutter commented Jan 5, 2023 • edited Loading

rincebrain commented Jan 5, 2023 • edited Loading

ethan-coe-renner commented Jan 5, 2023

ethan-coe-renner commented Jan 10, 2023

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf Feb 6, 2023

Choose a reason for hiding this comment

behlendorf Feb 6, 2023

Choose a reason for hiding this comment

mcmilk commented Feb 28, 2023 • edited Loading

mcmilk left a comment

Choose a reason for hiding this comment

ethan-coe-renner commented Jan 4, 2023 •

edited

Loading

tonyhutter commented Jan 5, 2023 •

edited

Loading

rincebrain commented Jan 5, 2023 •

edited

Loading

mcmilk commented Feb 28, 2023 •

edited

Loading