Add support for SQLAlchemy 1.4 #568
Conversation
mnbbrown
requested review from
codeboten and
srikanthccv
and removed request for
a team
| span = self.cursor_mapping.get(cursor, None) | ||
| if span is None: | ||
| return | ||
| context._span = span |
| if span.is_recording(): | ||
| span.set_status( | ||
| Status(StatusCode.ERROR, str(context.original_exception),) | ||
| ) |
There was a problem hiding this comment.
Do we expect the str() call to fail in some situations? if not, why put this under a try block?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
original_exception is always present: https://docs.sqlalchemy.org/en/13/core/connections.html#sqlalchemy.engine.ExceptionContext.original_exception so should be safe 👍
| @@ -13,4 +13,4 @@ | |||
| # limitations under the License. | |||
|
|
|||
|
|
|||
| _instruments = ("sqlalchemy",) | |||
| _instruments = ("sqlalchemy >= 1.3",) | |||
There was a problem hiding this comment.
Will the instrumentation not work with <1.3 after this change? That's a deal breaker IMO.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -242,4 +243,6 @@ def insert_players(session): | |||
| close_all_sessions() | |||
|
|
|||
| 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 | |||
There was a problem hiding this comment.
why did this change in this PR? Can we document the difference between vendors as a comment?
There was a problem hiding this 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
|
@owais i think I've addressed all your comments now :) |
tox.ini
Outdated
| @@ -177,6 +177,9 @@ deps = | |||
| 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 | |||
There was a problem hiding this comment.
can we change this to 1.2 or 1.1 so we test oldest reasonably supported version and the newest (1.4)?
There was a problem hiding this comment.
Updated to use 1.1.
| @@ -76,6 +89,13 @@ def _instrument(self, **kwargs): | |||
| """ | |||
| _w("sqlalchemy", "create_engine", _wrap_create_engine) | |||
| _w("sqlalchemy.engine", "create_engine", _wrap_create_engine) | |||
| if sqlalchemy.__version__.startswith("1.4"): | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good point - I've updated it to use packaging.
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.
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?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.