Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataSourceUnwrapper calls Wrapper.isWrapperFor with a interface rather than an class causing HikariDataSourceMetricsRegistry failure #24697

Closed
ankeway opened this issue Jan 8, 2021 · 9 comments
Assignees
Milestone

Comments

@ankeway
Copy link

@ankeway ankeway commented Jan 8, 2021

This problem causing hikariDataSource return null
because target.isInterface()

org.springframework.boot.actuate.autoconfigure.metrics.jdbc.DataSourcePoolMetricsAutoConfiguration.HikariDataSourceMetricsConfiguration.bindMetricsRegistryToHikariDataSources(Collection)

@Autowired
void bindMetricsRegistryToHikariDataSources(Collection dataSources) {
  for (DataSource dataSource : dataSources) {
    HikariDataSource hikariDataSource = DataSourceUnwrapper.unwrap(dataSource, HikariDataSource.class);
    if (hikariDataSource != null) {
        bindMetricsRegistryToHikariDataSource(hikariDataSource);
    }
  }
}
@ifrozenice
Copy link

@ifrozenice ifrozenice commented Jan 8, 2021

yes, in 2.3.7Version, there is no hikaricp metrics in actuator/prometheus endpoint。
in2.3.4Version, it's work well。

@philwebb
Copy link
Member

@philwebb philwebb commented Jan 8, 2021

@ankeway Can you provide some more details about your setup, or even better a sample application that replicates the issue. I'd like to know the DataSource class that gets passed to the unwrap method.

@ankeway
Copy link
Author

@ankeway ankeway commented Jan 12, 2021

@ankeway Can you provide some more details about your setup, or even better a sample application that replicates the issue. I'd like to know the DataSource class that gets passed to the unwrap method.

The most simple example that use HikariDataSource and Request actuator/prometheus endpoint
Both oracle and mysql, can not get the related data of DataSource
HikariDataSource hikariDataSource = DataSourceUnwrapper.unwrap(dataSource, HikariDataSource.class);
because hikariDataSource return null;
you can debug
org.springframework.boot.actuate.autoconfigure.metrics.jdbc.DataSourcePoolMetricsAutoConfiguration.HikariDataSourceMetricsConfiguration class, bindMetricsRegistryToHikariDataSources method
Spring Boot 2.3.6 don't have this problem, The only difference is target.isInterface()

@snicoll
Copy link
Member

@snicoll snicoll commented Jan 12, 2021

The most simple example that use HikariDataSource and Request actuator/prometheus endpoint

@ankeway unfortunately, that doesn't help. We're asking for a sample because we have several tests that exercise this scenario and they pass. As Phil has requested, can you please share a sample that showcases the problem? You can do so by attaching a zip to this issue or share a link to a GitHub repository.

@ankeway
Copy link
Author

@ankeway ankeway commented Jan 13, 2021

The most simple example that use HikariDataSource and Request actuator/prometheus endpoint

@ankeway unfortunately, that doesn't help. We're asking for a sample because we have several tests that exercise this scenario and they pass. As Phil has requested, can you please share a sample that showcases the problem? You can do so by attaching a zip to this issue or share a link to a GitHub repository.

https://github.com/ankeway/DsDemo.git

if add such as javamelody-spring-boot-starter , dataSource will become net.bull.javamelody.JdbcWrapper$DelegatingInvocationHandler
or like JdkDynamicAopProxy io.opentracing.contrib.spring.cloud.jdbc.JdbcAspect. a dynamic proxy,
so try to safeUnwrap JdbcWrapper or JdkDynamicAopProxy and all of them are not a isInterface
but spring boot 2.3.4, don't have target.isInterface(), DataSourceUnwrapper.unwrap(JdbcWrapper, DynamicRoutingDataSource.class) return DynamicRoutingDataSource;
spring boot 2.3.7 DataSourceUnwrapper.unwrap(JdbcWrapper, DynamicRoutingDataSource.class) return null;
I have to use custome DynamicRoutingDataSourceUnwrapper solve this problem

thanks

@snicoll
Copy link
Member

@snicoll snicoll commented Jan 13, 2021

@ankeway thanks for the sample although it does not reproduce the problem for me. The reference to net.bull.javamelody.JdbcWrapper$DelegatingInvocationHandler is helping though.

@snicoll
Copy link
Member

@snicoll snicoll commented Jan 13, 2021

We discussed this one today with two options:

  • Try to unwrap HikariConfigMXBean rather as it may expose everything that we need.
  • Handle the Oracle case explicitly and revert the interface check

If the first option works we'd have to report an issue against the HikariCP project to suggest an interface to be offered by the project for datasource unwrap needs like this one.

@snicoll snicoll self-assigned this Jan 13, 2021
snicoll added a commit to snicoll/spring-boot that referenced this issue Jan 14, 2021
This commit updates DataSourceUnwrapper to take a separate interface
type argument if the target datasource has to be unwrapped, given that
the target type is usually not an interface.

Closes spring-projectsgh-24697
@snicoll snicoll modified the milestones: 2.3.x, 2.3.8 Jan 14, 2021
@snicoll snicoll closed this in 283ed48 Jan 14, 2021
@oburgosm
Copy link

@oburgosm oburgosm commented Jan 27, 2021

But with the new implementation, call to DataSourceUnwrapper.unwrap with a class, for example DataSourceUnwrapper.unwrap(myds, AtomikosDataSourceBean.class), always return null.

As workaround I can call DataSourceUnwrapper.unwrap(myds, DataSource.class, AtomikosDataSourceBean.class), but I think it's a bug.

@wilkinsona
Copy link
Member

@wilkinsona wilkinsona commented Jan 27, 2021

Unfortunately, the contract for Wrapper.unwrap(Class<?>) states that the argument must be an interface. Prior to the change made in #24200 we were not adhering to the contract which caused problems with some DataSource implementations.

Using DataSourceUnwrapper.unwrap(DataSource, Class<I>, Class<T>) isn't a workaround and is what you should do when you want the result of unwrap to be a class rather than an interface. For Atomikos, I would call unwrap(myds, ConnectionPoolProperties.class, AtomikosDataSourceBean.class). This uses Atomikos's ConnectionPoolProperties interface to do the unwrapping and then casts it to AtomikosDataSourceBean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants