Fix Nacos/Spring Boot config refresh error by proxying instrumented DataSource

Author: Rajeswari

instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/jdbc/DataSourcePostProcessor.java

@@ -46,46 +46,65 @@ private static boolean isRoutingDatasource(Object bean) {
   }
 
   @CanIgnoreReturnValue
-  @Override
-  public Object postProcessAfterInitialization(Object bean, String beanName) {
-    // Exclude scoped proxy beans to avoid double wrapping
-    if (bean instanceof DataSource
-        && !isRoutingDatasource(bean)
-        && !ScopedProxyUtils.isScopedTarget(beanName)) {
-      DataSource dataSource = (DataSource) bean;
-      DataSource wrapped = JdbcTelemetry.builder(openTelemetryProvider.getObject())
-          .setStatementSanitizationEnabled(
-              InstrumentationConfigUtil.isStatementSanitizationEnabled(
-                  configPropertiesProvider.getObject(),
-                  "otel.instrumentation.jdbc.statement-sanitizer.enabled"))
-          .setCaptureQueryParameters(
-              configPropertiesProvider
-                  .getObject()
-                  .getBoolean(
-                      "otel.instrumentation.jdbc.experimental.capture-query-parameters", false))
-          .setTransactionInstrumenterEnabled(
-              configPropertiesProvider
-                  .getObject()
-                  .getBoolean("otel.instrumentation.jdbc.experimental.transaction.enabled", false))
-          .build()
-          .wrap(dataSource);
+@Override
+public Object postProcessAfterInitialization(Object bean, String beanName) {
+  // Exclude scoped proxy beans to avoid double wrapping
+  if (bean instanceof DataSource
+      && !isRoutingDatasource(bean)
+      && !ScopedProxyUtils.isScopedTarget(beanName)) {
+    DataSource dataSource = (DataSource) bean;
 
-      ProxyFactory proxyFactory = new ProxyFactory(DataSource.class);
-      proxyFactory.setTarget(bean);
-      proxyFactory.addAdvice(
-          new MethodInterceptor() {
-            @Nullable
-            @Override
-            public Object invoke(@Nonnull MethodInvocation invocation) throws Throwable {
-              return AopUtils.invokeJoinpointUsingReflection(
-                  wrapped, invocation.getMethod(), invocation.getArguments());
-            }
-          });
+    // Wrap the original DataSource with OpenTelemetry instrumentation
+    DataSource wrapped = JdbcTelemetry.builder(openTelemetryProvider.getObject())
+        .setStatementSanitizationEnabled(
+            InstrumentationConfigUtil.isStatementSanitizationEnabled(
+                configPropertiesProvider.getObject(),
+                "otel.instrumentation.jdbc.statement-sanitizer.enabled"))
+        .setCaptureQueryParameters(
+            configPropertiesProvider
+                .getObject()
+                .getBoolean(
+                    "otel.instrumentation.jdbc.experimental.capture-query-parameters", false))
+        .setTransactionInstrumenterEnabled(
+            configPropertiesProvider
+                .getObject()
+                .getBoolean("otel.instrumentation.jdbc.experimental.transaction.enabled", false))
+        .build()
+        .wrap(dataSource);
 
-      return proxyFactory.getProxy();
-    }
-    return bean;
+    /**
+     * Spring Boot's configuration binding and rebinding mechanisms (such as those triggered by
+     * Nacos configuration refresh) may attempt to reconstruct beans using their concrete class
+     * constructors. If a custom DataSource implementation (such as OpenTelemetryDataSource)
+     * is returned directly, Spring may not find a suitable constructor during rebinding, resulting
+     * in errors like "ExistingValue must be an instance of com.zaxxer.hikari.HikariDataSource".
+     *
+     * To prevent this, we create a JDK dynamic proxy implementing only the DataSource interface.
+     * The proxy delegates all method calls to the wrapped (instrumented) DataSource.
+     * This approach "hides" the actual implementation class and ensures that Spring interacts only
+     * with the DataSource interface, avoiding issues related to constructor resolution or type casting
+     * during bean rebinding.
+     */
+    ProxyFactory proxyFactory = new ProxyFactory(DataSource.class);
+    // Set the original bean as the target (important for AOP and bean lifecycle)
+    proxyFactory.setTarget(bean);
+    // Delegate all method calls to the wrapped, instrumented DataSource
+    proxyFactory.addAdvice(
+        new MethodInterceptor() {
+          @Nullable
+          @Override
+          public Object invoke(@Nonnull MethodInvocation invocation) throws Throwable {
+            return AopUtils.invokeJoinpointUsingReflection(
+                wrapped, invocation.getMethod(), invocation.getArguments());
+          }
+        });
+
+    // Return the proxy instead of the instrumented DataSource instance
+    // This ensures proper interaction with Spring's bean lifecycle and rebinding
+    return proxyFactory.getProxy();
   }
+  return bean;
+}
 
   // To be one of the first bean post-processors to be executed
   @Override

instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/jdbc/DataSourcePostProcessorContextTest.java

@@ -0,0 +1,66 @@
+package io.opentelemetry.instrumentation.spring.autoconfigure.internal.instrumentation.jdbc;
+
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.ObjectProvider;
+import org.springframework.boot.test.context.runner.ApplicationContextRunner;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.*;
+
+class DataSourcePostProcessorTest {
+
+  @Test
+  void shouldWrapDataSourceWithProxy() throws Exception {
+    // Set up mocks
+    DataSource original = mock(DataSource.class);
+    Connection mockConn = mock(Connection.class);
+    when(original.getConnection()).thenReturn(mockConn);
+
+    // Create mocks for OpenTelemetry and ConfigProperties
+    OpenTelemetry openTelemetry = mock(OpenTelemetry.class);
+    ConfigProperties configProperties = mock(ConfigProperties.class);
+    when(configProperties.getBoolean(anyString(), anyBoolean())).thenReturn(false);
+
+    // Mock JdbcTelemetry builder chain to return a mock DataSource as 'wrapped'
+    JdbcTelemetry.Builder builder = mock(JdbcTelemetry.Builder.class, RETURNS_SELF);
+    JdbcTelemetry telemetry = mock(JdbcTelemetry.class);
+    DataSource wrapped = mock(DataSource.class);
+    when(wrapped.getConnection()).thenReturn(mockConn);
+    when(telemetry.wrap(any(DataSource.class))).thenReturn(wrapped);
+    when(builder.build()).thenReturn(telemetry);
+
+    // Mock the static builder method (requires mockito-inline or PowerMockito)
+    try (var mocked = Mockito.mockStatic(JdbcTelemetry.class)) {
+      mocked.when(() -> JdbcTelemetry.builder(any())).thenReturn(builder);
+
+      ObjectProvider<OpenTelemetry> openTelemetryProvider = () -> openTelemetry;
+      ObjectProvider<ConfigProperties> configPropertiesProvider = () -> configProperties;
+
+      DataSourcePostProcessor postProcessor = new DataSourcePostProcessor(
+          openTelemetryProvider, configPropertiesProvider
+      );
+
+      // Act
+      Object processed = postProcessor.postProcessAfterInitialization(original, "myDataSource");
+
+      // Assert
+      assertThat(processed).isInstanceOf(DataSource.class);
+      assertThat(processed).isNotSameAs(original);
+
+      // The proxy should delegate calls to the wrapped DataSource
+      DataSource proxied = (DataSource) processed;
+      Connection proxiedConnection = proxied.getConnection();
+      assertThat(proxiedConnection).isEqualTo(mockConn);
+
+      // The original DataSource should NOT be called directly by the proxy
+      verify(original, never()).getConnection();
+      // The wrapped DataSource should be called
+      verify(wrapped, times(1)).getConnection();
+    }
+  }
+}