From ff1ac98a0c1cabb04f0165f51d9a91143db7a38a Mon Sep 17 00:00:00 2001 From: Rytis Bagdziunas Date: Thu, 28 Nov 2024 00:05:08 +0100 Subject: [PATCH 1/2] Remove references to SQLAlchemy engines which are disposed of EngineTracer in SQLAlchemy keeps weak references to a traced engine forever which can cause a noticeable memory leak if engines are constantly getting creating. --- .../instrumentation/sqlalchemy/engine.py | 13 +++++++++++++ .../tests/test_sqlalchemy.py | 5 +++++ 2 files changed, 18 insertions(+) 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 b64af796d1..a20e481819 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -167,6 +167,13 @@ def _pool_checkout( self._add_idle_to_connection_usage(-1) self._add_used_to_connection_usage(1) + @classmethod + def _dispose_of_event_listener(cls, obj): + try: + cls._remove_event_listener_params.remove(obj) + except ValueError: + pass + @classmethod def _register_event_listener(cls, target, identifier, func, *args, **kw): listen(target, identifier, func, *args, **kw) @@ -174,6 +181,12 @@ def _register_event_listener(cls, target, identifier, func, *args, **kw): (weakref.ref(target), identifier, func) ) + weakref.finalize( + target, + cls._dispose_of_event_listener, + (weakref.ref(target), identifier, func), + ) + @classmethod def remove_all_event_listeners(cls): for ( diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py index 957ae16237..18b9fa65f7 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py @@ -417,6 +417,10 @@ def test_no_memory_leakage_if_engine_diposed(self): from sqlalchemy import create_engine + from opentelemetry.instrumentation.sqlalchemy.engine import ( + EngineTracer, + ) + callback = mock.Mock() def make_shortlived_engine(): @@ -432,3 +436,4 @@ def make_shortlived_engine(): gc.collect() assert callback.call_count == 5 + assert len(EngineTracer._remove_event_listener_params) == 0 From b8b1972800f9e3dfc170c9ba1ee7e5097f015e30 Mon Sep 17 00:00:00 2001 From: Rytis Bagdziunas Date: Thu, 28 Nov 2024 00:15:39 +0100 Subject: [PATCH 2/2] Updated changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a28c5039c9..b43e09fa34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#3022](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3022)) - Replace all instrumentor unit test `assertEqualSpanInstrumentationInfo` calls with `assertEqualSpanInstrumentationScope` calls ([#3037](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3037)) +- `opentelemetry-instrumentation-sqlalchemy`: Fix a remaining memory leak in EngineTracer + ([#3053](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3053)) ### Breaking changes