Guard against null HttpContext

[breedx-splk]

Resolves #6787

[breedx-splk]

#6791 should address the test failure.

[laurit]

I don't think that this is the best approach. Considering that if the passed HttpContext is null a blank one will be created a few calls deeper we might as well create it here. This would avoid loosing the telemetry that requires access to response.

[breedx-splk]

I don't think that this is the best approach.

I will never assert that any change I have ever or will ever make will be the "best".

Considering that if the passed HttpContext is null a blank one will be created a few calls deeper we might as well create it here. This would avoid loosing the telemetry that requires access to response.

I'm not following this. Where does a blank HttpContext get created a few calls deeper? And what kind of response attributes are even useful when the context is null? Isn't it all worthless without context? Where does a null response inhibit other telemetry?

[laurit]

Considering that if the passed HttpContext is null a blank one will be created a few calls deeper we might as well create it here. This would avoid loosing the telemetry that requires access to response.

I'm not following this. Where does a blank HttpContext get created a few calls deeper? And what kind of response attributes are even useful when the context is null? Isn't it all worthless without context? Where does a null response inhibit other telemetry?

See https://github.com/apache/httpcomponents-client/blob/7ab435c271b6ff3bde4e737c2b02ba873ffa8b55/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalAbstractHttpAsyncClient.java#L177 for where http client handles null context.

Try adding null context to

opentelemetry-java-instrumentation/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpAsyncClientTest.java

Line 91 in da3eecf

return client.execute(request, null).get();

and

opentelemetry-java-instrumentation/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpAsyncClientTest.java

Line 96 in da3eecf

client.execute(request, new ResponseCallback(result));

when you run the test with your modifications you'll see that some tests fail because http.status_code and http.flavor are not filled because they are extracted from the response. Change

opentelemetry-java-instrumentation/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpAsyncClientInstrumentation.java

Line 70 in da3eecf

@Advice.Argument(3) HttpContext httpContext,

to @Advice.Argument(value = 3, readOnly = false) HttpContext httpContext and add

      if (httpContext == null) {
        httpContext = new BasicHttpContext();
      }

[breedx-splk]

Ok, I think I see where you're coming from. The fact that the client defaults the null httpContext to a BasicHttpContext inside of doExecute() is lost on us, because our instrumentation is sitting above that. Alright, I changed the test code locally to explicitly pass a null context and tried 3 things:

1. No null guard in WrappedFutureCallback, no defaulting httpContext inClientAdvice.
   Result: test hangs (presumably due to NPE)
2. Add null guard in WrappedFutureCallback, no defaulting httpContext in ClientAdvice.
   Result: Test failures due to missing http flavor and status code
3. Add null guard in WrappedFutureCallback, and default httpContext in ClientAdvice.
   Result: Test failures due to number of expected spans being wrong

It's feeling like one of the changes (either test code or instrumentation) is a semantic change. I'm not sure which it is....but explicitly passing in a null context caused the tests to fail before this change and with this change.

If you feel strongly about the null guard being a bad solution, I can close this.

[trask]

3. Add null guard in WrappedFutureCallback, and default httpContext in ClientAdvice.
   Result: Test failures due to number of expected spans being wrong

@breedx-splk did you maybe miss making the httpContext writable in ClientAdvice?

@Advice.Argument(value = 3, readOnly = false) HttpContext httpContext

[breedx-splk]

@breedx-splk did you maybe miss making the httpContext writable in ClientAdvice?

🤦🏻 yes...that was it. Thanks. I'll push a change.

[trask]

@breedx-splk can you rebase? github UI appears to be lying about your change set (at least to me), I've seen this a couple of times recently and rebasing to latest main should hopefully clean it up