From 4589148afe0e7144e6d2099a034dba6659d9d098 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serkan=20=C3=96ZAL?= Date: Thu, 15 Aug 2024 14:56:53 +0300 Subject: [PATCH 1/2] Add network peer address attributes to the span for the `okhttp-3.0` instrumentation --- .../okhttp/v3_0/OkHttp3Singletons.java | 3 +- .../okhttp/v3_0/OkHttpTelemetry.java | 6 +-- .../okhttp/v3_0/OkHttpTelemetryBuilder.java | 10 ++-- .../ConnectionErrorSpanInterceptor.java | 8 +-- .../v3_0/internal/OkHttpAttributesGetter.java | 53 +++++++++++++------ ...kHttpClientInstrumenterBuilderFactory.java | 4 +- .../v3_0/internal/TracingInterceptor.java | 10 ++-- .../okhttp/v3_0/AbstractOkHttp3Test.java | 12 ++++- .../junit/http/AbstractHttpClientTest.java | 9 +++- 9 files changed, 77 insertions(+), 38 deletions(-) diff --git a/instrumentation/okhttp/okhttp-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3Singletons.java b/instrumentation/okhttp/okhttp-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3Singletons.java index 2cc4f6f28975..f7e982f7edab 100644 --- a/instrumentation/okhttp/okhttp-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3Singletons.java +++ b/instrumentation/okhttp/okhttp-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3Singletons.java @@ -15,13 +15,12 @@ import io.opentelemetry.instrumentation.okhttp.v3_0.internal.TracingInterceptor; import io.opentelemetry.javaagent.bootstrap.internal.JavaagentHttpClientInstrumenters; import okhttp3.Interceptor; -import okhttp3.Request; import okhttp3.Response; /** Holder of singleton interceptors for adding to instrumented clients. */ public final class OkHttp3Singletons { - private static final Instrumenter INSTRUMENTER = + private static final Instrumenter INSTRUMENTER = JavaagentHttpClientInstrumenters.create( OkHttpClientInstrumenterBuilderFactory.create(GlobalOpenTelemetry.get())); diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetry.java b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetry.java index 22e6b1cf9995..cd4963f24654 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetry.java +++ b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetry.java @@ -14,7 +14,6 @@ import okhttp3.Callback; import okhttp3.Interceptor; import okhttp3.OkHttpClient; -import okhttp3.Request; import okhttp3.Response; /** Entrypoint for instrumenting OkHttp clients. */ @@ -32,10 +31,11 @@ public static OkHttpTelemetryBuilder builder(OpenTelemetry openTelemetry) { return new OkHttpTelemetryBuilder(openTelemetry); } - private final Instrumenter instrumenter; + private final Instrumenter instrumenter; private final ContextPropagators propagators; - OkHttpTelemetry(Instrumenter instrumenter, ContextPropagators propagators) { + OkHttpTelemetry( + Instrumenter instrumenter, ContextPropagators propagators) { this.instrumenter = instrumenter; this.propagators = propagators; } diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetryBuilder.java b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetryBuilder.java index 6dafba2c124d..1623ad127d8b 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetryBuilder.java +++ b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTelemetryBuilder.java @@ -15,13 +15,13 @@ import java.util.List; import java.util.Set; import java.util.function.Function; -import okhttp3.Request; +import okhttp3.Interceptor; import okhttp3.Response; /** A builder of {@link OkHttpTelemetry}. */ public final class OkHttpTelemetryBuilder { - private final DefaultHttpClientInstrumenterBuilder builder; + private final DefaultHttpClientInstrumenterBuilder builder; OkHttpTelemetryBuilder(OpenTelemetry openTelemetry) { builder = OkHttpClientInstrumenterBuilderFactory.create(openTelemetry); @@ -33,7 +33,7 @@ public final class OkHttpTelemetryBuilder { */ @CanIgnoreReturnValue public OkHttpTelemetryBuilder addAttributeExtractor( - AttributesExtractor attributesExtractor) { + AttributesExtractor attributesExtractor) { builder.addAttributeExtractor(attributesExtractor); return this; } @@ -95,7 +95,9 @@ public OkHttpTelemetryBuilder setEmitExperimentalHttpClientMetrics( /** Sets custom {@link SpanNameExtractor} via transform function. */ @CanIgnoreReturnValue public OkHttpTelemetryBuilder setSpanNameExtractor( - Function, ? extends SpanNameExtractor> + Function< + SpanNameExtractor, + ? extends SpanNameExtractor> spanNameExtractorTransformer) { builder.setSpanNameExtractor(spanNameExtractorTransformer); return this; diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/ConnectionErrorSpanInterceptor.java b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/ConnectionErrorSpanInterceptor.java index e34a2e4b3d69..3da6df3b25ce 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/ConnectionErrorSpanInterceptor.java +++ b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/ConnectionErrorSpanInterceptor.java @@ -21,9 +21,9 @@ */ public final class ConnectionErrorSpanInterceptor implements Interceptor { - private final Instrumenter instrumenter; + private final Instrumenter instrumenter; - public ConnectionErrorSpanInterceptor(Instrumenter instrumenter) { + public ConnectionErrorSpanInterceptor(Instrumenter instrumenter) { this.instrumenter = instrumenter; } @@ -43,9 +43,9 @@ public Response intercept(Chain chain) throws IOException { } finally { // only create a span when there wasn't any HTTP request if (HttpClientRequestResendCount.get(parentContext) == 0) { - if (instrumenter.shouldStart(parentContext, request)) { + if (instrumenter.shouldStart(parentContext, chain)) { InstrumenterUtil.startAndEnd( - instrumenter, parentContext, request, response, error, startTime, Instant.now()); + instrumenter, parentContext, chain, response, error, startTime, Instant.now()); } } } diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpAttributesGetter.java b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpAttributesGetter.java index b6b9aab4a5ff..0043f7dd52ed 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpAttributesGetter.java +++ b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpAttributesGetter.java @@ -6,47 +6,52 @@ package io.opentelemetry.instrumentation.okhttp.v3_0.internal; import io.opentelemetry.instrumentation.api.semconv.http.HttpClientAttributesGetter; +import java.net.InetSocketAddress; +import java.net.SocketAddress; import java.util.List; import javax.annotation.Nullable; -import okhttp3.Request; +import okhttp3.Connection; +import okhttp3.Interceptor; 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 enum OkHttpAttributesGetter implements HttpClientAttributesGetter { +public enum OkHttpAttributesGetter + implements HttpClientAttributesGetter { INSTANCE; @Override - public String getHttpRequestMethod(Request request) { - return request.method(); + public String getHttpRequestMethod(Interceptor.Chain chain) { + return chain.request().method(); } @Override - public String getUrlFull(Request request) { - return request.url().toString(); + public String getUrlFull(Interceptor.Chain chain) { + return chain.request().url().toString(); } @Override - public List getHttpRequestHeader(Request request, String name) { - return request.headers(name); + public List getHttpRequestHeader(Interceptor.Chain chain, String name) { + return chain.request().headers(name); } @Override public Integer getHttpResponseStatusCode( - Request request, Response response, @Nullable Throwable error) { + Interceptor.Chain chain, Response response, @Nullable Throwable error) { return response.code(); } @Override - public List getHttpResponseHeader(Request request, Response response, String name) { + public List getHttpResponseHeader( + Interceptor.Chain chain, Response response, String name) { return response.headers(name); } @Nullable @Override - public String getNetworkProtocolName(Request request, @Nullable Response response) { + public String getNetworkProtocolName(Interceptor.Chain chain, @Nullable Response response) { if (response == null) { return null; } @@ -67,7 +72,7 @@ public String getNetworkProtocolName(Request request, @Nullable Response respons @Nullable @Override - public String getNetworkProtocolVersion(Request request, @Nullable Response response) { + public String getNetworkProtocolVersion(Interceptor.Chain chain, @Nullable Response response) { if (response == null) { return null; } @@ -90,12 +95,28 @@ public String getNetworkProtocolVersion(Request request, @Nullable Response resp @Override @Nullable - public String getServerAddress(Request request) { - return request.url().host(); + public String getServerAddress(Interceptor.Chain chain) { + return chain.request().url().host(); } @Override - public Integer getServerPort(Request request) { - return request.url().port(); + public Integer getServerPort(Interceptor.Chain chain) { + return chain.request().url().port(); + } + + @Nullable + @Override + public InetSocketAddress getNetworkPeerInetSocketAddress( + Interceptor.Chain chain, @Nullable Response response) { + Connection connection = chain.connection(); + if (connection == null) { + return null; + } + SocketAddress socketAddress = connection.socket().getRemoteSocketAddress(); + if (socketAddress instanceof InetSocketAddress) { + return (InetSocketAddress) socketAddress; + } else { + return null; + } } } diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpClientInstrumenterBuilderFactory.java b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpClientInstrumenterBuilderFactory.java index 9ad002acda5e..4dbaf771d24d 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpClientInstrumenterBuilderFactory.java +++ b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/OkHttpClientInstrumenterBuilderFactory.java @@ -7,7 +7,7 @@ import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.instrumentation.api.incubator.builder.internal.DefaultHttpClientInstrumenterBuilder; -import okhttp3.Request; +import okhttp3.Interceptor; import okhttp3.Response; /** @@ -19,7 +19,7 @@ public class OkHttpClientInstrumenterBuilderFactory { private OkHttpClientInstrumenterBuilderFactory() {} - public static DefaultHttpClientInstrumenterBuilder create( + public static DefaultHttpClientInstrumenterBuilder create( OpenTelemetry openTelemetry) { return new DefaultHttpClientInstrumenterBuilder<>( INSTRUMENTATION_NAME, openTelemetry, OkHttpAttributesGetter.INSTANCE); diff --git a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/TracingInterceptor.java b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/TracingInterceptor.java index 202dfabf30e6..14f37fb687f8 100644 --- a/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/TracingInterceptor.java +++ b/instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/internal/TracingInterceptor.java @@ -20,11 +20,11 @@ */ public final class TracingInterceptor implements Interceptor { - private final Instrumenter instrumenter; + private final Instrumenter instrumenter; private final ContextPropagators propagators; public TracingInterceptor( - Instrumenter instrumenter, ContextPropagators propagators) { + Instrumenter instrumenter, ContextPropagators propagators) { this.instrumenter = instrumenter; this.propagators = propagators; } @@ -34,11 +34,11 @@ public Response intercept(Chain chain) throws IOException { Request request = chain.request(); Context parentContext = Context.current(); - if (!instrumenter.shouldStart(parentContext, request)) { + if (!instrumenter.shouldStart(parentContext, chain)) { return chain.proceed(chain.request()); } - Context context = instrumenter.start(parentContext, request); + Context context = instrumenter.start(parentContext, chain); request = injectContextToRequest(request, context); Response response = null; @@ -50,7 +50,7 @@ public Response intercept(Chain chain) throws IOException { error = e; throw e; } finally { - instrumenter.end(context, request, response, error); + instrumenter.end(context, chain, response, error); } } diff --git a/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java b/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java index 5f051141f7f5..f3f9a7bb90a4 100644 --- a/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java +++ b/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java @@ -12,6 +12,7 @@ import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest; import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult; import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions; +import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions; import io.opentelemetry.semconv.NetworkAttributes; import java.io.IOException; import java.net.URI; @@ -180,7 +181,16 @@ public void onResponse(Call call, Response response) { trace -> { trace.hasSpansSatisfyingExactly( span -> span.hasName("parent").hasKind(SpanKind.INTERNAL).hasNoParent(), - span -> span.hasName("GET").hasKind(SpanKind.CLIENT).hasParent(trace.getSpan(0)), + span -> + span.hasName("GET") + .hasKind(SpanKind.CLIENT) + .hasParent(trace.getSpan(0)) + .hasAttribute( + OpenTelemetryAssertions.satisfies( + NetworkAttributes.NETWORK_PEER_ADDRESS, + val -> val.isIn("127.0.0.1", "0:0:0:0:0:0:0:1"))) + .hasAttribute( + NetworkAttributes.NETWORK_PEER_PORT, (long) serverAddress().getPort()), span -> span.hasName("test-http-server") .hasKind(SpanKind.SERVER) diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java index e6b671629839..43b448b5b9aa 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java @@ -30,6 +30,7 @@ import io.opentelemetry.semconv.ServerAttributes; import io.opentelemetry.semconv.UrlAttributes; import io.opentelemetry.semconv.UserAgentAttributes; +import java.net.InetSocketAddress; import java.net.URI; import java.time.Duration; import java.util.ArrayList; @@ -87,6 +88,10 @@ void setupOptions() { options = builder.build(); } + protected InetSocketAddress serverAddress() { + return server.httpSocketAddress(); + } + /** * Override this method to configure the {@link HttpClientTestOptions} for the tested HTTP client. */ @@ -999,7 +1004,9 @@ SpanDataAssert assertClientSpan( // TODO: Move to test knob rather than always treating as optional if (attrs.get(NetworkAttributes.NETWORK_PEER_ADDRESS) != null) { assertThat(attrs) - .containsEntry(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"); + .hasEntrySatisfying( + NetworkAttributes.NETWORK_PEER_ADDRESS, + addr -> assertThat(addr).isIn("127.0.0.1", "0:0:0:0:0:0:0:1")); } if (attrs.get(NetworkAttributes.NETWORK_PEER_PORT) != null) { assertThat(attrs) From 85a64cdd71161a8a2b4c4e210e960574db173061 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Serkan=20=C3=96ZAL?= Date: Wed, 21 Aug 2024 20:58:19 +0300 Subject: [PATCH 2/2] Remove network peer address attribute check from `AbstractOkHttp3Test` as they are already tested in `AbstractHttpClientTest` --- .../okhttp/v3_0/AbstractOkHttp3Test.java | 12 +----------- .../testing/junit/http/AbstractHttpClientTest.java | 5 ----- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java b/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java index f3f9a7bb90a4..5f051141f7f5 100644 --- a/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java +++ b/instrumentation/okhttp/okhttp-3.0/testing/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.java @@ -12,7 +12,6 @@ import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest; import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult; import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions; -import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions; import io.opentelemetry.semconv.NetworkAttributes; import java.io.IOException; import java.net.URI; @@ -181,16 +180,7 @@ public void onResponse(Call call, Response response) { trace -> { trace.hasSpansSatisfyingExactly( span -> span.hasName("parent").hasKind(SpanKind.INTERNAL).hasNoParent(), - span -> - span.hasName("GET") - .hasKind(SpanKind.CLIENT) - .hasParent(trace.getSpan(0)) - .hasAttribute( - OpenTelemetryAssertions.satisfies( - NetworkAttributes.NETWORK_PEER_ADDRESS, - val -> val.isIn("127.0.0.1", "0:0:0:0:0:0:0:1"))) - .hasAttribute( - NetworkAttributes.NETWORK_PEER_PORT, (long) serverAddress().getPort()), + span -> span.hasName("GET").hasKind(SpanKind.CLIENT).hasParent(trace.getSpan(0)), span -> span.hasName("test-http-server") .hasKind(SpanKind.SERVER) diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java index 43b448b5b9aa..0e1c576ac731 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java @@ -30,7 +30,6 @@ import io.opentelemetry.semconv.ServerAttributes; import io.opentelemetry.semconv.UrlAttributes; import io.opentelemetry.semconv.UserAgentAttributes; -import java.net.InetSocketAddress; import java.net.URI; import java.time.Duration; import java.util.ArrayList; @@ -88,10 +87,6 @@ void setupOptions() { options = builder.build(); } - protected InetSocketAddress serverAddress() { - return server.httpSocketAddress(); - } - /** * Override this method to configure the {@link HttpClientTestOptions} for the tested HTTP client. */