Maoni0 / realmon

A monitoring tool that tells you when GCs happen in a process and some characteristics about these GCs
MIT License
281 stars 25 forks source link

Prettify console output using Spectre.Console #39

Closed ryandle closed 2 years ago

ryandle commented 2 years ago

This is the PR for issue #21 which updates the console output to use Spectre.Console for pretty formatting and colors.

Here's the before/after comparison between v0.5.0 of the global tool and the changes in this PR:

image

A walkthrough of the changes:

Some concerns I have and would like input on:

  1. There are now two formatting libraries being used, Sharprompt & Spectre.Console - if that is a concern I think we could replace the Sharprompt usage with Spectre.Console (but the other way likely doesn't work as Sharprompt does cover all of the scenarios Spectre does.)
  2. The colors are not currently configurable. I think it is possible to do that, so the question is do we need it up front or add it later? There is one small blocking issue that the colors used in the BreakdownChart in Program.cs are hard-coded. There is a code comment there explaining why (no way to go from string->Spectre.Color for the BreakdownChart as far as I can tell.)
  3. Redirecting console output (e.g. dotnet-gcmon -n <process> > out.txt) doesn't work after this change. I'm not sure if it can be supported with spectre live table. I left the original PrintUtilities methods around so that we could special handle the case where console output is being redirected and fall back to the plaintext formatting. Should I go ahead and do that?
  4. Since I'm using the Async APIs on the Channel, there were a few awkward sync-meets-async places in the code outlined above; should I try to switch to using the blocking Try* APIs on Channel instead, which should remove the async-related changes?

Happy new year!

Outstanding Work

MokoSan commented 2 years ago

There are now two formatting libraries being used, Sharprompt & Spectre.Console - if that is a concern I think we could replace the Sharprompt usage with Spectre.Console (but the other way likely doesn't work as Sharprompt does cover all of the scenarios Spectre does.)

Totally agreed - I was experimenting with the Spectre.Console API and it seems like it'll be easy to incorporate. I can start working on a PR to facilitate this once this PR is checked in.

This change looks AMAZING! Looking forward to seeing this in action.

Maoni0 commented 2 years ago

thanks so much for the PR!

I do think the colors would need to be configurable - when I run this in a cmd window with white background, it's not very readable -

image

you could just have 2 defaults though - one for white bg and the other for dark bg. and if you use any other colored bg, you can config it yourself.

There are now two formatting libraries being used, Sharprompt & Spectre.Console - if that is a concern I think we could replace the Sharprompt usage with Spectre.Console (but the other way likely doesn't work as Sharprompt does cover all of the scenarios Spectre does.)

I'm usually for having less components. I take it that Sharprompt does not do colors?

Redirecting console output (e.g. dotnet-gcmon -n > out.txt) doesn't work after this change.

it's definitely convenient to have the console output redirectable. if spectre simply cannot be made work for this, what do you think about making the output configurable, ie, by default it uses spectre but if you want to redirect, use the old way?

Since I'm using the Async APIs on the Channel, there were a few awkward sync-meets-async places in the code outlined above; should I try to switch to using the blocking Try* APIs on Channel instead, which should remove the async-related changes?

what kind of benefits do you believe async would provide here?

ryandle commented 2 years ago
ryandle commented 2 years ago

Working on a light color theme and ran into a small issue with the breakdown chart used in the stats view. Opened this issue to investigate a resolution: https://github.com/spectreconsole/spectre.console/issues/684

ryandle commented 2 years ago

Ok, @Maoni0 and @MokoSan I think this is ready for another pass. Here are some notes on the most recent changes:

MokoSan commented 2 years ago

Looks great!! Just tried this out on Linux to make sure we are good there, as well and things look good:

image

Details of the test machine:

MokoSan:GCRealTimeMon:% lsb_release -a                                                                                                                                                                
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 20.04 LTS
Release:        20.04
Codename:       focal

One note I am adding for myself when I tackle https://github.com/Maoni0/realmon/issues/40 is to include a prompt on the configuration side of things to include the ability to change the themes if the user wants to reconfigure the default config.

You can enable plaintext either via the yaml config or by redirecting stdout. I started to add command line parameter support too. I was thinking -nc and --no-color - but the current commandline options library only supports single character 'short' option names, and since p and n are already taken I didn't have a good idea for a param character to represent 'no color'. Open to ideas!

-d => devoid of color? We can still keep --no-color as the long name? LOL, sorry, couldn't think of anything better. 😛

Maoni0 commented 2 years ago

that's wonderful! I will take a look at the changes later today, thanks for making these changes, @ryandle!

Maoni0 commented 2 years ago

btw, do you mind explaining what is a "Spectre.Console live table"? what makes it live? 😀 does it need to look to see if the color specification changes while the process is running?

hez2010 commented 2 years ago

btw, do you mind explaining what is a "Spectre.Console live table"? what makes it live? 😀 does it need to look to see if the color specification changes while the process is running?

The "live" makes it able to update arbitrary widgets in-place, see https://spectreconsole.net/live/live-display

Maoni0 commented 2 years ago

thanks @hez2010. from the video it seems like "live" means "I can overwrite what's already displayed" but that's not what we are doing here - we don't need to overwrite anything that's already displayed. or did I understand it wrong?

ryandle commented 2 years ago

@Maoni0 I haven't yet gotten a block of time where I can sit down and reply to everything at once - but figured I should touch on the Live table Q.

So normal Spectre widgets are "Give it all the data at once and print it to the screen in your desired format." So to use a normal Spectre table, you need to give it all data up front and a full table is rendered.

The scenario for this tool is different, we want to "stream" data to the console as GC events happen. That seemed like a good fit for the Live table - which is a table where you can keep appending rows over time - you don't have to have all the data up front.

I originally tried to use the non-live table widget, thinking I could just not write the table header every time there is a new GC event and that might effectively just append a row. It didn't work as well though - IIRC there were still things like spacing before/after each table (i.e. each GC event) that made the data a lot less dense than a single continuous table.

(I also considered not using a table at all and just doing more custom formatting - in the end I thought the table looked the best and also reduced how much custom formatting/layout code we would maintain.)

I hope this helps answer your Q. LMK if you have more! And I will work on adding these details to a code comment on LiveOutputTable so it's clear what is being used and why.

Maoni0 commented 2 years ago

got it, thanks for the explanation, @ryandle! and no worries at all about responding - respond when it's convenient for you :)

Maoni0 commented 2 years ago

did you want to get rid of MessageRuleColor as we discussed above? other than that it looks great to me!

ryandle commented 2 years ago

Hey @Maoni0, I'm pushing the change now. One thing I missed above is that in the dark theme the "rule" color was different from the message color, so you had a green ruler w/ blue message like this:

image

And after the change to remove the rule color, we'll just have a single color for both like this:

image

Which is fine (I was using the same color for both already in the Light theme) I just wanted to call it out.

Maoni0 commented 2 years ago

cool; we are good to go. merging now!