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

#219 - changing snmp base collector to actually implement collect #310

Merged
merged 2 commits into from
Nov 25, 2016

Conversation

bmfurtado
Copy link
Contributor

Unless I missed something terribly obvious the SNMP base collector (used by all the other SNMP collectors like SNMPRawCollector, SNMPInterfaceCollector, etc) never implemented the collect() method, resulting in a NotImplementedException every time you tried to use one of these.

It's not really obvious to me how this was ever working or how it got to this state... Perhaps there was an API change at some point in the past or something.

Nevertheless, I've quickly hacked a change that makes it work (though I haven't really tested it with all the SNMP collectors, nor have I written any tests for this) and I'd like to get some feedback :)

@bmfurtado
Copy link
Contributor Author

Tests are failing due to lack of pysnmp... Will investigate later how other collectors are solving this problem with external dependencies.

@jrpaz
Copy link

jrpaz commented Oct 23, 2015

So we applied your fix and got the SNMPInterfaceCollector/SNMPRawCollector working but we have run into this issue.

BrightcoveOS/Diamond#263

Any thoughts ?

@bmfurtado
Copy link
Contributor Author

@jrpaz reading through that issue it seems like it was never fixed (just acknowledged that it was a problem) so it doesn't surprise me that's still the case...

@raphtheb
Copy link

I'm actually witnessing the same issue, it would be rather nice to have this fix merged in!

@jaingaurav
Copy link
Member

Seems like this might be a dup of PR #63, though that PR reaches further out

@bmfurtado
Copy link
Contributor Author

@jaingaurav I agree... I didn't notice that PR existed before I made this one... Feel free to close this one whenever you want assuming the other PR works (and eventually gets merged)

@olivierHa
Copy link

Hello,

@jaingaurav Any updates on this issue ? snmpcollector aren't working for a while :(

@jacksu
Copy link

jacksu commented Sep 8, 2016

why doesn‘t merge this request?

@josegonzalez
Copy link
Member

Merged, thanks for the pull request (and sorry for the wait).

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.

7 participants