Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ASGI middleware #716

Merged

Conversation

majorgreys
Copy link
Contributor

@majorgreys majorgreys commented May 20, 2020

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

@majorgreys majorgreys requested a review from a team May 20, 2020 22:21
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

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

majorgreys and others added 2 commits May 21, 2020 10:25
Co-authored-by: alrex <alrex.boten@gmail.com>
…telemetry-python into majorgreys/feature/ext_asgi
@majorgreys majorgreys requested a review from codeboten May 21, 2020 22:44
@majorgreys majorgreys force-pushed the majorgreys/feature/ext_asgi branch from ca868c1 to 3ac4620 Compare May 22, 2020 02:05
@majorgreys majorgreys force-pushed the majorgreys/feature/ext_asgi branch from cb9f3cb to 9e00976 Compare May 22, 2020 02:33
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Great! Thanks for addressing feedback. LGTM.

Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

👋 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 Outdated Show resolved Hide resolved
ext/opentelemetry-ext-asgi/README.rst Outdated Show resolved Hide resolved
Comment on lines 83 to 87
http_url = (
scope.get("scheme") + "://" + server_host + scope.get("path", "")
if scope.get("scheme") and server_host and scope.get("path")
else None
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines 200 to 202
if payload["type"] == "http.response.start":
status_code = payload["status"]
set_status_code(send_span, status_code)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

ext/opentelemetry-ext-asgi/tests/test_asgi_middleware.py Outdated Show resolved Hide resolved
tests/util/src/opentelemetry/test/asgitestutil.py Outdated Show resolved Hide resolved
send_span.set_attribute("type", payload["type"])
await send(payload)

await self.app(scope, wrapped_receive, wrapped_send)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Python OpenTelemetry client does not store the traceback.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
ext/opentelemetry-ext-asgi/tests/test_asgi_middleware.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-asgi/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-asgi/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-asgi/setup.cfg Outdated Show resolved Hide resolved
]


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

Choose a reason for hiding this comment

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

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

majorgreys and others added 4 commits May 26, 2020 19:34
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
Co-authored-by: alrex <alrex.boten@gmail.com>
@majorgreys majorgreys force-pushed the majorgreys/feature/ext_asgi branch from 70f229a to 3849da1 Compare May 27, 2020 01:10
@toumorokoshi
Copy link
Member

@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 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
@toumorokoshi toumorokoshi merged commit 64b3cf2 into open-telemetry:master May 27, 2020
@toumorokoshi
Copy link
Member

thanks!

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants