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

Enhancements to SNMPCollector #63

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

shaunduncan
Copy link
Contributor

In relation to Issue #59, I've taken the liberty to make some changes/enhancements based on some of the behavior I've seen. I couldn't tell if there was a major difference between the SNMPCollector and SNMPRawCollector, so I've combined that into just the base SNMPCollector. I've also included a large amount of tests with this as well. The changes here should be backwards compatible as some of the method signatures have not changed. I've also tested this on a VM running snmpd using both this improved SNMPCollector and the unmodified SNMPInterfaceCollector.

I've also added an enhancement to perform SNMP walks via a configuration. For example:

[oids]
1.2.3.* = foo.bar

Would produce metrics like foo.bar.1 and foo.bar.2.1.1.1.

Please let me know if this change makes sense (or if it's just a terrible idea). I'd be happy to hear the feedback. Thanks!

@bmhatfield
Copy link
Contributor

Hi - This PR appears to have PEP8 issues. Would you mind cleaning that up? Check the Travis build log for more information.

@shaunduncan
Copy link
Contributor Author

Absolutely. I'll take a gander at the failures.

@jaingaurav
Copy link
Member

I'd like to revisit this PR @shaunduncan is this still something you're willing to clean up and help get merged in?

@shaunduncan
Copy link
Contributor Author

@jaingaurav absolutely! I actually forgot I had this sitting around. I'll merge the upstream branch and re-push

Conflicts:
	src/collectors/snmp/test/testsnmp.py
	src/collectors/snmpraw/test/testsnmpraw.py
@josegonzalez
Copy link
Member

@shaunduncan so sorry, mind rebasing this one more time? It looks fucking awesome, just wanted to fix a few snmp bugs before this dropped. Sorry!

@jantman
Copy link

jantman commented Jun 9, 2017

Sure does bring back old memories when I stumbled across this PR.

@josegonzalez @shaunduncan If shaun doesn't have any interest in this project anymore, I'd be willing to pick up the PR and get it rebased and updated for the current state of the code. Shaun and I were working together at a previous employer when this was first opened, but I actually stumbled across it this morning when I was trying to add monitoring of my cable modem and router to a Diamond instance I run on my home computer.

I was a bit dismayed by the current state of the SNMP Collector, so I'd be happy to get it working and update this if there's a chance of it being merged and released.

@shortdudey123
Copy link
Member

@jantman Feel free to rebase this and open a new PR. Reference this one when doing so.

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

Successfully merging this pull request may close these issues.

6 participants