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

refactor: Only update sensor/bgp tables when values are changed #7707

Merged
merged 3 commits into from Nov 20, 2017

Conversation

Projects
None yet
4 participants
@laf
Member

laf commented Nov 10, 2017

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.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

This stops updates to the sensors table until the values have actually changed. For devices that have a lot of sensors (one debug output I've just looked at had probably 1000 updates against it. This will cut that down substantially.

@shimamizu is kindly going to test this in real life.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 11, 2017

Member

If I've not broken anything then for me:

Old: Update[203/0.74s]
New: Update[84/0.24s]

Time isn't a benefit here as it's a local mysql server but for high latency (or even slow) mysql servers this will help

Member

laf commented Nov 11, 2017

If I've not broken anything then for me:

Old: Update[203/0.74s]
New: Update[84/0.24s]

Time isn't a benefit here as it's a local mysql server but for high latency (or even slow) mysql servers this will help

@shimamizu

This comment has been minimized.

Show comment
Hide comment
@shimamizu

shimamizu Nov 11, 2017

K so earlier today before that patch 7707 we had this for mysql:
MySQL: Cell[17/0.03s] Row[-5/-0.00s] Rows[5/0.02s] Column[3/0.00s] Update[1139/4.24s] Insert[2/0.01s] Delete[0/0.00s]

and now after 7707
MySQL: Cell[17/7.91s] Row[-3/-5.23s] Rows[23/9.84s] Column[3/0.58s] Update[731/141.13s] Insert[3/1.72s] Delete[0/0.00s]

that did lower the updates it looks like

I'm going to enable Enable selected port polling? for this test device next and see if it goes even lower

Test device is an Arista 7508 for reference

shimamizu commented Nov 11, 2017

K so earlier today before that patch 7707 we had this for mysql:
MySQL: Cell[17/0.03s] Row[-5/-0.00s] Rows[5/0.02s] Column[3/0.00s] Update[1139/4.24s] Insert[2/0.01s] Delete[0/0.00s]

and now after 7707
MySQL: Cell[17/7.91s] Row[-3/-5.23s] Rows[23/9.84s] Column[3/0.58s] Update[731/141.13s] Insert[3/1.72s] Delete[0/0.00s]

that did lower the updates it looks like

I'm going to enable Enable selected port polling? for this test device next and see if it goes even lower

Test device is an Arista 7508 for reference

@shimamizu

This comment has been minimized.

Show comment
Hide comment
@shimamizu

shimamizu Nov 11, 2017

Wrong before comparison for the time it took on the slow server:
MySQL: Cell[17/9.43s] Row[3/-5.61s] Rows[30/12.11s] Column[3/0.96s] Update[1140/220.15s] Insert[3/1.72s] Delete[0/0.00s]

But still 220s before for updates -> 141s after for updates (about 400 less updates) with this change.

Also testing now with selected port polling next.

shimamizu commented Nov 11, 2017

Wrong before comparison for the time it took on the slow server:
MySQL: Cell[17/9.43s] Row[3/-5.61s] Rows[30/12.11s] Column[3/0.96s] Update[1140/220.15s] Insert[3/1.72s] Delete[0/0.00s]

But still 220s before for updates -> 141s after for updates (about 400 less updates) with this change.

Also testing now with selected port polling next.

@shimamizu

This comment has been minimized.

Show comment
Hide comment
@shimamizu

shimamizu Nov 11, 2017

seeing about the same # of mysql updates with selected port polling + this patch

SNMP: Get[416/5.68s] Getnext [0/0.00s] Walk [13/0.39s]
MySQL: Cell[17/9.46s] Row[2/-5.79s] Rows[61/23.83s] Column[3/0.97s] Update[724/141.50s] Insert[3/1.75s] Delete[0/0.00s]

shimamizu commented Nov 11, 2017

seeing about the same # of mysql updates with selected port polling + this patch

SNMP: Get[416/5.68s] Getnext [0/0.00s] Walk [13/0.39s]
MySQL: Cell[17/9.46s] Row[2/-5.79s] Rows[61/23.83s] Column[3/0.97s] Update[724/141.50s] Insert[3/1.75s] Delete[0/0.00s]

@laf laf changed the title from refactor: Only update sensor table when sensor values are changed to refactor: Only update sensor/bgp tables when values are changed Nov 11, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 11, 2017

Member

Yes you won't see much of a difference using selected port polling with or not with the patch as we already filter out updates that aren't needed.

So to be clear in your scenario @shimamizu:

Local poller in Isreal
Only this patch will help. SNMP should be as quick as you'd expect here.
Whether we can do something to speed up RRD is another question.

Local poller in US
Selected port polling will help here potentially as you are looking to save snmp time over anything else.
Disabling unused modules will also help.
You should also look at snmp max repeaters and also snmp max oids

Member

laf commented Nov 11, 2017

Yes you won't see much of a difference using selected port polling with or not with the patch as we already filter out updates that aren't needed.

So to be clear in your scenario @shimamizu:

Local poller in Isreal
Only this patch will help. SNMP should be as quick as you'd expect here.
Whether we can do something to speed up RRD is another question.

Local poller in US
Selected port polling will help here potentially as you are looking to save snmp time over anything else.
Disabling unused modules will also help.
You should also look at snmp max repeaters and also snmp max oids

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Nov 14, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Nov 14, 2017

The inspection completed: No new issues

@murrant murrant merged commit ba51dc2 into librenms:master Nov 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the laf:refactor/sensor-updates branch Nov 20, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

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