From bc77e8f54613d4a869bf6c87966b6da25149a73b Mon Sep 17 00:00:00 2001 From: "Qiu, David" Date: Wed, 31 Jul 2024 20:29:56 +0800 Subject: [PATCH 1/6] Fix psycopg2 intrusment issue Signed-off-by: Qiu, David --- .../instrumentation/psycopg2/__init__.py | 49 +++++++++++-------- .../tests/test_psycopg2_integration.py | 2 +- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py index de2e49f4c3..7fa549f22c 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -158,33 +158,40 @@ def _uninstrument(self, **kwargs): dbapi.unwrap_connect(psycopg2, "connect") # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql - @staticmethod - def instrument_connection(connection, tracer_provider=None): - if not hasattr(connection, "_is_instrumented_by_opentelemetry"): - connection._is_instrumented_by_opentelemetry = False - - if not connection._is_instrumented_by_opentelemetry: - setattr( - connection, _OTEL_CURSOR_FACTORY_KEY, connection.cursor_factory - ) - connection.cursor_factory = _new_cursor_factory( - tracer_provider=tracer_provider - ) - connection._is_instrumented_by_opentelemetry = True - else: - _logger.warning( - "Attempting to instrument Psycopg connection while already instrumented" - ) - return connection + # TODO: comment out below codes to fix issue #2522, will try to set the properties later. + def instrument_connection(self, connection, tracer_provider=None): + # if not hasattr(connection, "_is_instrumented_by_opentelemetry"): + # connection._is_instrumented_by_opentelemetry = False + + # if not connection._is_instrumented_by_opentelemetry: + # setattr( + # connection, _OTEL_CURSOR_FACTORY_KEY, connection.cursor_factory + # ) + # connection.cursor_factory = _new_cursor_factory( + # tracer_provider=tracer_provider + # ) + # connection._is_instrumented_by_opentelemetry = True + # else: + # _logger.warning( + # "Attempting to instrument Psycopg connection while already instrumented" + # ) + # return connection + return dbapi.instrument_connection( + __name__, + connection, + self._DATABASE_SYSTEM, + self._CONNECTION_ATTRIBUTES, + version=__version__, + tracer_provider=tracer_provider, + ) # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql - @staticmethod - def uninstrument_connection(connection): + def uninstrument_connection(self, connection): connection.cursor_factory = getattr( connection, _OTEL_CURSOR_FACTORY_KEY, None ) - return connection + return dbapi.uninstrument_connection(connection) # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index 369d63d5cf..ee685a3b1a 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -235,7 +235,7 @@ def test_uninstrument_connection_with_instrument(self): # pylint: disable=unused-argument def test_uninstrument_connection_with_instrument_connection(self): cnx = psycopg2.connect(database="test") - Psycopg2Instrumentor().instrument_connection(cnx) + cnx = Psycopg2Instrumentor().instrument_connection(cnx) query = "SELECT * FROM test" cursor = cnx.cursor() cursor.execute(query) From 224e03a05c5a6268efc5cecf0e3963b19b7f968c Mon Sep 17 00:00:00 2001 From: "Qiu, David" Date: Fri, 16 Aug 2024 17:19:06 +0800 Subject: [PATCH 2/6] Update psycopg2 connection instrument logic Signed-off-by: Qiu, David --- .../instrumentation/psycopg2/__init__.py | 35 ++++++++----------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py index 7fa549f22c..383ee1160e 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -105,6 +105,8 @@ import typing from typing import Collection +from wrapt import ObjectProxy + import psycopg2 from psycopg2.extensions import ( cursor as pg_cursor, # pylint: disable=no-name-in-module @@ -158,32 +160,25 @@ def _uninstrument(self, **kwargs): dbapi.unwrap_connect(psycopg2, "connect") # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql - # TODO: comment out below codes to fix issue #2522, will try to set the properties later. def instrument_connection(self, connection, tracer_provider=None): - # if not hasattr(connection, "_is_instrumented_by_opentelemetry"): - # connection._is_instrumented_by_opentelemetry = False - - # if not connection._is_instrumented_by_opentelemetry: - # setattr( - # connection, _OTEL_CURSOR_FACTORY_KEY, connection.cursor_factory - # ) - # connection.cursor_factory = _new_cursor_factory( - # tracer_provider=tracer_provider - # ) - # connection._is_instrumented_by_opentelemetry = True - # else: - # _logger.warning( - # "Attempting to instrument Psycopg connection while already instrumented" - # ) - # return connection - return dbapi.instrument_connection( + if isinstance(connection, ObjectProxy): + _logger.warning( + "Attempting to instrument Psycopg connection while already instrumented" + ) + return connection + + db_integration = DatabaseApiIntegration( __name__, - connection, self._DATABASE_SYSTEM, - self._CONNECTION_ATTRIBUTES, + connection_attributes=self._CONNECTION_ATTRIBUTES, version=__version__, tracer_provider=tracer_provider, ) + db_integration.get_connection_attributes(connection) + db_integration.cursor_factory = _new_cursor_factory( + tracer_provider=tracer_provider + ) + return dbapi.get_traced_connection_proxy(connection, db_integration) # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql def uninstrument_connection(self, connection): From 092b8ba8b27a381ac677a9382437b9f65601dbc1 Mon Sep 17 00:00:00 2001 From: "Qiu, David" Date: Wed, 21 Aug 2024 15:46:43 +0800 Subject: [PATCH 3/6] Addd changelog for psycopg2 instrument issue fix Signed-off-by: Qiu, David --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c31e01235..7660a1a2f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Fixed +- `opentelemetry-instrumentation-psycopg2` Fix psycopg2 instrument issue + ([#2795](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2795)) - `opentelemetry-instrumentation-fastapi` fix `fastapi` auto-instrumentation by removing `fastapi-slim` support, `fastapi-slim` itself is discontinued from maintainers ([2783](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2783)) - `opentelemetry-instrumentation-aws-lambda` Avoid exception when a handler is not present. From 4614565aa98b658cdd11006b1c1959767c8f3868 Mon Sep 17 00:00:00 2001 From: "Qiu, David" Date: Tue, 3 Sep 2024 17:09:08 +0800 Subject: [PATCH 4/6] Update test case to reproduce psycopg2 connection instrument issue Signed-off-by: Qiu, David --- .../tests/test_psycopg2_integration.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index ee685a3b1a..ab6a58c903 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -54,6 +54,7 @@ class MockConnection: def __init__(self, *args, **kwargs): self.cursor_factory = kwargs.pop("cursor_factory", None) + self._initialized = True def cursor(self): if self.cursor_factory: @@ -63,6 +64,17 @@ def cursor(self): def get_dsn_parameters(self): # pylint: disable=no-self-use return {"dbname": "test"} + def __setattr__(self, name, value): + """ + This method is overridden to prevent adding new attributes to the MockConnection object. + At runtime, the psycopg2 connection object does not have attribute + _is_instrumented_by_opentelemetry, it will throw AttributeError when setting this + attribute. + """ + if hasattr(self, '_initialized') and not hasattr(self, name): + raise AttributeError(f"Cannot add new attribute '{name}' to MockConnection") + super().__setattr__(name, value) + class TestPostgresqlIntegration(TestBase): def setUp(self): From 87d74c55318e5bdceceb4f712df977b706b40ec8 Mon Sep 17 00:00:00 2001 From: "Qiu, David" Date: Thu, 12 Sep 2024 10:39:54 +0800 Subject: [PATCH 5/6] Add assert for _is_instrumented_by_opentelemetry in connection object Signed-off-by: Qiu, David --- .../tests/test_psycopg2_integration.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index 08dc34e786..676aa07138 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -55,7 +55,6 @@ class MockConnection: def __init__(self, *args, **kwargs): self.cursor_factory = kwargs.pop("cursor_factory", None) - self._initialized = True def cursor(self): if self.cursor_factory: @@ -65,17 +64,6 @@ def cursor(self): def get_dsn_parameters(self): # pylint: disable=no-self-use return {"dbname": "test"} - def __setattr__(self, name, value): - """ - This method is overridden to prevent adding new attributes to the MockConnection object. - At runtime, the psycopg2 connection object does not have attribute - _is_instrumented_by_opentelemetry, it will throw AttributeError when setting this - attribute. - """ - if hasattr(self, '_initialized') and not hasattr(self, name): - raise AttributeError(f"Cannot add new attribute '{name}' to MockConnection") - super().__setattr__(name, value) - class TestPostgresqlIntegration(TestBase): def setUp(self): @@ -203,6 +191,7 @@ def test_instrument_connection(self): self.assertEqual(len(spans_list), 0) cnx = Psycopg2Instrumentor().instrument_connection(cnx) + self.assertFalse(hasattr(cnx, "_is_instrumented_by_opentelemetry")) cursor = cnx.cursor() cursor.execute(query) @@ -221,6 +210,7 @@ def test_instrument_connection_with_instrument(self): Psycopg2Instrumentor().instrument() cnx = Psycopg2Instrumentor().instrument_connection(cnx) + self.assertFalse(hasattr(cnx, "_is_instrumented_by_opentelemetry")) cursor = cnx.cursor() cursor.execute(query) From 30cff63ce89bd5c85c6c2d59568b3764399b0e15 Mon Sep 17 00:00:00 2001 From: "Qiu, David" Date: Thu, 26 Sep 2024 13:48:59 +0800 Subject: [PATCH 6/6] Add test case for psycopg2 duplicated instrumentation. Signed-off-by: Qiu, David --- .../instrumentation/psycopg2/__init__.py | 6 ++--- .../tests/test_psycopg2_integration.py | 27 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py index 383ee1160e..f2c20a43d7 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -105,13 +105,12 @@ import typing from typing import Collection -from wrapt import ObjectProxy - import psycopg2 from psycopg2.extensions import ( cursor as pg_cursor, # pylint: disable=no-name-in-module ) from psycopg2.sql import Composed # pylint: disable=no-name-in-module +from wrapt import ObjectProxy from opentelemetry.instrumentation import dbapi from opentelemetry.instrumentation.instrumentor import BaseInstrumentor @@ -181,7 +180,8 @@ def instrument_connection(self, connection, tracer_provider=None): return dbapi.get_traced_connection_proxy(connection, db_integration) # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql - def uninstrument_connection(self, connection): + @staticmethod + def uninstrument_connection(connection): connection.cursor_factory = getattr( connection, _OTEL_CURSOR_FACTORY_KEY, None ) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index 676aa07138..939075d5cb 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -283,3 +283,30 @@ def test_no_op_tracer_provider(self): cursor.execute(query) spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 0) + + # pylint: disable=unused-argument + def test_duplicated_instrumentation_works(self): + cnx = psycopg2.connect(database="test") + query = "SELECT * FROM test" + cursor = cnx.cursor() + cursor.execute(query) + + spans_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans_list), 0) + + Psycopg2Instrumentor().instrument() + Psycopg2Instrumentor().instrument() + cnx = Psycopg2Instrumentor().instrument_connection(cnx) + self.assertFalse(hasattr(cnx, "_is_instrumented_by_opentelemetry")) + cursor = cnx.cursor() + cursor.execute(query) + + spans_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans_list), 1) + + cnx = Psycopg2Instrumentor().uninstrument_connection(cnx) + cursor = cnx.cursor() + cursor.execute(query) + + spans_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans_list), 1)