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

Checking for valid CPU average values #97

Merged
merged 4 commits into from Oct 26, 2018
Merged

Checking for valid CPU average values #97

merged 4 commits into from Oct 26, 2018

Conversation

@patschi
Copy link
Member

@patschi patschi commented Jun 1, 2017

Added sanity-checks to validate return values by sys_getloadavg() before finally using them. This fixes errors on JavaScript side when it returns 0 (zero/null) or any other unexpected values.
Should solve #90 point 1.

Signed-off-by: Patrik Kernstock <info@pkern.at>
Signed-off-by: Patrik Kernstock <info@pkern.at>
@patschi
Copy link
Member Author

@patschi patschi commented Jun 1, 2017

cc @schiessle @nickvergessen May anyone check this if it's okay?

if (!(is_array($loadavg) && count($loadavg) === 3)) {
// either no array or too few array keys.
// returning back zeroes to prevent any errors on JS side.
$loadavg = [0, 0, 0];

This comment has been minimized.

@schiessle

schiessle Jun 8, 2017
Member

wonder if it would make more sense to return a error code (maybe 'N/A' like we use for other statistics as well) and then hide the CPU Load in the web interface or display something like "not available". Otherwise it looks like everything is OK and the system load is just super low, something which makes probably every admin happy. 😉

This comment has been minimized.

@patschi

patschi Jun 9, 2017
Author Member

Done! Similar as when failing to retrieve memory usage.
image

}

return [
'loadavg' => $loadavg

This comment has been minimized.

@schiessle

schiessle Jun 8, 2017
Member

why do you put this in another array and don't return $loadavg directly?

This comment has been minimized.

@patschi

patschi Jun 9, 2017
Author Member

To allow easier extending the information without breaking something on the JS side. To be more extensible for the future. when extending stats with more CPU graph lines someday (e.g. splitting up CPU usage in user, system, kernel or something like that). But I can remove array if you want.

*
* @return array load average with three values, 1/5/15 minutes average.
*/
protected function getProcessorUsage() {

This comment has been minimized.

@schiessle

schiessle Jun 8, 2017
Member

Think this would be a nice method to write a small unit test.

patschi added 2 commits Jun 9, 2017
Signed-off-by: Patrik Kernstock <info@pkern.at>
The graphs gets hidden when invalid data ('N/A') gets delivered from the server-side. When valid data gets delivered without reloading page again, graphs are still hidden. Fixed.

Signed-off-by: Patrik Kernstock <info@pkern.at>
@patschi
Copy link
Member Author

@patschi patschi commented Jun 14, 2017

As requested I've added warning on the client-side if CPU average couldn't retrieved on the server-side.

@MorrisJobke MorrisJobke requested review from Ivansss and nickvergessen Aug 20, 2018
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Aug 20, 2018
@nickvergessen nickvergessen requested review from schiessle and removed request for nickvergessen Aug 23, 2018
@MorrisJobke MorrisJobke mentioned this pull request Aug 30, 2018
6 of 6 tasks complete
@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Aug 30, 2018

I will move this to 15 so @schiessle can have a look at this.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Code looks good 👍

@MorrisJobke MorrisJobke merged commit 743fe8c into nextcloud:master Oct 26, 2018
3 checks passed
3 checks passed
Scrutinizer 2 updated code elements
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants