-
Notifications
You must be signed in to change notification settings - Fork 567
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
Add support for db cursors and connections in context managers #821
Add support for db cursors and connections in context managers #821
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;")
|
I signed it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @carlbordum. If you could add a unit test for this change and update the changelog, i'd be happy to review and approve.
@codeboten I want to write the test, but I can't quite figure out how to do it without mocking the entire functionality, which would make the tests pointless. Can you offer any guidance? |
@carlbordum |
@carlbordum I think @lzchen is right, the integration tests are probably the easiest place to test this, there's already some postgres tests you could take a look at opentelemetry-python/ext/opentelemetry-ext-docker-tests/tests/pymysql/test_pymysql_functional.py Line 78 in 8cbd9d4
|
@carlbordum any chance you'll find time to add the integration test? |
@codeboten should I write a test and open an alternative PR? The 2020-08-06 SIG notes mention this PR, but I don't know whether you have discussed anything afterwards. |
Hey thanks for showing interest in this. Yeah due to the lack of activity from the original owner, I think it would be great if you could take the changes and add a test for it. Once you put up a new PR, just tag this one and I will close this. |
closed by #1028 |
Co-authored-by: Olivier Albertini <olivier.albertini@montreal.ca>
Here is an example snippet that will not report tracing without this patch: