opentelemetry-python-contrib icon indicating copy to clipboard operation
opentelemetry-python-contrib copied to clipboard

Support SQLAlchemy 2 (in tests and doc strings)

Open musicinmybrain opened this issue 1 year ago • 3 comments
trafficstars

Description

Fedora 40 will ship with SQLAlchemy 2. A python-sqlalchemy1.4 compat package will be available for now, but the idea is that this should be a temporary stopgap.

Currently, there are 17 test failures with SQLAlchemy 2.0.25, all similar to the one represented at the end of the PR text. This PR fixes them all by ensuring that plain-text query strings in the tests are always wrapped in sqlalchemy.text(). It also updates an example in a doc-string.

Note that no changes were required to the actual library implementation.

Fixes # (I did not file an issue.)

Type of change

It’s hard to say whether to characterize this as a bug fix, a new feature, or neither, since it primarily just expands compatibility in the tests. There is certainly no breaking change, and I don’t think it requires a documentation update.

How Has This Been Tested?

I applied this as a patch to the python-opentelemetry-contrib package and confirmed it fixed the test failures. You can test it by running the sqlalchemy instrumentation tests with SQLAlchemy 2.x in whatever way makes the most sense to you.

I could try adding testing with SQLAlchemy 2.x to tox.ini, but this is a little hard for me to test locally, especially the Docker part.

Does This PR Require a Core Repo Change?

  • [ ] Yes. - Link to PR:
  • [x] No.

Checklist:

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

  • [x] Followed the style guidelines of this project
  • [ ] Changelogs have been updated No, should I?
  • [ ] Unit tests have been added N/A, this fixes tests
  • [ ] Documentation has been updated N/A

Sample test failure (before this PR):

___________ TestSqlalchemyInstrumentation.test_create_engine_wrapper ___________

self = <sqlalchemy.engine.base.Connection object at 0x7f87397c7c80>
statement = 'SELECT\t1 + 1;', parameters = None

    def execute(
        self,
        statement: Executable,
        parameters: Optional[_CoreAnyExecuteParams] = None,
        *,
        execution_options: Optional[CoreExecuteOptionsParameter] = None,
    ) -> CursorResult[Any]:
        r"""Executes a SQL statement construct and returns a
        :class:`_engine.CursorResult`.
    
        :param statement: The statement to be executed.  This is always
         an object that is in both the :class:`_expression.ClauseElement` and
         :class:`_expression.Executable` hierarchies, including:
    
         * :class:`_expression.Select`
         * :class:`_expression.Insert`, :class:`_expression.Update`,
           :class:`_expression.Delete`
         * :class:`_expression.TextClause` and
           :class:`_expression.TextualSelect`
         * :class:`_schema.DDL` and objects which inherit from
           :class:`_schema.ExecutableDDLElement`
    
        :param parameters: parameters which will be bound into the statement.
         This may be either a dictionary of parameter names to values,
         or a mutable sequence (e.g. a list) of dictionaries.  When a
         list of dictionaries is passed, the underlying statement execution
         will make use of the DBAPI ``cursor.executemany()`` method.
         When a single dictionary is passed, the DBAPI ``cursor.execute()``
         method will be used.
    
        :param execution_options: optional dictionary of execution options,
         which will be associated with the statement execution.  This
         dictionary can provide a subset of the options that are accepted
         by :meth:`_engine.Connection.execution_options`.
    
        :return: a :class:`_engine.Result` object.
    
        """
        distilled_parameters = _distill_params_20(parameters)
        try:
>           meth = statement._execute_on_connection
E           AttributeError: 'str' object has no attribute '_execute_on_connection'

/usr/lib64/python3.12/site-packages/sqlalchemy/engine/base.py:1412: AttributeError

The above exception was the direct cause of the following exception:

self = <tests.test_sqlalchemy.TestSqlalchemyInstrumentation testMethod=test_create_engine_wrapper>

    def test_create_engine_wrapper(self):
        SQLAlchemyInstrumentor().instrument()
        from sqlalchemy import create_engine  # pylint: disable-all
    
        engine = create_engine("sqlite:///:memory:")
        cnx = engine.connect()
>       cnx.execute("SELECT	1 + 1;").fetchall()

instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py:159: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <sqlalchemy.engine.base.Connection object at 0x7f87397c7c80>
statement = 'SELECT\t1 + 1;', parameters = None

    def execute(
        self,
        statement: Executable,
        parameters: Optional[_CoreAnyExecuteParams] = None,
        *,
        execution_options: Optional[CoreExecuteOptionsParameter] = None,
    ) -> CursorResult[Any]:
        r"""Executes a SQL statement construct and returns a
        :class:`_engine.CursorResult`.
    
        :param statement: The statement to be executed.  This is always
         an object that is in both the :class:`_expression.ClauseElement` and
         :class:`_expression.Executable` hierarchies, including:
    
         * :class:`_expression.Select`
         * :class:`_expression.Insert`, :class:`_expression.Update`,
           :class:`_expression.Delete`
         * :class:`_expression.TextClause` and
           :class:`_expression.TextualSelect`
         * :class:`_schema.DDL` and objects which inherit from
           :class:`_schema.ExecutableDDLElement`
    
        :param parameters: parameters which will be bound into the statement.
         This may be either a dictionary of parameter names to values,
         or a mutable sequence (e.g. a list) of dictionaries.  When a
         list of dictionaries is passed, the underlying statement execution
         will make use of the DBAPI ``cursor.executemany()`` method.
         When a single dictionary is passed, the DBAPI ``cursor.execute()``
         method will be used.
    
        :param execution_options: optional dictionary of execution options,
         which will be associated with the statement execution.  This
         dictionary can provide a subset of the options that are accepted
         by :meth:`_engine.Connection.execution_options`.
    
        :return: a :class:`_engine.Result` object.
    
        """
        distilled_parameters = _distill_params_20(parameters)
        try:
            meth = statement._execute_on_connection
        except AttributeError as err:
>           raise exc.ObjectNotExecutableError(statement) from err
E           sqlalchemy.exc.ObjectNotExecutableError: Not an executable object: 'SELECT\t1 + 1;'

/usr/lib64/python3.12/site-packages/sqlalchemy/engine/base.py:1414: ObjectNotExecutableError

musicinmybrain avatar Feb 09 '24 13:02 musicinmybrain

async tests should probably also be changed from 1.4 to 1.4 and 2+. E.g: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py#L283

vanchaxy avatar Feb 09 '24 14:02 vanchaxy

async tests should probably also be changed from 1.4 to 1.4 and 2+. E.g: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py#L283

Thanks. That’s a great suggestion. I’m pushing an additional commit that enables these for “1.4 and later,” using packaging.version.parse() the same way it’s already used in

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/47caeab7aff47070c565cd90536a39ec5eb06f34/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/init.py#L101

and

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/47caeab7aff47070c565cd90536a39ec5eb06f34/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/init.py#L188

musicinmybrain avatar Feb 09 '24 15:02 musicinmybrain

Rebased on main without changes.

musicinmybrain avatar Apr 01 '24 12:04 musicinmybrain