refactor: Improve sensors polling for performance increase #4725

Merged
merged 16 commits into from Oct 11, 2016

Projects

None yet

7 participants

@laf
Member
laf commented Oct 5, 2016

Please note

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

Do not merge yet

@laf laf added the Blocker label Oct 5, 2016
@Rosiak
Contributor
Rosiak commented Oct 5, 2016

I can test this one.

@laf laf Rebased
d441977
@laf laf added the Schema label Oct 6, 2016
@scrutinizer-notifier

The inspection completed: 11 new issues, 3 updated code elements

@laf
Member
laf commented Oct 10, 2016

Tested on my home install, I only really have a switch where this would benefit but overall no loss of data or increase in poller times.

image

@Rosiak
Contributor
Rosiak commented Oct 11, 2016

Tested and have had it running for 24 hrs, no issues seen.

@Rosiak Rosiak merged commit 6c324cd into librenms:master Oct 11, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laf laf deleted the laf:sensors-performance branch Oct 11, 2016
@worton worton added a commit to worton/librenms that referenced this pull request Oct 11, 2016
@laf @worton laf + worton refactor: Improve sensors polling for performance increase (#4725)
refactor: Improve sensors polling for performance increase
7f8d1cb
@paszczus

I think that commit has broke my UPS devices Runtime info. I has been merged 15 hours ago and just after midnigh when ./daily.sh is updating librenms to current version i have received a hundreds of notifications that runtime sensor is critical and also we can see this on graph:

upc_runtime

apc_smart

@MatGit5
MatGit5 commented Oct 12, 2016

Confirmed, it broke Runtime on my APC Smart-UPS 2200.

@paszczus

Is anyone going to revert/fix this?

@murrant
Contributor
murrant commented Oct 13, 2016

PR #4780 contains fixes for these issues, but none of the core developers have equipment to test them.
Feedback would be appreciated so we can get it merged sooner rather than later.

@Rosiak
Contributor
Rosiak commented Oct 13, 2016

Merged & tested with a device from the demo.
Please update and retest @MatGit5 @paszczus

@paszczus

Works fine for me now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment