add support for psycopg3 #396
Conversation
|
Hi @Dilski, thanks for contributing this instrumentation!
Please let us know if you have any questions or need more help setting up a local end-to-end test. (Referencing #395) |
change patching and remove unrelated tests
Hi @carolabadeer - we've tried to address the issues and have managed to run the test suite at our side. Can we request a re-review? |
add missing psycopg[pool] dependency
There was a problem hiding this comment.
Just a few small comments, but otherwise looking good!
Thanks @Dilski and @csteinle for addressing the CI issues and testing the instrumentation locally. I think it would also be very helpful to add a new section to the ReadMe for this instrumentation. It can contain a short code snippet (similar to these sections) on how to setup and use the psycopg3 instrumentation
| _xray_register_default_jsonb_fix | ||
| 'psycopg_pool.pool', | ||
| 'ConnectionPool._connect', | ||
| _xray_traced_connect |
There was a problem hiding this comment.
Can you give some insight into why these specific functions are being wrapped? I see a new dependency on psycopg pool that was not there in the initial commit, what is this package responsible for and is it sufficient to only patch the connection?
| ) | ||
|
|
||
|
|
||
| def _xray_traced_connect(wrapped, instance, args, kwargs): | ||
| conn = wrapped(*args, **kwargs) | ||
| parameterized_dsn = {c[0]: c[-1] for c in map(methodcaller('split', '='), conn.dsn.split(' '))} | ||
| parameterized_dsn = {c[0]: c[-1] for c in map(methodcaller('split', '='), conn.info.dsn.split(' '))} |
There was a problem hiding this comment.
Nit: add a comment explaining what parameterized_dsn is for readability
|
Hey folks, any progress on this? I'd also like to take advantage of this patching logic once it's in, and am weighing whether it's worth waiting for a contribution back to |
|
@jhubberts this is something we want in, but haven't prioritised/had the time to address feedback on. Any help would be appreciated |
Issue #, if available: #395
Description of changes: Add support for psycopg3 (now just psycopg)
I have not been able to test this locally, as I can't find any development documentation in this repo.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.