Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 35 commits
5d6df0c
e2b85ad
f729c3d
782e9b1
a343b7d
d6ac1e2
8a8f726
ac7cb72
2d32ef9
103ff1d
4bbc7a6
4833891
924fe35
5a13e0c
0df9c62
c88f152
f0ef937
b4356ac
c1d3cbe
353bf27
0e48edd
46f3565
2f94f83
429b8e2
83139f0
3759a51
a4cce67
da0a00b
16f37f7
0b9fbcd
3ac4620
9f3d5bb
9e00976
01f9310
553969c
9e04228
edc6044
bc6f0a9
3849da1
bb8506b
d888a6f
25b0534
8c9eded
22dbe06
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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:
In that case if a request hits
/api/v1/users
, thenpath
will be"/v1/users"
androot_path
will be"/api"
— but currentlyhttp_url
would be wrong, e.g.https://myserver.io/v1/users
.I think concatenating
root_path
andpath
should be enough: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.
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?