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

Client request tracing. Allow to get the request body in the trace. #2755

Closed
kowalski opened this issue Feb 23, 2018 · 9 comments
Closed

Client request tracing. Allow to get the request body in the trace. #2755

kowalski opened this issue Feb 23, 2018 · 9 comments
Labels
Milestone

Comments

@kowalski
Copy link
Contributor

Long story short

I would like to get the request_body in the tracing events. Obviously it cannot be included in on_request_start because body can come from generator.

I would be happy to contribute another event, say on_body_sent which would fire after the body is written after the body is finished.

@pfreixes
Copy link
Contributor

Please go ahead!

@asvetlov
Copy link
Member

Saving response body would be interesting as well.

@kowalski
Copy link
Contributor Author

Ok, great! Thanks for encouragement.

Before I get into coding I would like to discuss an idea around this.
The barest possible idea is to simply save the request body bytes into a variable and pass it to a Signal after everything is written. Unfortunately I can see how in certain use cases this could have catastrophic performance and memory footprint.
So instead I would like to propose, that I will move registering for grabbing a request/response body into a method on:

class TraceConfigContext(SimpleNamespace):

    def register_content_handlers(self, request_stream, response_stream):
          ....

The idea would be that this method is called from on_request_start callback and the results are gathered in on_request_end.

Also this means, that if user code chooses not to read the response body it will not be available for tracing. I mean getting 400 status when the code looks like:

async with session.get(url) as response:
    if response.status == 400:
        raise SomeError
    # here the normal processing follows, call response.json() and do something with in

What do you think ?

@pfreixes
Copy link
Contributor

Umm ... once the strings are allocated in memory I would say that internally the only thing that is given as a parameter is just a String object that points to that region. So, passing the request or the response as a parameter won't affect the memory, and only if the signal handle mutates the string a new copy will be made.

am I missing something?

@kowalski
Copy link
Contributor Author

Sorry for not being clear.
I meant a scenario when request body is not passed as string, but as for example aiohttp.payload.BufferedReaderPayload pointing to a file which will not fit in memory.

@asvetlov
Copy link
Member

We should call signal as many times as we call transport.write. In this case everything should fit in memory.

@pfreixes
Copy link
Contributor

We might thing on rename the signals as on_request_chunk_sent and on_response_chunk_received, if chunk is used this function will be called many times if not will be called once.

@asvetlov
Copy link
Member

asvetlov commented Mar 1, 2018

Fixed by #2767

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants