Added method to ask for active checks#72
Conversation
adubkov
left a comment
adubkov
left a comment
There was a problem hiding this comment.
Sorry for late review. I think it's good idea to support active checks. But I think it require some changes to follow best practices for easy maintaining and backward compatibility.
| :rtype: str | ||
| :return: Formatted zabbix request | ||
| """ | ||
| request = '{"request":"active checks","host":"%s"}' % host |
There was a problem hiding this comment.
It's better use format method here.
But actually, I don't think we need this as separate function, because it's just 2-3 lines of code and it used just in one place. I think it should go to send_active_checks
| logger.debug('Request: %s', request) | ||
| return request | ||
|
|
||
| def send_active_checks(self, host): |
There was a problem hiding this comment.
This function returns list of active checks from zabbix server, then why it calls send_active_checks instead of, an example, get_active_checks?
| request = self._create_active_checks_request(host) | ||
| packet = self._create_packet(request) | ||
|
|
||
| for host_addr in self.zabbix_uri: |
There was a problem hiding this comment.
I think this part must be refactored as a function _send because you duplicating big chunk code of _chunk_send here.
| if self.checks is None: | ||
| return '' | ||
| else: | ||
| return "[%s]" % ", ".join([ch.__repr__() for ch in self.checks]) |
There was a problem hiding this comment.
It's better represent it as json. Which you doing in ZabbixCheck.__repr__. I think it should be consisted.
| # lastlogsize is optional in 2.2; required in 2.4, 3.0, 3.2, 3.4 | ||
| # mtime is unused in 3.2; required in 2.4, 3.0, 3.2, 3.4 | ||
| ch = ZabbixCheck( | ||
| o.get('key'), |
There was a problem hiding this comment.
If some keys are required and not optional, we should get them explicitly to raise exception on error. Otherwise we may get empty checks on error.
| self.key = str(key) | ||
| self.delay = delay | ||
| self.lastlogsize = lastlogsize | ||
| self.mtime = mtime |
There was a problem hiding this comment.
It's usefull decorate properties as actually properties.
Example:
@property
def chunk(self):
return self._chunk
2 participants
Asks the Zabbix server for the active checks that are needed for a given host, following the protocol documented here: https://www.zabbix.com/documentation/3.4/manual/appendix/items/activepassive?s[]=agent&s[]=protocol
This PR includes a few unit tests for the new methods. I also wrote a functional test (not included here), but I wasn't sure how to update the Zabbix test data to make it pass. If you don't mind getting the test to pass yourself, let me know and I'll add it as a separate commit.
Thanks for this project!