Set c3p0 dataSourceName property to include Metabase specific info (#15681)

Set c3p0 dataSourceName property to include Metabase specific info

Change data-warehouse-connection-pool-properties multimethod to include database as 2nd param

Setting the dataSourceName c3p0 property to have the format db-<N>-<D>-<DB> where <N> is the DW database ID, <D> is the driver name, and <DB> is the database name from the db details

Adding test

Setting c3p0 dataSourceName for the app DB pool as well

modules/drivers/sparksql/src/metabase/driver/hive_like.clj

@@ -26,11 +26,11 @@
   :sunday)
 
 (defmethod sql-jdbc.conn/data-warehouse-connection-pool-properties :hive-like
-  [driver]
+  [driver database]
   ;; The Hive JDBC driver doesn't support `Connection.isValid()`, so we need to supply a test query for c3p0 to use to
   ;; validate connections upon checkout.
   (merge
-   ((get-method sql-jdbc.conn/data-warehouse-connection-pool-properties :sql-jdbc) driver)
+   ((get-method sql-jdbc.conn/data-warehouse-connection-pool-properties :sql-jdbc) driver database)
    {"preferredTestQuery" "SELECT 1"}))
 
 (defmethod sql-jdbc.sync/database-type->base-type :hive-like

src/metabase/db/connection_pool_setup.clj

@@ -25,10 +25,12 @@
      {"maxPoolSize" max-pool-size})))
 
 (s/defn ^:private connection-pool-spec :- {:datasource javax.sql.DataSource, s/Keyword s/Any}
-  [jdbc-spec :- (s/cond-pre s/Str su/Map)]
-  (if (string? jdbc-spec)
-    {:datasource (connection-pool/pooled-data-source-from-url jdbc-spec application-db-connection-pool-props)}
-    (connection-pool/connection-pool-spec jdbc-spec application-db-connection-pool-props)))
+  [db-type :- s/Keyword jdbc-spec :- (s/cond-pre s/Str su/Map)]
+  (let [ds-name    (format "metatabase-%s-app-db" (name db-type))
+        pool-props (assoc application-db-connection-pool-props "dataSourceName" ds-name)]
+    (if (string? jdbc-spec)
+      {:datasource (connection-pool/pooled-data-source-from-url jdbc-spec pool-props)}
+      (connection-pool/connection-pool-spec jdbc-spec pool-props))))
 
 (defn create-connection-pool!
   "Create a connection pool for the application DB and set it as the default Toucan connection. This is normally called
@@ -41,7 +43,7 @@
       (log/trace "Closing old application DB connection pool")
       (.close pool)))
   (log/debug (trs "Set default db connection with connection pool..."))
-  (let [pool-spec (connection-pool-spec jdbc-spec)]
+  (let [pool-spec (connection-pool-spec db-type jdbc-spec)]
     (db/set-default-db-connection! pool-spec)
     (db/set-default-jdbc-options! {:read-columns mdb.jdbc-protocols/read-columns})
     pool-spec))

src/metabase/driver/sql_jdbc/connection.clj

@@ -38,12 +38,12 @@
   powering Cards and the sync process, which are less sensitive to overhead than something like the application DB.
 
   Drivers that need to override the default properties below can provide custom implementations of this method."
-  {:arglists '([driver])}
+  {:arglists '([driver database])}
   driver/dispatch-on-initialized-driver
   :hierarchy #'driver/hierarchy)
 
 (defmethod data-warehouse-connection-pool-properties :default
-  [_]
+  [driver database]
   { ;; only fetch one new connection at a time, rather than batching fetches (default = 3 at a time). This is done in
    ;; interest of minimizing memory consumption
    "acquireIncrement"             1
@@ -76,7 +76,9 @@
    ;; if the parameter is to have any effect.
    ;;
    ;; Kill idle connections above the minPoolSize after 5 minutes.
-   "maxIdleTimeExcessConnections" (* 5 60)})
+   "maxIdleTimeExcessConnections" (* 5 60)
+   ;; Set the data source name so that the c3p0 JMX bean has a useful identifier
+   "dataSourceName"               (format "db-%d-%s-%s" (u/the-id database) (name driver) (-> database :details :db))})
 
 (defn- create-pool!
   "Create a new C3P0 `ComboPooledDataSource` for connecting to the given `database`."
@@ -85,7 +87,7 @@
   (log/debug (u/format-color 'cyan (trs "Creating new connection pool for {0} database {1} ..." driver id)))
   (let [details-with-tunnel (driver/incorporate-ssh-tunnel-details driver details) ;; If the tunnel is disabled this returned unchanged
         spec                (connection-details->spec driver details-with-tunnel)
-        properties          (data-warehouse-connection-pool-properties driver)]
+        properties          (data-warehouse-connection-pool-properties driver database)]
     (merge
       (connection-pool/connection-pool-spec spec properties)
       ;; also capture entries related to ssh tunneling for later use

test/metabase/db/connection_pool_setup_test.clj

@@ -8,8 +8,8 @@
 
 (deftest connection-pool-spec-test
   (testing "Should be able to create a connection pool"
-    (letfn [(test* [spec]
-              (let [{:keys [datasource], :as spec} (#'mdb.connection-pool-setup/connection-pool-spec spec)]
+    (letfn [(test* [db-type spec]
+              (let [{:keys [datasource], :as spec} (#'mdb.connection-pool-setup/connection-pool-spec db-type spec)]
                 (try
                   (is (instance? javax.sql.DataSource datasource))
                   (is (= [{:one 1}]
@@ -17,8 +17,8 @@
                   (finally
                     (connection-pool/destroy-connection-pool! datasource)))))]
       (testing "from a jdbc-spec map"
-        (test* {:subprotocol "h2"
-                :subname     (format "mem:%s;DB_CLOSE_DELAY=10" (mt/random-name))
-                :classname   "org.h2.Driver"}))
+        (test* :h2 {:subprotocol "h2"
+                    :subname     (format "mem:%s;DB_CLOSE_DELAY=10" (mt/random-name))
+                    :classname   "org.h2.Driver"}))
       (testing "from a connection URL"
-        (test* (format "jdbc:h2:mem:%s;DB_CLOSE_DELAY=10" (mt/random-name)))))))
+        (test* :h2 (format "jdbc:h2:mem:%s;DB_CLOSE_DELAY=10" (mt/random-name)))))))

test/metabase/driver/sql_jdbc/connection_test.clj

@@ -2,7 +2,9 @@
   (:require [clojure.java.jdbc :as jdbc]
             [clojure.test :refer :all]
             [metabase.db.spec :as db.spec]
+            [metabase.driver :as driver]
             [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+            [metabase.driver.sql-jdbc.test-util :as sql-jdbc.tu]
             [metabase.driver.util :as driver.u]
             [metabase.models.database :refer [Database]]
             [metabase.test :as mt]
@@ -56,3 +58,12 @@
                                     (u/id database)))))
               (testing "the pool has been destroyed"
                 (is @destroyed?)))))))))
+
+(deftest c3p0-datasource-name-test
+  (mt/test-drivers (sql-jdbc.tu/sql-jdbc-drivers)
+    (testing "The dataSourceName c3p0 property is set properly for a database"
+      (let [db       (mt/db)
+            props    (sql-jdbc.conn/data-warehouse-connection-pool-properties driver/*driver* db)
+            expected (format "db-%d-%s-%s" (u/the-id db) (name driver/*driver*) (-> db :details :db))]
+        (is (= expected
+               (get props "dataSourceName")))))))