From 9e268e2962cb8e0e3be46b47a3a29aad63e45e88 Mon Sep 17 00:00:00 2001 From: sroda Date: Wed, 3 May 2023 11:36:28 +0300 Subject: [PATCH 1/4] Expand sqlalchemy pool.name to follow the semantic conventions --- .../instrumentation/sqlalchemy/engine.py | 9 ++++- .../tests/test_sqlalchemy_metrics.py | 3 +- .../tests/sqlalchemy_tests/test_postgres.py | 37 +++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index ca691fc052..2014ced11b 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -119,7 +119,14 @@ def __init__( self._register_event_listener(engine, "checkout", self._pool_checkout) def _get_pool_name(self): - return self.engine.pool.logging_name or "" + if self.engine.pool.logging_name is not None: + return self.engine.pool.logging_name + else: + drivername = self.engine.url.drivername or "" + host = self.engine.url.host or "" + port = self.engine.url.port or "" + database = self.engine.url.database or "" + return f"{drivername}://{host}:{port}/{database}" def _add_idle_to_connection_usage(self, value): self.connections_usage.add( diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy_metrics.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy_metrics.py index 39e45945f7..2d753c3c42 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy_metrics.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy_metrics.py @@ -70,11 +70,12 @@ def test_metrics_one_connection(self): ) def test_metrics_without_pool_name(self): - pool_name = "" + pool_name = "pool_test_name" engine = sqlalchemy.create_engine( "sqlite:///:memory:", pool_size=5, poolclass=QueuePool, + pool_logging_name=pool_name, ) metrics = self.get_sorted_metrics() diff --git a/tests/opentelemetry-docker-tests/tests/sqlalchemy_tests/test_postgres.py b/tests/opentelemetry-docker-tests/tests/sqlalchemy_tests/test_postgres.py index bbc62bfbbf..ff664091c8 100644 --- a/tests/opentelemetry-docker-tests/tests/sqlalchemy_tests/test_postgres.py +++ b/tests/opentelemetry-docker-tests/tests/sqlalchemy_tests/test_postgres.py @@ -95,3 +95,40 @@ class PostgresCreatorTestCase(PostgresTestCase): "url": "postgresql://", "creator": lambda: psycopg2.connect(**POSTGRES_CONFIG), } + + +class PostgresMetricsTestCase(PostgresTestCase): + __test__ = True + + VENDOR = "postgresql" + SQL_DB = "opentelemetry-tests" + ENGINE_ARGS = { + "url": "postgresql://%(user)s:%(password)s@%(host)s:%(port)s/%(dbname)s" + % POSTGRES_CONFIG + } + + def test_metrics_pool_name(self): + with self.connection() as conn: + conn.execute("SELECT 1 + 1").fetchall() + + pool_name = "{}://{}:{}/{}".format( + self.VENDOR, + POSTGRES_CONFIG["host"], + POSTGRES_CONFIG["port"], + self.SQL_DB, + ) + metrics = self.get_sorted_metrics() + self.assertEqual(len(metrics), 1) + self.assert_metric_expected( + metrics[0], + [ + self.create_number_data_point( + value=0, + attributes={"pool.name": pool_name, "state": "idle"}, + ), + self.create_number_data_point( + value=0, + attributes={"pool.name": pool_name, "state": "used"}, + ), + ], + ) From ab0ab1c34fd96f67df2ff299a32d8533544a5010 Mon Sep 17 00:00:00 2001 From: sroda Date: Wed, 3 May 2023 11:42:35 +0300 Subject: [PATCH 2/4] Add a changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73fc2b9aa1..c4255dfc53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Expand sqlalchemy pool.name to follow the semantic conventions + ([#1778](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1778)) - Add `excluded_urls` functionality to `urllib` and `urllib3` instrumentations ([#1733](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1733)) - Make Django request span attributes available for `start_span`. From aff7ef927cc6473859809da5a5a0214d2af1b65f Mon Sep 17 00:00:00 2001 From: sroda Date: Wed, 3 May 2023 12:03:16 +0300 Subject: [PATCH 3/4] Fix pylint --- .../instrumentation/sqlalchemy/engine.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index 2014ced11b..c633d13538 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -121,12 +121,12 @@ def __init__( def _get_pool_name(self): if self.engine.pool.logging_name is not None: return self.engine.pool.logging_name - else: - drivername = self.engine.url.drivername or "" - host = self.engine.url.host or "" - port = self.engine.url.port or "" - database = self.engine.url.database or "" - return f"{drivername}://{host}:{port}/{database}" + + drivername = self.engine.url.drivername or "" + host = self.engine.url.host or "" + port = self.engine.url.port or "" + database = self.engine.url.database or "" + return f"{drivername}://{host}:{port}/{database}" def _add_idle_to_connection_usage(self, value): self.connections_usage.add( From 8ccce86e6fcad3cf62a5434fe472323ef6f55557 Mon Sep 17 00:00:00 2001 From: sroda Date: Wed, 3 May 2023 19:51:02 +0300 Subject: [PATCH 4/4] Fixed after CR --- .../opentelemetry/instrumentation/sqlalchemy/engine.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py index c633d13538..9ff6057728 100644 --- a/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py +++ b/instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py @@ -118,16 +118,18 @@ def __init__( self._register_event_listener(engine, "checkin", self._pool_checkin) self._register_event_listener(engine, "checkout", self._pool_checkout) - def _get_pool_name(self): - if self.engine.pool.logging_name is not None: - return self.engine.pool.logging_name - + def _get_connection_string(self): drivername = self.engine.url.drivername or "" host = self.engine.url.host or "" port = self.engine.url.port or "" database = self.engine.url.database or "" return f"{drivername}://{host}:{port}/{database}" + def _get_pool_name(self): + if self.engine.pool.logging_name is not None: + return self.engine.pool.logging_name + return self._get_connection_string() + def _add_idle_to_connection_usage(self, value): self.connections_usage.add( value,