Instrumentation breaks psycopg2 when registering types.

[owais]

Describe your environment
PostgreSQL supports JSON and JSONB types of DB columns. psycopg2 supports registering custom typecasters for these types. Most of the registration functions accept a connection or a cursor instance. The library crashes if anything else is passed in place of a connection or a cursor.

Most higher level frameworks and ORMs support these features out of the box meaning they crash as well. For example, django projects using psycopg2 driver will crash.

Steps to reproduce

Running the following program

import psycopg2.extras
from opentelemetry import trace
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.instrumentation.psycopg2 import Psycopg2Instrumentor

trace.set_tracer_provider(TracerProvider())
Psycopg2Instrumentor().instrument()

cnx = psycopg2.connect(host='localhost', port='5432', dbname='postgres', password='1234', user='postgres')
psycopg2.extras.register_default_jsonb(conn_or_curs=cnx, loads=lambda x: x)

results in the following exception:

❯ python main.py
Traceback (most recent call last):
  File "main.py", line 10, in <module>
    psycopg2.extras.register_default_jsonb(conn_or_curs=cnx, loads=lambda x: x)
  File "/Users/olone/playground/otel-psycopg2/venv/lib/python3.7/site-packages/psycopg2/_json.py", line 156, in register_default_jsonb
    loads=loads, oid=JSONB_OID, array_oid=JSONBARRAY_OID, name='jsonb')
  File "/Users/olone/playground/otel-psycopg2/venv/lib/python3.7/site-packages/psycopg2/_json.py", line 125, in register_json
    register_type(JSON, not globally and conn_or_curs or None)
TypeError: argument 2 must be a connection, cursor or None

What is the expected behavior?
For the instrumentation to not crash psycopg2 ever.

What is the actual behavior?
It crashes when psycopg2 enforces connection or cursor types.

Additional context
This is happening because instrumentation wraps connection factories and returns wrapt.ObjectProxy instances that wrap connection or cursor objects. These objects fail the type checks psycopg2 runs internally (probably in C code).

Here: https://github.com/open-telemetry/opentelemetry-python/blob/6019a91980ec84bbf969b0d82d44483c93f3ea4c/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py#L287-L304
And here: https://github.com/open-telemetry/opentelemetry-python/blob/6019a91980ec84bbf969b0d82d44483c93f3ea4c/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py#L361-L389

The solution might be to change how DBAPI instrumentation works and patch actual connection and cursor classes instead of wrap with object proxies.

[owais]

On a related note, we might want to reevaluate if we should be wrapping objects with different types or patching classes in place as a lot more libraries may rely on such runtime type checking. In general, instrumentation should not change the behavior of instrumented libraries in any way. I think swapping out types/implementations probably counts as changing behavior as isinstance/issubclass checks will fail if we do that.

[FinnStutzenstein]

Yes, using JSON/JSONB data types raises Invalid type Composed for attribute value. Expected one of ['bool', 'str', 'int', 'float'] or a sequence of those types, even with the build in extras.JSON in psycopg2.

[LouisStAmour]

Unless I've misunderstood, is it possible we did not fully port the ddtrace reference?

For example:

opentelemetry-python-contrib/reference/ddtrace/contrib/psycopg/patch.py

Lines 127 to 130 in 5aee3ce

	# register_type performs a c-level check of the object
	# type so we must be sure to pass in the actual db connection
	if scope and isinstance(scope, wrapt.ObjectProxy):
	scope = scope.__wrapped__

opentelemetry-python-contrib/reference/ddtrace/contrib/psycopg/patch.py

Lines 140 to 143 in 5aee3ce

	# register_type performs a c-level check of the object
	# type so we must be sure to pass in the actual db connection
	if scope and isinstance(scope, wrapt.ObjectProxy):
	scope = scope.__wrapped__

opentelemetry-python-contrib/reference/ddtrace/contrib/psycopg/patch.py

Lines 162 to 165 in 5aee3ce

	# prepare performs a c-level check of the object type so
	# we must be sure to pass in the actual db connection
	if isinstance(conn, wrapt.ObjectProxy):
	conn = conn.__wrapped__