Fix sqlalchemy uninstrument #1581

shalevr · 2023-01-15T14:52:54Z

Bug fix for sqlalchemy uninstrument, inside the EngineTracer we call listen on events but no one removes the event listeners after the uninstrument

Fixes #1163

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

tox -e test-instrumentation-sqlalchemy14

Does This PR Require a Core Repo Change?

No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added

avzis · 2023-01-16T19:57:07Z

Will it work if you create the engine separately?
like this:

SQLAlchemyInstrumentor().instrument(enable_commenter=True)
from sqlalchemy import create_engine
engine = create_engine("sqlite:///:memory:")

When you instrument like this the engine will not be saved as part of the instrumentation, and the engine wont be un-wrapped as well (I am not sure if this is a problem, just raising a discussion)

There are tests that create the engine separately, maybe you can check the uninstrument() on them

liorzmetis · 2023-01-16T20:12:48Z

...elemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/__init__.py

@@ -158,17 +160,22 @@ def _instrument(self, **kwargs):
                "create_async_engine",
                _wrap_create_async_engine(tracer_provider, enable_commenter),
            )
+
+        self.engines = []


Is there any reason to keep list of engines, why is it necessary to keep all the engines in array?

You can create one engine or engines, so if I keep them in array it will be easier to clean up

No problem, We should consider to remove the engine from that list when python finalize the engine by the garbage collector or when the user deleting the engine, otherwise potentially a memory leak could happen

i wonder if we should implement call uninstrument logic when the garbage collector trying to collect the engine
https://docs.python.org/3.6/library/weakref.html#finalizer-objects

I thought about this but the problem is after the instrumentation the EngineTracer returns to the user and we can't be sure the user delete it and when the garbage collector works

Thank you for the comments

liorzmetis · 2023-01-16T20:16:14Z

...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py

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


its a private method should start with _ prefix

It's not a private method, I need to use this function from sqlalchemy

liorzmetis · 2023-01-16T20:29:05Z

instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py

@@ -248,6 +248,7 @@ def test_uninstrument(self):

        self.memory_exporter.clear()
        SQLAlchemyInstrumentor().uninstrument()


Please dont call uninstrument on a new SQLAlchemyInstrumentor instance,
suggested code:

def test_uninstrument(self): engine = create_engine("sqlite:///:memory:") instrumentor = SQLAlchemyInstrumentor() instrumentor.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) # first span - the connection to the db self.assertEqual(spans[0].name, "connect") self.assertEqual(spans[0].kind, trace.SpanKind.CLIENT) # second span - the query itself self.assertEqual(spans[1].name, "SELECT :memory:") self.assertEqual(spans[1].kind, trace.SpanKind.CLIENT) self.memory_exporter.clear() instrumentor.uninstrument() cnx.execute("SELECT 2 + 2;").fetchall() spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0)

I'm not sure, can you explain what's the difference?

We dont want to create additional instrumentor class with new connection and new engine,
we would like to see that we remove listener events for specific instrumentation with specific engine

It will not create another sqlalchemy instrumentor, if I understand this correctly it's like Singelton:

opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py

Line 50 in c92ba14

if cls._instance is None:

What do you think?

shalevr · 2023-01-16T20:46:53Z

Will it work if you create the engine separately? like this:
SQLAlchemyInstrumentor().instrument(enable_commenter=True)
from sqlalchemy import create_engine
engine = create_engine("sqlite:///:memory:")
When you instrument like this the engine will not be saved as part of the instrumentation, and the engine wont be un-wrapped as well (I am not sure if this is a problem, just raising a discussion)

There are tests that create the engine separately, maybe you can check the uninstrument() on them

You need to pass engine/engines to SQLAlchemyInstrumentor to create EngineTracer, if not there is no EngineTracer to create so my fix will not change anything

avzis · 2023-01-16T22:34:19Z

Will it work if you create the engine separately? like this:
SQLAlchemyInstrumentor().instrument(enable_commenter=True)
from sqlalchemy import create_engine
engine = create_engine("sqlite:///:memory:")
When you instrument like this the engine will not be saved as part of the instrumentation, and the engine wont be un-wrapped as well (I am not sure if this is a problem, just raising a discussion)
There are tests that create the engine separately, maybe you can check the uninstrument() on them
You need to pass engine/engines to SQLAlchemyInstrumentor to create EngineTracer, if not there is no EngineTracer to create so my fix will not change anything

