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

Colorized output for supervisorctl status #530

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

msabramo
Copy link
Contributor

@msabramo msabramo commented Dec 3, 2014

How about a sexy new feature for 4.0...? 😄

I've long wanted this, because it makes the failed processes stand out from the OK ones in a long list.

supervisorctl_status_colored

I'm toying with the idea of colorizing the entire line rather than just the status. Or maybe the name and the status. Having the name colorized seems nice so that your eyes can just scan the first column. Kind of thinking of not colorizing the rest of the line according to status, because I had an idea of maybe colorizing the uptime if it less than a certain threshold (startsecs?) -- this is because we sometimes have processes that repeatedly spin up and die and use a lot of CPU, so it can be very useful sometimes to spot the processes that have the shortest uptime...

@mcdonc and @mnaberez: What do you guys think?

I didn't write tests, but I will add them if you're cool with the feature.

@@ -22,7 +22,8 @@
elif (3, 0) < py_version < (3, 2):
raise RuntimeError('On Python 3, Supervisor requires Python 3.2 or later')

requires = ['meld3 >= 1.0.0']
requires = ['meld3 >= 1.0.0',
'colorama']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that we do not add an external dependency just to get some colored output. I think a feature of Supervisor is that it has no dependencies besides meld3 (which is our package too), and at some point I'd like to find a way to remove meld3. Supervisor is widely used by non-Python people and is packaged by a lot of distributions. Having fewer dependencies makes it easier to install and package for those people.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the only concern, I can pull in some color constants.

colorama does something fancier to make colors work in Windows, but we could make that optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could even change this so that it only does the import colorama when the user turns on coloring (which is not on by default). If they turn on coloring and the packages are not installed, then I can say "Colorized output requires the 'colorama' package". Could even combine this idea with the one above so that color codes are vendored in and colorizing works out of the box on platforms other than windows. The message about colorama would only be installed if on Windows and the user requests color.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can pull in some color constants.

I think it would probably be fine to just emit some escape sequences, pretty much all modern terminals support vt10x codes, and then everyone could have color without the dependency. I don't know if supervisorctl works on Windows (we don't officially support Windows at all). If it does then there's only a very small number of users running it that way, and not having color would probably not be a big deal. They could get color with cygwin.

The color feature could be done in a ctlplugin but I think enough people would like this where it's probably worth considering putting it into supervisorctl. There is a lot that could be done to improve supervisorctl. We did some work on fixing command completion some months ago (I think you had a patch in that) but I'd really like for error messages to go to stderr and every command to have a defined set of exitcodes. It just hasn't been a priority because of test coverage, Python 3, etc.

@msabramo
Copy link
Contributor Author

msabramo commented Dec 5, 2014

OK, I switched from colorama to a new internal module called supervisor.colors.

@gjcarneiro
Copy link

+1

@msabramo
Copy link
Contributor Author

I'll try to come back to this in a few days. Any changes/ideas?

@jvrplmlmn
Copy link

The colorized output looks like a great addition 👍

Enable it by adding to config file:

    [supervisorctl]
    colorize_status = auto
@msabramo
Copy link
Contributor Author

msabramo commented Mar 8, 2015

Squashed it down to a single commit.

@darkone23
Copy link

+1

@mnaberez
Copy link
Member

mnaberez commented Apr 9, 2015

Thanks for updating this patch to remove the dependency. I think this is a nice addition and I would be happy to merge this if some test coverage was added.

I looked through the new patch and there's one thing I would like to discuss: Right now the option to use color is specific to the status command. Should we add one "use color" option that affects all of supervisorctl, or is there a use case for specifying color/no color per command?

There might be other uses for color, e.g. we might want to show errors in red, or plugins might want to colorize something. I'm wondering if we will accumulate color options for those cases or if one global "use color" option would be sufficient.

@msabramo
Copy link
Contributor Author

msabramo commented Apr 9, 2015

Interesting. I had only thought of using color for status and hadn't thought of other uses, but that's a good point -- perhaps there should be one global color option.

@dirkaholic
Copy link

Why was this PR never considered to be merged ? I find the feature mega useful.

@FAKERINHEART
Copy link

when the target to output is not a terminal, such as pipe, I think it is better not to output colorfully.

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

Successfully merging this pull request may close these issues.

7 participants