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 sqlalchemy uninstrument #1581

Merged
merged 13 commits into from
Jan 29, 2023
Merged
Show file tree
Hide file tree
Changes from all 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:/open-telemetry/opentelemetry-python-contrib/pull/1581))
- `opentelemetry-instrumentation-grpc` Fix code()/details() of _OpentelemetryServicerContext.
([#1578](https:/open-telemetry/opentelemetry-python-contrib/pull/1578))
- Fix aiopg instrumentation to work with aiopg < 2.0.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,4 @@ def _uninstrument(self, **kwargs):
unwrap(Engine, "connect")
if parse_version(sqlalchemy.__version__).release >= (1, 4):
unwrap(sqlalchemy.ext.asyncio, "create_async_engine")
EngineTracer.remove_all_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 @@ -95,6 +98,8 @@ def _wrap_connect_internal(func, module, args, kwargs):


class EngineTracer:
_remove_event_listener_params = []
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

@devopsmetis devopsmetis Jan 21, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@shalevr shalevr Jan 21, 2023

Choose a reason for hiding this comment

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

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


def __init__(
self, tracer, engine, enable_commenter=False, commenter_options=None
):
Expand All @@ -105,11 +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)
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 = []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,12 +248,41 @@ 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()
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
)
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(
Expand Down