-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fix redis.asyncio sync operations wrapper #782
Fix redis.asyncio sync operations wrapper #782
Conversation
|
…s-wrapper' into fix-redis-asyncio-sync-operations-wrapper
* Added separate instrumentation for redis.asyncio.client Merge main branch updates Add tests for newrelic/config.py (newrelic#860) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Modify redis tests * removed redis.asyncio from aioredis instrumentation removed aioredis instrumentation in redis asyncio client removed redis.asyncio from aioredis instrumentation --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Lalleh Rafeei <[email protected]> Co-authored-by: Lalleh Rafeei <[email protected]>
Codecov Report
@@ Coverage Diff @@
## develop-swap-redis-asyncio-commits #782 +/- ##
=====================================================================
Coverage ? 81.69%
=====================================================================
Files ? 185
Lines ? 18638
Branches ? 3265
=====================================================================
Hits ? 15226
Misses ? 2496
Partials ? 916 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@lrafeei |
@bc291 No worries--I'll reopen this and if you sign the CLA we can get this one merged |
…cio-sync-operations-wrapper
83fa78f
into
newrelic:develop-swap-redis-asyncio-commits
* Revert "Redis asyncio testing (#881)" (#911) * Revert "Redis asyncio testing (#881)" This reverts commit 05cff1b. * Add spublish to list of commands * Fix redis.asyncio sync operations wrapper (#782) * Fix redis.asyncio sync operations wrapper * Add clean ups * Fixes: - `loop` -> noqa - catch `TimeoutError` on `asyncio.timeout` * Added separate instrumentation for redis.asyncio.client (#808) * Added separate instrumentation for redis.asyncio.client Merge main branch updates Add tests for newrelic/config.py (#860) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * Modify redis tests * removed redis.asyncio from aioredis instrumentation removed aioredis instrumentation in redis asyncio client removed redis.asyncio from aioredis instrumentation --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Lalleh Rafeei <[email protected]> Co-authored-by: Lalleh Rafeei <[email protected]> * Modify tests * Tweak tests and instrumentation * Tweak tests to separate aioredis and redis.asyncio * Correctly separate commands to sync/async --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Ahmed <[email protected]> Co-authored-by: Lalleh Rafeei <[email protected]> Co-authored-by: Lalleh Rafeei <[email protected]> * Remove formatting in import --------- Co-authored-by: Błażej Cyrzon <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Ahmed <[email protected]>
Overview
redis.asyncio: Fix synchronous methods wrapped in coroutines
Related Github Issue
#623
Testing
Few tests added - I'm not quite sure if put in the right place
Bug reproduction
To reproduce this behaviour sync method of async redis may be used.
As example
redis.pubsub()
could be invoked, followed by pattern subscription:This results in
AttributeError: 'coroutine' object has no attribute 'psubscribe'
:Culprit
Bug perhaps lays here
Logic assumes that every
redis.*
interaction is an asynchronous operation and it breaks usage of e.g.pubsub()
,monitor()
,client()
and many more (see solution no 3 below).Solution
inspect.iscoroutinefunction - rejected
inspect.iscoroutinefunction
may be problematic here because -> coroutine happens after invocation:So we can invoke method prematurely, but I think that's going to break DatastoreTrace
inspect.signature - rejected
Another solution would be to
inspect.signature()
of wrapped function, e.g.:but after some investigation this is not reliable across python versions.
predefined sync methods list - imlemented in this PR
Using
_redis_client_methods
and someinspect.signature(wrapped).return_annotation
we can compile sync method list:and simply use that in wrapping function decision.