Not necessarily,
when you call instrument() you create wrappers for all the functions, including the create_engine() function,
So if you run the code like in my example, it will still be wrapped and work as intended - that's the reason that the engine is optional in instrument()

shalevr · 2023-01-17T09:05:11Z

Will it work if you create the engine separately? like this:
SQLAlchemyInstrumentor().instrument(enable_commenter=True)
from sqlalchemy import create_engine
engine = create_engine("sqlite:///:memory:")
When you instrument like this the engine will not be saved as part of the instrumentation, and the engine wont be un-wrapped as well (I am not sure if this is a problem, just raising a discussion)
There are tests that create the engine separately, maybe you can check the uninstrument() on them
You need to pass engine/engines to SQLAlchemyInstrumentor to create EngineTracer, if not there is no EngineTracer to create so my fix will not change anything
Not necessarily, when you call instrument() you create wrappers for all the functions, including the create_engine() function, So if you run the code like in my example, it will still be wrapped and work as intended - that's the reason that the engine is optional in instrument()

You are right the create engine wrapper creates EngineTracer and no one manages this instance, I will fix this

shalevr · 2023-01-18T13:20:44Z

I refactor the code so when EngineTracer registers to the event listener those params will be saved in list of tuples,
so we will know how to remove them from event listeners when uninstument.
and another test function was added for the case that create_engine called after the instrumentation
If the code is not clear enough let me know and I will document this change more

…ve them when uninstument

avzis · 2023-01-18T16:31:34Z

...ntelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py

@@ -95,6 +98,8 @@ def _wrap_connect_internal(func, module, args, kwargs):


 class EngineTracer:
+    _remove_event_listener_params = []


suggestion: maybe drop the 'remove'
like, call it event_listener_params, or even event_listeners

but the params are for the remove function
I called the list like this so it will be clear that I keep the params only for remove

i agree with the event_listeners name convention because that what you store in this list

and it would be better name to use: unregister_event_listeners

Not exactly... I don't store the event listeners params, its without args and kwargs, for exmaple its without(retval=True)
I store the remove event listernes params

devopsmetis · 2023-01-21T15:41:48Z

I refactor the code so when EngineTracer registers to the event listener those params will be saved in list of tuples, so we will know how to remove them from event listeners when uninstument. and another test function was added for the case that create_engine called after the instrumentation If the code is not clear enough let me know and I will document this change more

@avzis @shalevr

As a user i didnt its not clear at all how SQLAlchemy Implementation actually works.
why i can pass engine to the instrumentation if in any case all my engines will be instrumented ?

Its not related directly to your PR but it should be clear if SQLAlchemy Instrumentation will be created then all the created engines will be instrumented even if i specified what Engine to instrument .

shalevr · 2023-01-21T16:23:10Z

I refactor the code so when EngineTracer registers to the event listener those params will be saved in list of tuples, so we will know how to remove them from event listeners when uninstument. and another test function was added for the case that create_engine called after the instrumentation If the code is not clear enough let me know and I will document this change more

@avzis @shalevr

As a user i didnt its not clear at all how SQLAlchemy Implementation actually works. why i can pass engine to the instrumentation if in any case all my engines will be instrumented ?

Its not related directly to your PR but it should be clear if SQLAlchemy Instrumentation will be created then all the created engines will be instrumented even if i specified what Engine to instrument .

I agree with you It wasn't clear to me too, but @avzis is right it's possible to create an engine after sqlalchemy instrumentation, we don't want to miss engines that were created after the instrumentation.

avzis · 2023-01-21T19:31:01Z

Its not related directly to your PR but it should be clear if SQLAlchemy Instrumentation will be created then all the created engines will be instrumented even if i specified what Engine to instrument .

@devopsmetis I agree that this is a weird behavior,
one thing I will mention, is that if you import the create_engine function before instrumenting, then it wont happen..

And I also agree that this discussion is not related to this PR, if you think that it is problematic I suggest you open another issue and discuss it

Remove all engine event listeners after sqlalchemy uninstrument

8cd9168

shalevr requested a review from a team as a code owner January 15, 2023 14:52

shalevr and others added 2 commits January 15, 2023 16:53

Merge branch 'main' into Fix-sqlalchemy-uninstrument

3536964

Add changelog entry

0975434

shalevr force-pushed the Fix-sqlalchemy-uninstrument branch 2 times, most recently from 073a31a to 34218a9 Compare January 15, 2023 15:57

