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

Rework read_stats output formatting #703

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

Conversation

dementive
Copy link
Contributor

Makes the output of the --stats argument easier to read.

  • Formatted the output of each core properly into a table, the spacing was slightly off before.

  • Colored the headers of each table column in green for easier visual parsing

  • Colored the temperature reading in red for any core that is above 85 degrees so it's easy to quickly tell if something is going wrong. Might want to adjust the number it is pretty arbitrary.

This is kind of jank but only because it was already jank to begin with. The reading and writing of stats as a whole should probably be refactored to use a more convenient interface but it works fine as it is now so I'm not sure if it's actually necessary.

Old Output:
old_output

New Output:
new_output

Makes the output of the --stats argument a lot more readable.

* Formatted the output of each core properly into a table

* Colored the headers of each table column in green for easier visual parsing

* Colored the temperature reading in red for any core that is above 85 degrees so it's easy to quickly tell if something is going wrong. Might want to adjust the number it is pretty arbitrary.

This is kind of jank but only because it was already jank to begin with. The reading and writing of stats as a whole should probably be refactored to use a more convenient interface but it works fine as it is now so I'm not sure if it's actually necessary.
@shadeyg56
Copy link
Collaborator

Personally, I like this change and do think it looks much nicer, so I'm approving this, but leaving it up to @AdnanHodzic to merge this, in case he wants some changes.

The reading and writing of stats as a whole should probably be refactored to use a more convenient interface but it works fine as it is now so I'm not sure if it's actually necessary

I agree with this as well. I think taking over stdout for the stats is a bit unnecessary. Having the stats written to a file and read from there makes sense to me but doing it through stdout is weird and can be confusing at times. I think a good solution would be creating a class similar to Logging that can handle stats writing/reading without stopping up stdout, which I'm down to start working on a PR for.

@shadeyg56 shadeyg56 requested a review from AdnanHodzic May 17, 2024 22:23
@shadeyg56 shadeyg56 added the enhancement New feature or request label May 17, 2024
Copy link
Owner

@AdnanHodzic AdnanHodzic left a comment

Choose a reason for hiding this comment

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

I like how it looks and I love the addition of colors and I agree regarding logging, there was an old PR regarding this #182 that unfortunately never got finished.

Regardless, I have a small request. Currently if you're looking at stats they look static, and only after you scroll up that you realize that they are being refreshed.

-------------------------------------------------------------------------------

Linux distro: Ubuntu 24.04 Noble Numbat
Linux kernel: 6.8.0-31-generic
Processor: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
Cores: 8
Architecture: x86_64
Driver: intel_pstate

------------------------------ Current CPU stats ------------------------------

CPU max frequency: 800 MHz
CPU min frequency: 400 MHz

Core	Usage	Temperature	Frequency
CPU0    1.0%       40°C          800 MHz
CPU1    2.0%       39°C          800 MHz
CPU2    9.9%       39°C          400 MHz
CPU3    2.0%       40°C          400 MHz
CPU4    2.0%       40°C          801 MHz
CPU5    0.0%       39°C          400 MHz
CPU6    2.0%       39°C          800 MHz
CPU7    2.0%       40°C          800 MHz

CPU fan speed: 0 RPM

---------------------------- CPU frequency scaling ----------------------------

Battery is: discharging

Setting to use: "powersave" governor
Setting to use: "balance_power" EPP

Total CPU usage: 9.7 %
Total system load: 2.14
Average temp. of all cores: 39.50 °C

Load optimal (load average: 2.14, 1.94, 1.01)
setting turbo boost: off

-------------------------------------------------------------------------------

"auto-cpufreq" is about to refresh ...
Executed on: May 20 (Monday) - 21:00:50

-------------------------------------------------------------------------------

Linux distro: Ubuntu 24.04 Noble Numbat
Linux kernel: 6.8.0-31-generic
Processor: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
Cores: 8
Architecture: x86_64
Driver: intel_pstate

------------------------------ Current CPU stats ------------------------------

CPU max frequency: 800 MHz
CPU min frequency: 400 MHz

Core	Usage	Temperature	Frequency
CPU0    2.0%       40°C          800 MHz
CPU1    4.0%       39°C          800 MHz
CPU2    2.0%       39°C          800 MHz
CPU3    1.1%       40°C          800 MHz
CPU4    3.0%       40°C          800 MHz
CPU5    0.0%       39°C          400 MHz
CPU6    0.0%       39°C          400 MHz
CPU7    2.0%       40°C          800 MHz

CPU fan speed: 0 RPM

---------------------------- CPU frequency scaling ----------------------------

Battery is: discharging

