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

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

Merged
merged 3 commits into from Nov 20, 2017

Conversation

laf
Copy link
Member

@laf 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
Copy link
Member Author

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
Copy link

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
Copy link

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
Copy link

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 refactor: Only update sensor table when sensor values are changed refactor: Only update sensor/bgp tables when values are changed Nov 11, 2017
@laf
Copy link
Member Author

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


dbUpdate($peer['update'], 'bgpPeers', '`device_id` = ? AND `bgpPeerIdentifier` = ?', array($device['device_id'], $peer['bgpPeerIdentifier']));
foreach ($bgpPeers_fields as $db_field => $value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could actually use array_diff here.

It worked out pretty nicely for my entity state sensors.

$peer['update] = array_diff($bgpPeers_fields, $peer);

Will only return the fields from bgpPeers_fields that don't match one in $peer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shout, updated.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@murrant murrant merged commit ba51dc2 into librenms:master Nov 20, 2017
@laf laf deleted the refactor/sensor-updates branch November 20, 2017 21:43
@lock
Copy link

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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants