Fix tracing of async cursors for psycopg

[ibash]

Description

This copies the traced_execution of AsyncCursorTracer except query_method is awaited within the span.

Fixes #2486

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?

I ran this locally by making the corresponding changes in my site-packages folder. Then I checked logfire (what we're using for opentelemetry) to confirm that sql queries now had non-zero timings.

Also ran the tox command to run tests.

Happy to build and test the built package if you tell me how to build!

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]

The committers listed above are authorized under a signed CLA.

• ✅ login: ibash / name: Islam Sharabash (71f50a7, f940556, 0d8cd74, cc08874)
• ✅ login: aabmass / name: Aaron Abbott (bb47c80)

[ibash]

@federicobond would love a review when you get a chance!

[emilingerslev]

I've validated that this indeed solves the problem. Only concern here is minor; The override of traced_execution changes the signature from sync to async, effectively also changing the return type. Another option is to extend CursorTracer with a traced_execution_async method, keeping it all in one class, but supporting both.

But as I said this works ✅

[xrmx]

I've validated that this indeed solves the problem. Only concern here is minor; The override of traced_execution changes the signature from sync to async, effectively also changing the return type. Another option is to extend CursorTracer with a traced_execution_async method, keeping it all in one class, but supporting both.

Thanks for testing. Not sure I follow the return type concern, AFAIU the users of that _cursor_tracer are awaiting it so the fact it was not an async function was the issue?

[xrmx]

I think we are missing tests to avoid regressions in the future

[ibash]

@xrmx sorry for the delay here -- added a regression test. Please let me know if there's a better way to assert this behavior.

[jwilm]

Thanks @ibash for this PR and @xrmx for your reviews. I'm eager to see this PR land and published; is there anything I can do to help?

[ibash]

@jwilm not ideal but I did publish the patched version to github, in our requirements.txt we use:

opentelemetry-instrumentation-fastapi==0.52b1
opentelemetry-instrumentation-psycopg @ https://github.com/ibash/opentelemetry-python-contrib/releases/download/v52b1/opentelemetry_instrumentation_psycopg-0.52b1-py3-none-any.whl

[ibash]

@xrmx fixed the formatting issue -- any chance this can get reviewed / merged?

[Kludex]

I've validated that this indeed solves the problem. Only concern here is minor; The override of traced_execution changes the signature from sync to async, effectively also changing the return type. Another option is to extend CursorTracer with a traced_execution_async method, keeping it all in one class, but supporting both.

Thanks for testing. Not sure I follow the return type concern, AFAIU the users of that _cursor_tracer are awaiting it so the fact it was not an async function was the issue?

When you subclass, you need to maintain the method's signature (Liskov Substitution Principle).

[Kludex]

Thanks for using Logfire! ❤️

[ibash]

@Kludex @xrmx updated this to have traced_execution_async instead