Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
95bd528
added initial telemetry work
Jul 9, 2021
5430b78
added telemetry queue size
Jul 12, 2021
d7bf70d
added unit tests
Jul 23, 2021
9cc6c8b
fixing problem
Jul 23, 2021
00b76b7
corrected tests
Jul 23, 2021
5f85940
added dependency
Jul 23, 2021
5c831e8
added chardet==3.0.4
Jul 23, 2021
b489291
added idna==2.7
Jul 23, 2021
5b3979f
added mock==3.0.5
Jul 23, 2021
9f5f317
installing more dependencies
Jul 23, 2021
c4267fc
added dependencies
Jul 23, 2021
99988fc
fixed dependencies
Jul 23, 2021
8c126a7
added chardet==3.0.4 idna==2.7 mock==3.0.5
Jul 23, 2021
5e459f7
added dependency to pyramid
Jul 23, 2021
9549e0d
fixing versions
Jul 23, 2021
3336002
fixing dependencies
Jul 23, 2021
f5d9139
fixing dependencies
Jul 23, 2021
7cebc64
fixing mock version
Jul 23, 2021
ab02383
fixing mock version
Jul 23, 2021
b1628cd
fixing mock version
Jul 23, 2021
6a902d7
added mock to TWISTED
Jul 23, 2021
353dba8
added mock
Jul 24, 2021
5a82b30
added mock to starlette
Jul 26, 2021
52a8318
fix tests
Jul 26, 2021
766e10a
corrected tests
Jul 26, 2021
d2bf643
corrected tests
Jul 26, 2021
53132d1
corrected tests
Jul 26, 2021
ec5208f
added mock to TWISTED
Jul 26, 2021
ca9df67
Update rollbar/lib/telemetry.py
pawelsz-rb Aug 4, 2021
9f0d61d
Update rollbar/test/test_telemetry.py
pawelsz-rb Aug 4, 2021
e59e34c
Update rollbar/__init__.py
pawelsz-rb Aug 4, 2021
5797a30
Update rollbar/test/test_telemetry.py
pawelsz-rb Aug 4, 2021
cfafa97
Update rollbar/lib/telemetry.py
pawelsz-rb Aug 4, 2021
0f09f7e
Update rollbar/lib/telemetry.py
pawelsz-rb Aug 4, 2021
16e6a9b
Update rollbar/lib/telemetry.py
pawelsz-rb Aug 4, 2021
070b188
Update rollbar/lib/telemetry.py
pawelsz-rb Aug 4, 2021
b317564
Update rollbar/lib/telemetry.py
pawelsz-rb Aug 4, 2021
88d8da3
Update rollbar/test/test_telemetry.py
pawelsz-rb Aug 4, 2021
1f1d8da
addressing PR comments
Aug 5, 2021
7f57fae
corrected tests
Aug 5, 2021
2e09a11
corrected tests
Aug 5, 2021
1d27b19
addressing PR comments
Aug 5, 2021
2d270df
correcting tests
Aug 5, 2021
5d7c1f9
added support for urllib
Aug 6, 2021
91f8338
added support for httpx
Aug 6, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion rollbar/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import requests
import six

from rollbar.lib import events, filters, dict_merge, parse_qs, text, transport, urljoin, iteritems, defaultJSONEncode
from rollbar.lib import events, filters, dict_merge, parse_qs, text, transport, telemetry, urljoin, iteritems, defaultJSONEncode
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated


