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

Supplying custom HttpClientBuilder to ApacheConnector #4210

Open
garypgithub opened this issue Jul 26, 2019 · 8 comments
Open

Supplying custom HttpClientBuilder to ApacheConnector #4210

garypgithub opened this issue Jul 26, 2019 · 8 comments

Comments

@garypgithub
Copy link

I am writing a java AWS lambda and using Jersey as my REST client. AWS X-Ray is automatically connected to the lambda and I would to use the AWS X-Ray instrumented version of HttpClientBuilder but the current implementation of ApacheConnector does not permit that. This issue is similar to #4089 but for Apache HttpComponents HttpClient.

I propose adding a new property to ApacheClientProperties such as CLIENT_BUILDER which could be set to an HttpClientBuilder implementation which must be a subclass of org.apache.http.impl.client.HttpClientBuilder.

Then, the code in ApacheConnector could be enhanced to replace the line reading

final HttpClientBuilder clientBuilder = HttpClientBuilder.create();

with something like

final HttpClientBuilder clientBuilder = getHttpClientBuilder();

where getHttpClientBuilder() looks like

    private HttpClientBuilder getHttpClientBuilder() {
        final Object cbObject = config.getProperties().get(ApacheClientProperties.CLIENT_BUILDER);

        if (cbObject != null) {
            if (cbObject instanceof HttpClientBuilder ) {
                return (HttpClientBuilder ) cbObject;
            } else {
                LOGGER.log(
                        Level.WARNING,
                        LocalizationMessages.IGNORING_VALUE_OF_PROPERTY(
                                ApacheClientProperties.CLIENT_BUILDER,
                                cbObject.getClass().getName(),
                                HttpClientBuilder.class.getName())
                );
            }
        }

        // Create standard HttpClientBuilder.
        return HttpClientBuilder.create();
    }
@jansupol
Copy link
Contributor

jansupol commented Sep 2, 2019

Relates to #4104.

The problem is that Apache HttpClientBuilder does not provide getters, so it is not possible to know what was configured in the HttpClientBuilder and what is yet to be configured. There would be needed some other approach.

@garypgithub
Copy link
Author

garypgithub commented Sep 2, 2019

Thanks, @jansupol, for your comments. I'm pretty much open to any approach that would allow me to get my own HttpClientBuilder into ApacheConnector. I understand that you wouldn't know what had been set and what hadn't on my HttpClientBuilder and that things could get out of sync and it seems to me that that's a risk that someone would take when supplying his or her own builder. That risk could be documented. They'd have to understand that ApacheConnector would be making calls on their builder in accordance with its properties. That's a risk I'm willing to take.

In the meantime, I've had to clone ApacheConnector to create my own MyApacheConnector (and MyApacheConnectorProvider) that uses Amazon's HttpClientBuilder so that I can build an HttpClient client that coordinates with AWS X-Ray. I guess that this is an appropriate approach as well although I won't be able to automatically benefit from any enhancements to ApacheConnector.

Also, please note that I am suggesting providing an HttpClientBuilder whereas #4089 was talking about providing an HttpClient. I do see your comments in #4104 that adding another property into ApacheClientProperties has its own scaling problems and I appreciate that but I think that's what properties are for. So do you advise that creating my own custom connector provider and connector is the best way to go? Thanks.

@jansupol
Copy link
Contributor

jansupol commented Sep 2, 2019

How about this one:

/**
 * A callback interface used to configure {@link org.apache.http.impl.client.HttpClientBuilder}. It is called immediately before
 * the {@link ApacheConnectorProvider} creates {@link org.apache.http.client.HttpClient}, after the
 * {@link org.apache.http.impl.client.HttpClientBuilder} is configured using the properties.
 */
@Contract
public interface ApacheHttpClientBuilderConfigurator {
    /**
     * A callback method to configure the {@link org.apache.http.impl.client.HttpClientBuilder}
     * @param httpClientBuilder {@link org.apache.http.impl.client.HttpClientBuilder} object to be further configured
     * @return the configured {@link org.apache.http.impl.client.HttpClientBuilder}. If {@code null} is returned the
     * {@code httpClientBuilder} is used by {@link ApacheConnectorProvider} instead.
     */
    HttpClientBuilder configure(HttpClientBuilder httpClientBuilder);
}

Usually, one would use the pre-configured httpClientBuilder to further configure it and return. You, on the other hand, could return the instrumented one. Unfortunatelly, the configuration using the properties would not work for you, then.

The ApacheHttpClientBuilderConfigurator would just be registered on the Client

@jansupol
Copy link
Contributor

jansupol commented Sep 2, 2019

#4238

To get a broader Apache Community on board:
@jlorenzen Would this help you?

@garypgithub
Copy link
Author

This would definitely help me. It is unfortunate that there are no getter methods on HttpClientBuilder but I could set the ones I need that I would normally set via ApacheConnectorProvider. Thank you.

@jlorenzen
Copy link

@jansupol Overall, I like the proposed solution that uses ApacheHttpClientBuilderConfigurator. However, it only fixes part of what I was looking for in #4104. The proposed solution doesn't allow a caller to use a CachingHttpClientBuilder.

@jlorenzen
Copy link

@jansupol Have you given any more thought on being able to make this configurable by the caller in ApacheConnector?

final HttpClientBuilder clientBuilder = CachingHttpClients.custom();

@jlorenzen
Copy link

@jansupol Just pinging ya again. I see version 2.32 is out and that usually means me updating our fork for this one little change to use CachingHttpClients.

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

No branches or pull requests

3 participants