jdbc-observations / datasource-micrometer

Micrometer Observation API instrumentation and Spring Boot 3 Auto-Configuration for JDBC APIs
Apache License 2.0
58 stars 9 forks source link

Missing default jdbc.connections.* Spring Boot metrics #46

Closed stefano-salmaso closed 12 hours ago

stefano-salmaso commented 2 months ago

Hi all, using Spring Boot you have metrics about connections like

jdbc.connections.min
jdbc.connections.max
jdbc.connections.idle
jdbc.connections.active

When tracing is enabled we are creating a ProxyDataSourceBuilderCustomizer to have tracing info. Doing this we have the new metrics (jdbc.conection, jdbc.query, jdbc.result-set ...) but the metrics created by Spring Boot disappears!

Is there a way to get previous metrics as well when using datasource-micrometer?

Thanks

ttddyy commented 2 months ago

Hi @stefano-salmaso

Looking at the Spring Boot code, DataSourcePoolMetricsAutoConfiguration seems to be the one that registers the DataSourcePoolMetrics binder. Specifically here.

I don't see it would interfere with DataSourceObservationBeanPostProcessor which creates a proxy from datasource-micrometer.

Can you try to debug the behavior on the DataSourcePoolMetricsAutoConfiguration on your app to see whether it is registering the binders correctly??

stefano-salmaso commented 2 months ago

Hi @ttddyy I think the problem is related to the fact you are using a proxy. Here the metadataProviderreturn null passing the proxy... but return the value using real datasource. Going deeper.. here the function DataSourceUnwrapper.unwrap is not able to return the datasource. This method tries in multiple way to obtain the datasource... but with ProxyDataSource it seems not working...

stefano-salmaso commented 2 months ago

I found a workaround... adding this implementation of DataSourcePoolMetadataProvider as a Bean solve the issue

    @Bean
    DataSourcePoolMetadataProvider proxyDatasourceTomcatPoolDataSourceMetadataProvider() {
        return dataSource -> {
            if(dataSource instanceof ProxyDataSource) {
                org.apache.tomcat.jdbc.pool.DataSource tomcatDataSource = DataSourceUnwrapper.unwrap(((ProxyDataSource) dataSource).getDataSource(), ConnectionPoolMBean.class, org.apache.tomcat.jdbc.pool.DataSource.class);
                return tomcatDataSource != null ? new TomcatDataSourcePoolMetadata(tomcatDataSource) : null;
            }
            return null;
        };
    }

I didn't check if we have issue Hikari CP and, also, this implementation needs org.apache.tomcat:tomcat-jdbc dependency

ttddyy commented 12 hours ago

After more digging, it's a bug in tomcat-jdbc.

The org.apache.tomcat.jdbc.pool.DataSource always returns false for isWrapperFor and null for unwrap methods: https://github.com/apache/tomcat/blob/main/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java#L62-L73

I see there is a bug filed in 2016 but I don't see it has anything addressed. https://bz.apache.org/bugzilla/show_bug.cgi?id=59569

The reason why DataSourceUnwrapper.unwrap on org.apache.tomcat.jdbc.pool.DataSource works is that it checks the instance itself: https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DataSourceUnwrapper.java#L57-L59 So, essentially, it does datasource instanceof ConnectionPoolMBean, which returns true and returns the tomcat datasource. However, in this case, it should also return true for datasource.isWrapperFor(ConnectionPoolMBean.class), yet it returns false.

I think the suggested workaround in the above comment is a good approach for now.

Since the root cause is in tomcat-jdbc and cannot be resolved by datasource-proxy or datasource-micrometer, I am closing this issue.