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

[Requests] Replace span name callback with request and response hooks #411

Closed
owais opened this issue Apr 5, 2021 · 8 comments · Fixed by #1717
Closed

[Requests] Replace span name callback with request and response hooks #411

owais opened this issue Apr 5, 2021 · 8 comments · Fixed by #1717

Comments

@owais
Copy link
Contributor

owais commented Apr 5, 2021

Requests instrumentation accepts a span name callback which should be replaced with more generic request/response callbacks (hooks).

Details: #408

@NickSulistio
Copy link
Contributor

I can take on this issue.

cc @alolita

@lzchen
Copy link
Contributor

lzchen commented May 24, 2021

@NickSulistio
Are you still interested in taking this on?

@NickSulistio
Copy link
Contributor

@lzchen

Yeah! Sorry for the delays, I can get it done this week.

@ryokather
Copy link
Contributor

Hey @lzchen just wanted to confirm that this is still open since I'm cleaning up the remaining instrumentations without hooks :) (#412).

cc: @alolita

@lzchen
Copy link
Contributor

lzchen commented Jun 21, 2021

If you are working on all of the instrumentations that is great! We just have individual tasks for the hooks so be sure to link your PR to each when you submit it.

@lzchen
Copy link
Contributor

lzchen commented Sep 13, 2021

@ryokather
Do you have plans on adding hooks for the requests instrumentation?

@ryokather
Copy link
Contributor

Hi @lzchen. I believe in my previous PR I investigated into this and found out that implementing hooks on the requests instrumentation isn't easily achievable because the wrapper for Session.request provides no ability to access a request-like object which is necessary for a request_hook. I left it in case anyone else has a better idea!

@federicobond
Copy link
Member

Hi, I have been investigating this issue and apparently we could remove the instrumentation of Session.request and leave only Session.send (request goes through send anyway).

When you remove the instrumentation for request only one test fails: test_invalid_url. I went to look at how other instrumentations handle this case and, at least for the ones I checked (aiohttp and urllib), they do not create a span if they can't build a request due to an invalid URL.

Thus, by the time we reach send we always have a PreparedRequest object we can use for the request_hook callback argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants