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

aiohttp client tracing #1372

Closed
wants to merge 169 commits into from
Closed

Conversation

thehesiod
Copy link
Contributor

@thehesiod thehesiod commented Apr 18, 2020

Continuation of #294. This PR adds tracing support for

  • ClientSession
  • ClientResponse
  • Connector

The following changes are also made:

  • use async/await syntax

@macleodmac
Copy link

Would love to see this merged, it'll make our distributed tracing so much easier!

@zikphil
Copy link

zikphil commented Jan 6, 2021

Could we know where the status of this PR at?

@mergify
Copy link
Contributor

mergify bot commented Feb 3, 2021

@thehesiod this pull request is now in conflict 😩

@mergify mergify bot added the conflict label Feb 3, 2021
@mklokocka
Copy link

mklokocka commented Feb 6, 2021

It would be nice to have an option to use the client tracing without patching all sessions - i.e. having an option to instrument any ClientSession but not forcing all of them to be so (this is particularly useful when there are requests going to third parties where the user does not want the tracing headers propagated).

@thehesiod
Copy link
Contributor Author

@mklokocka I had support for this but removed it at request of DD for a forthcoming new way of achieving the same effect that I have yet to see

@mklokocka
Copy link

@thehesiod @Kyle-Verhoog Similar question as on #394, can this PR now be finished? :)

@RobinMcCorkell
Copy link

Any progress on this PR?

@gabrielelanaro
Copy link

If this is not being picked up I would like to give it a try and open a new one

@thehesiod
Copy link
Contributor Author

we have been using this in production for over a year (in particular the aio-libs one), my understanding is that they're working out an async rewrite in another PR before looking at this again. I was working with @Kyle-Verhoog , just waiting on an update from him on how to proceed

@Kyle-Verhoog
Copy link
Member

hey @RobinMcCorkell, @gabrielelanaro, @thehesiod! Yes, I think we've got the async work done that we wanted to (mainly #1936). I'm going to try to find the time to get these aio* PRs back up to date.

@emjohnson20
Copy link

This PR would be incredibly useful 👍

@louis-jaris
Copy link

Hey guys! Thank you for this work! I'm also looking forward to seeing this PR merged 👍
I'd love to help you ✌️

@jfalkenstein
Copy link

I'd really love to see this instrumented.

Kyle-Verhoog added a commit that referenced this pull request Mar 9, 2022
This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

#294 and later #1372 included additional support for ClientResponse and StreamReader which are omitted here with the intention of introducing them as follow ups.

Co-authored-by: Alexander Mohr <[email protected]>
@Kyle-Verhoog
Copy link
Member

This has been addressed in #3362! It will be included in 0.60 🙂

Thanks everyone for their patience!

Kyle-Verhoog added a commit that referenced this pull request Mar 9, 2022
This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

#294 and later #1372 included additional support for ClientResponse and StreamReader which are omitted here with the intention of introducing them as follow ups.

Co-authored-by: Alexander Mohr <[email protected]>
brettlangdon pushed a commit that referenced this pull request Mar 9, 2022
This PR adds support for aiohttp client.

Work was long ago started on this in (#294 (thank you @thehesiod!!!). There were a number of issues in the library that led to not being able to merge in the work such as async context management, service naming, integration configuration and more which have all since been addressed.

#294 and later #1372 included additional support for ClientResponse and StreamReader which are omitted here with the intention of introducing them as follow ups.

Co-authored-by: Alexander Mohr <[email protected]>

Co-authored-by: Kyle Verhoog <[email protected]>
Co-authored-by: Alexander Mohr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.