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

Script to collect port polling data and compare full walk vs selectiv… #7626

Merged
merged 9 commits into from Nov 17, 2017

Conversation

Projects
None yet
4 participants
@murrant
Member

murrant commented Nov 4, 2017

…e port polling.

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

@murrant murrant referenced this pull request Nov 4, 2017

Closed

check % disabled ports before per-port-polling #7608

1 of 1 task complete
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 5, 2017

Member

Works for me. Some comments:

Add support for specifying hostnames / wildcards?

Show what the values are in (seconds) for the time column.

Change color in difference column to show if the change would be a bonus (or indicate that a - in that column is a good thing).

Flag to auto enable selected port polling for devices it's worth it for?

Added to performance docs.

I know it's a lot to ask ^^ but this quickly shows where you can get some good savings.

Member

laf commented Nov 5, 2017

Works for me. Some comments:

Add support for specifying hostnames / wildcards?

Show what the values are in (seconds) for the time column.

Change color in difference column to show if the change would be a bonus (or indicate that a - in that column is a good thing).

Flag to auto enable selected port polling for devices it's worth it for?

Added to performance docs.

I know it's a lot to ask ^^ but this quickly shows where you can get some good savings.

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Nov 10, 2017

Member

Very nice script, I like it. I too have a feature request, to add a column with the difference in percent in addition to just seconds. I can help implement the features if you merge it, or help me on how commit to this PR. :P

Member

Zmegolaz commented Nov 10, 2017

Very nice script, I like it. I too have a feature request, to add a column with the difference in percent in addition to just seconds. I can help implement the features if you merge it, or help me on how commit to this PR. :P

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 10, 2017

Member

You should be able to either push to murrants branch if you add his repo as a remote or actually just click edit on the file here and edit it directly.

Member

laf commented Nov 10, 2017

You should be able to either push to murrants branch if you add his repo as a remote or actually just click edit on the file here and edit it directly.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 10, 2017

Member

I'm glad you guys are excited :) I just threw this together as a quick way to get data so we could make a better decision on automatically enabling per-port polling.

@Zmegolaz

git remote add murrant git@github.com:murrant/librenms.git
git fetch murrant
git checkout --track murrant/collect-port-polling 

Off the top of my head, so might be slightly wrong :D

Member

murrant commented Nov 10, 2017

I'm glad you guys are excited :) I just threw this together as a quick way to get data so we could make a better decision on automatically enabling per-port polling.

@Zmegolaz

git remote add murrant git@github.com:murrant/librenms.git
git fetch murrant
git checkout --track murrant/collect-port-polling 

Off the top of my head, so might be slightly wrong :D

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Nov 10, 2017

Member

I managed! I think I'm starting to understand git now.

I added all the features in our comments, but I'm not very happy with all the hoops I had to jump through to get the printf work correctly with colors. There are also docs and CLI help to be added.

But I'll have to leave that for now, will work on it more on Sunday or Monday. I have a party to plan and host. :)

Member

Zmegolaz commented Nov 10, 2017

I managed! I think I'm starting to understand git now.

I added all the features in our comments, but I'm not very happy with all the hoops I had to jump through to get the printf work correctly with colors. There are also docs and CLI help to be added.

But I'll have to leave that for now, will work on it more on Sunday or Monday. I have a party to plan and host. :)

Zmegolaz added some commits Nov 13, 2017

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Nov 13, 2017

Member

Script done, docs left. Any comments?

Member

Zmegolaz commented Nov 13, 2017

Script done, docs left. Any comments?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 13, 2017

Member

Will take a look as soon as I can.

Thanks for doing the updates :)

Member

laf commented Nov 13, 2017

Will take a look as soon as I can.

Thanks for doing the updates :)

Zmegolaz added some commits Nov 13, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 13, 2017

Member

Hmm, I see; thoughts on including hostname? Since we are now intending people use this to set up per-port polling should we include hostname in the output?

I originally excluded it because I just wanted people to send us the output so we could make an informed decision about enabling per-port polling automatically.

Member

murrant commented Nov 13, 2017

Hmm, I see; thoughts on including hostname? Since we are now intending people use this to set up per-port polling should we include hostname in the output?

I originally excluded it because I just wanted people to send us the output so we could make an informed decision about enabling per-port polling automatically.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 13, 2017

Member

I'm not sure one pass is enough to make a good decision about enabling/disabling per-port polling.

Possibly rename the script too.

Member

murrant commented Nov 13, 2017

I'm not sure one pass is enough to make a good decision about enabling/disabling per-port polling.

Possibly rename the script too.

Allow comma separated device_id list.
Better color formatting for output
Fix issue when using &$device by reference
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Nov 13, 2017

The inspection completed: 1 updated code elements

scrutinizer-notifier commented Nov 13, 2017

The inspection completed: 1 updated code elements

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 13, 2017

Member

I think we can definitely allow people to make more use of this with the extra features. Trying to use it to gauge if we should turn it on globally may not give us enough data to make that decision.

Would be better off for us to start tracking some of the data in polling so we can record max time for a full port walk vs selected port polling and switch from one to the other as we need to maintain performance.

Member

laf commented Nov 13, 2017

I think we can definitely allow people to make more use of this with the extra features. Trying to use it to gauge if we should turn it on globally may not give us enough data to make that decision.

Would be better off for us to start tracking some of the data in polling so we can record max time for a full port walk vs selected port polling and switch from one to the other as we need to maintain performance.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 14, 2017

Member

Perhaps store this data on stats? With either an opt-out or opt-in option for that.

Member

murrant commented Nov 14, 2017

Perhaps store this data on stats? With either an opt-out or opt-in option for that.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 15, 2017

Member

Perhaps store this data on stats? With either an opt-out or opt-in option for that.

I was more meaning locally. So that the local install can make the decisions (once support is added) and then make adjustments.

Member

laf commented Nov 15, 2017

Perhaps store this data on stats? With either an opt-out or opt-in option for that.

I was more meaning locally. So that the local install can make the decisions (once support is added) and then make adjustments.

@laf

laf approved these changes Nov 16, 2017

LGTM

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Nov 17, 2017

Member

Yeah, let's merge this, we can always try to figure out the best way forward later. This is a good first step.

Member

Zmegolaz commented Nov 17, 2017

Yeah, let's merge this, we can always try to figure out the best way forward later. This is a good first step.

@laf laf merged commit af27195 into librenms:master Nov 17, 2017

2 checks passed

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

@murrant murrant deleted the murrant:collect-port-polling branch Nov 18, 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.