Add ASGI middleware by majorgreys · Pull Request #716 · open-telemetry/opentelemetry-python

majorgreys · 2020-05-20T22:21:28Z

Updating #402 which had been inactive for some time to get remaining to resolve merge conflicts and address any remaining issues.

Signed-off-by: Emil Madsen <sovende@gmail.com>

…TP METHOD Signed-off-by: Emil Madsen <sovende@gmail.com>

toumorokoshi

Thanks for picking this up! looks like some changes are needed still, I copied over relevant comments from #402.

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/version.py

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/version.py

docs/opentelemetry.ext.asgi.rst

ext/opentelemetry-ext-asgi/setup.cfg

Co-authored-by: alrex <alrex.boten@gmail.com>

…telemetry-python into majorgreys/feature/ext_asgi

…e/ext_asgi

toumorokoshi

Great! Thanks for addressing feedback. LGTM.

florimondmanca

👋 Hey all, popping by as an ASGI nerd here. Overall this is fantastic work! Left some suggestions, mostly addressing nits wrt the ASGI spec. The core middleware functionality itself looks top notch.

ext/opentelemetry-ext-asgi/README.rst

florimondmanca · 2020-05-26T13:56:50Z

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py

+    http_url = (
+        scope.get("scheme") + "://" + server_host + scope.get("path", "")
+        if scope.get("scheme") and server_host and scope.get("path")
+        else None
+    )


There is a root_path field in the ASGI HTTP scope that I don't think gets taken into account here.

This field is non-empty when an application is mounted as a route, which can be a problem if OpenTelemetryMiddleware is applied only to a sub-app instead of the entire application.

For example if a user has their web API built as a separate application, and they only want to trace that part of their app, they could do:

api = ... api = OpenTelemetryMiddleware(api) app = ... app.mount("/api", api)

In that case if a request hits /api/v1/users, then path will be "/v1/users" and root_path will be "/api" — but currently http_url would be wrong, e.g. https://myserver.io/v1/users.

I think concatenating root_path and path should be enough:

full_path = scope.get("root_path", "") + scope.get("path", "") http_url = ( scheme + "://" + server_host + full_path ... )

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py

florimondmanca · 2020-05-26T14:06:52Z

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py

+                        if payload["type"] == "http.response.start":
+                            status_code = payload["status"]
+                            set_status_code(send_span, status_code)


Does OpenTelemetry support storing response headers? If so this is a good place to do it; otherwise please ignore. :-)

Not that I'm aware of and do not see it in the wsgi instrumentation.

ext/opentelemetry-ext-asgi/tests/test_asgi_middleware.py

tests/util/src/opentelemetry/test/asgitestutil.py

florimondmanca · 2020-05-26T14:20:53Z

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py

+                        send_span.set_attribute("type", payload["type"])
+                        await send(payload)
+
+                await self.app(scope, wrapped_receive, wrapped_send)


If OpenTelemetry supports some kind of "attach a traceback to a trace", then we could also try/except around the app call; otherwise, please ignore.

The Python OpenTelemetry client does not store the traceback.

I think this is still up for debate in the standard. But it's a good point, and something I know is highly valuable around existing DD integration. @majorgreys thoughts? should we raise this in the spec?

codeboten

Not an ASGI expert by any means, but it looks like @florimondmanca has that already covered :) The code looks good, just some nits around the copyright headers and setup.cfg versioning. Thanks for taking over this work!

ext/opentelemetry-ext-asgi/setup.py

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/version.py

ext/opentelemetry-ext-asgi/tests/test_asgi_middleware.py

tests/util/src/opentelemetry/test/asgitestutil.py

tests/util/src/opentelemetry/test/wsgitestutil.py

ext/opentelemetry-ext-asgi/setup.cfg

codeboten · 2020-05-26T15:51:36Z

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py

+    ]
+
+
+def http_status_to_canonical_code(code: int, allow_redirect: bool = True):


Added an issue to track refactoring this code as it appears in at least 2 other places #735

Co-authored-by: Florimond Manca <florimond.manca@gmail.com> Co-authored-by: alrex <alrex.boten@gmail.com>

…e/ext_asgi

toumorokoshi · 2020-05-27T04:09:38Z

@majorgreys Looks like you addressed all comments and have enough approvals. I'll merge this in the morning unless you have any objections. Thanks!

toumorokoshi · 2020-05-27T16:17:43Z

thanks!

Skeen and others added 17 commits May 20, 2020 14:15

ext-wsgi duplicated as ext-asgi

5d6df0c

Rename WSGI files

e2b85ad

WSGI to ASGI

f729c3d

Fix ext/testutil

782e9b1

Black reformatting

a343b7d

Isort reformatting

d6ac1e2

Flake8 reformatting

8a8f726

Pylint reformatting

ac7cb72

Documentation

2d32ef9

ASGI only runs under Python 3.5+, fixes to tox/coverage

103ff1d

Signed-off-by: Emil Madsen <sovende@gmail.com>

Added callback to override default span-name, default span name to HT…

4bbc7a6

