Add library instrumentation for java http client #8138
Conversation
| import java.net.http.HttpResponse; | ||
|
|
||
| /** Entrypoint for instrumenting Java HTTP Client. */ | ||
| public final class JavaHttpClientTelemetry { |
There was a problem hiding this comment.
I named the public api classes JavaHttpClient.. but some of the other classes still use JdkHttpClient.... I think we should choose how we want to call this and name all the classes in the same way.
There was a problem hiding this comment.
+1 on renaming everything from Jdk* to Java*
.../src/main/java/io/opentelemetry/instrumentation/httpclient/internal/OpenTelemetryClient.java
Outdated
Show resolved
Hide resolved
I think the |
There was a problem hiding this comment.
Should I also implement context propagation for
BodyHandlerin library instrumentation or should I just delete it? I guess it could come handy if someone builds a customBodyHandlerand wants to emit spans from there, though this doesn't feel too likely. I'd like deleting it more.
it looks like we don't have any tests for custom BodyHandler? I'm fine deleting it and adding it if there's demand.
| import java.net.http.HttpResponse; | ||
|
|
||
| /** Entrypoint for instrumenting Java HTTP Client. */ | ||
| public final class JavaHttpClientTelemetry { |
There was a problem hiding this comment.
+1 on renaming everything from Jdk* to Java*
.../src/main/java/io/opentelemetry/instrumentation/httpclient/internal/OpenTelemetryClient.java
Outdated
Show resolved
Hide resolved
| public final class OpenTelemetryClient extends HttpClient { | ||
| private final HttpClient client; | ||
| private final Instrumenter<HttpRequest, HttpResponse<?>> instrumenter; | ||
| private final HttpHeadersSetter headersSetter; |
There was a problem hiding this comment.
maybe could delete HttpHeadersSetter, moving its one method to this class, and pass in ContextPropagators here?
There was a problem hiding this comment.
I think it is better to keep it because javaagent instrumentation does not use OpenTelemetryClient, but uses HttpHeadersSetter
Co-authored-by: Trask Stalnaker <[email protected]>
Resolves #8069
The javaagent instrumentation also supports propagating context into BodyHandler implemented in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/BodyHandlerWrapper.java I think the initial idea behind it was that this allowed propagating context into callbacks. Because this didn't work for
connectionErrorUnopenedPortWithCallbacktest later we also added wrapping completable future to take care of propagating context into callbacks. Should I also implement context propagation forBodyHandlerin library instrumentation or should I just delete it? I guess it could come handy if someone builds a customBodyHandlerand wants to emit spans from there, though this doesn't feel too likely. I'd like deleting it more.