From 1a1163e919f1e573009b1ba5bb43a7365a80f58f Mon Sep 17 00:00:00 2001
From: Shalev Roda <65566801+shalevr@users.noreply.github.com>
Date: Sat, 6 May 2023 20:39:52 +0300
Subject: [PATCH] Expand sqlalchemy pool.name to follow the semantic
 conventions (#1778)

---
 CHANGELOG.md                                  |  2 +
 .../instrumentation/sqlalchemy/engine.py      | 11 +++++-
 .../tests/test_sqlalchemy_metrics.py          |  3 +-
 .../tests/sqlalchemy_tests/test_postgres.py   | 37 +++++++++++++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ead8d5c134..15e7f953c2 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`. 
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..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,8 +118,17 @@ def __init__(
         self._register_event_listener(engine, "checkin", self._pool_checkin)
         self._register_event_listener(engine, "checkout", self._pool_checkout)
 
+    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):
-        return self.engine.pool.logging_name or ""
+        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(
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"},
+                ),
+            ],
+        )