conn.commit() raise InterfaceError because connection instrument wrapper has no _sock member

[kchow1985]

Describe your environment Describe any aspect of your environment relevant to the problem, including your Python version, platform, version numbers of installed dependencies, information about your cloud hosting provider, etc. If you're reporting a problem with a specific version of a library in this repo, please check whether the problem has been fixed on main.

instrumentation version: opentelemetry-instrumentation-pymysql 0.33b0

Steps to reproduce
When using opentelemetry-instrument command to automatically instrument pymysql

conn = pymysql.connect(...)

with conn.cursor() as cursor:
  cursor.execute(...) # run any insert statement
  conn.commit() # this line will trigger InterfaceError

Current bandaid solution:

conn = pymysql.connect(...)
conn._sock = conn._connection._sock # add this line to work around

with conn.cursor() as cursor:
  cursor.execute(...)
  conn.commit() # this line will no longer trigger InterfaceError

Tracing down the stack will show that on line 792 of the pymysql/connections/.py, the Exception will be thrown because self now refers to the wrapper instead of the internal connection object:

    def _execute_command(self, command, sql):
        """
        :raise InterfaceError: If the connection is closed.
        :raise ValueError: If no username was specified.
        """
        if not self._sock:
            raise err.InterfaceError(0, "")
        ...

If I do not instrument pymysql then this will not happen.

[rogersd]

@ocelotl I'm having this same issue and it appears to be caused by #1097

This may also be the cause of #1213

Also, should this be changed?

diff --git a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py
index 0645d4c5..8bca74dd 100644
--- a/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py
+++ b/instrumentation/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py
@@ -197,7 +197,7 @@ def instrument_connection(
     Returns:
         An instrumented connection.
     """
-    if isinstance(connection, wrapt.ObjectProxy):
+    if isinstance(connection, _TracedConnectionProxy):
         _logger.warning("Connection already instrumented")
         return connection

[yabigfish]

PyMySQL specific

# file: PyMySQL/pymysql/connections.py

class Connection:
    # ...
    # class attribute
    _sock = None
    # ...

    def __init__(self):
        # ...
        # instance attribute
        # self._sock assigned to new socket by self.connect()
        # ...

_sock is a class attribute, also an instance attribute, which is used by the method connection.commit and connection.close and others

Before #1097

# file: opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py

class TracedConnectionProxy(wrapt.ObjectProxy):
    def __init__(self, connection, *args, **kwargs):
        wrapt.ObjectProxy.__init__(self, connection)

accessing self._sock via wrapt.ObjectProxy to the underlying instance attribute connection._sock, which is good.

After #1097

class TracedConnectionProxy(type(connection), _TracedConnectionProxy):
    def __init__(self, connection):
        self._connection = connection

self._sock is the class attribute Connection._sock (by inheriting type(connection)), which is bad, because Connection._sock is None

[rogersd]

@srikanthccv Any thoughts here?

[ocelotl]

Will look into this, thanks for reporting ✌️

[rogersd]

@ocelotl Would be great if the fix could make it in to the next release. I (and I'm sure many others) am blocked from upgrading until this is fixed.