added initial telemetry work #386

pawelsz-rb · 2021-07-09T19:45:21Z

Description of the change

to enable telemetry, put that in init:

logFormatter = logging.Formatter("%(asctime)s [%(threadName)-12.12s] [%(levelname)-5.5s]  %(message)s")

rollbar.init('access_token', log_telemetry=True, log_telemetry_formatter=logFormatter,
             network_telemetry=True, enable_response_headers=True, enable_req_headers=True)

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fix #88309

Checklists

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
"Ready for review" label attached to the PR and reviewers mentioned in a comment
Changes have been reviewed by at least one other engineer
Issue from task tracker has a link to this pull request

rollbar/lib/telemetry.py

bxsx

Thanks for the PR @pawelsz-rb. I think this is a good start, but a lot of the Telemetry's functionality is missing or not working correctly.

I left some inline comments. Here's also a quick summary of what at first glance is missing:

Architecture and API for Telemetry must be well designed.
Telemetry doesn't work with most supported web frameworks. I was able to log telemetry while using Starlette or FastAPI, but e.g. Flask or Django has a cold start (no Telemetry data in the first occurrence). Other frameworks also don't work well.
The Telemetry events history should be local for each HTTP request.
report_message() includes global Telemetry history for each call.
Network telemetry should fully support the urllib, requests, and httpx packages.
Network telemetry should support non-HTTP network events (like DNS errors).
Telemetry should support User Inputs events by extracting the request object
Missing tests for provided functionality and supported frameworks.

to enable telemetry, put that in init:

logFormatter = logging.Formatter("%(asctime)s [%(threadName)-12.12s] [%(levelname)-5.5s]  %(message)s")

rollbar.init('access_token', log_telemetry=True, log_telemetry_formatter=logFormatter,
              network_telemetry=True, enable_response_headers=True, enable_req_headers=True)

It's overcomplicated. The default formatter and other flags should be SDK-initialized defaults.

I would expect to enable telemetry with something more user-friendly like:
rollbar.init('access_token', telemetry=True).
Other options should be optional.

rollbar/__init__.py

bxsx · 2021-07-27T12:14:47Z

.github/workflows/ci.yml

+          - FLASK_VERSION=0.10.1 Werkzeug\>=0.7,\<1.0 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - FLASK_VERSION=0.11.1 Werkzeug\>=0.7,\<1.0 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - FLASK_VERSION=0.12.4 Werkzeug\>=0.7,\<1.0 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - FLASK_VERSION=1.0.2 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - TWISTED_VERSION=15.5.0 treq==15.1.0 zope.interface==4.1.3 mock==2.0.0
+          - TWISTED_VERSION=16.1.0 treq==16.12.0 zope.interface==4.1.3 mock==2.0.0
+          - TWISTED_VERSION=16.2.0 treq==16.12.0 zope.interface==4.1.3 mock==2.0.0
+          - TWISTED_VERSION=16.3.0 treq==16.12.0 zope.interface==4.2.0 mock==2.0.0
+          - TWISTED_VERSION=16.4.0 treq==16.12.0 zope.interface==4.5.0 mock==2.0.0
+          - TWISTED_VERSION=16.5.0 treq==16.12.0 zope.interface==4.5.0 mock==2.0.0
+          - TWISTED_VERSION=16.6.0 treq==16.12.0 zope.interface==4.5.0 mock==2.0.0
+          - TWISTED_VERSION=17.1.0 treq==16.12.0 zope.interface==4.5.0 mock==2.0.0
+          - DJANGO_VERSION=1.11.20 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - DJANGO_VERSION=2.0.13 mock==2.0.0
+          - DJANGO_VERSION=2.1.7 mock==2.0.0
+          - DJANGO_VERSION=2.1.15 mock==2.0.0
+          - PYRAMID_VERSION=1.9.2 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - PYRAMID_VERSION=1.10.4 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - STARLETTE_VERSION=0.12.12 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
+          - STARLETTE_VERSION=0.12.13 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
+          - STARLETTE_VERSION=0.14.2 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
+          - FASTAPI_VERSION=0.40.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
+          - FASTAPI_VERSION=0.50.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
+          - FASTAPI_VERSION=0.63.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5


