Add library instrumentation for Apache HTTPClient 4.3 #3623
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
...a/io/opentelemetry/instrumentation/apachehttpclient/v4_3/HostAndRequestAsHttpUriRequest.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientRequest.java
Outdated
Show resolved
Hide resolved
…y/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientRequest.java Co-authored-by: Lauri Tulmin <[email protected]>
| error = e; | ||
| throw e; | ||
| } finally { | ||
| if (!pendingRedirect(context, httpContext, request, instrumenterRequest, response)) { |
There was a problem hiding this comment.
It is unfortunate that we can't wrap around redirect and automatic retry. Perhaps we should just let this instrumentation create multiple spans when there is a redirect? If I understand correctly if retry is enable then we only record the first failing request even when retry succeeds?
There was a problem hiding this comment.
Nah, the normal redirect tests in HttpClientTest are passing so we're recording it as a success. And the timing is around all the redirects since we end the span after the last one.
The code is sort of complex but I think the modeling is consistent with our other instrumentation so guess it's not too bad.
There was a problem hiding this comment.
Yeah redirects look ok. There is also an option to retry request when it fails, imo it would need similar handling as redirects to make sure that we don't end span on the first attempt. We don't have a test for that.
There was a problem hiding this comment.
@laurit We discussed a bit and figure it's good to try this code first and iterate later, since we don't have any tests for retries yet. We'd probably need to check everything in the future more broadly
There was a problem hiding this comment.
I just wanted to point out that with library instrumentation we have limited options to force the framework to do things as we like so we could instead create spans in a way that fits best with the integration points the framework provides us. This way there should be less corner cases we have to worry about. Sure it will be bad that different library instrumentations don't behave in a consistent way and even worse would be testing them, but instrumentation would be simpler and the chance of something breaking because http client is configured slightly differently or customized a bit should be smaller.
| * items. The {@link AttributesExtractor} will be executed after all default extractors. | ||
| */ | ||
| public ApacheHttpClientTracingBuilder addAttributeExtractor( | ||
| AttributesExtractor<? super ApacheHttpClientRequest, ? super HttpResponse> |
There was a problem hiding this comment.
Should ApacheHttpClientRequest be public? We're exposing a possibility to add a custom attributes extractor, but you can't really use request in it.
There was a problem hiding this comment.
@trask This is an interesting case for wanting to go back to the HttpUriRequest interface perhaps :-)
For now I just made public with a single method returning the delegate.
There was a problem hiding this comment.
i'm not sure wrangling into HttpUriRequest is ideal for custom attribute extractors either, I think ideally they would want access to HttpHost + HttpRequest in the case where an HttpHost + HttpRequest overload is used
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ |
There was a problem hiding this comment.
| /* | |
| * Copyright The OpenTelemetry Authors | |
| * SPDX-License-Identifier: Apache-2.0 | |
| */ |
| HttpRequestWrapper request, | ||
| ApacheHttpClientRequest instrumenterRequest, | ||
| @Nullable CloseableHttpResponse response) | ||
| throws ProtocolException { |
There was a problem hiding this comment.
This method can throw and is called in a finally block - would it make sense to catch whatever it throws and addSuppressed() to error? Or is it a case when you have to declare throws but nothing should ever fail here?
There was a problem hiding this comment.
Thanks for noticing! Tightened up the below catch to prevent it from leaking, which would be bad for many reasons not just being in a finally (well technically being in the finally would save the app from crashing maybe :)
…nstrumentation into apache-library
…va-instrumentation into apache-library
I referred to both opentracing and zipkin to help with corner cases but mostly wrote from scratch. Both of them have and lack features vs the other one with regards to apache.
For now I have copied much from 4.0 javaagent instrumentation, in a future PR will try to share in a common library.