From 8cd9168663191cbdadb1829590bba37f1a57bec4 Mon Sep 17 00:00:00 2001 From: sroda Date: Sun, 15 Jan 2023 16:47:03 +0200 Subject: [PATCH 1/5] Remove all engine event listeners after sqlalchemy uninstrument --- .../instrumentation/sqlalchemy/__init__.py | 22 +++++++++++++------ .../instrumentation/sqlalchemy/engine.py | 10 ++++++++- .../tests/test_sqlalchemy.py | 1 + 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py index 6c91ae16e0..f86ef124d4 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py @@ -158,17 +158,22 @@ def _instrument(self, **kwargs): "create_async_engine", _wrap_create_async_engine(tracer_provider, enable_commenter), ) + + self.engines = [] if kwargs.get("engine") is not None: - return EngineTracer( - _get_tracer(tracer_provider), - kwargs.get("engine"), - kwargs.get("enable_commenter", False), - kwargs.get("commenter_options", {}), + self.engines.append( + EngineTracer( + _get_tracer(tracer_provider), + kwargs.get("engine"), + kwargs.get("enable_commenter", False), + kwargs.get("commenter_options", {}), + ) ) - if kwargs.get("engines") is not None and isinstance( + return self.engines[0] + elif kwargs.get("engines") is not None and isinstance( kwargs.get("engines"), Sequence ): - return [ + self.engines = [ EngineTracer( _get_tracer(tracer_provider), engine, @@ -177,6 +182,7 @@ def _instrument(self, **kwargs): ) for engine in kwargs.get("engines") ] + return self.engines return None @@ -186,3 +192,5 @@ def _uninstrument(self, **kwargs): unwrap(Engine, "connect") if parse_version(sqlalchemy.__version__).release >= (1, 4): unwrap(sqlalchemy.ext.asyncio, "create_async_engine") + for engine in self.engines: + engine.remove_event_listeners() diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index 33d0183ef0..e14293b831 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -14,7 +14,10 @@ import os import re -from sqlalchemy.event import listen # pylint: disable=no-name-in-module +from sqlalchemy.event import ( + listen, + remove, +) # pylint: disable=no-name-in-module from opentelemetry import trace from opentelemetry.instrumentation.sqlalchemy.package import ( @@ -111,6 +114,11 @@ def __init__( listen(engine, "after_cursor_execute", _after_cur_exec) listen(engine, "handle_error", _handle_error) + def remove_event_listeners(self): + remove(self.engine, "before_cursor_execute", self._before_cur_exec) + remove(self.engine, "after_cursor_execute", _after_cur_exec) + remove(self.engine, "handle_error", _handle_error) + def _operation_name(self, db_name, statement): parts = [] if isinstance(statement, str): diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py index 7f0e655a9e..16977a5ce2 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py @@ -248,6 +248,7 @@ def test_uninstrument(self): self.memory_exporter.clear() SQLAlchemyInstrumentor().uninstrument() + cnx.execute("SELECT 1 + 1;").fetchall() engine2 = create_engine("sqlite:///:memory:") cnx2 = engine2.connect() cnx2.execute("SELECT 2 + 2;").fetchall() From 09754342bb5b0af93c031dbfb8509f4b082401aa Mon Sep 17 00:00:00 2001 From: sroda Date: Sun, 15 Jan 2023 17:01:03 +0200 Subject: [PATCH 2/5] Add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 068a067dd1..f5ee257f96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fix SQLAlchemy uninstrumentation + ([#1581](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1581)) - Fix aiopg instrumentation to work with aiopg < 2.0.0 ([#1473](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1473)) - `opentelemetry-instrumentation-aws-lambda` Adds an option to configure `disable_aws_context_propagation` by From 0bac6a0b68f338c14c341e5e5420ccbca87b4895 Mon Sep 17 00:00:00 2001 From: sroda Date: Sun, 15 Jan 2023 17:11:35 +0200 Subject: [PATCH 3/5] Fix lint --- .../src/opentelemetry/instrumentation/sqlalchemy/__init__.py | 4 +++- .../src/opentelemetry/instrumentation/sqlalchemy/engine.py | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py index f86ef124d4..975c9ca385 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py @@ -119,6 +119,8 @@ class SQLAlchemyInstrumentor(BaseInstrumentor): See `BaseInstrumentor` """ + engines = [] + def instrumentation_dependencies(self) -> Collection[str]: return _instruments @@ -170,7 +172,7 @@ def _instrument(self, **kwargs): ) ) return self.engines[0] - elif kwargs.get("engines") is not None and isinstance( + if kwargs.get("engines") is not None and isinstance( kwargs.get("engines"), Sequence ): self.engines = [ diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index e14293b831..62eba4b08d 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -14,10 +14,10 @@ import os import re -from sqlalchemy.event import ( +from sqlalchemy.event import ( # pylint: disable=no-name-in-module listen, remove, -) # pylint: disable=no-name-in-module +) from opentelemetry import trace from opentelemetry.instrumentation.sqlalchemy.package import ( From 8edb72aa855b2451e7e7b65d70c88b87f280a529 Mon Sep 17 00:00:00 2001 From: sroda Date: Wed, 18 Jan 2023 15:06:17 +0200 Subject: [PATCH 4/5] Change the code to save all listen params so we will know how to remove them when uninstument --- .../instrumentation/sqlalchemy/__init__.py | 23 +++++----------- .../instrumentation/sqlalchemy/engine.py | 26 +++++++++++++------ .../tests/test_sqlalchemy.py | 19 ++++++++++++++ 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py index 975c9ca385..b19de5ec96 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py @@ -119,8 +119,6 @@ class SQLAlchemyInstrumentor(BaseInstrumentor): See `BaseInstrumentor` """ - engines = [] - def instrumentation_dependencies(self) -> Collection[str]: return _instruments @@ -160,22 +158,17 @@ def _instrument(self, **kwargs): "create_async_engine", _wrap_create_async_engine(tracer_provider, enable_commenter), ) - - self.engines = [] if kwargs.get("engine") is not None: - self.engines.append( - EngineTracer( - _get_tracer(tracer_provider), - kwargs.get("engine"), - kwargs.get("enable_commenter", False), - kwargs.get("commenter_options", {}), - ) + return EngineTracer( + _get_tracer(tracer_provider), + kwargs.get("engine"), + kwargs.get("enable_commenter", False), + kwargs.get("commenter_options", {}), ) - return self.engines[0] if kwargs.get("engines") is not None and isinstance( kwargs.get("engines"), Sequence ): - self.engines = [ + return [ EngineTracer( _get_tracer(tracer_provider), engine, @@ -184,7 +177,6 @@ def _instrument(self, **kwargs): ) for engine in kwargs.get("engines") ] - return self.engines return None @@ -194,5 +186,4 @@ def _uninstrument(self, **kwargs): unwrap(Engine, "connect") if parse_version(sqlalchemy.__version__).release >= (1, 4): unwrap(sqlalchemy.ext.asyncio, "create_async_engine") - for engine in self.engines: - engine.remove_event_listeners() + EngineTracer.remove_all_event_listeners() diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index 62eba4b08d..f95751d9c6 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -98,6 +98,8 @@ def _wrap_connect_internal(func, module, args, kwargs): class EngineTracer: + _remove_event_listener_params = [] + def __init__( self, tracer, engine, enable_commenter=False, commenter_options=None ): @@ -108,16 +110,24 @@ def __init__( self.commenter_options = commenter_options if commenter_options else {} self._leading_comment_remover = re.compile(r"^/\*.*?\*/") - listen( + self._register_event_listener( engine, "before_cursor_execute", self._before_cur_exec, retval=True ) - listen(engine, "after_cursor_execute", _after_cur_exec) - listen(engine, "handle_error", _handle_error) - - def remove_event_listeners(self): - remove(self.engine, "before_cursor_execute", self._before_cur_exec) - remove(self.engine, "after_cursor_execute", _after_cur_exec) - remove(self.engine, "handle_error", _handle_error) + self._register_event_listener( + engine, "after_cursor_execute", _after_cur_exec + ) + self._register_event_listener(engine, "handle_error", _handle_error) + + @classmethod + def _register_event_listener(cls, target, identifier, func, *args, **kw): + listen(target, identifier, func, *args, **kw) + cls._remove_event_listener_params.append((target, identifier, func)) + + @classmethod + def remove_all_event_listeners(cls): + for remove_params in cls._remove_event_listener_params: + remove(*remove_params) + cls._remove_event_listener_params.clear() def _operation_name(self, db_name, statement): parts = [] diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py index 9788fe1cc1..68061ccf50 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py @@ -255,6 +255,25 @@ def test_uninstrument(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) + def test_uninstrument_without_engine(self): + SQLAlchemyInstrumentor().instrument( + tracer_provider=self.tracer_provider + ) + from sqlalchemy import create_engine + + engine = create_engine("sqlite:///:memory:") + + cnx = engine.connect() + cnx.execute("SELECT 1 + 1;").fetchall() + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 2) + + self.memory_exporter.clear() + SQLAlchemyInstrumentor().uninstrument() + cnx.execute("SELECT 1 + 1;").fetchall() + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 0) + def test_no_op_tracer_provider(self): engine = create_engine("sqlite:///:memory:") SQLAlchemyInstrumentor().instrument( From df24e6b9e12a07966c0733c51bc7cac63a8081ab Mon Sep 17 00:00:00 2001 From: sroda Date: Wed, 18 Jan 2023 15:48:23 +0200 Subject: [PATCH 5/5] Verify that sqlalchemy instrumentation works after uninstument --- .../tests/test_sqlalchemy.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py index 68061ccf50..e00148320c 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py @@ -255,6 +255,15 @@ def test_uninstrument(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) + SQLAlchemyInstrumentor().instrument( + engine=engine, + tracer_provider=self.tracer_provider, + ) + cnx = engine.connect() + cnx.execute("SELECT 1 + 1;").fetchall() + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 2) + def test_uninstrument_without_engine(self): SQLAlchemyInstrumentor().instrument( tracer_provider=self.tracer_provider