Use AOP proxy for DataSource telemetry to preserve bean type

[dngyukm]

This PR changes the DataSource wrapping approach to fix #13512.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: otelbot[bot] (144eb06)
• ✅ login: dngyukm (ce342d8)

[otelbot-java-instrumentation]

🔧 The result from spotlessApply was committed to the PR branch.

[laurit]

does this work when application contains multiple data sources?

[dngyukm]

The approach using DataSourcePostProcessor seems to pose challenges in resolving related issues due to the bean lifecycle, and it appears that automatic handling of multiple data sources may not be feasible with this method. Would there happen to be a better alternative?

[laurit]

The approach using DataSourcePostProcessor seems to pose challenges in resolving related issues due to the bean lifecycle, and it appears that automatic handling of multiple data sources may not be feasible with this method. Would there happen to be a better alternative?

I'm sure there is. For example could try something like

diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/jdbc/DataSourcePostProcessor.java b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/i
nstrumentation/spring/autoconfigure/internal/instrumentation/jdbc/DataSourcePostProcessor.java
index 2435fbc2ba..58268f3523 100644
--- a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/jdbc/DataSourcePostProcessor.java
+++ b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/jdbc/DataSourcePostProcessor.java
@@ -10,8 +10,14 @@ import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.instrumentation.jdbc.datasource.JdbcTelemetry;
 import io.opentelemetry.instrumentation.spring.autoconfigure.internal.properties.InstrumentationConfigUtil;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import javax.sql.DataSource;
+import org.aopalliance.intercept.MethodInterceptor;
+import org.aopalliance.intercept.MethodInvocation;
+import org.springframework.aop.framework.ProxyFactory;
 import org.springframework.aop.scope.ScopedProxyUtils;
+import org.springframework.aop.support.AopUtils;
 import org.springframework.beans.factory.ObjectProvider;
 import org.springframework.beans.factory.config.BeanPostProcessor;
 import org.springframework.core.Ordered;
@@ -50,7 +56,7 @@ final class DataSourcePostProcessor implements BeanPostProcessor, Ordered {
         && !isRoutingDatasource(bean)
         && !ScopedProxyUtils.isScopedTarget(beanName)) {
       DataSource dataSource = (DataSource) bean;
-      return JdbcTelemetry.builder(openTelemetryProvider.getObject())
+      DataSource wrapped = JdbcTelemetry.builder(openTelemetryProvider.getObject())
           .setStatementSanitizationEnabled(
               InstrumentationConfigUtil.isStatementSanitizationEnabled(
                   configPropertiesProvider.getObject(),
@@ -66,6 +72,21 @@ final class DataSourcePostProcessor implements BeanPostProcessor, Ordered {
                   .getBoolean("otel.instrumentation.jdbc.experimental.transaction.enabled", false))
           .build()
           .wrap(dataSource);
+
+      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());
+            }
+          });
+
+      return proxyFactory.getProxy();
     }
     return bean;
   }

I didn't test it with the original issue so can't be sure that it will help. The idea is that instead of returning our wrapped datasource we'll return a proxy that unwraps to the original datasource but delegates invocation to our wrapped datasource. This could work if whatever does the reconfiguration is smart enough to unwrap aop proxies, which it probably is. Note that there may be a better way to implement this.

[laurit]

Should use new ProxyFactory(new Class<?>[] {DataSource.class}); instead of new ProxyFactory(DataSource.class);
@zeitlinger tagging you in case you wish to pick this up

[zeitlinger]

@jeanbisutti I think you worked on this bit before - do you want to pick that up?

[dngyukm]

@laurit I’ve applied the changes using the method you suggested. Thank you!

[otelbot-java-instrumentation]

🔧 The result from spotlessApply was committed to the PR branch.

[jeanbisutti]

Perhaps it may be worth to try to reproduce #13512 by refreshing beans, to be sure that the change fixes the problem.

It may have something specific to do in the case of an HikariDataSource (used in #13512):

https://docs.spring.io/spring-cloud-commons/reference/spring-cloud-commons/application-context-services.html#refresh-scope

In the initial implementation, I have tried to avoid the usage of a proxy to ease the GraalVM native compatibility.

It seems that the new implementation needs more GraalVM hints to make it work on the native mode:

 org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'dataSource': The program tried to reflectively access the proxy class inheriting
   [javax.sql.DataSource, org.springframework.aop.SpringProxy, org.springframework.aop.framework.Advised, org.springframework.core.DecoratingProxy]
without it being registered for runtime reflection. Add [javax.sql.DataSource, org.springframework.aop.SpringProxy, org.springframework.aop.framework.Advised, org.springframework.core.DecoratingProxy] to the dynamic-proxy metadata to solve this problem. Note: The order of interfaces used to create proxies matters. See https://www.graalvm.org/latest/reference-manual/native-image/metadata/#dynamic-proxy for help.

@dngyukm If you could reproduce #13512 error, I could try to have a look.

[otelbot-java-instrumentation]

🔧 The result from spotlessApply was committed to the PR branch.

[laurit]

In the initial implementation, I have tried to avoid the usage of a proxy to ease the GraalVM native compatibility.

I think we can replace the proxy with a custom class that looks like a proxy to spring but really is a plain java class. That class could implement the interfaces that are needed for unwrapping and be named so that spring believes that it could be a bytecode proxy.