Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix instrumentation of SQLAlchemy when using sqlalchemy.engine_from_config #2816

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#2792](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2792))
- `opentelemetry-instrumentation-tornado` Handle http client exception and record exception info into span
([#2563](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2563))
- `opentelemetry-instrumentation-sqlalchemy` Fixes engines from `sqlalchemy.engine_from_config` not being fully instrumented
([#2816](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2816))
- `opentelemetry-instrumentation` fix `http.host` new http semantic convention mapping to depend on `kind` of span
([#2814](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2814))
- `opentelemetry-instrumentation` Fix the description of `http.server.duration` and `http.server.request.duration`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,13 @@ def _instrument(self, **kwargs):
tracer, connections_usage, enable_commenter, commenter_options
),
)
_w(
"sqlalchemy.engine.create",
"create_engine",
_wrap_create_engine(
tracer, connections_usage, enable_commenter, commenter_options
),
)
Comment on lines +184 to +190
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Suggested change
_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
),
)

Copy link
Member

@emdneto emdneto Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add a comment to this

_w(
"sqlalchemy.engine.base",
"Engine.connect",
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this to the if statement parse_version(sqlalchemy.__version__).release >= (1, 4) as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

unwrap(Engine, "connect")
if parse_version(sqlalchemy.__version__).release >= (1, 4):
unwrap(sqlalchemy.ext.asyncio, "create_async_engine")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,17 @@ def test_create_engine_wrapper(self):
"opentelemetry.instrumentation.sqlalchemy",
)

def test_instrument_engine_from_config(self):
SQLAlchemyInstrumentor().instrument()
from sqlalchemy import engine_from_config # pylint: disable-all

engine = engine_from_config({"sqlalchemy.url": "sqlite:///:memory:"})
cnx = engine.connect()
cnx.execute(text("SELECT 1 + 1;")).fetchall()
spans = self.memory_exporter.get_finished_spans()

self.assertEqual(len(spans), 2)

def test_create_engine_wrapper_enable_commenter(self):
logging.getLogger("sqlalchemy.engine").setLevel(logging.INFO)
SQLAlchemyInstrumentor().instrument(
Expand Down
Loading