Skip to content

Conversation

@dspatoulas
Copy link

@dspatoulas dspatoulas commented Nov 18, 2025

Description

Update PsycopgInstrumentor.instrument_connection to support both sync and async psycopg connections by configuring the appropriate cursor_factory.

Previously, instrument_connection always used the sync cursor factory and did not properly handle AsyncConnection instances. As a result, calls made through an async connection would fail because of the async context.

With this change:

  • If connection is a psycopg.AsyncConnection, we set connection.cursor_factory to _new_cursor_async_factory(...).
  • Otherwise, we set connection.cursor_factory to _new_cursor_factory(...).

This aligns instrument_connection with the behavior of the global instrument() helper and ensures both sync and async connections are correctly traced.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Updated the tests in the psycopg instrumentation test suite to cover both sync and async connections, added a new test for the async connection code path that was previously not tested.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 18, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@dspatoulas dspatoulas force-pushed the fix-psycopg-async-cursor branch from 93f0638 to b3a3ee1 Compare November 18, 2025 16:28
@herin049
Copy link
Contributor

herin049 commented Nov 19, 2025

Can we add some unit tests to cover this scenario? Also make sure to add a changelog entry.

@dspatoulas
Copy link
Author

Can we add some unit tests to cover this scenario? Also make sure to add a changelog entry.

Yup, updated the existing tests and added a new test for this change.

@dspatoulas dspatoulas force-pushed the fix-psycopg-async-cursor branch from ddb7153 to e2ff5af Compare November 22, 2025 05:36
@xrmx xrmx moved this to Ready for review in @xrmx's Python PR digest Nov 27, 2025
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase / merge main and add a CHANGELOG entry.

@xrmx xrmx moved this from Ready for review to Approved PRs in @xrmx's Python PR digest Nov 28, 2025
@dspatoulas dspatoulas force-pushed the fix-psycopg-async-cursor branch from e2ff5af to 5670b96 Compare November 29, 2025 03:27
@dspatoulas dspatoulas requested a review from a team as a code owner November 29, 2025 03:39
@dspatoulas
Copy link
Author

Please rebase / merge main and add a CHANGELOG entry.

Rebased and updated the CHANGELOG with fix details

@xrmx xrmx moved this from Approved PRs to Reviewed PRs that need fixes in @xrmx's Python PR digest Dec 1, 2025
@xrmx
Copy link
Contributor

xrmx commented Dec 1, 2025

@dspatoulas pylint is not happy

opentelemetry-instrumentation-psycopg/tests/test_psycopg_integration.py:116:4: W0221: Number of parameters was 2 in 'AsyncConnection.cursor' and is now 1 in overriding 'MockAsyncConnection.cursor' method (arguments-differ)

@dspatoulas
Copy link
Author

dspatoulas commented Dec 1, 2025

@dspatoulas pylint is not happy

opentelemetry-instrumentation-psycopg/tests/test_psycopg_integration.py:116:4: W0221: Number of parameters was 2 in 'AsyncConnection.cursor' and is now 1 in overriding 'MockAsyncConnection.cursor' method (arguments-differ)

@xrmx sorry about that, I ran the tests locally but not the linter 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Reviewed PRs that need fixes

Development

Successfully merging this pull request may close these issues.

3 participants