…TP METHOD Signed-off-by: Emil Madsen <sovende@gmail.com>

Set send_span name immediately, instead of updating it

4833891

Changed span-names to asgi.{scope["type"]}.send/receive

924fe35

Handle scope["server"] = None, by defaulting to 0.0.0.0:80

5a13e0c

Set http.server_name based on Host header

0df9c62

fix tests

c88f152

fix propagation

f0ef937

majorgreys requested a review from a team May 20, 2020 22:21

update changelog

b4356ac

toumorokoshi suggested changes May 20, 2020

View reviewed changes

majorgreys added 3 commits May 20, 2020 21:20

add http.user_agent

c1d3cbe

update version

353bf27

add websocket, disable for lifespan

0e48edd

codeboten reviewed May 21, 2020

View reviewed changes

ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/version.py Outdated Show resolved Hide resolved

codeboten reviewed May 21, 2020

View reviewed changes

docs/opentelemetry.ext.asgi.rst Outdated Show resolved Hide resolved

codeboten reviewed May 21, 2020

View reviewed changes

ext/opentelemetry-ext-asgi/setup.cfg Outdated Show resolved Hide resolved

codeboten reviewed May 21, 2020

View reviewed changes

ext/opentelemetry-ext-asgi/setup.cfg Show resolved Hide resolved

majorgreys and others added 2 commits May 21, 2020 10:25

Apply suggestions from code review

46f3565

Co-authored-by: alrex <alrex.boten@gmail.com>

Merge branch 'majorgreys/feature/ext_asgi' of github.com:DataDog/open…

2f94f83

…telemetry-python into majorgreys/feature/ext_asgi

majorgreys mentioned this pull request May 21, 2020

Add HTTP user-agent to WSGI instrumentation #720

Closed

majorgreys requested a review from codeboten May 21, 2020 22:44

majorgreys added 2 commits May 21, 2020 18:45

Merge remote-tracking branch 'upstream/master' into majorgreys/featur…

0b9fbcd

…e/ext_asgi

handle empty attributes

3ac4620

majorgreys force-pushed the majorgreys/feature/ext_asgi branch from ca868c1 to 3ac4620 Compare May 22, 2020 02:05

majorgreys added 2 commits May 21, 2020 22:17

fix lint

9f3d5bb

Merge remote-tracking branch 'upstream/master' into majorgreys/featur…

9e00976

…e/ext_asgi

majorgreys force-pushed the majorgreys/feature/ext_asgi branch from cb9f3cb to 9e00976 Compare May 22, 2020 02:33

Merge branch 'master' into majorgreys/feature/ext_asgi

01f9310

toumorokoshi approved these changes May 23, 2020

View reviewed changes

Merge branch 'master' into majorgreys/feature/ext_asgi

553969c

florimondmanca reviewed May 26, 2020

View reviewed changes

codeboten approved these changes May 26, 2020

View reviewed changes

majorgreys and others added 4 commits May 26, 2020 19:34

Apply suggestions from code review

9e04228

Co-authored-by: Florimond Manca <florimond.manca@gmail.com> Co-authored-by: alrex <alrex.boten@gmail.com>

more review updates

edc6044

Merge remote-tracking branch 'upstream/master' into majorgreys/featur…

bc6f0a9

…e/ext_asgi

better handling for query_string

3849da1

majorgreys force-pushed the majorgreys/feature/ext_asgi branch from 70f229a to 3849da1 Compare May 27, 2020 01:10

majorgreys added 3 commits May 26, 2020 21:17

added versions for install_requires

bb8506b

at least asgiref 3.0

d888a6f

remove unused import

25b0534

toumorokoshi added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label May 27, 2020

majorgreys added 2 commits May 27, 2020 07:45

Merge branch 'master' into majorgreys/feature/ext_asgi

8c9eded

Merge branch 'master' into majorgreys/feature/ext_asgi

22dbe06

toumorokoshi merged commit 64b3cf2 into open-telemetry:master May 27, 2020

majorgreys mentioned this pull request May 28, 2020

Middleware for Starlette/FastAPI #710

Closed

majorgreys mentioned this pull request Jun 8, 2020

REQUEST: New membership for majorgreys open-telemetry/community#378

Closed

6 tasks

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

docs: typo in Ville de Montréal (open-telemetry#716)

96bfebb

		]


		def http_status_to_canonical_code(code: int, allow_redirect: bool = True):

Conversation

majorgreys commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

florimondmanca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

florimondmanca May 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

florimondmanca May 26, 2020

Choose a reason for hiding this comment

Uh oh!

majorgreys May 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

florimondmanca May 26, 2020

Choose a reason for hiding this comment

Uh oh!

majorgreys May 27, 2020

Choose a reason for hiding this comment

Uh oh!

toumorokoshi May 27, 2020

Choose a reason for hiding this comment

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codeboten May 26, 2020

Choose a reason for hiding this comment

Uh oh!

toumorokoshi commented May 27, 2020

Uh oh!

toumorokoshi commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

majorgreys commented May 20, 2020 •

edited

Loading