Add support for SQLAlchemy 1.4 by mnbbrown · Pull Request #568 · open-telemetry/opentelemetry-python-contrib

mnbbrown · 2021-07-06T16:31:13Z

Description

This adds support for SQLAlchemy 1.4. SQLAlchemy 1.4 (and ultimately SA 2.0) introduces a new async variant of the Engine and Session classes (called AsyncEngine and AsyncSession respectively). More details are available at https://docs.sqlalchemy.org/en/14/orm/extensions/asyncio.html

The current implementation uses thread locals to maintain a mapping between spans and threads. As thread locals are not available in an async/await environment this approach does not support the new AsyncEngine.

By attaching the span to the execution context state we avoid any interaction with concurrency primitives and instead rely on SQLAlchemy's underlying implementation.

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?

This uses the existing test suite for SQLAlchemy + an additional test to cover the async engine.

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 · 2021-07-06T16:31:18Z

The committers are authorized under a signed CLA.

✅ Matthew Brown (06710d0, 67b6a42, dbcb2ab, cb4a41f)

owais

The explanation and goal sounds very good. Took a quick look and left some initial comments.

owais · 2021-07-07T23:37:57Z

...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py

-        span = self.cursor_mapping.get(cursor, None)
-        if span is None:
-            return
+        context._span = span


use _otel_span instead?

owais · 2021-07-07T23:39:48Z

...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py

+        if span.is_recording():
+            span.set_status(
+                Status(StatusCode.ERROR, str(context.original_exception),)
+            )


Do we expect the str() call to fail in some situations? if not, why put this under a try block?

Went back through history and it seems to have always been that way - but I can't imagine why. Even if it's None it will just become the "None". I've moved it out of the try catch block but want to check what should be passed into Status if no exception is available.

original_exception is always present: https://docs.sqlalchemy.org/en/13/core/connections.html#sqlalchemy.engine.ExceptionContext.original_exception so should be safe 👍

owais · 2021-07-07T23:41:03Z

...telemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/package.py



-_instruments = ("sqlalchemy",)
+_instruments = ("sqlalchemy >= 1.3",)


Will the instrumentation not work with <1.3 after this change? That's a deal breaker IMO.

anything < 1.3 is marked EOL by SQLAlchemy (https://www.sqlalchemy.org/download.html). That said, I tested it with 1.2 and it worked so have removed the constraint.

We don't have a policy that drops support for unsupported software. Companies can easily get paid support for software that'd otherwise EOL'ed for example or continue to run old software without support. Personally I think we should not drop support until it gets painful to do it for very old versions. If it works for free, let's not drop support for it.

owais · 2021-07-07T23:42:46Z

tests/opentelemetry-docker-tests/tests/sqlalchemy_tests/mixins.py

        spans = self.memory_exporter.get_finished_spans()
-        self.assertEqual(len(spans), 5)
+        self.assertEqual(
+            len(spans), 5 if self.VENDOR not in ["postgresql"] else 3


why did this change in this PR? Can we document the difference between vendors as a comment?

Added a comment. It's an optimisation that's enabled by default in 1.4 See https://docs.sqlalchemy.org/en/14/changelog/migration_14.html#orm-batch-inserts-with-psycopg2-now-batch-statements-with-returning-in-most-cases

mnbbrown · 2021-07-11T11:27:18Z

@owais i think I've addressed all your comments now :)

owais

LGTM. Just one comment about testing the oldest and newest supported versions instead of the two latest ones.

owais · 2021-07-13T17:48:29Z

tox.ini

  elasticsearch6: elasticsearch>=6.0,<7.0
  elasticsearch7: elasticsearch-dsl>=7.0,<8.0
  elasticsearch7: elasticsearch>=7.0,<8.0
+  sqlalchemy13: sqlalchemy~=1.3,<1.4


can we change this to 1.2 or 1.1 so we test oldest reasonably supported version and the newest (1.4)?

Updated to use 1.1.

codeboten

Code looks great, just one question around how the version is checked.

codeboten · 2021-07-21T00:13:16Z

...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py

        """
        _w("sqlalchemy", "create_engine", _wrap_create_engine)
        _w("sqlalchemy.engine", "create_engine", _wrap_create_engine)
+        if sqlalchemy.__version__.startswith("1.4"):


Would this only work for version 1.4, what happens with version 1.5 is released? Would it make sense to use packaging to compare the version?

Good point - I've updated it to use packaging.

codeboten

👍

mnbbrown marked this pull request as ready for review July 6, 2021 16:48

mnbbrown requested review from a team, codeboten and srikanthccv and removed request for a team July 6, 2021 16:48

mnbbrown force-pushed the sqlalchemy-14 branch from e87920c to 8812117 Compare July 7, 2021 10:52

owais reviewed Jul 7, 2021

View reviewed changes

codeboten requested a review from owais July 12, 2021 17:14

owais reviewed Jul 13, 2021

View reviewed changes

owais approved these changes Jul 14, 2021

View reviewed changes

codeboten reviewed Jul 21, 2021

View reviewed changes

mnbbrown added 17 commits July 24, 2021 11:25

test removing thread specific logic. upgrade to sqlalchemy ~1.4

57e182a

wip

8d58c2b

add _span to sqlalchemy execution context instead of maintaining mapping

51c53ff

account for add_all optimisations in sqlalchemy 1.4

3d4087c

update docstring

4a723bb

add tests for async. fix black issues

5ade34e

lint fixes

ee58049

run tox -e generate

c034e9f

use pytest mark to skip tests. fix work directory for sqlalchemy tests

0eabf8e

fix lint issues

83937c0

address pylint

9164d9d

fix linter issues + generate

b51e4f1

remove IsolatedAsyncioTestCase as not supported in python < 3.8

07b2f2b

update CHANGELOG

f1985f6

address PR comments

ae8ce9b

add comment about execute_values for psycopg2 dialect

3a7c84d

use SQLAlchemy 1.1 for tests

5e38f5a

mnbbrown force-pushed the sqlalchemy-14 branch from bec94cf to 5e38f5a Compare July 24, 2021 10:27

mnbbrown added 2 commits July 24, 2021 11:29

remove gen file

83e0e4f

use packaging to test versions

125de32

mnbbrown requested review from codeboten and owais July 24, 2021 10:44

codeboten approved these changes Jul 26, 2021

View reviewed changes

codeboten closed this Jul 26, 2021

codeboten reopened this Jul 26, 2021

Merge branch 'main' into sqlalchemy-14

daadcec

srikanthccv approved these changes Aug 3, 2021

View reviewed changes

Merge branch 'main' into sqlalchemy-14

62cbe6d

lzchen approved these changes Aug 4, 2021

View reviewed changes

lzchen merged commit c8b6de6 into open-telemetry:main Aug 4, 2021



		_instruments = ("sqlalchemy",)
		_instruments = ("sqlalchemy >= 1.3",)

Conversation

mnbbrown commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

Uh oh!

linux-foundation-easycla bot commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnbbrown commented Jul 11, 2021

Uh oh!

owais left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mnbbrown commented Jul 6, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jul 6, 2021 •

edited

Loading