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

Apache Connector: add a config property to choose an HttpClientBuilder #5808

Open
Mahoney opened this issue Nov 21, 2024 · 4 comments
Open
Labels
enhancement New feature or request

Comments

@Mahoney
Copy link

Mahoney commented Nov 21, 2024

Currently both the 4.x & 5.x Apache Connector Providers hard code the creation of the HttpClientBuilder.

4.x connector (3.1 branch):

class ApacheConnector implements Connector {
    ApacheConnector(final Client client, final Configuration config) {
        final HttpClientBuilder clientBuilder = HttpClientBuilder.create();
        this.client = configuredBuilder.build();
    }
}

5.x connnector (4.0 branch):

class Apache5Connector implements Connector {
    Apache5Connector(final Client client, final Configuration config) {
        final HttpClientBuilder clientBuilder = HttpClientBuilder.create();
        this.client = configuredBuilder.build();
    }
}

It would be convenient to be able to choose a different implementation of HttpClientBuilder. For instance, opentelemetry-apache-httpclient extends HttpClientBuilder in order to override a protected method.

I propose a new property Apache5?ClientProperties.CLIENT_BUILDER. ApacheConnector would extract the object at that property, use it if it is an instance of HttpClientBuilder, and fall back to HttpClientBuilder.create() if it is absent / not an instance of HttpClientBuilder.

Happy to contribute a PR if it would be accepted in principle.

@jansupol
Copy link
Contributor

jansupol commented Dec 9, 2024

Would ApacheHttpClientBuilderConfigurator be the class you seek? Just register your implementation to the client and you may configure the HttpClientBuilder there.

@jansupol
Copy link
Contributor

jansupol commented Dec 9, 2024

Apache5HttpClientBuilderConfigurator for Apache Http Client 5.

@Mahoney
Copy link
Author

Mahoney commented Dec 9, 2024

By that stage all the configuration of the HttpClientBuilder has been done, so while it is possible to return a different instance of a subtype of HttpClientBuilder from an ApacheHttpClientBuilderConfigurator, the implementation has to copy all the previous configuration onto it, which seems arduous and error prone.

@jansupol
Copy link
Contributor

jansupol commented Dec 9, 2024

I see. There are multiple possibilities how to implement this.

If a property, I assume it would better be a supplier of the HttpClientBuilder.
Other option is to extend the Apache5HttpClientBuilderConfigurator by a create() method that would be invoked the first before configuring the builder.

Not sure which one is better at the moment. The latter seems more consistent, but would cause issues when multiple registered. They could be sorted by Priority, though. The first brings another property, but for a supplier this time. Not as type-safe as the interface option.

@jansupol jansupol added the enhancement New feature or request label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants