From 69d1ea3e38cae6113ff6448b70503463ccfcc5ac Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 22 Nov 2024 16:13:56 -0800 Subject: [PATCH 01/12] Fix psycopg2 instrument_connection --- .../instrumentation/psycopg2/__init__.py | 64 ++++++++++++------- 1 file changed, 42 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..152560a550 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -106,18 +106,19 @@ from typing import Collection import psycopg2 +import wrapt 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 opentelemetry import trace as trace_api from opentelemetry.instrumentation import dbapi from opentelemetry.instrumentation.instrumentor import BaseInstrumentor from opentelemetry.instrumentation.psycopg2.package import _instruments from opentelemetry.instrumentation.psycopg2.version import __version__ _logger = logging.getLogger(__name__) -_OTEL_CURSOR_FACTORY_KEY = "_otel_orig_cursor_factory" class Psycopg2Instrumentor(BaseInstrumentor): @@ -130,6 +131,8 @@ class Psycopg2Instrumentor(BaseInstrumentor): _DATABASE_SYSTEM = "postgresql" + _otel_cursor_factory = None + def instrumentation_dependencies(self) -> Collection[str]: return _instruments @@ -158,32 +161,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: + def instrument_connection( + self, + connection, + tracer_provider: typing.Optional[trace_api.TracerProvider] = None, + enable_commenter: bool = False, + commenter_options: dict = None, + ): + if isinstance(connection, wrapt.ObjectProxy): + # The connection is already instrumented from wrapt.wrap_function_wrapper + # of the psycopg2 module's `connect` method by DB-API `wrap_connect`, + # so the Psycopg2Instrumentor is marked as instrumenting. _logger.warning( "Attempting to instrument Psycopg connection while already instrumented" ) + self._is_instrumented_by_opentelemetry = True + return connection + + # Save cursor_factory at instrumentor level because + # psycopg2 connection type does not allow arbitrary attrs + self._otel_cursor_factory = connection.cursor_factory + connection.cursor_factory = _new_cursor_factory( + base_factory=connection.cursor_factory, + tracer_provider=tracer_provider, + enable_commenter=enable_commenter, + commenter_options=commenter_options, + ) + self._is_instrumented_by_opentelemetry = True + return connection # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql - @staticmethod - def uninstrument_connection(connection): - connection.cursor_factory = getattr( - connection, _OTEL_CURSOR_FACTORY_KEY, None - ) - + def uninstrument_connection(self, connection): + self._is_instrumented_by_opentelemetry = False + connection.cursor_factory = self._otel_cursor_factory return connection @@ -231,7 +242,13 @@ def get_statement(self, cursor, args): return statement -def _new_cursor_factory(db_api=None, base_factory=None, tracer_provider=None): +def _new_cursor_factory( + db_api: dbapi.DatabaseApiIntegration = None, + base_factory: pg_cursor = None, + tracer_provider: typing.Optional[trace_api.TracerProvider] = None, + enable_commenter: bool = False, + commenter_options: dict = None, +): if not db_api: db_api = DatabaseApiIntegration( __name__, @@ -239,6 +256,9 @@ def _new_cursor_factory(db_api=None, base_factory=None, tracer_provider=None): connection_attributes=Psycopg2Instrumentor._CONNECTION_ATTRIBUTES, version=__version__, tracer_provider=tracer_provider, + enable_commenter=enable_commenter, + commenter_options=commenter_options, + connect_module=psycopg2, ) base_factory = base_factory or pg_cursor From c6181efe8d1d115bc304ffdc998e1ec284dc406c Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 22 Nov 2024 16:25:52 -0800 Subject: [PATCH 02/12] Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 173abf88f4..1c4166261e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,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-psycopg2`: fix AttributeError at `instrument_connection` + ([#3043](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3043)) ### Breaking changes From ea8d73d503008d53711dbb9283cdc9792ab61a1e Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Wed, 4 Dec 2024 16:00:13 -0800 Subject: [PATCH 03/12] Fix unit test --- .../tests/test_psycopg2_integration.py | 1 + 1 file changed, 1 insertion(+) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index 9a6a5ff2fa..eed43f9f62 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -208,6 +208,7 @@ def test_instrument_connection_with_instrument(self): self.assertEqual(len(spans_list), 0) Psycopg2Instrumentor().instrument() + cnx = psycopg2.connect(database="test") cnx = Psycopg2Instrumentor().instrument_connection(cnx) cursor = cnx.cursor() cursor.execute(query) From a0f74ca04d1234508fcf485d08b084c39d3ab31d Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Wed, 4 Dec 2024 16:00:50 -0800 Subject: [PATCH 04/12] Rm psycopg2 wrapt dep --- .../opentelemetry/instrumentation/psycopg2/__init__.py | 8 +++----- 1 file changed, 3 insertions(+), 5 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 152560a550..d8b9030189 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -106,7 +106,6 @@ from typing import Collection import psycopg2 -import wrapt from psycopg2.extensions import ( cursor as pg_cursor, # pylint: disable=no-name-in-module ) @@ -168,10 +167,9 @@ def instrument_connection( enable_commenter: bool = False, commenter_options: dict = None, ): - if isinstance(connection, wrapt.ObjectProxy): - # The connection is already instrumented from wrapt.wrap_function_wrapper - # of the psycopg2 module's `connect` method by DB-API `wrap_connect`, - # so the Psycopg2Instrumentor is marked as instrumenting. + if self._is_instrumented_by_opentelemetry: + # _instrument (via BaseInstrumentor) or instrument_connection (this) + # was already called _logger.warning( "Attempting to instrument Psycopg connection while already instrumented" ) From a0c9940a0f69f5fcf74b5ce2ca3083aae0ac4dae Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Wed, 4 Dec 2024 16:04:48 -0800 Subject: [PATCH 05/12] Fix docs and comments --- docs-requirements.txt | 1 + .../opentelemetry-instrumentation-psycopg2/README.rst | 4 ++-- .../opentelemetry/instrumentation/psycopg2/__init__.py | 10 +++++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/docs-requirements.txt b/docs-requirements.txt index d547e806a3..9274aeef8d 100644 --- a/docs-requirements.txt +++ b/docs-requirements.txt @@ -32,6 +32,7 @@ mysqlclient~=2.1.1 openai >= 1.26.0 psutil>=5 psycopg~=3.1.17 +psycopg2~=2.9.9 pika>=0.12.0 pymongo~=4.6.3 PyMySQL~=1.1.1 diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst b/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst index 77f3e6858f..19a49985dd 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst @@ -1,4 +1,4 @@ -OpenTelemetry Psycopg Instrumentation +OpenTelemetry Psycopg2 Instrumentation ===================================== |pypi| @@ -16,6 +16,6 @@ Installation References ---------- -* `OpenTelemetry Psycopg Instrumentation `_ +* `OpenTelemetry Psycopg2 Instrumentation `_ * `OpenTelemetry Project `_ * `OpenTelemetry Python Examples `_ 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 d8b9030189..a0c5bb1059 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -13,10 +13,10 @@ # limitations under the License. """ -The integration with PostgreSQL supports the `Psycopg`_ library, it can be enabled by +The integration with PostgreSQL supports the `Psycopg2`_ library, it can be enabled by using ``Psycopg2Instrumentor``. -.. _Psycopg: http://initd.org/psycopg/ +.. _Psycopg2: https://www.psycopg.org/docs/ SQLCOMMENTER ***************************************** @@ -136,8 +136,8 @@ def instrumentation_dependencies(self) -> Collection[str]: return _instruments def _instrument(self, **kwargs): - """Integrate with PostgreSQL Psycopg library. - Psycopg: http://initd.org/psycopg/ + """Integrate with PostgreSQL Psycopg2 library. + Psycopg2: https://www.psycopg.org/docs/ """ tracer_provider = kwargs.get("tracer_provider") enable_sqlcommenter = kwargs.get("enable_commenter", False) @@ -171,7 +171,7 @@ def instrument_connection( # _instrument (via BaseInstrumentor) or instrument_connection (this) # was already called _logger.warning( - "Attempting to instrument Psycopg connection while already instrumented" + "Attempting to instrument Psycopg2 connection while already instrumented" ) self._is_instrumented_by_opentelemetry = True return connection From b591b0a07b856a88c67360964947042fb7af422d Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 5 Dec 2024 13:07:25 -0800 Subject: [PATCH 06/12] Add and update tests --- .../tests/test_psycopg2_integration.py | 77 +++++++++++++++++-- 1 file changed, 72 insertions(+), 5 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index eed43f9f62..982020c635 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -16,10 +16,13 @@ from unittest import mock import psycopg2 +import pytest import opentelemetry.instrumentation.psycopg2 from opentelemetry import trace -from opentelemetry.instrumentation.psycopg2 import Psycopg2Instrumentor +from opentelemetry.instrumentation.psycopg2 import ( + Psycopg2Instrumentor, +) from opentelemetry.sdk import resources from opentelemetry.test.test_base import TestBase @@ -66,6 +69,10 @@ def get_dsn_parameters(self): # pylint: disable=no-self-use class TestPostgresqlIntegration(TestBase): + @pytest.fixture(autouse=True) + def inject_fixtures(self, caplog): + self.caplog = caplog # pylint: disable=attribute-defined-outside-init + def setUp(self): super().setUp() self.cursor_mock = mock.patch( @@ -190,7 +197,13 @@ def test_instrument_connection(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 0) - cnx = Psycopg2Instrumentor().instrument_connection(cnx) + instrumentor = Psycopg2Instrumentor() + original_cursor_factory = cnx.cursor_factory + cnx = instrumentor.instrument_connection(cnx) + self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) + self.assertEqual( + instrumentor._otel_cursor_factory, original_cursor_factory + ) cursor = cnx.cursor() cursor.execute(query) @@ -207,9 +220,52 @@ def test_instrument_connection_with_instrument(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 0) - Psycopg2Instrumentor().instrument() + instrumentor = Psycopg2Instrumentor() + instrumentor.instrument() + self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) + + cnx = psycopg2.connect(database="test") + cnx = Psycopg2Instrumentor().instrument_connection(cnx) + self.assertEqual( + self.caplog.records[0].getMessage(), + "Attempting to instrument Psycopg2 connection while already instrumented", + ) + self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) + # Will not set cursor_factory if already instrumented + self.assertEqual(instrumentor._otel_cursor_factory, None) + cursor = cnx.cursor() + cursor.execute(query) + + spans_list = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans_list), 1) + + def test_instrument_connection_with_instrument_connection(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) + + cnx = psycopg2.connect(database="test") + original_cursor_factory = cnx.cursor_factory + instrumentor = Psycopg2Instrumentor() + cnx = instrumentor.instrument_connection(cnx) + self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) + self.assertEqual( + instrumentor._otel_cursor_factory, original_cursor_factory + ) + cnx = Psycopg2Instrumentor().instrument_connection(cnx) + self.assertEqual( + self.caplog.records[0].getMessage(), + "Attempting to instrument Psycopg2 connection while already instrumented", + ) + self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) + self.assertEqual( + instrumentor._otel_cursor_factory, original_cursor_factory + ) cursor = cnx.cursor() cursor.execute(query) @@ -218,7 +274,8 @@ def test_instrument_connection_with_instrument(self): # pylint: disable=unused-argument def test_uninstrument_connection_with_instrument(self): - Psycopg2Instrumentor().instrument() + instrumentor = Psycopg2Instrumentor() + instrumentor.instrument() cnx = psycopg2.connect(database="test") query = "SELECT * FROM test" cursor = cnx.cursor() @@ -226,6 +283,7 @@ def test_uninstrument_connection_with_instrument(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) + self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) cnx = Psycopg2Instrumentor().uninstrument_connection(cnx) cursor = cnx.cursor() @@ -233,17 +291,24 @@ def test_uninstrument_connection_with_instrument(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) + self.assertFalse(instrumentor._is_instrumented_by_opentelemetry) # pylint: disable=unused-argument def test_uninstrument_connection_with_instrument_connection(self): cnx = psycopg2.connect(database="test") - Psycopg2Instrumentor().instrument_connection(cnx) + original_cursor_factory = cnx.cursor_factory + instrumentor = Psycopg2Instrumentor() + instrumentor.instrument_connection(cnx) query = "SELECT * FROM test" cursor = cnx.cursor() cursor.execute(query) spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) + self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) + self.assertEqual( + instrumentor._otel_cursor_factory, original_cursor_factory + ) cnx = Psycopg2Instrumentor().uninstrument_connection(cnx) cursor = cnx.cursor() @@ -251,6 +316,8 @@ def test_uninstrument_connection_with_instrument_connection(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) + self.assertFalse(instrumentor._is_instrumented_by_opentelemetry) + self.assertEqual(instrumentor._otel_cursor_factory, None) @mock.patch("opentelemetry.instrumentation.dbapi.wrap_connect") def test_sqlcommenter_enabled(self, event_mocked): From 59fa855ec4a35d1b55474db2309c183eb598599a Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 5 Dec 2024 13:35:22 -0800 Subject: [PATCH 07/12] Rm enable_commenter fixes for a different pr --- .../instrumentation/psycopg2/__init__.py | 15 +-------------- 1 file changed, 1 insertion(+), 14 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 a0c5bb1059..6354a3caf2 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -164,8 +164,6 @@ def instrument_connection( self, connection, tracer_provider: typing.Optional[trace_api.TracerProvider] = None, - enable_commenter: bool = False, - commenter_options: dict = None, ): if self._is_instrumented_by_opentelemetry: # _instrument (via BaseInstrumentor) or instrument_connection (this) @@ -182,8 +180,6 @@ def instrument_connection( connection.cursor_factory = _new_cursor_factory( base_factory=connection.cursor_factory, tracer_provider=tracer_provider, - enable_commenter=enable_commenter, - commenter_options=commenter_options, ) self._is_instrumented_by_opentelemetry = True @@ -240,13 +236,7 @@ def get_statement(self, cursor, args): return statement -def _new_cursor_factory( - db_api: dbapi.DatabaseApiIntegration = None, - base_factory: pg_cursor = None, - tracer_provider: typing.Optional[trace_api.TracerProvider] = None, - enable_commenter: bool = False, - commenter_options: dict = None, -): +def _new_cursor_factory(db_api=None, base_factory=None, tracer_provider=None): if not db_api: db_api = DatabaseApiIntegration( __name__, @@ -254,9 +244,6 @@ def _new_cursor_factory( connection_attributes=Psycopg2Instrumentor._CONNECTION_ATTRIBUTES, version=__version__, tracer_provider=tracer_provider, - enable_commenter=enable_commenter, - commenter_options=commenter_options, - connect_module=psycopg2, ) base_factory = base_factory or pg_cursor From dd7e3ea14d40afbccd8be649724ed34cc3932077 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 5 Dec 2024 13:39:50 -0800 Subject: [PATCH 08/12] Revert doc reqs --- docs-requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/docs-requirements.txt b/docs-requirements.txt index 9274aeef8d..d547e806a3 100644 --- a/docs-requirements.txt +++ b/docs-requirements.txt @@ -32,7 +32,6 @@ mysqlclient~=2.1.1 openai >= 1.26.0 psutil>=5 psycopg~=3.1.17 -psycopg2~=2.9.9 pika>=0.12.0 pymongo~=4.6.3 PyMySQL~=1.1.1 From 40f077ba395fc65565a8b1bbfa2c8b1d80aa555d Mon Sep 17 00:00:00 2001 From: Tammy Baylis <96076570+tammy-baylis-swi@users.noreply.github.com> Date: Fri, 6 Dec 2024 10:06:19 -0800 Subject: [PATCH 09/12] Update readme Co-authored-by: Riccardo Magliocchetti --- .../opentelemetry-instrumentation-psycopg2/README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst b/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst index 19a49985dd..b070a2fd6c 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/README.rst @@ -1,5 +1,5 @@ OpenTelemetry Psycopg2 Instrumentation -===================================== +====================================== |pypi| From e8d47bf3e61ef164abccb4072951ebf0f24c762c Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 20 Dec 2024 16:50:07 -0800 Subject: [PATCH 10/12] Rm instrument_connection is-instrumented check; use dict to store cnx --- .../instrumentation/psycopg2/__init__.py | 23 +++---- .../tests/test_psycopg2_integration.py | 61 ++++++++++--------- 2 files changed, 40 insertions(+), 44 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 f4e8a8d086..d0705d63e5 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -146,7 +146,7 @@ class Psycopg2Instrumentor(BaseInstrumentor): _DATABASE_SYSTEM = "postgresql" - _otel_cursor_factory = None + _otel_cursor_factories = {} def instrumentation_dependencies(self) -> Collection[str]: return _instruments @@ -193,30 +193,25 @@ def instrument_connection( Returns: An instrumented psycopg2 connection object. """ - if self._is_instrumented_by_opentelemetry: - # _instrument (via BaseInstrumentor) or instrument_connection (this) - # was already called - _logger.warning( - "Attempting to instrument Psycopg2 connection while already instrumented" - ) - self._is_instrumented_by_opentelemetry = True - return connection + # TODO Add check for attempt to instrument a connection when already instrumented + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 - # Save cursor_factory at instrumentor level because + # Save cursor_factory in instrumentor map because # psycopg2 connection type does not allow arbitrary attrs - self._otel_cursor_factory = connection.cursor_factory + self._otel_cursor_factories[connection] = connection.cursor_factory connection.cursor_factory = _new_cursor_factory( base_factory=connection.cursor_factory, tracer_provider=tracer_provider, ) - self._is_instrumented_by_opentelemetry = True + _logger.warning("factories: %s", self._otel_cursor_factories) return connection # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql def uninstrument_connection(self, connection): - self._is_instrumented_by_opentelemetry = False - connection.cursor_factory = self._otel_cursor_factory + connection.cursor_factory = self._otel_cursor_factories.pop( + connection, None + ) return connection diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index 982020c635..a8fc443936 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -16,7 +16,6 @@ from unittest import mock import psycopg2 -import pytest import opentelemetry.instrumentation.psycopg2 from opentelemetry import trace @@ -69,10 +68,6 @@ def get_dsn_parameters(self): # pylint: disable=no-self-use class TestPostgresqlIntegration(TestBase): - @pytest.fixture(autouse=True) - def inject_fixtures(self, caplog): - self.caplog = caplog # pylint: disable=attribute-defined-outside-init - def setUp(self): super().setUp() self.cursor_mock = mock.patch( @@ -200,9 +195,8 @@ def test_instrument_connection(self): instrumentor = Psycopg2Instrumentor() original_cursor_factory = cnx.cursor_factory cnx = instrumentor.instrument_connection(cnx) - self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) self.assertEqual( - instrumentor._otel_cursor_factory, original_cursor_factory + instrumentor._otel_cursor_factories[cnx], original_cursor_factory ) cursor = cnx.cursor() cursor.execute(query) @@ -222,22 +216,27 @@ def test_instrument_connection_with_instrument(self): instrumentor = Psycopg2Instrumentor() instrumentor.instrument() - self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) cnx = psycopg2.connect(database="test") + original_cursor_factory = cnx.cursor_factory + orig_cnx = cnx cnx = Psycopg2Instrumentor().instrument_connection(cnx) + + # TODO Should not set cursor_factory if already instrumented + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 + # self.assertEqual(instrumentor._otel_cursor_factories[cnx], None) self.assertEqual( - self.caplog.records[0].getMessage(), - "Attempting to instrument Psycopg2 connection while already instrumented", + instrumentor._otel_cursor_factories.get(orig_cnx), + original_cursor_factory, ) - self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) - # Will not set cursor_factory if already instrumented - self.assertEqual(instrumentor._otel_cursor_factory, None) cursor = cnx.cursor() cursor.execute(query) spans_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(spans_list), 1) + # TODO Add check for attempt to instrument a connection when already instrumented + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 + # self.assertEqual(len(spans_list), 1) + self.assertEqual(len(spans_list), 2) def test_instrument_connection_with_instrument_connection(self): cnx = psycopg2.connect(database="test") @@ -251,26 +250,29 @@ def test_instrument_connection_with_instrument_connection(self): cnx = psycopg2.connect(database="test") original_cursor_factory = cnx.cursor_factory instrumentor = Psycopg2Instrumentor() + orig_cnx = cnx cnx = instrumentor.instrument_connection(cnx) - self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) self.assertEqual( - instrumentor._otel_cursor_factory, original_cursor_factory + instrumentor._otel_cursor_factories.get(orig_cnx), + original_cursor_factory, ) - cnx = Psycopg2Instrumentor().instrument_connection(cnx) - self.assertEqual( - self.caplog.records[0].getMessage(), - "Attempting to instrument Psycopg2 connection while already instrumented", - ) - self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) + orig_cnx = cnx + original_cursor_factory = cnx.cursor_factory + instrumentor = Psycopg2Instrumentor() + cnx = instrumentor.instrument_connection(cnx) self.assertEqual( - instrumentor._otel_cursor_factory, original_cursor_factory + instrumentor._otel_cursor_factories.get(orig_cnx), + original_cursor_factory, ) cursor = cnx.cursor() cursor.execute(query) spans_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(spans_list), 1) + # TODO Add check for attempt to instrument a connection when already instrumented + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 + # self.assertEqual(len(spans_list), 1) + self.assertEqual(len(spans_list), 2) # pylint: disable=unused-argument def test_uninstrument_connection_with_instrument(self): @@ -283,7 +285,6 @@ def test_uninstrument_connection_with_instrument(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) - self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) cnx = Psycopg2Instrumentor().uninstrument_connection(cnx) cursor = cnx.cursor() @@ -291,7 +292,6 @@ def test_uninstrument_connection_with_instrument(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) - self.assertFalse(instrumentor._is_instrumented_by_opentelemetry) # pylint: disable=unused-argument def test_uninstrument_connection_with_instrument_connection(self): @@ -305,19 +305,20 @@ def test_uninstrument_connection_with_instrument_connection(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) - self.assertTrue(instrumentor._is_instrumented_by_opentelemetry) self.assertEqual( - instrumentor._otel_cursor_factory, original_cursor_factory + instrumentor._otel_cursor_factories[cnx], original_cursor_factory ) + orig_cnx = cnx 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) - self.assertFalse(instrumentor._is_instrumented_by_opentelemetry) - self.assertEqual(instrumentor._otel_cursor_factory, None) + self.assertEqual( + instrumentor._otel_cursor_factories.get(orig_cnx), None + ) @mock.patch("opentelemetry.instrumentation.dbapi.wrap_connect") def test_sqlcommenter_enabled(self, event_mocked): From 6f67b333f826becbaa44c5ee1e0dd9a37c6401a5 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Fri, 10 Jan 2025 10:45:29 -0800 Subject: [PATCH 11/12] Rm cursor_factory restore at uninstrument_connection --- .../instrumentation/psycopg2/__init__.py | 18 +++----- .../tests/test_psycopg2_integration.py | 45 ++++--------------- 2 files changed, 13 insertions(+), 50 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 d0705d63e5..0602d3d7ef 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py @@ -146,8 +146,6 @@ class Psycopg2Instrumentor(BaseInstrumentor): _DATABASE_SYSTEM = "postgresql" - _otel_cursor_factories = {} - def instrumentation_dependencies(self) -> Collection[str]: return _instruments @@ -176,8 +174,8 @@ 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( - self, connection, tracer_provider: typing.Optional[trace_api.TracerProvider] = None, ): @@ -195,23 +193,17 @@ def instrument_connection( """ # TODO Add check for attempt to instrument a connection when already instrumented # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 - - # Save cursor_factory in instrumentor map because - # psycopg2 connection type does not allow arbitrary attrs - self._otel_cursor_factories[connection] = connection.cursor_factory connection.cursor_factory = _new_cursor_factory( base_factory=connection.cursor_factory, tracer_provider=tracer_provider, ) - _logger.warning("factories: %s", self._otel_cursor_factories) - return connection # TODO(owais): check if core dbapi can do this for all dbapi implementations e.g, pymysql and mysql - def uninstrument_connection(self, connection): - connection.cursor_factory = self._otel_cursor_factories.pop( - connection, None - ) + @staticmethod + def uninstrument_connection(connection): + # TODO Add check for attempt to instrument a connection when already instrumented + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 return connection diff --git a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py index a8fc443936..6ec103017e 100644 --- a/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py +++ b/instrumentation/opentelemetry-instrumentation-psycopg2/tests/test_psycopg2_integration.py @@ -193,11 +193,7 @@ def test_instrument_connection(self): self.assertEqual(len(spans_list), 0) instrumentor = Psycopg2Instrumentor() - original_cursor_factory = cnx.cursor_factory cnx = instrumentor.instrument_connection(cnx) - self.assertEqual( - instrumentor._otel_cursor_factories[cnx], original_cursor_factory - ) cursor = cnx.cursor() cursor.execute(query) @@ -218,17 +214,8 @@ def test_instrument_connection_with_instrument(self): instrumentor.instrument() cnx = psycopg2.connect(database="test") - original_cursor_factory = cnx.cursor_factory - orig_cnx = cnx cnx = Psycopg2Instrumentor().instrument_connection(cnx) - # TODO Should not set cursor_factory if already instrumented - # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 - # self.assertEqual(instrumentor._otel_cursor_factories[cnx], None) - self.assertEqual( - instrumentor._otel_cursor_factories.get(orig_cnx), - original_cursor_factory, - ) cursor = cnx.cursor() cursor.execute(query) @@ -248,23 +235,11 @@ def test_instrument_connection_with_instrument_connection(self): self.assertEqual(len(spans_list), 0) cnx = psycopg2.connect(database="test") - original_cursor_factory = cnx.cursor_factory instrumentor = Psycopg2Instrumentor() - orig_cnx = cnx cnx = instrumentor.instrument_connection(cnx) - self.assertEqual( - instrumentor._otel_cursor_factories.get(orig_cnx), - original_cursor_factory, - ) - orig_cnx = cnx - original_cursor_factory = cnx.cursor_factory instrumentor = Psycopg2Instrumentor() cnx = instrumentor.instrument_connection(cnx) - self.assertEqual( - instrumentor._otel_cursor_factories.get(orig_cnx), - original_cursor_factory, - ) cursor = cnx.cursor() cursor.execute(query) @@ -290,13 +265,14 @@ def test_uninstrument_connection_with_instrument(self): cursor = cnx.cursor() cursor.execute(query) - spans_list = self.memory_exporter.get_finished_spans() - self.assertEqual(len(spans_list), 1) + # TODO Add check for attempt to instrument a connection when already instrumented + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 + # spans_list = self.memory_exporter.get_finished_spans() + # self.assertEqual(len(spans_list), 1) # pylint: disable=unused-argument def test_uninstrument_connection_with_instrument_connection(self): cnx = psycopg2.connect(database="test") - original_cursor_factory = cnx.cursor_factory instrumentor = Psycopg2Instrumentor() instrumentor.instrument_connection(cnx) query = "SELECT * FROM test" @@ -305,20 +281,15 @@ def test_uninstrument_connection_with_instrument_connection(self): spans_list = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans_list), 1) - self.assertEqual( - instrumentor._otel_cursor_factories[cnx], original_cursor_factory - ) - orig_cnx = cnx 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) - self.assertEqual( - instrumentor._otel_cursor_factories.get(orig_cnx), None - ) + # TODO Add check for attempt to instrument a connection when already instrumented + # https://github.com/open-telemetry/opentelemetry-python-contrib/issues/3138 + # spans_list = self.memory_exporter.get_finished_spans() + # self.assertEqual(len(spans_list), 1) @mock.patch("opentelemetry.instrumentation.dbapi.wrap_connect") def test_sqlcommenter_enabled(self, event_mocked): From 680b0f5d29afda7716a3174a1f36af038b5226e1 Mon Sep 17 00:00:00 2001 From: tammy-baylis-swi Date: Thu, 27 Feb 2025 09:36:06 -0800 Subject: [PATCH 12/12] Changelog --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f74240f8bb..eda7f1b1a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#3247](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3247)) - `opentelemetry-instrumentation-asyncpg` Fix fallback for empty queries. ([#3253](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3253)) +- `opentelemetry-instrumentation-psycopg2`: fix AttributeError at `instrument_connection` + ([#3043](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3043)) ## Version 1.30.0/0.51b0 (2025-02-03) @@ -142,9 +144,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `opentelemetry-instrumentation-sqlalchemy`: Fix a remaining memory leak in EngineTracer ([#3053](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3053)) - `opentelemetry-instrumentation-sqlite3`: Update documentation on explicit cursor support of tracing - ([#3088](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3088))' -- `opentelemetry-instrumentation-psycopg2`: fix AttributeError at `instrument_connection` - ([#3043](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3043)) + ([#3088](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3088)) ### Breaking changes