-
Notifications
You must be signed in to change notification settings - Fork 623
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
Add ASGI middleware #716
Conversation
Signed-off-by: Emil Madsen <[email protected]>
…TP METHOD Signed-off-by: Emil Madsen <[email protected]>
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.
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
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/version.py
Outdated
Show resolved
Hide resolved
Co-authored-by: alrex <[email protected]>
…telemetry-python into majorgreys/feature/ext_asgi
ca868c1
to
3ac4620
Compare
cb9f3cb
to
9e00976
Compare
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.
Great! Thanks for addressing feedback. LGTM.
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.
👋 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.
http_url = ( | ||
scope.get("scheme") + "://" + server_host + scope.get("path", "") | ||
if scope.get("scheme") and server_host and scope.get("path") | ||
else None | ||
) |
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.
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
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py
Outdated
Show resolved
Hide resolved
if payload["type"] == "http.response.start": | ||
status_code = payload["status"] | ||
set_status_code(send_span, status_code) |
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.
Does OpenTelemetry support storing response headers? If so this is a good place to do it; otherwise please ignore. :-)
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.
Not that I'm aware of and do not see it in the wsgi instrumentation.
send_span.set_attribute("type", payload["type"]) | ||
await send(payload) | ||
|
||
await self.app(scope, wrapped_receive, wrapped_send) |
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.
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.
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.
The Python OpenTelemetry client does not store the traceback.
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.
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?
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.
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/src/opentelemetry/ext/asgi/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/version.py
Outdated
Show resolved
Hide resolved
] | ||
|
||
|
||
def http_status_to_canonical_code(code: int, allow_redirect: bool = True): |
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.
Added an issue to track refactoring this code as it appears in at least 2 other places #735
Co-authored-by: Florimond Manca <[email protected]> Co-authored-by: alrex <[email protected]>
70f229a
to
3849da1
Compare
@majorgreys Looks like you addressed all comments and have enough approvals. I'll merge this in the morning unless you have any objections. Thanks! |
thanks! |
Updating #402 which had been inactive for some time to get remaining to resolve merge conflicts and address any remaining issues.