Add alert probe for Zabbix#85
Conversation
It implements (openrca#9). Signed-off-by: Aleksandra Galara <a.galara@samsung.com>
bzurkowski
left a comment
bzurkowski
left a comment
There was a problem hiding this comment.
Well, it's an elegant piece of code 💯 Added only a few purely cosmetic remarks. It was hard to catch anything else 👌 I really like it! Thanks 😉
| labels = {} | ||
| labels['node'] = entity['hosts'][0]['host'] | ||
| return labels |
There was a problem hiding this comment.
I think we can wrap it into a one-liner 😉
| python-arango # MIT | ||
| pyyaml # MIT | ||
| requests # Apache License 2.0 | ||
| pyzabbix |
There was a problem hiding this comment.
Please, add license information.
| """Upstream proxy for Zabbix.""" | ||
|
|
||
| def get_all(self): | ||
| return self._client.trigger.get(only_true=1, |
There was a problem hiding this comment.
Following the convention in neighbour code. it would be more consistent to place arguments in a new line, like this:
some_method_with_a_very_long_name_and_lots_of_args(
arg1=1,
arg2=2,
arg3=3)| @classmethod | ||
| def get(cls, graph): | ||
| zabbix_client = ZabbixAPI(CONFIG.probes.zabbix.url) | ||
| zabbix_client.login(CONFIG.probes.zabbix.username, CONFIG.probes.zabbix.password) |
There was a problem hiding this comment.
Consider moving the login step to the upstream class. I think this is where it belongs since the other client invocations already live there (separation of concerns). The upstream class is in charge of controlling the Zabbix client object.
bzurkowski
left a comment
bzurkowski
left a comment
There was a problem hiding this comment.
Thanks for fixes! 😉 I have a few more remarks. The most important is related to multiple host values in trigger payload. Please, share your opinion about how we could handle this?
| from orca.topology import upstream | ||
| from pyzabbix import ZabbixAPI | ||
| from orca.common import config |
There was a problem hiding this comment.
Imports should be sorted according to the following convention:
imports from the standard Python library, e.g. sys, os, json
<empty line>
imports from external libraries, e.g. pyzabbix, prometheus
<empty line>
imports from project package, i.e. orca imports
| from orca.common import config | ||
| from orca.topology import probe, utils | ||
| from orca.topology.alerts.zabbix import extractor, upstream | ||
| from pyzabbix import ZabbixAPI |
There was a problem hiding this comment.
Please, sort the imports as described in the previous comment.
Signed-off-by: Aleksandra Galara <a.galara@samsung.com>
bzurkowski
left a comment
bzurkowski
left a comment
There was a problem hiding this comment.
@aleksandra-galara Thanks for the fixes. Last one remark and it's perfect 😉
| properties = {} | ||
| properties['host'] = host['host'] | ||
| properties['description'] = trigger['description'] | ||
| properties['status'] = 'active' if trigger['value'] == '1' else 'inactive' | ||
| properties['priority'] = trigger['priority'] |
There was a problem hiding this comment.
It would be better to move this logic to the extractor. It is the responsibility of the extractor to determine the status based on a value and map fields for priority and description. We shouldn't mix that into the upstream class, which is just a simple abstraction around retrieving payloads from API requests.
Although this may seem counterintuitive, I suggest we reimplement the upstream class to return a "raw" structure of format:
[{"host": <host>, "trigger": <trigger>}, ...]
Then, process individual items in extractor using the extraction code above.
The <trigger> could be a dict of keys cut from the full trigger payload, limited to only those keys that are required for further extraction.
Signed-off-by: Aleksandra Galara <a.galara@samsung.com>
bzurkowski
left a comment
bzurkowski
left a comment
There was a problem hiding this comment.
Looks good to me! 👍 Merging...
2 participants
It implements (#9).
Signed-off-by: Aleksandra Galara a.galara@samsung.com