__version__ = '0.16.1'
Expand Down Expand Up @@ -381,6 +381,22 @@ def init(access_token, environment='production', scrub_fields=None, url_fields=N
if SETTINGS.get('allow_logging_basic_config'):
logging.basicConfig()

queue_size = SETTINGS.get('set_custom_queue_size')
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
if queue_size:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is buggy. You use the value as a number in other parts of the code but the value pulled from SETTINGS['set_custom_queue_size'] may be passed as a string like "0" that would raise exception later or the condition's result can be incorrect.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine. We have more settings that accept a number. Not sure why you would want to pass a string when it expects a number.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python does not support strong typing, and you can't assume a source of the configuration data for the SDKs. It's common to read configuration settings from a serialized text file or even from a keyboard.
So there can be many cases where the value can be of type string and lead to the bug.

We have more settings that accept a number.

I am not against accepting numbers but the context of use. If you noticed any other settings that can lead to any bug, you can fix them by the way.

telemetry.TELEMETRY_QUEUE = telemetry.Queue(queue_size)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a global queue? This messes up events between requests. IMHO would be much more valuable to keep a unique queue per request.


if SETTINGS.get('log_telemetry'):
formatter = SETTINGS.get('log_telemetry_formatter')
telemetry.set_log_telemetry(formatter)
if SETTINGS.get('network_telemetry'):
enable_req_headers = SETTINGS.get('enable_req_headers')
enable_response_headers = SETTINGS.get('enable_response_headers')
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
requests.get = telemetry.request(requests.get, enable_req_headers, enable_response_headers )
requests.post = telemetry.request(requests.post, enable_req_headers, enable_response_headers)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POST requests to the Rollbar API are NOT reported (probably ok to ignore it) because of using requests.sessions.Session object instead of invoking the requests.post() function. However, requests send by the Session objects shall be reported.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POST requests to Rollbar API should be ignored.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but other requests from Session objects should be collected and reported.

requests.put = telemetry.request(requests.put, enable_req_headers, enable_response_headers)
requests.patch = telemetry.request(requests.patch, enable_req_headers, enable_response_headers)
requests.delete = telemetry.request(requests.delete, enable_req_headers, enable_response_headers)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only partial support for the requests package.
The wrapped API functions are at the top of the stack. There are other ways to send requests such as requests.request() or requests.sessions.Session().request(). The Session.request is a low-level function and it should be enough to wrap only it.

Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated

if SETTINGS.get('handler') == 'agent':
agent_log = _create_agent_log()

Expand Down Expand Up @@ -777,6 +793,7 @@ def _report_exc_info(exc_info, request, extra_data, payload_data, level=None):
_add_request_data(data, request)
_add_person_data(data, request)
_add_lambda_context_data(data)
_add_telemetry(data)
data['server'] = _build_server_data()

if payload_data:
Expand Down Expand Up @@ -857,6 +874,7 @@ def _report_message(message, level, request, extra_data, payload_data):
_add_request_data(data, request)
_add_person_data(data, request)
_add_lambda_context_data(data)
_add_telemetry(data)
data['server'] = _build_server_data()

if payload_data:
Expand Down Expand Up @@ -1119,6 +1137,12 @@ def _add_request_data(data, request):
data['request'] = request_data


def _add_telemetry(data):
telemetry_data = telemetry.TELEMETRY_QUEUE.get_items()
if telemetry_data:
data['body']['telemetry'] = telemetry_data


def _check_add_locals(frame, frame_num, total_frames):
"""
Returns True if we should record local variables for the given frame.
Expand Down
82 changes: 82 additions & 0 deletions rollbar/lib/telemetry.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import logging
import threading
import time

import rollbar


class Queue:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 queue.Queue as they are optimized and well tested. Based on your current implementation, you should probably be using collections.deque objects.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the Python documentation:

class collections.deque([iterable[, maxlen]])
(...)
If maxlen is not specified or is None, deques may grow to an arbitrary length. Otherwise, the deque is bounded to the specified maximum length. Once a bounded length deque is full, when new items are added, a corresponding number of items are discarded from the opposite end. Bounded length deques provide functionality similar to the tail filter in Unix. They are also useful for tracking transactions and other pools of data where only the most recent activity is of interest.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but it does not have a function to retrieve all the items. I guess I can do the list()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

I guess I can do the list()

If you need a list, you can do so (thanks to the iterator protocol).

def __init__(self, size):
self.size = size
self.items = []
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
self.lock = threading.Lock()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


TELEMETRY_QUEUE_SIZE = 50
TELEMETRY_QUEUE = Queue(TELEMETRY_QUEUE_SIZE)
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated


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': int(time.time()), 'type': 'log',
'level': record.levelname}

TELEMETRY_QUEUE.put(data)


def set_log_telemetry(log_formatter=None):
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
logging.getLogger().addHandler(TelemetryLogHandler(log_formatter))


def request(request_function, enable_req_headers, enable_response_headers):
def telemetry(*args, **kwargs):

Comment thread
pawelsz-rb marked this conversation as resolved.
def clean_headers(headers):
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
for h in headers:
if h in rollbar.SETTINGS['scrub_fields']:
del headers[h]
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
return headers

data = {'level': 'info'}
data_body = {'status_code': None}

response = request_function(*args, **kwargs)

if response:
data_body['status_code'] = response.status_code
if response.status_code >= 500:
data['level'] = 'critical'
elif response.status_code >= 400:
data['level'] = 'error'
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
if enable_response_headers:
data_body['response'] = {'headers': clean_headers(response.headers)}
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
if enable_req_headers:
data_body['request_headers'] = clean_headers(kwargs.get('headers'))
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
data_body['url'] = args[0]
data_body['method'] = request_function.__name__
Comment thread
pawelsz-rb marked this conversation as resolved.
Outdated
data_body['subtype'] = 'http'
data['body'] = data_body

data['source'] = 'client'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be server?

data['timestamp_ms'] = int(time.time())
data['type'] = 'network'
TELEMETRY_QUEUE.put(data)

return response
return telemetry