proc.plugin: add pressure stall information by hexchain · Pull Request #7209 · netdata/netdata · GitHub

Summary

Add PSI monitoring to proc.plugin. Fixes #5901.

Component Name

collectors/proc.plugin

Additional Information

This is WIP because:

Explanation text on the dashboard is currently missing
CGroup2 PSI monitoring is currently missing

Things I'd like to make sure before continuing:

Currently, it resides under "System", and has 5 graphs. Does it deserve its own section?
Is there a better way to arrange them?
The "total" fields are not graphed. Do we need them?
Do we need a switch for each graph (like in proc_loadavg)?

The procfile_lines returns 2 for /proc/pressure/cpu, which should only have 1 line, for example:

% python                   
Python 3.7.4 (default, Oct  4 2019, 06:57:26) 
[GCC 9.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> open('/proc/pressure/cpu', 'rb').read()
b'some avg10=3.22 avg60=4.15 avg300=4.07 total=11747246754\n'

Is this intended or is it a bug? Currently, I'm using lines > 2 to workaround this behavior. What would be a better way to handle this?

Screenshot

Manage this branch in Squash

Test this branch here: https://hexchainproc-plugin-psi-w3rzz.squash.io

All committers have signed the CLA.

Approving changes to the files for which I am the code owner.

Currently, it resides under "System", and has 5 graphs. Does it deserve its own section?

Yes

Is there a better way to arrange them?

I don't know if it's a better way, but you can put the cpu and memory charts under the relevant first-level menu sections. Personally, I'm OK with the way it is.

The "total" fields are not graphed. Do we need them?

The total absolute stall time is a different metric from what the ratios show. I think we need separate charts for the totals.

Do we need a switch for each graph (like in proc_loadavg)?

Do you mean enable/disable configuration options? Yes, we need them.

The procfile_lines returns 2 for /proc/pressure/cpu, which should only have 1 line... Is this intended or is it a bug?

I think it is a bug, but I suggest to stick with the workaround for now.

Updated to add a "pressure" section (no dashboard changes yet) and a configuration toggle for every graph.

@hexchain please avoid force-pushes

#7059 (comment)

@ilyam8 Sure, sorry for that.

Just a few small fixes and the docs will be ready.

I would tend to agree with @ilyam8 on this one. Conceptually, PSI is supposed to be a better replacement for the load average metrics, so I'd suggest at least putting it in the System Overview section like the load average is.

In an ideal situation, I'd expect to see the CPU PSI info directly below the main CPU chart (where Load Average is right now), the IO PSI info directly below the main Disk IO charts (though that might be a bit misleading, as the IO tracking is all IO, not just disk IO), and the Memory PSI info directly below the main RAM and Swap charts.

Ok, i read the article

https://lwn.net/Articles/759658/

And i agree with @Ferroin ideal situation

I can implement this but that would be another 5 more charts

I don't see any problems with five more charts since they are useful. If you wish, you can implement them in a different PR.

I can implement this but that would be another 5 more charts

I don't see any problems with five more charts since they are useful. If you wish, you can implement them in a different PR.

Fine, I'll defer that to another PR.

I can implement this but that would be another 5 more charts

I don't see any problems with five more charts since they are useful. If you wish, you can implement them in a different PR.

Fine, I'll defer that to another PR.

Just a quick thought, but might it make sense to have the total absolute time charts be optional? Some people may not be interested in them and if there are a lot of cgroups on the system they may want to reduce Netdata's resource usage (RAM and disk space) by turning them off without losing the other charts.

Now that I think of it, it may make sense to do similarly for each type of chart (CPU/Memory/IO).

Now that I think of it, it may make sense to do similarly for each type of chart (CPU/Memory/IO).

Currently, all charts are optional and can be turned off individually.

@hexchain please finish with the menu sections so we could merge the PR.

In an ideal situation, I'd expect to see the CPU PSI info directly below the main CPU chart (where Load Average is right now), the IO PSI info directly below the main Disk IO charts (though that might be a bit misleading, as the IO tracking is all IO, not just disk IO), and the Memory PSI info directly below the main RAM and Swap charts.

The charts are now arranged in this way, but I'm kinda unsure about the description texts.

In an ideal situation, I'd expect to see the CPU PSI info directly below the main CPU chart (where Load Average is right now), the IO PSI info directly below the main Disk IO charts (though that might be a bit misleading, as the IO tracking is all IO, not just disk IO), and the Memory PSI info directly below the main RAM and Swap charts.

The charts are now arranged in this way, but I'm kinda unsure about the description texts.

I think what you've got right now is good actually, it's concise but still conveys the most important information and includes a link to more details, though I'd suggest replacing 'on a given resource' with a reference to the particular resource (CPU/Memory/IO).

@Ferroin

I'd suggest replacing 'on a given resource' with a reference to the particular resource (CPU/Memory/IO).

Done.

For some reason I can't see my previous requested changes, but at this point the .md and dashboard_info.js content LGTM.

@hexchain thanks! good work 👍

* proc.plugin: add pressure stall information * dashboard_info: add "Pressure" section * proc.plugin: mention PSI collector in doc * dashboard_info: fix grammar in PSI section * proc_pressure: fix wrong line name for "full" metrics * proc_pressure: fix copypasta * proc_pressure: refactor to prepare for cgroup changes * cgroups.plugin: add pressure monitoring * add proc_pressure.h to targets * Makefile.am: fix indentation * cgroups.plugin: remove a useless comment * cgroups.plugin: fix pressure config name * proc.plugin: arrange pressure charts under corresponding sections * dashboard_info: rearrange pressure chart descriptions * dashboard_info: reword PSI descriptions

hexchain requested review from cakrit, cosmix, knatsakis, ktsaou, mfundul and vlvkobal as code owners October 28, 2019 19:07

knatsakis previously approved these changes Oct 28, 2019

View reviewed changes

netdatabot added the area/collectors Everything related to data collection label Oct 29, 2019

hexchain dismissed knatsakis’s stale review via eb87be2 October 29, 2019 21:02

hexchain force-pushed the proc-plugin-psi branch from 40aede2 to eb87be2 Compare October 29, 2019 21:02

hexchain requested review from joelhans and vlegakis as code owners October 30, 2019 00:04

netdatabot added area/docs area/web labels Oct 30, 2019

joelhans suggested changes Oct 30, 2019

View reviewed changes

Comment thread web/gui/dashboard_info.js Outdated

Comment thread web/gui/dashboard_info.js Outdated

hexchain changed the title [WIP] proc.plugin: add pressure stall information proc.plugin: add pressure stall information Nov 14, 2019

hexchain added 8 commits November 14, 2019 15:44

proc.plugin: add pressure stall information

8225a63

dashboard_info: add "Pressure" section

9dead5c

proc.plugin: mention PSI collector in doc

9f8a506

dashboard_info: fix grammar in PSI section

3c39b73

proc_pressure: fix wrong line name for "full" metrics

0745a3b

proc_pressure: fix copypasta

a1476f5

proc_pressure: refactor to prepare for cgroup changes

47d4f14

cgroups.plugin: add pressure monitoring

326cc42

hexchain added 2 commits November 28, 2019 15:41

cgroups.plugin: fix pressure config name

1b7992a

proc.plugin: arrange pressure charts under corresponding sections

9806dee

dashboard_info: rearrange pressure chart descriptions

c8a07ba

vlvkobal previously approved these changes Nov 29, 2019

View reviewed changes

ilyam8 requested a review from stelfrag November 29, 2019 09:09

dashboard_info: reword PSI descriptions

e50b815

hexchain dismissed vlvkobal’s stale review via e50b815 November 29, 2019 14:40

Ferroin approved these changes Dec 2, 2019

View reviewed changes

joelhans approved these changes Dec 2, 2019

View reviewed changes

vlvkobal approved these changes Dec 2, 2019

View reviewed changes

thiagoftsm approved these changes Dec 2, 2019

View reviewed changes

thiagoftsm merged commit 8a70725 into netdata:master Dec 2, 2019

hexchain deleted the proc-plugin-psi branch December 2, 2019 21:28

Saruspete mentioned this pull request Dec 15, 2019

Cannot read from file '/proc/pressure/cpu' on fd 91 (errno 95, Operation not supported) #7531

Closed

This was referenced Dec 16, 2019

> Greetings, #7540

Closed

netdata ERROR : MAIN : Master thread EXPORTING takes too long to exit. Giving up... #7541

Closed

rbjorklin mentioned this pull request Oct 23, 2020

Add Pressure Stall Information (PSI) for cgroups #10134

Closed

proc.plugin: add pressure stall information by hexchain · Pull Request #7209 · netdata/netdata · GitHub

Uh oh!

proc.plugin: add pressure stall information - #7209

proc.plugin: add pressure stall information#7209thiagoftsm merged 15 commits intonetdata:masternetdata/netdata:masterfrom hexchain:proc-plugin-psihexchain/netdata:proc-plugin-psiCopy head branch name to clipboard

Conversation

hexchain commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Component Name

Additional Information

Uh oh!

squash-labs Bot commented Oct 28, 2019

Manage this branch in Squash

Uh oh!

CLAassistant commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

vlvkobal commented Oct 29, 2019

Uh oh!

hexchain commented Oct 29, 2019

Uh oh!

ilyam8 commented Oct 29, 2019

Uh oh!

hexchain commented Oct 29, 2019

Uh oh!

joelhans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ferroin commented Nov 27, 2019

Uh oh!

ilyam8 commented Nov 27, 2019

Uh oh!

vlvkobal commented Nov 27, 2019

Uh oh!

hexchain commented Nov 27, 2019

Uh oh!

Ferroin commented Nov 28, 2019

Uh oh!

hexchain commented Nov 28, 2019

Uh oh!

vlvkobal commented Nov 28, 2019

Uh oh!

hexchain commented Nov 28, 2019

Uh oh!

Ferroin commented Nov 29, 2019

Uh oh!

hexchain commented Nov 29, 2019

Uh oh!

joelhans left a comment

Choose a reason for hiding this comment

Uh oh!

ilyam8 commented Dec 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

proc.plugin: add pressure stall information#7209
thiagoftsm merged 15 commits into
netdata:masterfrom
hexchain:proc-plugin-psi

hexchain commented Oct 28, 2019 •
edited
Loading

CLAassistant commented Oct 28, 2019 •
edited
Loading