Hikaricp: Provide a cleaner way to check if a `MetricsTrackerFactory` can be provided

[snicoll]

HikariCP version: 2.7.7

If one wants to configure a MetricsTrackerFactory this is the pseudo-code they have to write

if (dataSource.getMetricRegistry() == null
        && dataSource.getMetricsTrackerFactory() == null) {
    dataSource.setMetricsTrackerFactory(
            new MicrometerMetricsTrackerFactory(registry));
}

Calling two getters and check for null is a bit weird. Can't we have something where that logic is abstracted away?

[brettwooldridge]

@snicoll I'm not at all happy with the current implemention of the "sealed" configuration introduced in v2.7.6. It's weird and has a code smell. I'll starting looking at it right away.

[snicoll]

@brettwooldridge thank you. Did you mean to write that comment on #1095 by any chance? I am not sure this particular issue is related to that feature.

[brettwooldridge]

@snicoll Oh, sorry. But do I think they are somewhat related. I believe that setting the metric registry is currently broken, because of the sealed configuration, if the HikariDataSource is constructed from a HikariConfig rather than from the no-arg constructor...

[brettwooldridge]

@snicoll As I noted in #1095, the change in v2.7.7 also broke special behavior for the metricRegistry and metricsTrackerFactory settings. We allow those to be set one time, whether the pool has started or not.

v2.7.7, by sealing the configuration upon HikariDataSource construction, broke this use-case.

[snicoll]

We allow those to be set one time

That doesn't matter if I have to check for two getters. I am auto-configuring thing so if the user has set a factory, I still need to check if one is is set. If you're saying that in a future release, all I have to do is to check one getter and have the guarantee the call to the setter works, then I am happy.

[brettwooldridge]

To be clear, the behavior with the latest commit is as it was. Are you asking for additional behavior on top of what was there?

To be honest, the whole ability to “set once even though the pool has started” hack was added because of Spring. Now an additional method is requested?

I guess I would flip the script. I would prefer to take out the existing hack. Is it too much to ask that the DataSource not be constructed until all of the contributing configuration is known?

[snicoll]

I lost you. I don't know what you mean by "because of Spring".

Please look at my original description. If you have changed code since I reported the issue, can you please link the commit because I don't understand you.

To rephrase: to check if I can set a MetricsTrackerFactory, I have to invoke two methods. Why do I have to know about that stuff? I am ok to check one method that will indicate whether I am allowed to set a MetricsTrackerFactory though the whole business of "you can only call a setter once" looks very weird to me.

Is it too much to ask that the DataSource not be constructed until all of the contributing configuration is known?

Yes. The component that creates the datasource doesn't know _anything_ about micrometer. Why do we have to have a centralized place where every single feature that Hikari offers is known. That doesn't sound very sane.

[brettwooldridge]

@snicoll The original code -- that allows the MetricsTracker to be set after the pool is initialized -- was added "because of Spring". As reported in #279, "...we are writing a spring module which "injects" metrics if HikariDataSources are found. We would be autowiring the datasources and if they are HikariDataSource instances, set the MetricsTracker accordingly. However, we have a bit of a catch-22...".

Simply stated, and you probably know better than I, in any dependency injection framework the order of dependency resolution is important. That's kindof the point of an IoC framework. I can't subscribe to an event broker before the event broker is initialized. If the event broker is backed by a DataSource, the event broker cannot initialize until the DataSource has initialized, etc. I thought that kind of order-of-dependency resolution was Spring's forte? I come from the OSGi world, where such auto-wiring (in correct order) is just par for the course.

There doesn't need to be any kind of centralized place per se, where every single feature of HikariCP is known, but clearly there is some code some place that knows about both Metrics and HikariCP because that code was failing due to the changes in v2.7.7. I fail to see why that code doesn't, or cannot, depend on Metrics and HikariCP as pre-requisites for execution. Maybe Spring just can't do it?

In OSGi, Metrics in this case would be an "optional" dependency. If Metrics was present in the configuration, the code that initializes HikariCP would wait until Metrics had initialized/started, at which point it would have everything it needed to proceed.

Am I missing something about Spring's auto-wiring capabilities?

[snicoll]

Am I missing something about Spring's auto-wiring capabilities?

I don't know but how is that relevant to the issue I've raised? I didn't raise #279 but IMO it defines a perfectly legit problem. You have a component that creates the DataSource and another component that would like to enable an additional feature on it (reporting of metrics). Requiring every use case to converge in a single place is not realistic IMO and I don't think it has anything to do with IoC or Spring for that matter.

