open-telemetry / opentelemetry-python-contrib

OpenTelemetry instrumentation for Python modules
https://opentelemetry.io
Apache License 2.0
699 stars 580 forks source link

`opentelemetry-instrumentation-psycopg` claims to work for async queries, but doesn't record time #2486

Open samuelcolvin opened 4 months ago

samuelcolvin commented 4 months ago

This was orignally reported as https://github.com/pydantic/logfire/issues/65 which has version details etc.

But in summary, using opentelemetry-instrumentation-psycopg="0.45b0" to instrument asyncio queries, doesn't work properly.

The cause is as follows:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/5116305f77bcd4c8ab18ef302a4351bb5b724c1e/instrumentation/opentelemetry-instrumentation-psycopg/src/opentelemetry/instrumentation/psycopg/__init__.py#L335-L338

calls

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/5116305f77bcd4c8ab18ef302a4351bb5b724c1e/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py#L472-L475

calls

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/5116305f77bcd4c8ab18ef302a4351bb5b724c1e/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py#L408-L414

which isn't designed to support an awaitable query_method, so it immediately returns a future, which gets awaited after the span has closed, hence zero time spans:

From Logfire, we see: image

when we'd expect something similar to what's observed with sync calls: image

I'm not clear why TracedCursorProxy is required rather than just using CursorTracer, but either way, I'd suggest implementing an async varient of traced_execution which tries to share as much logic as possible with the sync method.

If you agree, I'll try to create a PR for this over the next week or so.

xrmx commented 4 months ago

cc @federicobond

federicobond commented 4 months ago

Thanks for the ping @xrmx. Agree that this is a bug and your proposed solution sounds reasonable @samuelcolvin, happy to review your PR