-
Notifications
You must be signed in to change notification settings - Fork 504
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
feat(aiohttp): add instrumentation of client requests #1761
feat(aiohttp): add instrumentation of client requests #1761
Conversation
Just wanted to comment and say it would be cool to get this merged, as we are using aiohttp entirely for making client requests in our app and not at all for the webserver side of it (we use FastAPI/Starlette for that). |
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
I would still like to see this PR merged. I think it has value for the community. |
27d1187
to
109be3a
Compare
I'd like to bump this again because I'd love to see this merged. |
Hey @ccastleberry ! Thanks for bumping. We have now one more full time engineer that will spend time with the Python SDK, so we should get faster in the next couple of weeks (hopefully) |
Hey @md384, thanks for this PR. I wanted to merge master into it to bring it up to the current SDK changes and run the tests, but it seems like the PR doesn't allow edits by others. Could you enable them or update the PR directly? I'll take a look and test this locally today. |
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.
Looks great! Couple of small things need changing (most of them have to do with changes that happened after this PR was opened -- sorry for the long wait!). @md384 please let us know if you have time to look at this. Alternatively you can set the PR branch to allow edits from us and then we can take it from here.
No worries if you don't have time to do either, we can also proceed by merging this and then making the changes on top.
Thanks again!
@sentrivana I can make these changes and will get them back to you soon |
3a6198f
to
e1622b4
Compare
@sentrivana some of the existing tests are failing for aiohttp for me. I think this is because we now use the aiohttp client for testing the aiohttp server integration where we try and set the sentry-trace header directly but my changes now create a span (and therefore a trace-id) for the client requests, resulting in a trace-id mismatch. I'll try and fix these up. EDIT: I've added an extra commit to pull out the trace id that was added to the request automatically by the client instrumentation so we can compare to the trace-id on the server generated spans in the unit tests. |
@md384 Thanks a lot for the super fast fixes! The code looks good to me, I'll give it a spin next week and see if we need anything additional (like docs) and then we can merge this. |
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.
Found two minor things.
Thanks for your patience @md384. I've enabled automerge so this now only needs to be rebased on master or having master merged into it to apply cleanly. |
c4e3664
to
ac5a45c
Compare
Adds instrumentation of client requests for aiohttp. See https://docs.aiohttp.org/en/stable/client_advanced.html#client-tracing for aiohttp documentation on tracing hooks for client requests.
This change could arguably go in a separate AioHttpClientIntegration but will leave that for discussion.