Callbacks for Connection initialization

[tippl]

As discussed on the mailing list, implements a callback for Connection initialization steps.
Useful for collection of metrics on how long each step takes for network troubleshooting.

[ok2c]

@tippl I apologize if I have said was misleading but the idea was to provide hooks for performance metrics without exposing the whole thing in the public APIs.

1. Please add those callback methods as protected empty methods to the DefaultHttpClientConnectionOperator. One would need to subclass DefaultHttpClientConnectionOperator and override relevant methods in order to add custom logic
2. Symmetric changes need to be made DefaultHttpClientConnectionOperator. We must keep the classic and the async client implementation consistent.

[tippl]

@tippl I apologize if I have said was misleading but the idea was to provide hooks for performance metrics without exposing the whole thing in the public APIs.

1. Please add those callback methods as protected empty methods to the DefaultHttpClientConnectionOperator. One would need to subclass DefaultHttpClientConnectionOperator and override relevant methods in order to add custom logic
2. Symmetric changes need to be made DefaultHttpClientConnectionOperator. We must keep the classic and the async client implementation consistent.

@ok2c
Okay, i do have a few things then:

1. While keeping the PoolingHttpClientConnectionManager constructor internal, should I add a way to provide a custom HttpClientConnectionOperator to the PoolingHttpClientConnectionManager?
2. The DefaultAsyncClientConnectionOperator is final, i'll open it up to extending.
3. There is no "easy" way at least from first look to hook around DNS resolution in the DefaultAsyncClientConnectionOperator, since that is inside of the MultihomeIOSessionRequester. But you can work around that by implementing your own DnsResolver and have that handle metrics, so I'll remove the callbacks for DNS.

If you're okay with these, I'll work on implementing the changes.
Thanks.

[ok2c]

1. While keeping the `PoolingHttpClientConnectionManager` constructor internal, should I add a way to provide a custom `HttpClientConnectionOperator` to the `PoolingHttpClientConnectionManager`?

@tippl Do you mean PoolingHttpClientConnectionManager builder? if so, I would rather not couple it with an internal interface..

2. The `DefaultAsyncClientConnectionOperator` is final, i'll open it up to extending.

Sure.

3. There is no "easy" way at least from first look to hook around DNS resolution in the `DefaultAsyncClientConnectionOperator`, since that is inside of the `MultihomeIOSessionRequester`. But you can work around that by implementing your own `DnsResolver` and have that handle metrics, so I'll remove the callbacks for DNS.

Sure thing. Makes sense.

[tippl]

@tippl Do you mean PoolingHttpClientConnectionManager builder? if so, I would rather not couple it with an internal interface..

@ok2c

As it stands now, ignoring the deprecated constructors of PoolingHttpClientConnectionManager, it only has a default constructor with no parameters and 2 "internal" protected constructors that allow you to set a HttpClientConnectionOperator. (these are already used by the builder)

So there is no way to actually create a PoolingHttpClientConnectionManager with a custom HttpClientConnectionOperator, either by itself or through the Builder.

In the mail thread, you suggested making the protected constructor public, but I feel rather adding HttpClientConnectionOperator to the builder and keeping the internal constructors as they are without changing the visibility. Since besides the absolute basic default constructor, the intent seems to be to mainly create it through the builder.

So if you specify a HttpClientConnectionOperator in the builder, that is used, otherwise a default is created. This feels like a better solution than having to extend/reimplement parts of the PoolingHttpClientConnectionManager along with the builder to be able to use it with a custom Operator.

This is what i meant.

[ok2c]

So there is no way to actually create a PoolingHttpClientConnectionManager with a custom HttpClientConnectionOperator, either by itself or through the Builder.

One can subclass PoolingHttpClientConnectionManager and pass a custom HttpClientConnectionOperator through the protected super constructor.

[tippl]

One can subclass PoolingHttpClientConnectionManager and pass a custom HttpClientConnectionOperator through the protected super constructor.

Is there a reason for not being able to add the HttpClientConnectionOperator to the builder?

To me it feels counterproductive. Past several releases of this library have been focused on making the Builder be the main/only way to create a PoolingHttpClientConnectionManager. Adding an useful feature that is locked behind not being able to use the Builder at all, having to extend the PoolingHttpClientConnectionManager, and then configuring it all manually without the builder feels like a step back.

Especially since the only reason for extending in this case would be to access a protected constructor. If this is a feature the library provides, an user shouldn't use workarounds to use it at all.

[ok2c]

Is there a reason for not being able to add the HttpClientConnectionOperator to the builder?

@tippl The are two reasons:

1. Internal interfaces ideally should not leak into the public APIs. There is a few instances where it was necessary but I am reluctant to create more
2. HttpClientConnectionOperator would be mutually exclusive with several attributes of the builder: SchemePortResolver, DnsResolver and TlsSocketStrategy. That is just bad.

Especially since the only reason for extending in this case would be to access a protected constructor. If this is a feature the library provides, an user shouldn't use workarounds to use it at all.

That is exactly the thing. This feature may be very important for your application but, dare I say, for majority of users it is completely irrelevant.

[tippl]

I've come across the idea for these metrics here micrometer-metrics/micrometer#1617, so I cannot say i'm the only one who wants these metrics, but the pool of people is certainly small. Which is why I wanted to make it fairly simple to implement into micrometer, so that I wouldn't have to make these ugly hacks myself and more people could use them.

Your point about conflicting attributes is valid though. I'll update the PR when i have time.

[tippl]

@ok2c Sorry for the delay.
Hopefully this is more to your liking.

I've tried to avoid the need of having to extend the PoolingHttpClientConnectionManager to use a custom Operator, so i made the constructor public as per what you suggested in the mail communication, however i've kept the Internal annotation to signify it shouldn't be used during regular use. (to still push people towards the builder)

I've also modified the bulilders to allow extending, this way even if you want to use the callbacks, you don't have to fully manually configure the PoolingHttpClientConnectionManager, allowing you to extend the builder and override just the build method to use the custom Operator.

[ok2c]

I've also modified the bulilders to allow extending

@tippl Everything looks good to me except for builders. Your change exposes every single internal implementation detail to the public APIs making it almost impossible to modify the classes without breaking the APIs. If you are absolutely sure that you must make those builders extendable, please provide an internal protected method instead. See internal protected methods in HttpClientBuilder as a reference.

[garydgregory]

Changing to protected instance variables are a very bad idea IMO: we are handcuffing ourselves to names, types and details that break encapsulation. A subclass should use methods to access what it needs. This gives us the most flexibility to change code and implementation in the future.

[tippl]

Hopefully this is how you meant it. :)

Yeah, exposing the private variables wasn't ideal, but all of this is kind of not ideal since it's trying to expose callbacks for api users without exposing the callbacks in the api and there's not much room to jam it in and not make creating the objects an absolute pain that would make no one want to use the callbacks. :(

I still feel that exposing a callback interface in the builder as i did the first time is the least intrusive, but i'll defer to the maintainers.

[tippl]

One more edit, making the Builder constructor not protected means you cannot extend it. Made an oversight when removing the protected there.