Skip to content

Fix sqlalchemy uninstrument #1581

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

Merged
merged 13 commits into from
Jan 29, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ class SQLAlchemyInstrumentor(BaseInstrumentor):
See `BaseInstrumentor`
"""

engines = []

def instrumentation_dependencies(self) -> Collection[str]:
return _instruments

Expand Down Expand Up @@ -158,17 +160,22 @@ def _instrument(self, **kwargs):
"create_async_engine",
_wrap_create_async_engine(tracer_provider, enable_commenter),
)

self.engines = []

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

@liorzmetis liorzmetis Jan 16, 2023

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@shalevr shalevr Jan 16, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the comments

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", {}),
)
)
return self.engines[0]
if kwargs.get("engines") is not None and isinstance(
kwargs.get("engines"), Sequence
):
return [
self.engines = [
EngineTracer(
_get_tracer(tracer_provider),
engine,
Expand All @@ -177,6 +184,7 @@ def _instrument(self, **kwargs):
)
for engine in kwargs.get("engines")
]
return self.engines

return None

Expand All @@ -186,3 +194,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()
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
import os
import re

from sqlalchemy.event import listen # pylint: disable=no-name-in-module
from sqlalchemy.event import ( # pylint: disable=no-name-in-module
listen,
remove,
)

from opentelemetry import trace
from opentelemetry.instrumentation.sqlalchemy.package import (
Expand Down Expand Up @@ -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):

Choose a reason for hiding this comment

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

its a private method should start with _ prefix

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ def test_uninstrument(self):

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

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


What do you think?

cnx.execute("SELECT 1 + 1;").fetchall()
engine2 = create_engine("sqlite:///:memory:")
cnx2 = engine2.connect()
cnx2.execute("SELECT 2 + 2;").fetchall()
Expand Down