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

webgui: Added ability to set warning percentage for CPU, mempools fro… #5901

Merged
merged 3 commits into from Mar 3, 2017

Conversation

Projects
None yet
8 participants
@rpenziol
Contributor

rpenziol commented Feb 15, 2017

…m device edit page (#5895)

*Set health progress bars for store, CPU, mempools to reach red threshold based on warning percentage

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 15, 2017

Thank you for submitting a PR @rpenziol! We have found the following @murrant, @Rosiak and @laf based on the history of these files to review this PR.

mention-bot commented Feb 15, 2017

Thank you for submitting a PR @rpenziol! We have found the following @murrant, @Rosiak and @laf based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Feb 15, 2017

Auto-Deploy finished, Test PR at http://5901.ci.librenms.org or https://5901.ci.librenms.org

Show outdated Hide outdated build.sql
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 17, 2017

Member

Absolutely brilliant, thanks for contributing this :)

I'm not sure I'd agree the defaults for all of this should be 90%, it's too close to the max to be potentially useful. 60% or upto 75% seems more appropriate imho ( @librenms/reviewers ?)

The other alternative is we have a $config array that we use to set this and people can override:

$config['health']['limits']['storage'] = array(
    'high' => 90,
); 
$config['health']['limits']['memory'] = array(
    'high' => 90,
);
Member

laf commented Feb 17, 2017

Absolutely brilliant, thanks for contributing this :)

I'm not sure I'd agree the defaults for all of this should be 90%, it's too close to the max to be potentially useful. 60% or upto 75% seems more appropriate imho ( @librenms/reviewers ?)

The other alternative is we have a $config array that we use to set this and people can override:

$config['health']['limits']['storage'] = array(
    'high' => 90,
); 
$config['health']['limits']['memory'] = array(
    'high' => 90,
);
@rpenziol

This comment has been minimized.

Show comment
Hide comment
@rpenziol

rpenziol Feb 18, 2017

Contributor

Thanks @laf ! The CPU warning level defaulted to 60% before. I changed it to 90% to match the normal 'red' colored progress bar threshold. In hindsight I probably shouldn't have touched that value. Keeping it at 60% to not mess up anyone's existing settings sounds like the best course of action.

Contributor

rpenziol commented Feb 18, 2017

Thanks @laf ! The CPU warning level defaulted to 60% before. I changed it to 90% to match the normal 'red' colored progress bar threshold. In hindsight I probably shouldn't have touched that value. Keeping it at 60% to not mess up anyone's existing settings sounds like the best course of action.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 23, 2017

Member

@rpenziol are you updating this to revert the default back to 60%?

Member

laf commented Feb 23, 2017

@rpenziol are you updating this to revert the default back to 60%?

@rpenziol

This comment has been minimized.

Show comment
Hide comment
@rpenziol

rpenziol Feb 23, 2017

Contributor

@laf Yes, I will revert it and push my changes. I'll try to push my changes either tomorrow or Monday

Contributor

rpenziol commented Feb 23, 2017

@laf Yes, I will revert it and push my changes. I'll try to push my changes either tomorrow or Monday

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 23, 2017

Member

Ace, thanks :)

Member

laf commented Feb 23, 2017

Ace, thanks :)

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Feb 24, 2017

Auto-Deploy finished, Test PR at http://5901.ci.librenms.org or https://5901.ci.librenms.org

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 25, 2017

Member

You've still got some defaults of 90 in there and imho a default of 90 is too high.

@librenms/reviewers anyone else?

Member

laf commented Feb 25, 2017

You've still got some defaults of 90 in there and imho a default of 90 is too high.

@librenms/reviewers anyone else?

@crcro

This comment has been minimized.

Show comment
Hide comment
@crcro

crcro Feb 25, 2017

Contributor

it think a value around 70-75% should be more eloquent then 60 or 90 ...

Contributor

crcro commented Feb 25, 2017

it think a value around 70-75% should be more eloquent then 60 or 90 ...

@rpenziol

This comment has been minimized.

Show comment
Hide comment
@rpenziol

rpenziol Feb 25, 2017

Contributor

@laf @crcro My motivation to setting it to 90% is that this value will determine the threshold on progress bars for CPU/mempool usage to turn bright red. Currently that threshold is 90% for all progress bars. The thought being that if this is lowered it may alarm users - as many more progress bars will be bright red than before. I also realize people will be able to set alerts based off of these new values, and 90% could be considered too high for many. I have no problem changing it per your recommendations.

Contributor

rpenziol commented Feb 25, 2017

@laf @crcro My motivation to setting it to 90% is that this value will determine the threshold on progress bars for CPU/mempool usage to turn bright red. Currently that threshold is 90% for all progress bars. The thought being that if this is lowered it may alarm users - as many more progress bars will be bright red than before. I also realize people will be able to set alerts based off of these new values, and 90% could be considered too high for many. I have no problem changing it per your recommendations.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 1, 2017

Member

It's my opinion (and crcro as well) so I'd be interested in other opinions ( @librenms/reviewers ) but yes, 75% would be an acceptable value.

I realise this may show more red visually but the alerts are what are more important as those are automated, the former relies on people actually browsing to those specific pages.

Member

laf commented Mar 1, 2017

It's my opinion (and crcro as well) so I'd be interested in other opinions ( @librenms/reviewers ) but yes, 75% would be an acceptable value.

I realise this may show more red visually but the alerts are what are more important as those are automated, the former relies on people actually browsing to those specific pages.

@rpenziol

This comment has been minimized.

Show comment
Hide comment
@rpenziol

rpenziol Mar 1, 2017

Contributor

@laf I don't have an objection with 75%. I'll wait for more opinions or until you give the go-ahead to update my pull request with an agreed-upon percentage.

Contributor

rpenziol commented Mar 1, 2017

@laf I don't have an objection with 75%. I'll wait for more opinions or until you give the go-ahead to update my pull request with an agreed-upon percentage.

@geordish

This comment has been minimized.

Show comment
Hide comment
@geordish

geordish Mar 1, 2017

Contributor

75% sounds good to me.

Contributor

geordish commented Mar 1, 2017

75% sounds good to me.

Robert Penziol added some commits Feb 10, 2017

Robert Penziol
webgui: Added ability to set warning percentage for CPU, mempools fro…
…m device edit page (#5895)

	*Set health progress bars for store, CPU, mempools to reach red threshold based on warning percentage
Robert Penziol
webui: Set warning percentage to 75% for CPU/mempools/storage (#5895)…
…. Rebased with master to bring 1b30173 up to date.

Updated schema
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Mar 1, 2017

Auto-Deploy finished, Test PR at http://5901.ci.librenms.org or https://5901.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Mar 3, 2017

Auto-Deploy finished, Test PR at http://5901.ci.librenms.org or https://5901.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Mar 3, 2017

The inspection completed: 2 new issues

scrutinizer-notifier commented Mar 3, 2017

The inspection completed: 2 new issues

@laf laf merged commit afb838b into librenms:master Mar 3, 2017

2 checks passed

Auto-Deploy Build finished.
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