Fix lint

0bac6a0

shalevr force-pushed the Fix-sqlalchemy-uninstrument branch from 34218a9 to 0bac6a0 Compare January 15, 2023 16:05

liorzmetis suggested changes Jan 16, 2023

View reviewed changes

shalevr marked this pull request as draft January 17, 2023 09:05

shalevr force-pushed the Fix-sqlalchemy-uninstrument branch from 0c17709 to 0a7cbdf Compare January 18, 2023 13:09

Change the code to save all listen params so we will know how to remo…

8edb72a

…ve them when uninstument

shalevr force-pushed the Fix-sqlalchemy-uninstrument branch from 0a7cbdf to 8edb72a Compare January 18, 2023 13:29

Verify that sqlalchemy instrumentation works after uninstument

df24e6b

shalevr marked this pull request as ready for review January 18, 2023 13:48

Merge branch 'main' into Fix-sqlalchemy-uninstrument

7fca774

avzis reviewed Jan 18, 2023

View reviewed changes

shalevr added 2 commits January 19, 2023 09:50

Merge branch 'main' into Fix-sqlalchemy-uninstrument

c85c256

Merge branch 'main' into Fix-sqlalchemy-uninstrument

c883c48

srikanthccv and others added 2 commits January 24, 2023 07:55

Merge branch 'main' into Fix-sqlalchemy-uninstrument

e12a2b2

Merge branch 'main' into Fix-sqlalchemy-uninstrument

2a0e744

srikanthccv approved these changes Jan 26, 2023

View reviewed changes

shalevr added 2 commits January 26, 2023 16:17

Merge branch 'main' into Fix-sqlalchemy-uninstrument

ec12619

Merge branch 'main' into Fix-sqlalchemy-uninstrument

1f8b2a2

devopsmetis approved these changes Jan 28, 2023

View reviewed changes

srikanthccv merged commit bbe7578 into open-telemetry:main Jan 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sqlalchemy uninstrument #1581

Fix sqlalchemy uninstrument #1581

shalevr commented Jan 15, 2023 •

edited

Loading

avzis commented Jan 16, 2023

liorzmetis Jan 16, 2023

shalevr Jan 16, 2023

liorzmetis Jan 16, 2023 •

edited

Loading

liorzmetis Jan 16, 2023

shalevr Jan 16, 2023 •

edited

Loading

shalevr Jan 16, 2023

liorzmetis Jan 16, 2023

shalevr Jan 16, 2023

liorzmetis Jan 16, 2023

shalevr Jan 16, 2023

liorzmetis Jan 16, 2023

shalevr Jan 16, 2023

shalevr commented Jan 16, 2023 •

edited

Loading

avzis commented Jan 16, 2023

shalevr commented Jan 17, 2023

shalevr commented Jan 18, 2023

avzis Jan 18, 2023

shalevr Jan 18, 2023

devopsmetis Jan 21, 2023 •

edited

Loading

shalevr Jan 21, 2023 •

edited

Loading

devopsmetis commented Jan 21, 2023

shalevr commented Jan 21, 2023

avzis commented Jan 21, 2023

		@@ -248,6 +248,7 @@ def test_uninstrument(self):

		self.memory_exporter.clear()
		SQLAlchemyInstrumentor().uninstrument()

		@@ -95,6 +98,8 @@ def _wrap_connect_internal(func, module, args, kwargs):


		class EngineTracer:
		_remove_event_listener_params = []

Fix sqlalchemy uninstrument #1581

Fix sqlalchemy uninstrument #1581

Conversation

shalevr commented Jan 15, 2023 • edited Loading

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

avzis commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorzmetis Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalevr Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalevr commented Jan 16, 2023 • edited Loading

avzis commented Jan 16, 2023

shalevr commented Jan 17, 2023

shalevr commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devopsmetis Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

shalevr Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

devopsmetis commented Jan 21, 2023

shalevr commented Jan 21, 2023

avzis commented Jan 21, 2023

shalevr commented Jan 15, 2023 •

edited

Loading

liorzmetis Jan 16, 2023 •

edited

Loading

shalevr Jan 16, 2023 •

edited

Loading

shalevr commented Jan 16, 2023 •

edited

Loading

devopsmetis Jan 21, 2023 •

edited

Loading

shalevr Jan 21, 2023 •

edited

Loading