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

Add service check to the SNMP check #1236

Merged
merged 2 commits into from
Dec 22, 2014
Merged

Add service check to the SNMP check #1236

merged 2 commits into from
Dec 22, 2014

Conversation

LotharSee
Copy link
Contributor

Add a service check "snmp.can_check".

Since pysnmp doesn't distinguish network errors, MIB errors, ... I only report a check if we manage to collect all the metrics properly.

In the same time:

  • Add tests to cover the service check
  • Make timeout/retries changeable in the tests to run faster

I also thought about building service check from the configuration (report a specific OID as a service check) but I'm not sure that it makes sense/it is useful. So I think that it's good for now!

@LotharSee LotharSee changed the title Add service checks to the SNMP check Add service check to the SNMP check Dec 8, 2014
@LeoCavaille LeoCavaille added this to the 5.2.0 milestone Dec 10, 2014
tags = ["snmp_device:%s" % ip_address]
if "service_check_error" in instance:
self.service_check(service_check_name, AgentCheck.CRITICAL, tags=tags,
message=instance["service_check_error"])
Copy link
Member

Choose a reason for hiding this comment

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

If we look at line 140/141, we can set service_check_error without raising, and because you never reset this key in the instance dict, it will fail indefinitely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the previous logic around error_indication and error_status, however in both case I'll return a ERROR service check.
So this service check will be ERROR if we get at least one error somewhere, OK otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

You sneaky deepcopy 🙈

@LeoCavaille
Copy link
Member

Looks good overall! Thanks for adding some tests 👍

LeoCavaille added a commit that referenced this pull request Dec 22, 2014
Add service check to the SNMP check
@LeoCavaille LeoCavaille merged commit c985e3b into master Dec 22, 2014
@LeoCavaille LeoCavaille deleted the snmp-service-check branch December 22, 2014 14:30
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.

2 participants