Remove JMX registration requirement for Apache DBCP 2.0 metrics

instrumentation/apache-dbcp-2.0/javaagent/README.md

@@ -3,7 +3,10 @@
 Provides OpenTelemetry auto-instrumentation
 for [Apache DBCP](https://commons.apache.org/proper/commons-dbcp/).
 
-This auto-instrumentation uses `MBeanRegistration` methods for lifecycle detection, therefore it
-only activates if the `BasicDataSource` is registered to an `MBeanServer`. If using Spring Boot,
-this happens automatically as all Spring beans that support JMX registration are automatically
-registered by default.
+This auto-instrumentation registers metrics after `BasicDataSource` completes
+`startPoolMaintenance()`, when the connection pool has been initialized. It unregisters metrics
+when `BasicDataSource` is closed or deregistered. JMX registration is not required. When a JMX
+`ObjectName` is available, its `name` property is used as the pool name; if the property is
+absent, the full `ObjectName` is used. Otherwise, the JDBC URL and connection properties are used
+to derive `server.address[:server.port][/db.namespace]`. If neither server address nor database
+namespace is available, `apache-dbcp2` is used.

instrumentation/apache-dbcp-2.0/javaagent/build.gradle.kts

@@ -16,7 +16,21 @@ dependencies {
 
   implementation(project(":instrumentation:apache-dbcp-2.0:library"))
 
+  bootstrap(project(":instrumentation:jdbc:bootstrap"))
+  compileOnly(
+    project(
+      path = ":instrumentation:jdbc:library",
+      configuration = "shadow",
+    ),
+  )
+
   testImplementation(project(":instrumentation:apache-dbcp-2.0:testing"))
+  testInstrumentation(
+    project(
+      path = ":instrumentation:jdbc:library",
+      configuration = "shadow",
+    ),
+  )
 }
 
 tasks {

instrumentation/apache-dbcp-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachedbcp/v2_0/ApacheDbcpInstrumentationModule.java

@@ -18,6 +18,11 @@ public ApacheDbcpInstrumentationModule() {
     super("apache-dbcp", "apache-dbcp-2.0");
   }
 
+  @Override
+  public boolean isHelperClass(String className) {
+    return "org.apache.commons.dbcp2.OpenTelemetryBasicDataSourceUtil".equals(className);
+  }
+
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
     return singletonList(new BasicDataSourceInstrumentation());

instrumentation/apache-dbcp-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachedbcp/v2_0/ApacheDbcpSingletons.java

@@ -7,6 +7,10 @@
 
 import io.opentelemetry.api.GlobalOpenTelemetry;
 import io.opentelemetry.instrumentation.apachedbcp.v2_0.ApacheDbcpTelemetry;
+import io.opentelemetry.instrumentation.jdbc.internal.JdbcConnectionUrlParser;
+import io.opentelemetry.javaagent.bootstrap.jdbc.DbInfo;
+import org.apache.commons.dbcp2.BasicDataSource;
+import org.apache.commons.dbcp2.OpenTelemetryBasicDataSourceUtil;
 
 public class ApacheDbcpSingletons {
 
@@ -17,5 +21,31 @@ public static ApacheDbcpTelemetry telemetry() {
     return telemetry;
   }
 
+  public static String getDataSourceName(BasicDataSource dataSource) {
+    DbInfo dbInfo =
+        JdbcConnectionUrlParser.parse(
+            dataSource.getUrl(),
+            OpenTelemetryBasicDataSourceUtil.getConnectionProperties(dataSource));
+    String serverAddress = dbInfo.getServerAddress();
+    Integer serverPort = dbInfo.getServerPort();
+    String dbNamespace = dbInfo.getDbNamespace();
+
+    StringBuilder poolName = new StringBuilder();
+    if (serverAddress != null) {
+      poolName.append(serverAddress);
+      if (serverPort != null) {
+        poolName.append(':').append(serverPort);
+      }
+    }
+    if (dbNamespace != null) {
+      if (poolName.length() > 0) {
+        poolName.append('/');
+      }
+      poolName.append(dbNamespace);
+    }
+
+    return poolName.length() > 0 ? poolName.toString() : "apache-dbcp2";
+  }
+
   private ApacheDbcpSingletons() {}
 }

instrumentation/apache-dbcp-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachedbcp/v2_0/BasicDataSourceInstrumentation.java

@@ -5,9 +5,11 @@
 
 package io.opentelemetry.javaagent.instrumentation.apachedbcp.v2_0;
 
+import static io.opentelemetry.javaagent.instrumentation.apachedbcp.v2_0.ApacheDbcpSingletons.getDataSourceName;
 import static io.opentelemetry.javaagent.instrumentation.apachedbcp.v2_0.ApacheDbcpSingletons.telemetry;
 import static net.bytebuddy.matcher.ElementMatchers.isPublic;
 import static net.bytebuddy.matcher.ElementMatchers.named;
+import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
 import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
 
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
@@ -17,6 +19,7 @@
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
 import org.apache.commons.dbcp2.BasicDataSource;
+import org.apache.commons.dbcp2.OpenTelemetryBasicDataSourceUtil;
 
 class BasicDataSourceInstrumentation implements TypeInstrumentation {
   @Override
@@ -27,35 +30,36 @@ public ElementMatcher<TypeDescription> typeMatcher() {
   @Override
   public void transform(TypeTransformer typeTransformer) {
     typeTransformer.applyAdviceToMethod(
-        isPublic().and(named("preRegister")).and(takesArguments(2)),
-        getClass().getName() + "$PreRegisterAdvice");
+        named("startPoolMaintenance").and(takesArguments(0)),
+        getClass().getName() + "$StartPoolMaintenanceAdvice");
 
     typeTransformer.applyAdviceToMethod(
-        isPublic().and(named("postDeregister")), getClass().getName() + "$PostDeregisterAdvice");
+        isPublic().and(namedOneOf("close", "postDeregister")).and(takesArguments(0)),
+        getClass().getName() + "$DeregisterAdvice");
   }
 
   @SuppressWarnings("unused")
-  public static class PreRegisterAdvice {
-    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
-    public static void onExit(
-        @Advice.This BasicDataSource dataSource, @Advice.Return ObjectName objectName) {
-      String dataSourceName;
+  public static class StartPoolMaintenanceAdvice {
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void onExit(@Advice.This BasicDataSource dataSource) {
+      String dataSourceName = null;
+      ObjectName objectName = OpenTelemetryBasicDataSourceUtil.getRegisteredJmxName(dataSource);
       if (objectName != null) {
         dataSourceName = objectName.getKeyProperty("name");
         if (dataSourceName == null) {
           dataSourceName = objectName.toString();
         }
-      } else {
-        // fallback just in case it is somehow registered without a name
-        dataSourceName = "dbcp2-" + System.identityHashCode(dataSource);
+      }
+      if (dataSourceName == null) {
+        dataSourceName = getDataSourceName(dataSource);
       }
       telemetry().registerMetrics(dataSource, dataSourceName);
     }
   }
 
   @SuppressWarnings("unused")
-  public static class PostDeregisterAdvice {
-    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+  public static class DeregisterAdvice {
+    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
     public static void onExit(@Advice.This BasicDataSource dataSource) {
       telemetry().unregisterMetrics(dataSource);
     }

instrumentation/apache-dbcp-2.0/javaagent/src/main/java/org/apache/commons/dbcp2/OpenTelemetryBasicDataSourceUtil.java

@@ -0,0 +1,25 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.apache.commons.dbcp2;
+
+import java.util.Properties;
+import javax.annotation.Nullable;
+import javax.management.ObjectName;
+
+// Helper for accessing non-public BasicDataSource methods from the same package.
+public final class OpenTelemetryBasicDataSourceUtil {
+
+  @Nullable
+  public static ObjectName getRegisteredJmxName(BasicDataSource dataSource) {
+    return dataSource.getRegisteredJmxName();
+  }
+
+  public static Properties getConnectionProperties(BasicDataSource dataSource) {
+    return dataSource.getConnectionProperties();
+  }
+
+  private OpenTelemetryBasicDataSourceUtil() {}
+}

instrumentation/apache-dbcp-2.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/apachedbcp/v2_0/ApacheDbcpInstrumentationTest.java

@@ -9,8 +9,10 @@
 import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import java.lang.management.ManagementFactory;
+import javax.management.MBeanServer;
 import javax.management.ObjectName;
 import org.apache.commons.dbcp2.BasicDataSource;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 class ApacheDbcpInstrumentationTest extends AbstractApacheDbcpInstrumentationTest {
@@ -24,14 +26,119 @@ protected InstrumentationExtension testing() {
   }
 
   @Override
-  protected void configure(BasicDataSource dataSource, String dataSourceName) throws Exception {
-    dataSource.preRegister(
-        ManagementFactory.getPlatformMBeanServer(),
-        new ObjectName("io.opentelemetry.db:name=" + dataSourceName));
+  protected void configure(BasicDataSource dataSource, String dataSourceName) {
+    dataSource.setJmxName("org.apache.commons.dbcp2:type=BasicDataSource,name=" + dataSourceName);
   }
 
   @Override
   protected void shutdown(BasicDataSource dataSource) {
     dataSource.postDeregister();
   }
+
+  @Test
+  void shouldUseJdbcUrlForDataSourceNameWhenJmxNameIsNull() throws Exception {
+    BasicDataSource dataSource = createDataSource();
+    dataSource.setUrl("jdbc:postgresql://db.example:5432/orders");
+
+    assertDataSourceName(dataSource, "db.example:5432/orders");
+  }
+
+  @Test
+  void shouldUseConnectionPropertiesForDataSourceNameWhenJmxNameIsInvalid() throws Exception {
+    BasicDataSource dataSource = createDataSource();
+    dataSource.setJmxName("invalid-jmx-name");
+    dataSource.setUrl("jdbc:postgresql:ignored");
+    dataSource.addConnectionProperty("serverName", "properties.example");
+    dataSource.addConnectionProperty("portNumber", "5433");
+    dataSource.addConnectionProperty("databaseName", "inventory");
+
+    assertDataSourceName(dataSource, "properties.example:5433/inventory");
+  }
+
+  @Test
+  void shouldUseServerAddressWhenPortAndNamespaceAreMissing() throws Exception {
+    BasicDataSource dataSource = createDataSource();
+    dataSource.setUrl("jdbc:custom:ignored");
+    dataSource.addConnectionProperty("serverName", "address-only.example");
+
+    assertDataSourceName(dataSource, "address-only.example");
+  }
+
+  @Test
+  void shouldUseDbNamespaceWhenServerAddressIsMissing() throws Exception {
+    BasicDataSource dataSource = createDataSource();
+    dataSource.setUrl("jdbc:h2:mem:orders");
+
+    assertDataSourceName(dataSource, "orders");
+  }
+
+  @Test
+  void shouldUseFixedDataSourceNameWhenServerAddressAndNamespaceAreMissing() throws Exception {
+    BasicDataSource dataSource = createDataSource();
+
+    assertDataSourceName(dataSource, "apache-dbcp2");
+  }
+
+  @Test
+  void shouldPreferJmxNameOverRegisteredJmxName() throws Exception {
+    BasicDataSource dataSource = createDataSource();
+    dataSource.setJmxName("org.apache.commons.dbcp2:type=BasicDataSource,name=configuredPool");
+
+    ObjectName objectName =
+        new ObjectName("org.apache.commons.dbcp2:type=BasicDataSource,name=registeredPool");
+    MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer();
+    mbeanServer.registerMBean(dataSource, objectName);
+
+    try {
+      dataSource.getConnection().close();
+
+      assertDataSourceMetrics("configuredPool");
+    } finally {
+      dataSource.close();
+      if (mbeanServer.isRegistered(objectName)) {
+        mbeanServer.unregisterMBean(objectName);
+      }
+      shutdown(dataSource);
+    }
+
+    assertNoMetrics();
+  }
+
+  @Test
+  void shouldUseRegisteredJmxNameWhenJmxNameIsNull() throws Exception {
+    BasicDataSource dataSource = createDataSource();
+
+    ObjectName objectName =
+        new ObjectName("org.apache.commons.dbcp2:type=BasicDataSource,name=registeredPoolFallback");
+    MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer();
+    mbeanServer.registerMBean(dataSource, objectName);
+
+    try {
+      dataSource.getConnection().close();
+
+      assertDataSourceMetrics("registeredPoolFallback");
+    } finally {
+      dataSource.close();
+      if (mbeanServer.isRegistered(objectName)) {
+        mbeanServer.unregisterMBean(objectName);
+      }
+      shutdown(dataSource);
+    }
+
+    assertNoMetrics();
+  }
+
+  private void assertDataSourceName(BasicDataSource dataSource, String dataSourceName)
+      throws Exception {
+    try {
+      dataSource.getConnection().close();
+
+      assertDataSourceMetrics(dataSourceName);
+    } finally {
+      dataSource.close();
+      shutdown(dataSource);
+    }
+
+    assertNoMetrics();
+  }
 }

instrumentation/apache-dbcp-2.0/metadata.yaml

@@ -2,10 +2,13 @@ display_name: Apache DBCP
 description: >
   This instrumentation enables database connection pools metrics for Apache DBCP.
 
-  The instrumentation uses `MBeanRegistration` methods for lifecycle detection, therefore it
-  only activates if the `BasicDataSource` is registered to an `MBeanServer`. If using Spring Boot,
-  this happens automatically as all Spring beans that support JMX registration are automatically
-  registered by default.
+  The auto-instrumentation registers metrics after `BasicDataSource` completes
+  `startPoolMaintenance()`, when the connection pool has been initialized. It unregisters metrics
+  when `BasicDataSource` is closed or deregistered. JMX registration is not required. When a JMX
+  `ObjectName` is available, its `name` property is used as the pool name; if the property is
+  absent, the full `ObjectName` is used. Otherwise, the JDBC URL and connection properties are
+  used to derive `server.address[:server.port][/db.namespace]`. If neither server address nor
+  database namespace is available, `apache-dbcp2` is used.
 library_link: https://commons.apache.org/proper/commons-dbcp/
 semantic_conventions:
   - DATABASE_POOL_METRICS

instrumentation/apache-dbcp-2.0/testing/src/main/java/io/opentelemetry/instrumentation/apachedbcp/AbstractApacheDbcpInstrumentationTest.java

@@ -37,36 +37,46 @@ protected abstract void configure(BasicDataSource dataSource, String dataSourceN
 
   @Test
   void shouldReportMetrics() throws Exception {
-    // given
+    String dataSourceName = "dataSourceName";
+    BasicDataSource dataSource = createDataSource();
+    try {
+      configure(dataSource, dataSourceName);
+
+      dataSource.getConnection().close();
+
+      assertDataSourceMetrics(dataSourceName);
+    } finally {
+      dataSource.close();
+      shutdown(dataSource);
+    }
+
+    assertNoMetrics();
+  }
+
+  protected BasicDataSource createDataSource() throws Exception {
     when(driverMock.connect(any(), any())).thenReturn(connectionMock);
     when(connectionMock.isValid(anyInt())).thenReturn(true);
 
-    String dataSourceName = "dataSourceName";
     BasicDataSource dataSource = new BasicDataSource();
     dataSource.setDriver(driverMock);
     dataSource.setUrl("db:///url");
     dataSource.postDeregister();
-    configure(dataSource, dataSourceName);
-
-    // when
-    dataSource.getConnection().close();
+    return dataSource;
+  }
 
-    // then
+  protected void assertDataSourceMetrics(String dataSourceName) {
     DbConnectionPoolMetricsAssertions.create(testing(), INSTRUMENTATION_NAME, dataSourceName)
         .disableConnectionTimeouts()
         .disableCreateTime()
         .disableWaitTime()
         .disableUseTime()
         .disablePendingRequests()
         .assertConnectionPoolEmitsMetrics();
+  }
 
-    // when
-    dataSource.close();
-    shutdown(dataSource);
-
+  protected void assertNoMetrics() {
     testing().clearData();
 
-    // then
     await()
         .untilAsserted(
             () ->