opentelemetry-java-contrib
opentelemetry-java-contrib copied to clipboard
open-telemetry
Replace SimpleHttpClient (okhttp based) by JdkHttpClient (HttpURLConnection based)
Description:
Replaced existing okhttp based HttpClient (SimpleHttpClient) by a JDK 1.8 implementation using HttpURLConnection. okhttp adds too many dependencies to a project that performs simple HTTP calls to discover resource metadata in AWS ECS and EKS. The AWS endpoints supports HTTP/1.1 and there is no need to higher protocol version (HTTP/2).
See the okhttp dependency tree in a Java project:
Is is well known that having okhttp as a dependency makes very difficult (or impossible) to create a Graal native image of a consuming application.
Also, okio has known vulnerabilities
OpenTelemetry OTLP exporter supports excluding okhttp based implementation in favor of a JDK-11 implementation via ServiceManager. I do not think the same solution is needed here since the HTTP client requirements for this project are very simple and not a high throughput implementation is required for a single HTTP request in the application lifespan.
Initial PR does not remove the okhttp implementation nor the dependency (made optional). An update is required before merging it to completely remove okhttp.
check out https://github.com/open-telemetry/opentelemetry-java/pull/3991, in particular:
notably this will prevent resource detection requests from being traced during agent startup due to https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/4642
cc @LikeTheSalad who is investigating options for suppress tracing of specific calls
there's work in-progress which could unblock this: https://github.com/open-telemetry/opentelemetry-java/pull/5918
I think that's to avoid having the call traced, whereas this PR is about removing the okhttp dependency completely for this specific resource?
the reverse of this PR (moving from HttpURLConnection to OkHttp) was done a while ago in https://github.com/open-telemetry/opentelemetry-java/pull/3991 (when this code lived in that repository)
I think really it's only a problem for the AWS distribution of the OTel Java agent, since that distribution pulls this component in, while the upstream Java agent doesn't (yet).
The reason why OkHttp calls aren't traced in the Java agent is because OkHttp is loaded in the agent class loader, and instrumentation isn't applied in that class loader. While HttpURLConnection is loaded in the bootstrap class loader and so instrumentation is applied.
Another aspect that might be relevant to deal with once this PR is merged is providing the ability to reuse the SimpleHttpClient for other resource providers implementations when adding those to the contrib repo: https://github.com/open-telemetry/opentelemetry-java-contrib/issues/1074.
there's work in-progress which could unblock this: open-telemetry/opentelemetry-java#5918
Is there anything else preventing the PR from being merged? The above mentioned change was already merged...
@rjbaucells can you integrate that change into your PR?
I looked at the details of PR 5918 and it provides a way to supress instrumentation of internal calls in "exporters" via setting thesuppress_internal_exporter_instrumentation context variable to true.
Looking at the implementation of the JDK (11+) based sender, it is not suppressing the instrumentation of the requests; only the okHttp sender implementation is using it. I would assume the java agent will process calls to the JDK HttpClient the same way as the HttpURLConnection. Do we really need to suppress these calls for the HttpURLConnection?
only the okHttp sender implementation is using it
right, so the okhttp sender is using it so that auto-instrumentation of OkHTTP won't capture those http requests
in the same way, I think the code here could use it so that auto-instrumentation of HttpURLConnection won't capture these http requests
The Context value in the above mentioned PR is suppress_internal_exporter_instrumentation. Looking at the name it is very specific to "exporters". I did not find the place the agent is reading the boolean context value to skip the call instrumentation.
Is there a generic Context value I can set prior to the HTTP call that will instruct the agent to skip the instrumentation?
check out https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/9739
The https://github.com/open-telemetry/opentelemetry-java/pull/6546 PR was just merged and will be available with 1.41.0 SDK release, so the issue about using an exporter-related context entry (here) is solved.
The logical next steps are thus:
- release of sdk 1.41.0
- updating this PR to use the new internal API introduced in https://github.com/open-telemetry/opentelemetry-java/pull/6546
This of course assumes that @rjbaucells has time to dedicate to this, let us know if you need any help here.
I don't know if there is an official release schedule, but looking at past releases hint that we have roughly one release per month: https://github.com/open-telemetry/opentelemetry-java/releases, so it would likely be available within 2/3 weeks.
I don't know if there is an official release schedule
check out https://github.com/open-telemetry/opentelemetry-java/blob/main/RELEASING.md#release-cadence, so Aug release target is Fri, Aug 9
@driverpt if I'm not mistaken the io.opentelemetry:opentelemetry-sdk version is provided by io.opentelemetry:opentelemetry-bom which is itself provided byopentelemetry-instrumentation-bom, so in order to update the SDK version we need to have 1) the SDK version bump (opentelemetry-java-instrumentation#11985) and 2) a release of the instrumentation repo (likely for 2.7.0).
Oh. I was not aware of that. Apologies then.
Do you know +/- an ETA for this to be "releaseable"?
Thanks