Add alerts endpoint#99
Conversation
It implements (openrca#98). Signed-off-by: Aleksandra Galara <a.galara@samsung.com>
|
Payload example: |
bzurkowski
left a comment
bzurkowski
left a comment
There was a problem hiding this comment.
@aleksandra-galara Thanks for the PR! 👍 I added some remarks to consider. Most of them are related to the payload structure. We must change it a little bit based on the discussion that we had yesterday with the UI team (some fields were missing). Also, the payload structure should be more flat to make it convenient for processing in the UI:
{
"id": "xyz",
"origin": "prometheus",
"name": "SomethingHorribleHappenedWithAPod",
"message": "It's bad :(",
"severity": "critical",
"source": {
"origin": "kubernetes",
"kind": "pod",
"properties": {
"name": "elasticsearch",
"namespace": "logging"
}
},
"created_at": 123456789,
"updated_at": 123456789
}| from orca.api.resources.v1 import graph as graph_ns | ||
| from orca.api.resources.v1 import ingestor as ingestor_ns | ||
|
|
||
| from orca.api.resources.v1 import alert as alerts_ns |
There was a problem hiding this comment.
Let's follow the convention and use the singular form: alert_ns.
| return marshal(data, alerts_fields) | ||
|
|
||
|
|
||
| def initialize(alerts): |
There was a problem hiding this comment.
The variable name alerts is a little bit misleading. Let's rename to graph since the initialize method takes a graph instance as its argument.
| 'kind': fields.String(attribute='properties.source_mapping.kind'), | ||
| 'instance': fields.String(attribute='properties.source_mapping.properties.name'), | ||
| 'namespace': fields.String(attribute='properties.source_mapping.properties.namespace', default="n/a"), |
There was a problem hiding this comment.
Let's put the source_mapping fields into a nested source field in the alert payload:
{
"source": {
"origin": "kubernetes",
"kind": "pod",
"properties": {
"name": "elasticsearch",
"namespace": "logging"
}
}
}Also note that the source origin field is missing.
| node_fields = Model('Alert Node', { | ||
| 'id': fields.String, | ||
| 'origin': fields.String, | ||
| 'properties': { |
There was a problem hiding this comment.
Let's remove the nesting via the properties field and treat the alerts payload flat instead.
| 'message': fields.String(attribute='properties.message'), | ||
| 'severity': fields.String(attribute='properties.severity') | ||
| } | ||
| }) |
There was a problem hiding this comment.
Let's add timestamp fields: created_at, updated_at.
|
|
||
| class Alerts(Resource): | ||
|
|
||
| def __init__(self, api, alerts): |
There was a problem hiding this comment.
The variable name alerts is a little bit misleading. Let's rename to graph since the API constructor takes a graph instance as its argument.
|
|
||
| from flask_restx import Model, Namespace, Resource, fields, marshal | ||
|
|
||
| node_fields = Model('Alert Node', { |
There was a problem hiding this comment.
node_fields is a left-over after the Graph API. Let's rename to alert_fields. We could also rename the alerts_fields (below) to alert_list_fields.
|
|
||
| def get(self): | ||
| data = { | ||
| 'nodes': self._alerts.get_nodes(kind='alert'), |
There was a problem hiding this comment.
I think we don't have to use the dict at this point. Instead, we should be able to directly pass the list of alert nodes to the marshal method. Try it out 😉
It introduces changes due to review. (openrca#98) Signed-off-by: Aleksandra Galara <a.galara@samsung.com>
|
As for now, the payload looks like that: |
|
|
||
| def __init__(self, api, graph): | ||
| super().__init__() | ||
| self._alerts = graph |
There was a problem hiding this comment.
Let's call things by name. Graph is not an alert list. Why not just: self._graph = graph? 😃
| 'source': { | ||
| 'origin': fields.String(attribute='properties.source_mapping.origin'), | ||
| 'kind': fields.String(attribute='properties.source_mapping.kind'), | ||
| 'properties': { | ||
| 'name': fields.String(attribute='properties.source_mapping.properties.name'), | ||
| 'namespace': fields.String(attribute='properties.source_mapping.properties.namespace', default="n/a") | ||
| } |
There was a problem hiding this comment.
I'm wondering if we could refactor this a little bit by using the Nested field.
It introduces changes due to review. (openrca#98) 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. I wonder how the new endpoint operates with the new alerts dashboard in the UI 😉 Good work! 👍
2 participants
It implements (#98).
Signed-off-by: Aleksandra Galara a.galara@samsung.com