Add support for aiobotocore #6
Conversation
aws_xray_sdk/core/async_recorder.py
Outdated
| def capture_async(self, name=None): | ||
| """ | ||
| A decorator that records enclosed function in a subsegment. | ||
| It only works with synchronous functions. |
| return wrapped(*args, **kwargs) | ||
|
|
||
|
|
||
| def aws_meta_processor(wrapped, instance, args, kwargs, |
There was a problem hiding this comment.
The aws_meta_processor can be shared between botocore and aiobotocore I believe? You can probably move the aws_meta_process one level above so it can be used by two patchers.
There was a problem hiding this comment.
Yep, good shout, Most of the functions in that module can also be shared.
| ) | ||
|
|
||
|
|
||
| async def _xray_traced_aiobotocore(wrapped, instance, args, kwargs): |
There was a problem hiding this comment.
Could you explain more on using async def/await on the patcher and previously aiohttp while using async.corotines/yield from on the recorder?
There was a problem hiding this comment.
Ahh, my bad. It was a sort of lazy copy and paste, originally the methods in the async recorder were in the main recorder which had @coroutine on them from compat. Currently them being @asyncio.coroutine will allow them to be used in py34 without complaints, aiobotocore uses aiohttp which has dropped py34 support and is now using async def syntax. So either the async recorder stays compatible with py34 and people can use it with other frameworks, or we just draw a line and just do async purely on py35+. Your choice, I have no preference
There was a problem hiding this comment.
Yes we should support py35+ only.
docs/thirdparty.rst
Outdated
|
|
||
| On top of patching aioboto3 or aiobotocore, the xray_recorder also needs to be | ||
| configured to use the ``AsyncContext``. The following snippet shows how to set | ||
| up the X-Ray SDK with an Async Context, you probably want to do this near the |
There was a problem hiding this comment.
You can remove "you probably want to ....". A redirect to the section for configuring the global recorder should be sufficient. And you also need to mention that it only supports python 3.5 and above.
tests/test_async_recorder.py
Outdated
| pass | ||
|
|
||
|
|
||
| async def test_capture(loop): |
There was a problem hiding this comment.
Could you add a test for nested async functions? An decorated async function calls another decorated async function should result in two nested subsegments.
There was a problem hiding this comment.
Ahh cool I didnt realise that, but it makes sense. Will do
|
|
||
| commands = | ||
| py{27,34}: coverage run --source aws_xray_sdk -m py.test tests --ignore tests/ext/aiohttp --ignore tests/test_async_local_storage.py | ||
| py{27,34}: coverage run --source aws_xray_sdk -m py.test tests --ignore tests/ext/aiohttp --ignore tests/ext/aiobotocore --ignore tests/test_async_local_storage.py --ignore tests/test_async_recorder.py |
There was a problem hiding this comment.
Again the keyword you used for capture_async actually works with 3.4 but the aiobotocore patcher only work with 3.5.
There was a problem hiding this comment.
Have used async/await throughout so best to exclude those tests from py34
There was a problem hiding this comment.
Yeah I mean let's get all async related stuff to be 3.5+ only.
There was a problem hiding this comment.
Yeah 😄, async in 3.4 is pretty nasty anyway. I "think" I've excluded 3.4 from anything async and purged any @asyncio.coroutine's too
|
I also updated the code that loads in json to use pkg_resources so then the module can also be used in egg/zip form without being extracted. Had to do a |
|
One last thing is that I like the new structure of ext/resources/botocore_para_whitelist.json. Could we rename it to aws_para_whitelist.json? It's more obvious as more and more files added under resources/ folder. Also botocore causes some confusion on if that json file is effective to boto3 or aiobotocore etc. |
|
Sure, yeah exactly. |
|
Should be good |
|
Oh one thing I forgot is that feel free to add |
|
Sure, I'll push some more changes tomorrow. |
Also fixed typo from my last PR yield from is not valid syntax in py27 so was causing issues in the recorder module, instead recorder is subclassed to provide some py3 compatible async functions and the recorder class] will be chosen depending on if py2 or py3. Set the correct context class in all test cases as they would inherit the context of the last test ran, in some cases botocore was trying to use a async context. Makes use of async syntax consistent. Reduced duplication of boto patching functions Made the library zipsafe when loading resources Improved test for testing nested subsegments Renamed botocore_para_whitelist.json Updated README
|
Added capture async example and a slimmed down example of adding middleware to aiohttp |
Closes #5
Added Async functions for begin subsegment and capture decorator.
Also fixed typo from my last PR
The
yield fromsyntax is not valid syntax in py27 so was causing issues when I tried to add the async functions in the recorder module. Even tried some evil generator hacks but then that didn't work when py3 tried to use the functions.Instead the recorder is subclassed to provide some py3 compatible async functions. When the SDK is imported in py3, the async compatible recorder class will be chosen.
The
get_new_stubbed_recorderfunction was also updated to match the above logic. All the test cases were modified to explicitly set the context, as I was having issues where botocore was using an AsyncContext and then getting in a mess.Updated the docs to mention if one is using aioboto3/aiobotocore that they should use the AsyncContext and provided a snippet.
Tox seems to be running ok on py27,34,35,36.