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: Attach request body #220

Closed
untitaker opened this issue Dec 19, 2018 · 10 comments · Fixed by #527
Closed

AIOHTTP: Attach request body #220

untitaker opened this issue Dec 19, 2018 · 10 comments · Fixed by #527
Labels
Help wanted Extra attention is needed

Comments

@untitaker
Copy link
Member

Methods for reading the request body are async in AIOHTTP, but event processors are not. So right now we can't attach any request bodies to the event without severely affecting perf.

@untitaker untitaker added enhancement Help wanted Extra attention is needed labels Dec 19, 2018
@sivakov512
Copy link

Any solutions here?

@untitaker
Copy link
Member Author

@sivakov512 no, I would prefer if the community investigated how other code deals with aiohttp for reading bodies twice

@mister-vio
Copy link

mister-vio commented Jul 9, 2019

@untitaker
You don't need to deal with reading twice, because the aiohttp does a cache of the body in bytes on the first read call. BaseRequest.read()

@mister-vio
Copy link

@untitaker
I could be out of the concept, by from the processor you could get a loop and create async task which will get the body and send it to sentry. something like this:
`from aiohttp import web
import asyncio

loop = asyncio.get_event_loop()

async def async_task(request: web.Request):
    body = await request.read()
    # send to sentry

def processor():
    loop = asyncio.get_event_loop()
    request: web.Request = None
    loop.create_task(async_task(request))


async def async_func():
    processor()

loop.run_until_complete(async_func())`

@untitaker
Copy link
Member Author

@EasyLovv that sounds a bit dangerous. I wonder if we should just grab request._post and request._read_bytes if it's already there, and just do nothing if it isn't.

@mister-vio
Copy link

mister-vio commented Jul 12, 2019

@untitaker yeah, it is also an option, would be great to have at least that. But in my view, I can't see anything dangerous, we are just reading the request body, any unsupported behavior or type could be covered by try-catch and posted to sentry with some additional "Unsupported body" tag etc.

@untitaker
Copy link
Member Author

@EasyLovv my issue is that I need the request body as string in the processor. I can't just spawn a task and return. So I block for async_task in processor, which ends up behaving like a blocking sync operation in async_func. Not cool for performance.

@untitaker
Copy link
Member Author

@EasyLovv do you want to make a PR for this so people can try it out?

@samuelcolvin
Copy link

In case it's of any interest or help to anyone else, I have a fulling working aiohttp-sentry integration implemented as middleware, see here.

Also aio-libs/aiohttp#3557 and aio-libs/aiohttp#3767 mean that with aiohttp 4 all error logging will possible in access loggers without adding middleware or messing with the nuts and bolts of running servers.

@untitaker
Copy link
Member Author

The integration takes extra precautions to not make scope data leak across request boundaries. Your middleware avoids this issue by assembling the entire event payload before calling capture_event, but this means that custom calls to capture_* will not have request data.

I would really like the work for request bodies to land in the SDK, but possibly hidden behind an option (kwarg to the integration object).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants