Add support for db cursors and connections in context managers#1028
Add support for db cursors and connections in context managers#1028codeboten merged 11 commits intoopen-telemetry:masterfrom
Conversation
Here is an example snippet that will not report tracing without this patch:
with psycopg2.connect(...) as conn, conn.cursor() as cursor:
cursor.execute("select 1;")
|
Kind of forgot there were other drivers I might want to add the same test for. However, running into a test fail with that, so I'll mark as draft until I've looked into that. |
Since anonymous volumes do not get reused, each test run left behind new volumes.
The same function exists in the aiopg tests under a different name.
ffe4
force-pushed
the
pr-821-docker-test
branch
from
735a7f0 to
7f43a85
Compare
|
Turns out the error was just due to The wrapper is pretty straight-forward, so I was not sure whether to add the tests to all packages and stopped half-way. I also removed some unused imports and renamed a helper function for consistency. |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Changes look good, thanks for picking this up. Could you add a note in the dbapi changelog?
|
Done. |
|
Awesome! Thanks @ffe4 |
aabmass
left a comment
aabmass
left a comment
There was a problem hiding this comment.
Is this PR just for dbapi? If you're willing, looks like asyncpg also supports context managers for transactions https://magicstack.github.io/asyncpg/current/api/index.html?highlight=context%20manager#transactions
tests/opentelemetry-docker-tests/tests/asyncpg/test_asyncpg_functional.py
Show resolved
Hide resolved
The async with self._connection.transaction():
await self._connection.execute("SELECT 42;") |
|
@codeboten LGTM
Looks like this is covered already since it just executes the |
|
Thank you for finishing this, @ffe4 |
4 participants
Description
This PR is a replacement for #821 with added docker tests.
I also added the
-vflag todocker-compose downin the tox config to clean up anonymous volumes that get left behind and not reused.Type of change
Checklist: