-
Notifications
You must be signed in to change notification settings - Fork 1k
Add network timing attributes to okhttp3 library #15664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,16 +76,24 @@ public void cancel() { | |
| } | ||
|
|
||
| @Override | ||
| public Call clone() throws CloneNotSupportedException { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are these changes needed?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In okhttp 3.11, CloneNotSupportedException is not thrown from the method, we still need to have the try catch block because super.clone can throw the exception. |
||
| public Call clone() { | ||
| if (cloneMethod == null) { | ||
| return (Call) super.clone(); | ||
| try { | ||
| return (Call) super.clone(); | ||
| } catch (CloneNotSupportedException e) { | ||
| throw new AssertionError(e); | ||
| } | ||
| } | ||
| try { | ||
| // we pull the current context here, because the cloning might be happening in a different | ||
| // context than the original call creation. | ||
| return new TracingCall((Call) cloneMethod.invoke(delegate), Context.current()); | ||
| } catch (IllegalAccessException | InvocationTargetException e) { | ||
| return (Call) super.clone(); | ||
| try { | ||
| return (Call) super.clone(); | ||
| } catch (CloneNotSupportedException cloneException) { | ||
| throw new AssertionError(cloneException); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.instrumentation.okhttp.v3_0.internal; | ||
|
|
||
| import io.opentelemetry.api.common.AttributeKey; | ||
| import io.opentelemetry.api.trace.Span; | ||
| import java.io.IOException; | ||
| import java.net.InetAddress; | ||
| import java.net.InetSocketAddress; | ||
| import java.net.Proxy; | ||
| import java.util.List; | ||
| import javax.annotation.Nullable; | ||
| import okhttp3.Call; | ||
| import okhttp3.Connection; | ||
| import okhttp3.EventListener; | ||
| import okhttp3.Handshake; | ||
| import okhttp3.Protocol; | ||
| import okhttp3.Request; | ||
| import okhttp3.Response; | ||
|
|
||
| /** | ||
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. | ||
| */ | ||
| public final class NetworkTimingEventListener extends EventListener { | ||
|
|
||
| // Raw timestamp attribute keys | ||
| private static final AttributeKey<Long> CALL_START = AttributeKey.longKey("http.call.start_time"); | ||
| private static final AttributeKey<Long> DNS_START = AttributeKey.longKey("http.dns.start_time"); | ||
| private static final AttributeKey<Long> DNS_END = AttributeKey.longKey("http.dns.end_time"); | ||
| private static final AttributeKey<Long> CONNECT_START = | ||
| AttributeKey.longKey("http.connect.start_time"); | ||
| private static final AttributeKey<Long> CONNECT_END = | ||
| AttributeKey.longKey("http.connect.end_time"); | ||
| private static final AttributeKey<Long> SECURE_CONNECT_START = | ||
| AttributeKey.longKey("http.secure_connect.start_time"); | ||
| private static final AttributeKey<Long> SECURE_CONNECT_END = | ||
| AttributeKey.longKey("http.secure_connect.end_time"); | ||
| private static final AttributeKey<Long> REQUEST_HEADERS_START = | ||
| AttributeKey.longKey("http.request.headers.start_time"); | ||
| private static final AttributeKey<Long> REQUEST_HEADERS_END = | ||
| AttributeKey.longKey("http.request.headers.end_time"); | ||
| private static final AttributeKey<Long> REQUEST_BODY_START = | ||
| AttributeKey.longKey("http.request.body.start_time"); | ||
| private static final AttributeKey<Long> REQUEST_BODY_END = | ||
| AttributeKey.longKey("http.request.body.end_time"); | ||
| private static final AttributeKey<Long> RESPONSE_HEADERS_START = | ||
| AttributeKey.longKey("http.response.headers.start_time"); | ||
| private static final AttributeKey<Long> RESPONSE_HEADERS_END = | ||
| AttributeKey.longKey("http.response.headers.end_time"); | ||
| private static final AttributeKey<Long> RESPONSE_BODY_START = | ||
| AttributeKey.longKey("http.response.body.start_time"); | ||
| private static final AttributeKey<Long> RESPONSE_BODY_END = | ||
| AttributeKey.longKey("http.response.body.end_time"); | ||
| private static final AttributeKey<Long> CALL_END = AttributeKey.longKey("http.call.end_time"); | ||
|
|
||
| // Singleton instance of stateless NetworkTimingEventListener | ||
| private static final NetworkTimingEventListener INSTANCE = new NetworkTimingEventListener(); | ||
|
|
||
| private NetworkTimingEventListener() {} | ||
|
|
||
| @Override | ||
| public void callStart(Call call) { | ||
| Span.current().setAttribute(CALL_START, System.currentTimeMillis()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might not work when the event happens on a background thread. Are you sure that all of these callbacks are invoked so that the can see the current http client span? |
||
| } | ||
|
|
||
| @Override | ||
| public void dnsStart(Call call, String domainName) { | ||
| Span.current().setAttribute(DNS_START, System.currentTimeMillis()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The span start/end timestamps use nanoseconds, perhaps to be consistent with that and to be able to measure sub millisecond operations it would also make sense to use nanos here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually the span event itself already includes the event creation timestamp in nanoseconds so perhaps adding another timestamp isn't needed at all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will using |
||
| } | ||
|
|
||
| @Override | ||
| public void dnsEnd(Call call, String domainName, List<InetAddress> inetAddressList) { | ||
| Span.current().setAttribute(DNS_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void connectStart(Call call, InetSocketAddress inetSocketAddress, Proxy proxy) { | ||
| Span.current().setAttribute(CONNECT_START, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void secureConnectStart(Call call) { | ||
| Span.current().setAttribute(SECURE_CONNECT_START, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void secureConnectEnd(Call call, @Nullable Handshake handshake) { | ||
| Span.current().setAttribute(SECURE_CONNECT_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void connectEnd( | ||
| Call call, InetSocketAddress inetSocketAddress, Proxy proxy, @Nullable Protocol protocol) { | ||
| Span.current().setAttribute(CONNECT_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void connectFailed( | ||
| Call call, | ||
| InetSocketAddress inetSocketAddress, | ||
| Proxy proxy, | ||
| @Nullable Protocol protocol, | ||
| IOException ioe) {} | ||
|
|
||
| @Override | ||
| public void connectionAcquired(Call call, Connection connection) {} | ||
|
|
||
| @Override | ||
| public void connectionReleased(Call call, Connection connection) {} | ||
|
|
||
| @Override | ||
| public void requestHeadersStart(Call call) { | ||
| Span.current().setAttribute(REQUEST_HEADERS_START, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void requestHeadersEnd(Call call, Request request) { | ||
| Span.current().setAttribute(REQUEST_HEADERS_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void requestBodyStart(Call call) { | ||
| Span.current().setAttribute(REQUEST_BODY_START, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void requestBodyEnd(Call call, long byteCount) { | ||
| Span.current().setAttribute(REQUEST_BODY_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void responseHeadersStart(Call call) { | ||
| Span.current().setAttribute(RESPONSE_HEADERS_START, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void responseHeadersEnd(Call call, Response response) { | ||
| Span.current().setAttribute(RESPONSE_HEADERS_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void responseBodyStart(Call call) { | ||
| Span.current().setAttribute(RESPONSE_BODY_START, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void responseBodyEnd(Call call, long byteCount) { | ||
| Span.current().setAttribute(RESPONSE_BODY_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| @Override | ||
| public void callEnd(Call call) { | ||
| Span.current().setAttribute(CALL_END, System.currentTimeMillis()); | ||
| } | ||
|
|
||
| /** | ||
| * Factory for creating NetworkTimingEventListener instances. A singleton instance is returned as | ||
| * the listener is stateless and thread-safe. | ||
| * | ||
| * <p>NetworkTimingEventListener captures raw network timing timestamps and adds them as | ||
| * attributes to the current OpenTelemetry span. | ||
| * | ||
| * <p>Works with both synchronous and asynchronous OkHttp calls when used with proper context | ||
| * propagation. | ||
| * | ||
| * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
| * at any time. | ||
| */ | ||
| public static final class Factory implements EventListener.Factory { | ||
| @Override | ||
| public EventListener create(Call call) { | ||
| return INSTANCE; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,12 +62,13 @@ public String getNetworkProtocolName(Interceptor.Chain chain, @Nullable Response | |
| return "http"; | ||
| case SPDY_3: | ||
| return "spdy"; | ||
| default: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this needed?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was required as build was failing with error prone issue - Missing cases in enum switch |
||
| // added in 3.11.0 | ||
| if ("H2_PRIOR_KNOWLEDGE".equals(response.protocol().name())) { | ||
| return "http"; | ||
| } | ||
| return null; | ||
| } | ||
| // added in 3.11.0 | ||
| if ("H2_PRIOR_KNOWLEDGE".equals(response.protocol().name())) { | ||
| return "http"; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Nullable | ||
|
|
@@ -85,12 +86,13 @@ public String getNetworkProtocolVersion(Interceptor.Chain chain, @Nullable Respo | |
| return "2"; | ||
| case SPDY_3: | ||
| return "3.1"; | ||
| default: | ||
| // added in 3.11.0 | ||
| if ("H2_PRIOR_KNOWLEDGE".equals(response.protocol().name())) { | ||
| return "2"; | ||
| } | ||
| return null; | ||
| } | ||
| // added in 3.11.0 | ||
| if ("H2_PRIOR_KNOWLEDGE".equals(response.protocol().name())) { | ||
| return "2"; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of a tangent to this PR, but I'd be surprised if folks are still using versions this old given it was released back in 2018, and would imagine most folks are using either 4 or 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create an issue to discuss and evaluate this and address it as needed in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion creating an issue won't help as this PR won't be merged before there is a decision on whether to increase the minimum supported version or not. Generally we are open to bumping the minimum supported version for library instrumetnations, but prefer to keep supporting old versions in the agent instrumentation. Here I suspect that bumping the version isn't really required since the instrumentation actually works with the old version, only the new network listener doesn't. You could avoid the discussion by replacing
library("com.squareup.okhttp3:okhttp:3.0.0")withcompileOnly("com.squareup.okhttp3:okhttp:3.11.0")andtestLibrary("com.squareup.okhttp3:okhttp:3.0.0")and placing the network listener test into separate suite that tests with3.11.0.