added initial telemetry work #386
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import logging | ||
| import threading | ||
| import time | ||
|
|
||
| import rollbar | ||
|
|
||
|
|
||
| class Queue: | ||
|
There was a problem hiding this comment. Do not implement your own data structures unless there is a strong reason to do so. Use the structures available in the standard library such as There was a problem hiding this comment. I looked at the functionality of these, and I could not find what I was looking for. I need to return all the items with the predefined queue size. and if the new item makes the queue size bigger than the predefined queue size then the first item must be removed. Like I said, I did not see anything here that would work efficiently. There was a problem hiding this comment. From the Python documentation:
There was a problem hiding this comment. yes, but it does not have a function to retrieve all the items. I guess I can do the list() There was a problem hiding this comment. There is no need to provide such a function to any of the Python containers. In fact, none of the built-in containers provide such a function. Python supports the iterator protocol (see PEP 234).
If you need a list, you can do so (thanks to the iterator protocol). |
||
| def __init__(self, size): | ||
| self.size = size | ||
| self.items = [] | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| self.lock = threading.Lock() | ||
|
There was a problem hiding this comment. Locking thread breaks asynchronous code. You can't use it here. |
||
|
|
||
| def put(self, item): | ||
| with self.lock: | ||
| if len(self.items) >= self.size: | ||
| self.items = self.items[1:] | ||
| self.items.append(item) | ||
|
|
||
| def get_items(self): | ||
| with self.lock: | ||
| return self.items | ||
|
|
||
| def clear_items(self): | ||
| with self.lock: | ||
| self.items = [] | ||
|
|
||
|
|
||
| TELEMETRY_QUEUE_SIZE = 50 | ||
| TELEMETRY_QUEUE = Queue(TELEMETRY_QUEUE_SIZE) | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| class TelemetryLogHandler(logging.Handler): | ||
| def __init__(self, formatter=None): | ||
| super(TelemetryLogHandler, self).__init__() | ||
| self.formatter = formatter | ||
|
|
||
| def emit(self, record): | ||
| self.setFormatter(self.formatter) | ||
| msg = {'message': self.format(record)} | ||
| data = {'body': msg, 'source': 'client', 'timestamp_ms': get_current_timestamp(), 'type': 'log', | ||
| 'level': record.levelname} | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| TELEMETRY_QUEUE.put(data) | ||
|
|
||
|
|
||
| def set_log_telemetry(log_formatter=None): | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| logging.getLogger().addHandler(TelemetryLogHandler(log_formatter)) | ||
|
|
||
|
|
||
| def get_current_timestamp(): | ||
| return int(time.time()) | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def request(request_function, enable_req_headers, enable_response_headers): | ||
| def telemetry(*args, **kwargs): | ||
|
|
||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| def clean_headers(headers): | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if not headers: | ||
| return [] | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for h in headers: | ||
| if h in rollbar.SETTINGS['scrub_fields']: | ||
| headers[h] = '[FILTERED]' | ||
| return headers | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| data = {'level': 'info'} | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| data_body = {'status_code': None} | ||
| try: | ||
| response = request_function(*args, **kwargs) | ||
| except: # noqa: E722 | ||
| response = None | ||
|
Comment on lines
+53
to
+56
There was a problem hiding this comment. This is a bug. You shouldn't modify the behavior of the wrapped object. The response object is returned later. There was a problem hiding this comment. I don't modify anything response object is not return at all There was a problem hiding this comment. I think it should re-raise after logging the telemetry event. It looks like the caught exception gets lost here. There was a problem hiding this comment.
You modified (95bd528) the response object by transforming (scrubbing) mutual objects: if enable_response_headers:
data_body['response'] = {'headers': clean_headers(response.headers)}
if enable_req_headers:
data_body['request_headers'] = clean_headers(kwargs.get('headers'))Then the response object is returned at the end of the function (line 90 at the moment).
I'm sure that this (and the bare except in general) will be resolved implicitly after adding support for non-HTTP network events. There was a problem hiding this comment. I thought you are referring to cleaning the headers is already fixed |
||
|
|
||
| if response is not None: | ||
| data_body['status_code'] = response.status_code | ||
| if response.status_code >= 500: | ||
| data['level'] = 'critical' | ||
| elif response.status_code >= 400: | ||
| data['level'] = 'error' | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if enable_response_headers: | ||
| data_body['response'] = {'headers': clean_headers(response.headers)} | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if enable_req_headers: | ||
| data_body['request_headers'] = clean_headers(kwargs.get('headers')) | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| data_body['url'] = args[0] | ||
| data_body['method'] = request_function.__name__ | ||
pawelsz-rb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| data_body['subtype'] = 'http' | ||
| data['body'] = data_body | ||
|
|
||
| data['source'] = 'client' | ||
|
|
||
| data['timestamp_ms'] = get_current_timestamp() | ||
| data['type'] = 'network' | ||
| TELEMETRY_QUEUE.put(data) | ||
|
|
||
| return response | ||
|
|
||
| return telemetry | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Python versions from the matrix except 2.7 have the
mockmodule included in the standard library. We don't need to install it manually.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but it does not hurt to install it, and just for that I would need to pull 2.7 out of this matrix and have a different definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO requiring the installation of unnecessary packages hurts quality.
Our other test suites solve it in this way:
pyrollbar/rollbar/test/test_rollbar.py
Lines 14 to 17 in f67253a
You should follow the pattern. In my opinion, this PR does not require any changes in the CI builds. The builds failed for a good reason.
The installation of the
mockpackage for Python <3.3 is already implemented:pyrollbar/setup.py
Lines 20 to 24 in f67253a