All Python versions from the matrix except 2.7 have the mock module included in the standard library. We don't need to install it manually.

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.

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

try:

from unittest import mock

except ImportError:

import mock

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 mock package for Python <3.3 is already implemented:

pyrollbar/setup.py

Lines 20 to 24 in f67253a

tests_require = [

'webob',

'blinker',

'unittest2',

'mock<=3.0.5; python_version < "3.3"',

bxsx · 2021-07-27T12:16:00Z

.github/workflows/ci.yml

-            framework: DJANGO_VERSION=2.0.13
+            framework: DJANGO_VERSION=2.0.13 mock==2.0.0
          - python-version: 2.7
-            framework: DJANGO_VERSION=2.1.7
+            framework: DJANGO_VERSION=2.1.7 mock==2.0.0
          - python-version: 2.7
-            framework: DJANGO_VERSION=2.1.15
+            framework: DJANGO_VERSION=2.1.15 mock==2.0.0
          - python-version: 3.4
-            framework: DJANGO_VERSION=2.1.7
+            framework: DJANGO_VERSION=2.0.13 mock==2.0.0
          - python-version: 3.4
-            framework: DJANGO_VERSION=2.1.15
+            framework: DJANGO_VERSION=2.1.7 mock==2.0.0
+          - python-version: 3.4
+            framework: DJANGO_VERSION=2.1.15 mock==2.0.0
+          - python-version: 3.5
+            framework: DJANGO_VERSION=2.1.15 mock==2.0.0
+          - python-version: 3.5
+            framework: DJANGO_VERSION=2.0.13 mock==2.0.0
          - python-version: 3.5
-            framework: DJANGO_VERSION=2.1.15
+            framework: DJANGO_VERSION=2.1.7 mock==2.0.0
          - python-version: 3.6
-            framework: DJANGO_VERSION=2.1.15
+            framework: DJANGO_VERSION=2.1.15 mock==2.0.0
          - python-version: 3.7
-            framework: DJANGO_VERSION=2.1.15
+            framework: DJANGO_VERSION=2.1.15 mock==2.0.0


As a result of the comment above these changes are also not required.

bxsx · 2021-07-27T12:16:52Z

.github/workflows/ci.yml

-            framework: STARLETTE_VERSION=0.12.12 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.12.12 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 2.7
-            framework: STARLETTE_VERSION=0.12.13 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.12.13 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 2.7
-            framework: STARLETTE_VERSION=0.14.2 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.14.2 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.4
-            framework: STARLETTE_VERSION=0.12.12 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.12.12 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.4
-            framework: STARLETTE_VERSION=0.12.13 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.12.13 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.4
-            framework: STARLETTE_VERSION=0.14.2 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.14.2 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.5
-            framework: STARLETTE_VERSION=0.12.12 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.12.12 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.5
-            framework: STARLETTE_VERSION=0.12.13 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.12.13 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.5
-            framework: STARLETTE_VERSION=0.14.2 httpx==0.18.1 python-multipart==0.0.5
+            framework: STARLETTE_VERSION=0.14.2 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5

          - python-version: 2.7
-            framework: FASTAPI_VERSION=0.40.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.40.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 2.7
-            framework: FASTAPI_VERSION=0.50.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.50.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 2.7
-            framework: FASTAPI_VERSION=0.63.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.63.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.4
-            framework: FASTAPI_VERSION=0.40.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.40.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.4
-            framework: FASTAPI_VERSION=0.50.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.50.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.4
-            framework: FASTAPI_VERSION=0.63.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.63.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.5
-            framework: FASTAPI_VERSION=0.40.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.40.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.5
-            framework: FASTAPI_VERSION=0.50.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.50.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5
          - python-version: 3.5
-            framework: FASTAPI_VERSION=0.63.0 httpx==0.18.1 python-multipart==0.0.5
+            framework: FASTAPI_VERSION=0.63.0 httpx==0.18.1 python-multipart==0.0.5 mock==3.0.5


Same as above.

bxsx · 2021-07-27T12:18:06Z

.github/workflows/ci.yml

-            framework: FLASK_VERSION=0.10.1
+            framework: FLASK_VERSION=0.10.1 mock==2.0.0
          - python-version: 3.3
-            framework: FLASK_VERSION=0.11.1
+            framework: FLASK_VERSION=0.11.1 mock==2.0.0
          - python-version: 3.3
-            framework: FLASK_VERSION=0.12.4
+            framework: FLASK_VERSION=0.12.4 mock==2.0.0
          - python-version: 3.3
-            framework: FLASK_VERSION=1.0.2
+            framework: FLASK_VERSION=1.0.2 mock==2.0.0
          - python-version: 3.3
-            framework: DJANGO_VERSION=1.6.11
+            framework: DJANGO_VERSION=1.6.11 mock==2.0.0
          - python-version: 3.3
-            framework: DJANGO_VERSION=1.8.19
+            framework: DJANGO_VERSION=1.8.19 mock==2.0.0
          - python-version: 3.4
-            framework: DJANGO_VERSION=1.7.11
+            framework: DJANGO_VERSION=1.7.11 chardet==3.0.4 idna==2.7 mock==3.0.5
          - python-version: 3.4
-            framework: DJANGO_VERSION=1.8.19
+            framework: DJANGO_VERSION=1.8.19 chardet==3.0.4 idna==2.7 mock==3.0.5
          - python-version: 3.4
-            framework: DJANGO_VERSION=1.9.13
+            framework: DJANGO_VERSION=1.9.13 chardet==3.0.4 idna==2.7 mock==3.0.5
          - python-version: 3.4
-            framework: DJANGO_VERSION=1.10.8
+            framework: DJANGO_VERSION=1.10.8 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - python-version: 3.4
+            framework: DJANGO_VERSION=2.0.13 chardet==3.0.4 idna==2.7 mock==3.0.5
+          - python-version: 3.5
+            framework: DJANGO_VERSION=1.8.19 mock==3.0.5
+          - python-version: 3.5
+            framework: DJANGO_VERSION=1.9.13 mock==3.0.5
          - python-version: 3.5
-            framework: DJANGO_VERSION=1.8.19
+            framework: DJANGO_VERSION=1.10.8 mock==3.0.5
          - python-version: 3.5
-            framework: DJANGO_VERSION=1.9.13
+            framework: DJANGO_VERSION=2.0.13 mock==3.0.5
          - python-version: 3.5
-            framework: DJANGO_VERSION=1.10.8
+            framework: DJANGO_VERSION=2.1.7 mock==3.0.5
          - python-version: 3.7
-            framework: TWISTED_VERSION=18.9.0 treq==20.4.1 zope.interface==4.5.0
+            framework: TWISTED_VERSION=18.9.0 treq==20.4.1 zope.interface==4.5.0 mock==3.0.5
          - python-version: 3.7
-            framework: TWISTED_VERSION=19.10.0 treq==20.4.1 zope.interface==4.6.0
+            framework: TWISTED_VERSION=19.10.0 treq==20.4.1 zope.interface==4.6.0 mock==3.0.5
          - python-version: 3.7
-            framework: TWISTED_VERSION=20.3.0 treq==20.4.1 zope.interface==4.7.0
+            framework: TWISTED_VERSION=20.3.0 treq==20.4.1 zope.interface==4.7.0 mock==3.0.5
          - python-version: 3.8
-            framework: TWISTED_VERSION=18.9.0 treq==20.4.1 zope.interface==4.5.0
+            framework: TWISTED_VERSION=18.9.0 treq==20.4.1 zope.interface==4.5.0 mock==3.0.5
          - python-version: 3.8
-            framework: TWISTED_VERSION=19.10.0 treq==20.4.1 zope.interface==4.6.0
+            framework: TWISTED_VERSION=19.10.0 treq==20.4.1 zope.interface==4.6.0 mock==3.0.5
          - python-version: 3.8
-            framework: TWISTED_VERSION=20.3.0 treq==20.4.1 zope.interface==4.7.0
+            framework: TWISTED_VERSION=20.3.0 treq==20.4.1 zope.interface==4.7.0 mock==3.0.5


Python 3.3+ includes the mock module. I don't think it is necessary.

rollbar/test/test_telemetry.py

rollbar/__init__.py

bxsx · 2021-08-03T16:19:34Z

rollbar/test/test_telemetry.py

+    def test_telemetry_request(self, timestamp):
+        timestamp.return_value = 1000000
+
+        requests.get("http://example.com")


You can stub the actual HTTP connection since you don't return the response object.

rollbar/test/test_telemetry.py

rollbar/__init__.py

Co-authored-by: Bart Skowron <[email protected]>

Pawel Szczodruch added 2 commits July 9, 2021 12:44

added initial telemetry work

95bd528

added telemetry queue size

5430b78

waltjones reviewed Jul 21, 2021

View reviewed changes

rollbar/lib/telemetry.py Outdated Show resolved Hide resolved

Pawel Szczodruch added 26 commits July 22, 2021 22:39

added unit tests

d7bf70d

fixing problem

9cc6c8b

corrected tests

00b76b7

added dependency

5f85940

added chardet==3.0.4

5c831e8

added idna==2.7

b489291

added mock==3.0.5

5b3979f

installing more dependencies

9f5f317

added dependencies

c4267fc

fixed dependencies

99988fc

added chardet==3.0.4 idna==2.7 mock==3.0.5

8c126a7

added dependency to pyramid

5e459f7

fixing versions

9549e0d

fixing dependencies

3336002

fixing dependencies

f5d9139

fixing mock version

7cebc64

fixing mock version

ab02383

fixing mock version

b1628cd

added mock to TWISTED

6a902d7

added mock

353dba8

added mock to starlette

5a82b30

fix tests

52a8318

corrected tests

766e10a

corrected tests

d2bf643

corrected tests

53132d1

added mock to TWISTED

ec5208f

pawelsz-rb marked this pull request as ready for review July 26, 2021 17:45

bxsx suggested changes Aug 4, 2021

View reviewed changes

pawelsz-rb and others added 17 commits August 4, 2021 15:39

Update rollbar/lib/telemetry.py

ca9df67

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/test/test_telemetry.py

9f0d61d

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/__init__.py

e59e34c

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/test/test_telemetry.py

5797a30

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/lib/telemetry.py

cfafa97

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/lib/telemetry.py

0f09f7e

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/lib/telemetry.py

16e6a9b

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/lib/telemetry.py

070b188

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/lib/telemetry.py

b317564

Co-authored-by: Bart Skowron <[email protected]>

Update rollbar/test/test_telemetry.py

88d8da3

Co-authored-by: Bart Skowron <[email protected]>

addressing PR comments

1f1d8da

corrected tests

7f57fae

corrected tests

2e09a11

addressing PR comments

1d27b19

correcting tests

2d270df

added support for urllib

5d7c1f9

added support for httpx

91f8338

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added initial telemetry work #386

added initial telemetry work #386

pawelsz-rb commented Jul 9, 2021 •

edited

Loading

bxsx left a comment

bxsx Jul 27, 2021

pawelsz-rb Aug 5, 2021

bxsx Aug 10, 2021

bxsx Jul 27, 2021

bxsx Jul 27, 2021

bxsx Jul 27, 2021

bxsx Aug 3, 2021

	tests_require = [
	'webob',
	'blinker',
	'unittest2',
	'mock<=3.0.5; python_version < "3.3"',

added initial telemetry work #386

Are you sure you want to change the base?

added initial telemetry work #386

Conversation

pawelsz-rb commented Jul 9, 2021 • edited Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

bxsx left a comment

Choose a reason for hiding this comment

bxsx Jul 27, 2021

Choose a reason for hiding this comment

pawelsz-rb Aug 5, 2021

Choose a reason for hiding this comment

bxsx Aug 10, 2021

Choose a reason for hiding this comment

bxsx Jul 27, 2021

Choose a reason for hiding this comment

bxsx Jul 27, 2021

Choose a reason for hiding this comment

bxsx Jul 27, 2021

Choose a reason for hiding this comment

bxsx Aug 3, 2021

Choose a reason for hiding this comment

pawelsz-rb commented Jul 9, 2021 •

edited

Loading