If another mechanism has to be used to update the pool, so be it. But I'd argue that:

1. Users should get a fair warning with a dedicated version (Hikari 3.0 M1 or something)
2. Hikari should offer an easy way to do that programmatically
3. If calling the existing setter is not what you want, then they shouldn't be so visible as they are now.

With all that said, let's say that stuff should be set at the same time as the url, the name of the pool or whatever. Why do I have to check two methods to figure out if the MetricsTrackerFactory is set? This is the only question that I have and I don't have a feeling it is being answered.

Thank you.

[wilkinsona]

I was evidently writing the below while @snicoll was also commenting. We work on the Boot team together but disagree here.

Injecting a dependency and then further configuring it (as the code described in #279 was apparently doing) isn't a good idea for exactly this sort of reason. Spring Framework provides a BeanPostProcessor abstraction for this situation. It allows further configuration of a bean before it's used. That's what we're using in Spring Boot 2.0 to configure the metrics tracker factory before anyone uses the DataSource.

Things are a little more complicated for us as the user may have already configured a metrics tracker factory or metrics tracker registry when they defined the bean. In that case, our post-processor needs to back off. I think all this issue is really about is making it a bit easier to check whether or not we can set a metrics tracker factory on the DataSource.

[brettwooldridge]

@snicoll It should be noted that HikariCP takes a performance hit for providing this capability.

If the pool configuration is known, in its entirity, at start time then the IMetricsTracker reference could be final, and therefore monomorphic.

However, because the pool must start, and the IMetricsTracker be set at any point later, then the pool must start with a no-op implementation. Setting the IMetricsTracker later forces the JVM to evict the JITed code, and inflates all of the callsites to bi-morphic sites. This roughly doubles the method dispatch overhead.

[snicoll]

@brettwooldridge we're using a BPP (as we should) so please don't hold "Spring" responsible for that change and feel free to adapt to whatever you feel necessary.

[brettwooldridge]

@snicoll @wilkinsona I'm sorry if my feedback seemed tangential. I'll try to summarize it:

1/ As far as I can tell, Spring's initialization behavior is the only use-case I have that justifies the current behavior of allowing the IMetricsTracker to be set after pool initialization.

2/ The request to "provide a cleaner way" to check if initialization is possible rubbed me the wrong way. I apologize. In my mind it feels like, "this is kinda hacky, can you make it cleaner?" At which point I want to jump up and down and point at 1/.

Essentially, there would be no need to make anything cleaner if the hack that we put in place to accomodate Spring could be removed.

The break caused by v2.7.7 was not intentional. I simply forgot about the Spring hack that was put in 3 years ago. This is now corrected, and a unit test added to ensure it does not happen again.

However, I will express again my desire that Spring use its own capability to simply ensure that the pre-requistites for initializing HikariCP are resolved upfront, which would enable us to remove this hack (and regain a small but measurable bit of performance).

[wilkinsona]

However, I will express again my desire that Spring use its own capability to simply ensure that the pre-requistites for initializing HikariCP are resolved upfront, which would enable us to remove this hack (and regain a small but measurable bit of performance).

That sounds like a good goal and I hope we can do our bit in Spring Boot to help. I think we should start thinking of Hikari as using the builder pattern. By that I mean that we could create a HikariConfig instance as the "builder" for the HikariDataSource. The config could then be customised by other components in Spring Boot (for setting up metrics, for example) as well as by users. Once we know that customisation is finished we can then use the HikariConfig instance to create a HikariDataSource.

[brettwooldridge]

@wilkinsona In a perfect world, the pool configuration would be sealed, meaning no changes once the pool is initialized. The only exceptions being the properties that are changable via the HikariConfigMXbean interface.

In that world, the question would simply be, "Has the pool initialized or hasn't it?"

This is the point that I would like to get to. To be honest, and not trying to be mean, really, it should be a little bit painful to use the hack that exists. 😁 Otherwise, what is the motivation to refactor?

[brettwooldridge]

@wilkinsona It's a shame that Valentine's Day has passed, otherwise I'd send you a box of chocolates. 💝

[brettwooldridge]

@snicoll @wilkinsona HikariCP v2.7.8 has been published to the Sonatype repository. It is available directly there now, or from the central repository in several hours.

[brettwooldridge]

SpaceX launch in 7 minutes. 👍