Setting to use: "powersave" governor
Setting to use: "balance_power" EPP

Total CPU usage: 2.5 %
Total system load: 2.29
Average temp. of all cores: 39.50 °C

Load optimal (load average: 2.29, 1.98, 1.02)
setting turbo boost: off

-------------------------------------------------------------------------------

But could you please add the: "auto-cpufreq" is about to refresh ... at the bottom from how it used to be? This way, whoever's looking at stats get an impression something is happening/about to happen, i.e:

-------------------------------------------------------------------------------

Linux distro: Ubuntu 24.04 Noble Numbat
Linux kernel: 6.8.0-31-generic
Processor: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
Cores: 8
Architecture: x86_64
Driver: intel_pstate

------------------------------ Current CPU stats ------------------------------

CPU max frequency: 800 MHz
CPU min frequency: 400 MHz

Core	Usage	Temperature	Frequency
CPU0     22.2%        40 °C       700 MHz
CPU1     12.4%        39 °C       699 MHz
CPU2     11.2%        39 °C       700 MHz
CPU3     31.6%        40 °C       700 MHz
CPU4     22.2%        40 °C       700 MHz
CPU5     10.2%        39 °C       700 MHz
CPU6     15.5%        39 °C       700 MHz
CPU7     13.4%        40 °C       700 MHz

CPU fan speed: 0 RPM

---------------------------- CPU frequency scaling ----------------------------

Battery is: discharging

Setting to use: "powersave" governor
Setting to use: "balance_power" EPP

Total CPU usage: 23.1 %
Total system load: 2.06
Average temp. of all cores: 39.50 °C 

High CPU load (load average: 2.06, 1.58, 1.12)
setting turbo boost: on

-------------------------------------------------------------------------------

		"auto-cpufreq" is about to refresh ...
		Executed on: May 20 (Monday) - 21:07:14

-------------------------------------------------------------------------------

Linux distro: Ubuntu 24.04 Noble Numbat
Linux kernel: 6.8.0-31-generic
Processor: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
Cores: 8
Architecture: x86_64
Driver: intel_pstate

------------------------------ Current CPU stats ------------------------------

CPU max frequency: 4600 MHz
CPU min frequency: 400 MHz

Core	Usage	Temperature	Frequency
CPU0     12.1%        40 °C       800 MHz
CPU1      9.2%        40 °C       800 MHz
CPU2     16.5%        40 °C       800 MHz
CPU3     18.6%        40 °C       799 MHz
CPU4     33.7%        40 °C       800 MHz
CPU5      7.2%        40 °C       800 MHz
CPU6      7.0%        40 °C       800 MHz
CPU7      5.2%        40 °C       800 MHz

CPU fan speed: 0 RPM

---------------------------- CPU frequency scaling ----------------------------

Battery is: discharging

Setting to use: "powersave" governor
Setting to use: "balance_power" EPP

Total CPU usage: 18.6 %
Total system load: 1.90
Average temp. of all cores: 40.00 °C 

Load optimal (load average: 1.90, 1.56, 1.12)
setting turbo boost: off

-------------------------------------------------------------------------------

		"auto-cpufreq" is about to refresh ...

@dementive
Copy link
Contributor Author

I just tried to add the auto-cpufreq" is about to refresh ... part to the bottom but I can't get it working for some reason. It is currently already printing the about to refresh text but it is printing it at the top of the next refresh instead of the bottom of the last refresh and because of the way the stdout stuff works I don't see a way to fix it but im probably just missing something. Adding it to the bottom of the for loop didn't work and checking for it inside the for loop also didn't work...

@shadeyg56
Copy link
Collaborator

Yeah it might just be best to merge this into its own branch since I'm in the midst of the stats rewrite right now. Then I can include your changes with mine and add you as a co-author

@dementive
Copy link
Contributor Author

I agree, no need to merge this a stats rework will be much better, this code is pretty weird anyway because of how the stats currently work. You can just grab the parts you need out of this and include them in your rework its probably not necessary to merge it.

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented May 25, 2024

Okay, then let's leave this PR open until PR with @shadeyg56 changes are merged, or even feel free to close it @dementive and create a new PR once ready? Up to you ...

@AdnanHodzic
Copy link
Owner

Are there any updates regarding this PR?

@dementive
Copy link
Contributor Author

Did that stats rework ever come through? If not iirc this should probably be fine to merge unless you still have some tweaks you want me to make.

@AdnanHodzic
Copy link
Owner

Did that stats rework ever come through? If not iirc this should probably be fine to merge unless you still have some tweaks you want me to make.

@shadeyg56 ?

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

Successfully merging this pull request may close these issues.

3 participants