Request Content-Length not recorded for Apache Http Clients

[anuragagarwal561994]

Describe the bug
As per the change #6415 the request and the response content lengths are to be recorded for every http clients and servers. But for apache http clients, request content lengths are not getting recorded.

Steps to reproduce
Make a http post request with a body with apache http client instrumentation to validate that http.request_content_length is missing in traces and metrics.

What did you expect to see?
http.request_content_length to be populated with the actual body length.

What did you see instead?
No http.request_content_length being present in traces and metrics.

What version are you using?
1.18.0

Environment
Compiler: Zulu11.58+15-CA (build 11.0.16+8-LTS)
OS: MacOS

Additional context
#6415 (comment)

[anuragagarwal561994]

@trask @mateuszrzeszutek I would like to work on this issue, but have few doubts of my own related to the design. Can you help me resolve the same.

So as stated in the comment, the issue here is the request being intercepted during instrumentation is not the same as the one being sent from the client as the copy is made from within the library and then manipulations are done over it.

Now as far as I understand, in open telemetry auto instrumentations we are instrumenting over the publicly available methods as it is difficult to instrument over what is happening internally inside the library as the implementation underneath can keep on changing.

What I see here is there is no easy way to deep dive within the implementation details of the library, we are left with 2 choices here to resolve this issue.

Replace request at the completion stage

The final request being sent to the user is actually put in http context of the client and during the on complete, failed or cancelled method (when we are ending the instrumentation) we may some how build the functionality to retrieve this request. Since the attribute and the request are recorded at the end of the instrumentation, we can replace this request to get these details.

Issue with this approach -> as I was checking for http async client 5.0, if there an invalid url which may lead to 404, this internal request can even have a different HTTP method than original and hence not reliable.

Not fetch the detail from Content-Length

In the current implementations we get EntityDetail objects which has content-length method, but since the method has been removed to modify the content-length behaviour we may want to add it as attribute while instrumenting

[anuragagarwal561994]

@trask I have given a rough spin for apache http clients 4.x and 5.x.

The suggested solution is not yet complete, but I am facing few dilemmas to resolve these, hence would be needing help from the community to be able to contribute for the desired results.

Content-Length header is added to the copied request in the subsequent steps internally which the current instrumentation doesn't have access to. This is post applying any compression or so. Hence the only way to get the final request sent by the library is to fetch the Content-length header from HttpContext from http.request attribute key.

For the async http clients, the problem is much simpler as all the subsequent methods of the interface falls back to the same method and generally the instrumentations are applied on this method, hence at the time of completion of the callback we can use the HttpContext passed by the user while calling any of the methods (or by creating one if none passed by the user) and using this http context to get the internal request and pass this fetched request to actually end the instrumentation from where the metrics and other things can be recorded.

The problem multiplies in case of Non AsyncClients as they don't fallback to the same method in some cases.
Hence here instead of instrumenting on the interface I applied the instrumentation on the internal abstract class CloseableHttpClient and over it's protected method doExecute over which all the methods fallbacks to in case of httpclient 5.0. This makes un instrumenting all the other 8 methods as we were previously doing.

In http clients 4.x there are 2 more cases to handle as far as I have seen:
for < 4.3.x this abstract class is AbstractHttpClient and the method to instrument would be execute which is a final method.

For >= 4.3.x this abstract class is CloseableHttpClient and the method to instrument would be doExecute, same as http client 5.x.

Just wanted to understand if I am moving in the right direction and would this be the right way to resolve this issue.