Fix instrumentation of SQLAlchemy when using sqlalchemy.engine_from_config #2816
Conversation
sqlalchemy.engine_from_config directly calls create_engine imported from that path; if we don't wrap that copy of the `create_engine` call then engines created via engine_from_config are not instrumented.
|
|
Co-authored-by: Emídio Neto <[email protected]>
|
Thanks for putting this in! Currently 5 of the unit tests are failing with |
Sorry for dropping the ball on this for a while, @tammy-baylis-swi. I've not contributed to otel before, and not used tox before either; would you be able to elaborate on what I need to do with the |
No worries @moredip ! Coincidentally, yesterday we merged 2 PRs to address some of what I mentioned:
I'm going to merge |
instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py
Outdated
Show resolved
Hide resolved
…/test_sqlalchemy.py Co-authored-by: Tammy Baylis <[email protected]>
| _w( | ||
| "sqlalchemy.engine.create", | ||
| "create_engine", | ||
| _wrap_create_engine( | ||
| tracer, connections_usage, enable_commenter, commenter_options | ||
| ), | ||
| ) |
There was a problem hiding this comment.
We're getting ModuleNotFoundError: No module named 'sqlalchemy.engine.create' from the unit tests and that comes from this instrumentor supporting SQLAlchemy 1.1, which does not have the sqlalchemy.engine.create module.
Please could you try something like what's below on L196 and see if that helps (not sure if it was introduced in 1.4 or 2.0):
| _w( | |
| "sqlalchemy.engine.create", | |
| "create_engine", | |
| _wrap_create_engine( | |
| tracer, connections_usage, enable_commenter, commenter_options | |
| ), | |
| ) | |
| if parse_version(sqlalchemy.__version__).release >= (1, 4): | |
| _w( | |
| "sqlalchemy.engine.create", | |
| "create_engine", | |
| _wrap_create_engine( | |
| tracer, connections_usage, enable_commenter, commenter_options | |
| ), | |
| ) |
There was a problem hiding this comment.
I would also add a comment to this
| @@ -224,6 +231,7 @@ def _instrument(self, **kwargs): | |||
| def _uninstrument(self, **kwargs): | |||
| unwrap(sqlalchemy, "create_engine") | |||
| unwrap(sqlalchemy.engine, "create_engine") | |||
| unwrap(sqlalchemy.engine.create, "create_engine") | |||
There was a problem hiding this comment.
Should we move this to the if statement parse_version(sqlalchemy.__version__).release >= (1, 4) as well?
Description
Fixes a bug where SQLAlchemy engines created via
sqlalchemy.engine_from_config(...)are not instrumented bySQLAlchemyInstrumentor.sqlalchemy.engine_from_config(...)directly callscreate_engine(...)imported fromsqlalchemy.engine.create. If we don't wrap that copy of thecreate_enginemethod then engines created viaengine_from_configare notinstrumented.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.