Instrumentation for Apache async HttpClient5

[anuragagarwal561994]

The current implementation of Apache HttpClient 5 is just for ClassicHttpRequest while async http client 5 is missing.

Support for Apache async HttpClient 5 would be a great addition as it is not right now supported by even some very prominent telemetry tools in market. Even newrelic lacks support for the same.

#2157 (comment)

[anuragagarwal561994]

@trask @ok2c I was thinking of intrumenting DefaultHttpProcessor

This should cover all the HttpClient both the classic and the async version.

I can instrument the request and response interceptor methods here and I believe it should cover all the use cases.

[trask]

@anuragagarwal561994 that sounds great!

[anuragagarwal561994]

I was not able to use the DefaultHttpProcessor for 2 reasons:

1. I don't have the parent context here if needed this could have been implemented in the following 2 ways:
   A. Using a virtual field which keeps request by context like done in okhttp.
   B. By using the HttpContext and keeping the parent context in it and using when the processor is called, but that would have exposed internal otel context to the user as well
2. DefaultRequestProcessor is ideally an internal class although it is exposed as public but is not directly available for the user to use. Interface of it is HttpProcessor but that as well a user can't supply on his own. So didn't seem right to instrument an internal class although it has been exposed publicly.

So to keep the implementation same as http async client 4 I have decorated request channel along with decorating the future and the request producer since the request producer in the new version doesn't expose the request like in http async client 4.

Rest of the base logic remains same.

I have made the changes a few test cases need fixing and I would be submitting a merge request by this week. Do let me know if any changes are requested or any of the above approaches are preferable.

@ok2c and @trask do let me know about the approach being followed here.

[trask]

@anuragagarwal561994 this approach sounds very reasonable 👍

[ok2c]

@anuragagarwal561994 Any reason for not using custom execution interceptors? I believe they might be a better interception point for collecting verious execution details.

[anuragagarwal561994]

@ok2c Yes there is a reason for the same, minimal http clients do not expose a method to add custom interceptors nor they are using a builder.

So that solution will only be applicable for certain clients and will leave the others.

If we have that functionality available I guess that we can extract the interface part out in a common class and then we can modify the non async client implementation as well. We will just have to add I believe instrumentation points.

Refer to the discussion here: #2254

Second reason is that for async client we won't have the parent context from where the client has been called so we will either need to store it in the http context or add a map of request to context as I have discussed above.

[trask]

hey @anuragagarwal561994, we have been migrating all of the Groovy tests to Java (for better maintainability and contributor experience), if you are able to write new any tests using Java instead of Groovy that would be great, check out #5660 for a recent example converting apache-httpasyncclient-4.1 tests from Groovy to Java

[anuragagarwal561994]

Sure @trask noted

[anuragagarwal561994]

@ok2c @trask I see one more issue, not a blocker but something we should keep in mind.

The public interface org.apache.hc.client5.http.async.HttpAsyncClient exposes method:

<T> Future<T> execute(
            AsyncRequestProducer requestProducer,
            AsyncResponseConsumer<T> responseConsumer,
            HandlerFactory<AsyncPushConsumer> pushHandlerFactory,
            HttpContext context,
            FutureCallback<T> callback);

Then we have a public abstract implementation CloseableHttpAsyncClient with multiple methods where most of them converge to the implementation of the above method, but then this method itself converge to

public final <T> Future<T> execute(
            final HttpHost target,
            final AsyncRequestProducer requestProducer,
            final AsyncResponseConsumer<T> responseConsumer,
            final HandlerFactory<AsyncPushConsumer> pushHandlerFactory,
            final HttpContext context,
            final FutureCallback<T> callback)

which is not part of the interface, now as an instrumenting library, I believe that we should just instrument the interface because doing this we don't have to deep dive in what's inside and if it changes in future. But on the flip side this won't instrument the requests for the user which are using this method as it is public. User might not be using the interface but rather the public abstract class.

[anuragagarwal561994]

@trask when do we plan to release the new version or is there anyway we can try the snapshot version of the instrumentation?

[trask]

hey @anuragagarwal561994, check out https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/RELEASING.md#snapshot-builds for snapshots. next release should be end of this week.