Clickhouse changes #1
Conversation
| @@ -534,43 +540,57 @@ def get_hits_terms(self, rule, starttime, endtime, index, key, qk=None, size=Non | |||
| return {endtime: buckets} | |||
|
|
|||
| def get_hits_aggregation(self, rule, starttime, endtime, index, query_key, term_size=None): | |||
There was a problem hiding this comment.
You are changing the common method being used, so I think it will effect the existing alerts? Can you check?
Also is this required just for response_time alert
There was a problem hiding this comment.
in traces use case it will be used only for response time avg alert. As we already discussed we will host elastalert for trace separately from logs, this should not be an issue
elastalert/elastalert.py
Outdated
| if term_size is None: | ||
| term_size = rule.get('terms_size', 50) | ||
| query = self.get_aggregation_query(base_query, rule, query_key, term_size, rule['timestamp_field']) | ||
| agg_key = rule['metric_agg_type']+"("+rule['metric_agg_key']+")" |
There was a problem hiding this comment.
Is there a better way to do string interpolation and concatenation in Python. In Python 3, We can do like thisf'{var}'
elastalert/elastalert.py
Outdated
| term_size = rule.get('terms_size', 50) | ||
| query = self.get_aggregation_query(base_query, rule, query_key, term_size, rule['timestamp_field']) | ||
| agg_key = rule['metric_agg_type']+"("+rule['metric_agg_key']+")" | ||
| query = rule['filter'][0]['query_string']['query'] |
There was a problem hiding this comment.
What if there is no filter? Will config itself throw that error or will it get stuck here?
elastalert/ruletypes.py
Outdated
| def __init__(self, *args): | ||
| super(ErrorRateRule, self).__init__(*args) | ||
| self.ts_field = self.rules.get('timestamp_field', '@timestamp') | ||
| self.rules['metric_agg_key'] = "1" |
There was a problem hiding this comment.
Is this default? Shall we add a comment here as this is default?
| return {} | ||
|
|
||
| payload = {'error_count': error_data, 'total_count': total_data} | ||
| self.num_hits += int(total_count) |
There was a problem hiding this comment.
why num_hits is required?
There was a problem hiding this comment.
num hits is mainly used in looping very large dataset in batches using scroll in ES. Also it appears in the alert. Updating it to follow the convention followed across the code
| return {endtime: payload} | ||
| return None,0 | ||
| res = json.loads(res.content) | ||
| return res['data'][0][agg_key], res['rows'] |
There was a problem hiding this comment.
What would be there in these two values?
There was a problem hiding this comment.
agg value and the number of rows satisfied the filter criteria
elastalert/ruletypes.py
Outdated
| super(ErrorRateRule, self).__init__(*args) | ||
| self.ts_field = self.rules.get('timestamp_field', '@timestamp') | ||
| self.rules['metric_agg_key'] = "1" | ||
| self.rules['metric_agg_type'] = "count" |
There was a problem hiding this comment.
We need to have cardinality query when finding total traces and just count query for error_traces? So metric_agg_type should be cardinality. But it's going as count only by default.
No description provided.