From 1c78bf80ccffe50e47e1d88bb9b74d1c2b61b2e5 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 27 Jul 2023 15:58:22 +0200 Subject: [PATCH 1/3] Reactor Netty: emit actual HTTP client spans spans on connection errors --- .../NettyClientInstrumenterFactory.java | 70 +++++++-------- .../client/NettyInstrumentationFlag.java | 20 +++++ .../client/NoopConnectionInstrumenter.java | 34 ++++++++ .../internal/client/NoopSslInstrumenter.java | 28 ++++++ .../v4_0/client/NettyClientSingletons.java | 6 +- .../netty/v4_1/NettyClientSingletons.java | 6 +- .../v4_1/NettyClientTelemetryBuilder.java | 5 +- .../v1_0/FailedRequestWithUrlMaker.java | 85 +++++++++++++++++++ .../HttpResponseReceiverInstrumenter.java | 22 ++--- .../v1_0/InstrumentationContexts.java | 13 +++ .../v1_0/ReactorNettySingletons.java | 7 +- .../AbstractReactorNettyHttpClientTest.java | 19 +---- .../v1_0/ReactorNettyClientSslTest.java | 2 - .../v1_0/ReactorNettyConnectionSpanTest.java | 2 - 14 files changed, 240 insertions(+), 79 deletions(-) create mode 100644 instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyInstrumentationFlag.java create mode 100644 instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopConnectionInstrumenter.java create mode 100644 instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopSslInstrumenter.java create mode 100644 instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java diff --git a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java index 952a5ec104cb..8a30bd11231a 100644 --- a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java +++ b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java @@ -34,22 +34,22 @@ public final class NettyClientInstrumenterFactory { private final OpenTelemetry openTelemetry; private final String instrumentationName; - private final boolean connectionTelemetryEnabled; - private final boolean sslTelemetryEnabled; + private final NettyInstrumentationFlag connectionTelemetryState; + private final NettyInstrumentationFlag sslTelemetryState; private final Map peerServiceMapping; private final boolean emitExperimentalHttpClientMetrics; public NettyClientInstrumenterFactory( OpenTelemetry openTelemetry, String instrumentationName, - boolean connectionTelemetryEnabled, - boolean sslTelemetryEnabled, + NettyInstrumentationFlag connectionTelemetryState, + NettyInstrumentationFlag sslTelemetryState, Map peerServiceMapping, boolean emitExperimentalHttpClientMetrics) { this.openTelemetry = openTelemetry; this.instrumentationName = instrumentationName; - this.connectionTelemetryEnabled = connectionTelemetryEnabled; - this.sslTelemetryEnabled = sslTelemetryEnabled; + this.connectionTelemetryState = connectionTelemetryState; + this.sslTelemetryState = sslTelemetryState; this.peerServiceMapping = peerServiceMapping; this.emitExperimentalHttpClientMetrics = emitExperimentalHttpClientMetrics; } @@ -83,49 +83,37 @@ public Instrumenter createHttpInstrumenter( } public NettyConnectionInstrumenter createConnectionInstrumenter() { - InstrumenterBuilder instrumenterBuilder = + if (connectionTelemetryState == NettyInstrumentationFlag.DISABLED) { + return NoopConnectionInstrumenter.INSTANCE; + } + + boolean connectionTelemetryFullyEnabled = + connectionTelemetryState == NettyInstrumentationFlag.ENABLED; + + Instrumenter instrumenter = Instrumenter.builder( openTelemetry, instrumentationName, NettyConnectionRequest::spanName) .addAttributesExtractor( PeerServiceAttributesExtractor.create( - NettyConnectHttpAttributesGetter.INSTANCE, peerServiceMapping)); - - if (connectionTelemetryEnabled) { - // TODO: this will most likely be no longer true with the new semconv, since the connection - // phase happens *before* the actual HTTP request is sent over the wire - // TODO (mateusz): refactor this after reactor-netty is fully converted to low-level HTTP - // instrumentation - // when the connection telemetry is enabled, we do not want these CONNECT spans to be - // suppressed by higher-level HTTP spans - this would happen in the reactor-netty - // instrumentation - // the solution for this is to deliberately avoid using the HTTP extractor and use the plain - // net attributes extractor instead - instrumenterBuilder.addAttributesExtractor( - NetClientAttributesExtractor.create(NettyConnectHttpAttributesGetter.INSTANCE)); - } else { - // when the connection telemetry is not enabled, netty creates CONNECT spans whenever a - // connection error occurs - because there is no HTTP span in that scenario, if raw netty - // connection occurs before an HTTP message is even formed - // we don't want that span when a higher-level HTTP library (like reactor-netty or async http - // client) is used, the connection phase is a part of the HTTP span for these - // for that to happen, the CONNECT span will "pretend" to be a full HTTP span when connection - // telemetry is off - instrumenterBuilder.addAttributesExtractor( - HttpClientAttributesExtractor.create(NettyConnectHttpAttributesGetter.INSTANCE)); - } - - Instrumenter instrumenter = - instrumenterBuilder.buildInstrumenter( - connectionTelemetryEnabled - ? SpanKindExtractor.alwaysInternal() - : SpanKindExtractor.alwaysClient()); + NettyConnectHttpAttributesGetter.INSTANCE, peerServiceMapping)) + .addAttributesExtractor( + HttpClientAttributesExtractor.create(NettyConnectHttpAttributesGetter.INSTANCE)) + .buildInstrumenter( + connectionTelemetryFullyEnabled + ? SpanKindExtractor.alwaysInternal() + : SpanKindExtractor.alwaysClient()); - return connectionTelemetryEnabled + return connectionTelemetryFullyEnabled ? new NettyConnectionInstrumenterImpl(instrumenter) : new NettyErrorOnlyConnectionInstrumenter(instrumenter); } public NettySslInstrumenter createSslInstrumenter() { + if (sslTelemetryState == NettyInstrumentationFlag.DISABLED) { + return NoopSslInstrumenter.INSTANCE; + } + + boolean sslTelemetryFullyEnabled = sslTelemetryState == NettyInstrumentationFlag.ENABLED; NettySslNetAttributesGetter netAttributesGetter = new NettySslNetAttributesGetter(); Instrumenter instrumenter = Instrumenter.builder( @@ -134,11 +122,11 @@ public NettySslInstrumenter createSslInstrumenter() { .addAttributesExtractor( PeerServiceAttributesExtractor.create(netAttributesGetter, peerServiceMapping)) .buildInstrumenter( - sslTelemetryEnabled + sslTelemetryFullyEnabled ? SpanKindExtractor.alwaysInternal() : SpanKindExtractor.alwaysClient()); - return sslTelemetryEnabled + return sslTelemetryFullyEnabled ? new NettySslInstrumenterImpl(instrumenter) : new NettySslErrorOnlyInstrumenter(instrumenter); } diff --git a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyInstrumentationFlag.java b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyInstrumentationFlag.java new file mode 100644 index 000000000000..a5fdfbcf6abe --- /dev/null +++ b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyInstrumentationFlag.java @@ -0,0 +1,20 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.netty.v4.common.internal.client; + +/** + * This class is internal and is hence not for public use. Its APIs are unstable and can change at + * any time. + */ +public enum NettyInstrumentationFlag { + ENABLED, + ERROR_ONLY, + DISABLED; + + public static NettyInstrumentationFlag enabledOrErrorOnly(boolean b) { + return b ? ENABLED : ERROR_ONLY; + } +} diff --git a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopConnectionInstrumenter.java b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopConnectionInstrumenter.java new file mode 100644 index 000000000000..bea14d35a73d --- /dev/null +++ b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopConnectionInstrumenter.java @@ -0,0 +1,34 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.netty.v4.common.internal.client; + +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import io.netty.channel.Channel; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.netty.common.internal.NettyConnectionRequest; +import javax.annotation.Nullable; + +enum NoopConnectionInstrumenter implements NettyConnectionInstrumenter { + INSTANCE; + + @Override + public boolean shouldStart(Context parentContext, NettyConnectionRequest request) { + return false; + } + + @CanIgnoreReturnValue + @Override + public Context start(Context parentContext, NettyConnectionRequest request) { + return parentContext; + } + + @Override + public void end( + Context context, + NettyConnectionRequest request, + Channel channel, + @Nullable Throwable error) {} +} diff --git a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopSslInstrumenter.java b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopSslInstrumenter.java new file mode 100644 index 000000000000..cc5bbcdd0669 --- /dev/null +++ b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NoopSslInstrumenter.java @@ -0,0 +1,28 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.netty.v4.common.internal.client; + +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import io.opentelemetry.context.Context; +import javax.annotation.Nullable; + +enum NoopSslInstrumenter implements NettySslInstrumenter { + INSTANCE; + + @Override + public boolean shouldStart(Context parentContext, NettySslRequest request) { + return false; + } + + @CanIgnoreReturnValue + @Override + public Context start(Context parentContext, NettySslRequest request) { + return parentContext; + } + + @Override + public void end(Context context, NettySslRequest request, @Nullable Throwable error) {} +} diff --git a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java index cd3abdef0a7e..be82123486dd 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java +++ b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.instrumentation.netty.v4_0.client; +import static io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag.enabledOrErrorOnly; + import io.netty.handler.codec.http.HttpResponse; import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; @@ -43,8 +45,8 @@ public final class NettyClientSingletons { new NettyClientInstrumenterFactory( GlobalOpenTelemetry.get(), "io.opentelemetry.netty-4.0", - connectionTelemetryEnabled, - sslTelemetryEnabled, + enabledOrErrorOnly(connectionTelemetryEnabled), + enabledOrErrorOnly(sslTelemetryEnabled), CommonConfig.get().getPeerServiceMapping(), CommonConfig.get().shouldEmitExperimentalHttpClientMetrics()); INSTRUMENTER = diff --git a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java index 322ef142bb13..02acd8423b46 100644 --- a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java +++ b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.instrumentation.netty.v4_1; +import static io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag.enabledOrErrorOnly; + import io.netty.handler.codec.http.HttpResponse; import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; @@ -43,8 +45,8 @@ public final class NettyClientSingletons { new NettyClientInstrumenterFactory( GlobalOpenTelemetry.get(), "io.opentelemetry.netty-4.1", - connectionTelemetryEnabled, - sslTelemetryEnabled, + enabledOrErrorOnly(connectionTelemetryEnabled), + enabledOrErrorOnly(sslTelemetryEnabled), CommonConfig.get().getPeerServiceMapping(), CommonConfig.get().shouldEmitExperimentalHttpClientMetrics()); INSTRUMENTER = diff --git a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java index ea8050d1640e..07aadaf99ae3 100644 --- a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java +++ b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java @@ -12,6 +12,7 @@ import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractorBuilder; import io.opentelemetry.instrumentation.netty.v4.common.HttpRequestAndChannel; import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyClientInstrumenterFactory; +import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -111,8 +112,8 @@ public NettyClientTelemetry build() { new NettyClientInstrumenterFactory( openTelemetry, "io.opentelemetry.netty-4.1", - false, - false, + NettyInstrumentationFlag.DISABLED, + NettyInstrumentationFlag.DISABLED, Collections.emptyMap(), emitExperimentalHttpClientMetrics) .createHttpInstrumenter(extractorConfigurer, additionalAttributesExtractors)); diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java new file mode 100644 index 000000000000..b03a6756d9bb --- /dev/null +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java @@ -0,0 +1,85 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.reactornetty.v1_0; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import reactor.netty.http.client.HttpClientConfig; +import reactor.netty.http.client.HttpClientRequest; + +final class FailedRequestWithUrlMaker { + + static HttpClientRequest create(HttpClientConfig config, HttpClientRequest failedRequest) { + return (HttpClientRequest) + Proxy.newProxyInstance( + FailedRequestWithUrlMaker.class.getClassLoader(), + new Class[] {HttpClientRequest.class}, + new HttpRequestInvocationHandler(config, failedRequest)); + } + + private static final class HttpRequestInvocationHandler implements InvocationHandler { + + private final HttpClientConfig config; + private final HttpClientRequest failedRequest; + + private HttpRequestInvocationHandler(HttpClientConfig config, HttpClientRequest failedRequest) { + this.config = config; + this.failedRequest = failedRequest; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if ("resourceUrl".equals(method.getName())) { + return computeUrlFromConfig(); + } + try { + return method.invoke(failedRequest, args); + } catch (InvocationTargetException exception) { + throw exception.getCause(); + } + } + + private String computeUrlFromConfig() { + String uri = config.uri(); + if (isAbsolute(uri)) { + return uri; + } + + // use the baseUrl if it was configured + String baseUrl = config.baseUrl(); + if (baseUrl != null) { + if (baseUrl.endsWith("/") && uri.startsWith("/")) { + baseUrl = baseUrl.substring(0, baseUrl.length() - 1); + } + return baseUrl + uri; + } + + // otherwise, use the host+port config to construct the full url + SocketAddress hostAddress = config.remoteAddress().get(); + if (hostAddress instanceof InetSocketAddress) { + InetSocketAddress inetHostAddress = (InetSocketAddress) hostAddress; + return (config.isSecure() ? "https://" : "http://") + + inetHostAddress.getHostString() + + ":" + + inetHostAddress.getPort() + + (uri.startsWith("/") ? "" : "/") + + uri; + } + + return uri; + } + + private static boolean isAbsolute(String uri) { + return uri != null && !uri.isEmpty() && !uri.startsWith("/"); + } + } + + private FailedRequestWithUrlMaker() {} +} diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/HttpResponseReceiverInstrumenter.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/HttpResponseReceiverInstrumenter.java index 86f7e3ec9c21..e2524adab5f3 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/HttpResponseReceiverInstrumenter.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/HttpResponseReceiverInstrumenter.java @@ -16,6 +16,7 @@ import reactor.core.publisher.Mono; import reactor.netty.Connection; import reactor.netty.http.client.HttpClient; +import reactor.netty.http.client.HttpClientConfig; import reactor.netty.http.client.HttpClientRequest; import reactor.netty.http.client.HttpClientResponse; @@ -31,13 +32,14 @@ public static HttpClient.ResponseReceiver instrument(HttpClient.ResponseRecei // implements ResponseReceiver if (receiver instanceof HttpClient) { HttpClient client = (HttpClient) receiver; + HttpClientConfig config = client.configuration(); InstrumentationContexts instrumentationContexts = new InstrumentationContexts(); HttpClient modified = client .mapConnect(new CaptureParentContext(instrumentationContexts)) - .doOnRequestError(new EndOperationWithRequestError(instrumentationContexts)) + .doOnRequestError(new EndOperationWithRequestError(config, instrumentationContexts)) .doOnRequest(new StartOperation(instrumentationContexts)) .doOnResponseError(new EndOperationWithResponseError(instrumentationContexts)) .doAfterResponseSuccess(new EndOperationWithSuccess(instrumentationContexts)) @@ -103,9 +105,12 @@ public void accept(HttpClientRequest request, Connection connection) { private static final class EndOperationWithRequestError implements BiConsumer { + private final HttpClientConfig config; private final InstrumentationContexts instrumentationContexts; - EndOperationWithRequestError(InstrumentationContexts instrumentationContexts) { + EndOperationWithRequestError( + HttpClientConfig config, InstrumentationContexts instrumentationContexts) { + this.config = config; this.instrumentationContexts = instrumentationContexts; } @@ -114,15 +119,10 @@ public void accept(HttpClientRequest request, Throwable error) { instrumentationContexts.endClientSpan(null, error); if (HttpClientResend.get(instrumentationContexts.getParentContext()) == 0) { - // TODO: emit connection error span - - // FIXME: this branch requires lots of changes around the NettyConnectionInstrumenter - // currently it also creates that connection error span (when the connection telemetry is - // turned off), but without HTTP semantics - it does not have access to any HTTP information - // after all - // it should be possible to completely disable it, and just start and end the span here - // this requires lots of refactoring and pretty uninteresting changes in the netty code, so - // I'll do that in a separate PR - for better readability + // request is an instance of FailedHttpClientRequest, which does not implement a correct + // resourceUrl() method -- we have to work around that + request = FailedRequestWithUrlMaker.create(config, request); + instrumentationContexts.startAndEndConnectionErrorSpan(request, error); } } } diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/InstrumentationContexts.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/InstrumentationContexts.java index 34f1252daf14..d87e59d125ac 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/InstrumentationContexts.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/InstrumentationContexts.java @@ -9,6 +9,8 @@ import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientResend; +import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil; +import io.opentelemetry.instrumentation.api.internal.Timer; import java.util.Queue; import java.util.concurrent.LinkedBlockingQueue; import javax.annotation.Nullable; @@ -18,6 +20,7 @@ final class InstrumentationContexts { private volatile Context parentContext; + private volatile Timer timer; // on retries, reactor-netty starts the next resend attempt before it ends the previous one (i.e. // it calls the callback functions in that order); thus for a short moment there can be multiple // coexisting HTTP client spans @@ -25,6 +28,7 @@ final class InstrumentationContexts { void initialize(Context parentContext) { this.parentContext = HttpClientResend.initialize(parentContext); + timer = Timer.start(); } Context getParentContext() { @@ -55,6 +59,15 @@ void endClientSpan(@Nullable HttpClientResponse response, @Nullable Throwable er } } + void startAndEndConnectionErrorSpan(HttpClientRequest request, Throwable error) { + Context parentContext = this.parentContext; + if (instrumenter().shouldStart(parentContext, request)) { + Timer timer = this.timer; + InstrumenterUtil.startAndEnd( + instrumenter(), parentContext, request, null, error, timer.startTime(), timer.now()); + } + } + static final class RequestAndContext { final HttpClientRequest request; final Context context; diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java index ca04206e065e..c77f078ecd84 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java @@ -16,6 +16,7 @@ import io.opentelemetry.instrumentation.api.instrumenter.net.PeerServiceAttributesExtractor; import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyClientInstrumenterFactory; import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyConnectionInstrumenter; +import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag; import io.opentelemetry.javaagent.bootstrap.internal.CommonConfig; import io.opentelemetry.javaagent.bootstrap.internal.DeprecatedConfigProperties; import io.opentelemetry.javaagent.bootstrap.internal.InstrumentationConfig; @@ -70,8 +71,10 @@ public final class ReactorNettySingletons { new NettyClientInstrumenterFactory( GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, - connectionTelemetryEnabled, - false, + connectionTelemetryEnabled + ? NettyInstrumentationFlag.ENABLED + : NettyInstrumentationFlag.DISABLED, + NettyInstrumentationFlag.DISABLED, CommonConfig.get().getPeerServiceMapping(), CommonConfig.get().shouldEmitExperimentalHttpClientMetrics()); CONNECTION_INSTRUMENTER = instrumenterFactory.createConnectionInstrumenter(); diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/AbstractReactorNettyHttpClientTest.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/AbstractReactorNettyHttpClientTest.java index 167b9e8629b9..ad82083281c4 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/AbstractReactorNettyHttpClientTest.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/AbstractReactorNettyHttpClientTest.java @@ -10,7 +10,6 @@ import static io.opentelemetry.api.trace.SpanKind.SERVER; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo; -import static java.util.Collections.emptySet; import static org.assertj.core.api.Assertions.catchThrowable; import io.netty.handler.codec.http.HttpMethod; @@ -124,29 +123,19 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) { return exception; }); - // TODO: see the comment in HttpResponseReceiverInstrumenter.EndOperationWithRequestError - optionsBuilder.setExpectedClientSpanNameMapper( - AbstractReactorNettyHttpClientTest::getExpectedClientSpanName); optionsBuilder.setHttpAttributes(this::getHttpAttributes); } - private static String getExpectedClientSpanName(URI uri, String method) { - // unopened port or non routable address - if ("http://localhost:61/".equals(uri.toString()) - || "https://192.0.2.1/".equals(uri.toString())) { - return "CONNECT"; - } - return HttpClientTestOptions.DEFAULT_EXPECTED_CLIENT_SPAN_NAME_MAPPER.apply(uri, method); - } - protected Set> getHttpAttributes(URI uri) { + Set> attributes = new HashSet<>(HttpClientTestOptions.DEFAULT_HTTP_ATTRIBUTES); + // unopened port or non routable address if ("http://localhost:61/".equals(uri.toString()) || "https://192.0.2.1/".equals(uri.toString())) { - return emptySet(); + attributes.remove(SemanticAttributes.NET_PROTOCOL_NAME); + attributes.remove(SemanticAttributes.NET_PROTOCOL_VERSION); } - Set> attributes = new HashSet<>(HttpClientTestOptions.DEFAULT_HTTP_ATTRIBUTES); if (uri.toString().contains("/read-timeout")) { attributes.remove(SemanticAttributes.NET_PROTOCOL_NAME); attributes.remove(SemanticAttributes.NET_PROTOCOL_VERSION); diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyClientSslTest.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyClientSslTest.java index e35dcfc88c27..8bef9816a566 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyClientSslTest.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyClientSslTest.java @@ -81,7 +81,6 @@ void shouldFailSslHandshake() throws SSLException { .hasNoParent() .hasStatus(StatusData.error()) .hasException(thrown), - /* FIXME: this span will be brought back in the next PR, when connection error spans are reintroduced span -> span.hasName("GET") .hasKind(CLIENT) @@ -95,7 +94,6 @@ void shouldFailSslHandshake() throws SSLException { equalTo(SemanticAttributes.HTTP_URL, uri), equalTo(SemanticAttributes.NET_PEER_NAME, "localhost"), equalTo(SemanticAttributes.NET_PEER_PORT, server.httpsPort())), - */ span -> span.hasName("RESOLVE") .hasKind(INTERNAL) diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyConnectionSpanTest.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyConnectionSpanTest.java index 04abf0bd56ae..8ac6a7ed13eb 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyConnectionSpanTest.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyConnectionSpanTest.java @@ -148,7 +148,6 @@ void testFailingRequest() { .hasNoParent() .hasStatus(StatusData.error()) .hasException(thrown), - /* FIXME: this span will be brought back in the next PR, when connection error spans are reintroduced span -> span.hasName("GET") .hasKind(CLIENT) @@ -160,7 +159,6 @@ void testFailingRequest() { equalTo(SemanticAttributes.HTTP_URL, uri), equalTo(SemanticAttributes.NET_PEER_NAME, "localhost"), equalTo(SemanticAttributes.NET_PEER_PORT, PortUtils.UNUSABLE_PORT)), - */ span -> span.hasName("RESOLVE") .hasKind(INTERNAL) From 25eb3757117d15a1fae04cabef1aa6f9d64e2235 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Fri, 28 Jul 2023 09:30:45 +0200 Subject: [PATCH 2/3] rename to NettyConnectionInstrumentationFlag --- .../client/NettyClientInstrumenterFactory.java | 17 +++++++++-------- ... => NettyConnectionInstrumentationFlag.java} | 4 ++-- .../v4_0/client/NettyClientSingletons.java | 2 +- .../netty/v4_1/NettyClientSingletons.java | 2 +- .../netty/v4_1/NettyClientTelemetryBuilder.java | 6 +++--- .../v1_0/ReactorNettySingletons.java | 8 ++++---- 6 files changed, 20 insertions(+), 19 deletions(-) rename instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/{NettyInstrumentationFlag.java => NettyConnectionInstrumentationFlag.java} (73%) diff --git a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java index 8a30bd11231a..1420d63c0d33 100644 --- a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java +++ b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyClientInstrumenterFactory.java @@ -34,16 +34,16 @@ public final class NettyClientInstrumenterFactory { private final OpenTelemetry openTelemetry; private final String instrumentationName; - private final NettyInstrumentationFlag connectionTelemetryState; - private final NettyInstrumentationFlag sslTelemetryState; + private final NettyConnectionInstrumentationFlag connectionTelemetryState; + private final NettyConnectionInstrumentationFlag sslTelemetryState; private final Map peerServiceMapping; private final boolean emitExperimentalHttpClientMetrics; public NettyClientInstrumenterFactory( OpenTelemetry openTelemetry, String instrumentationName, - NettyInstrumentationFlag connectionTelemetryState, - NettyInstrumentationFlag sslTelemetryState, + NettyConnectionInstrumentationFlag connectionTelemetryState, + NettyConnectionInstrumentationFlag sslTelemetryState, Map peerServiceMapping, boolean emitExperimentalHttpClientMetrics) { this.openTelemetry = openTelemetry; @@ -83,12 +83,12 @@ public Instrumenter createHttpInstrumenter( } public NettyConnectionInstrumenter createConnectionInstrumenter() { - if (connectionTelemetryState == NettyInstrumentationFlag.DISABLED) { + if (connectionTelemetryState == NettyConnectionInstrumentationFlag.DISABLED) { return NoopConnectionInstrumenter.INSTANCE; } boolean connectionTelemetryFullyEnabled = - connectionTelemetryState == NettyInstrumentationFlag.ENABLED; + connectionTelemetryState == NettyConnectionInstrumentationFlag.ENABLED; Instrumenter instrumenter = Instrumenter.builder( @@ -109,11 +109,12 @@ public NettyConnectionInstrumenter createConnectionInstrumenter() { } public NettySslInstrumenter createSslInstrumenter() { - if (sslTelemetryState == NettyInstrumentationFlag.DISABLED) { + if (sslTelemetryState == NettyConnectionInstrumentationFlag.DISABLED) { return NoopSslInstrumenter.INSTANCE; } - boolean sslTelemetryFullyEnabled = sslTelemetryState == NettyInstrumentationFlag.ENABLED; + boolean sslTelemetryFullyEnabled = + sslTelemetryState == NettyConnectionInstrumentationFlag.ENABLED; NettySslNetAttributesGetter netAttributesGetter = new NettySslNetAttributesGetter(); Instrumenter instrumenter = Instrumenter.builder( diff --git a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyInstrumentationFlag.java b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyConnectionInstrumentationFlag.java similarity index 73% rename from instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyInstrumentationFlag.java rename to instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyConnectionInstrumentationFlag.java index a5fdfbcf6abe..58400aec230d 100644 --- a/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyInstrumentationFlag.java +++ b/instrumentation/netty/netty-4-common/library/src/main/java/io/opentelemetry/instrumentation/netty/v4/common/internal/client/NettyConnectionInstrumentationFlag.java @@ -9,12 +9,12 @@ * This class is internal and is hence not for public use. Its APIs are unstable and can change at * any time. */ -public enum NettyInstrumentationFlag { +public enum NettyConnectionInstrumentationFlag { ENABLED, ERROR_ONLY, DISABLED; - public static NettyInstrumentationFlag enabledOrErrorOnly(boolean b) { + public static NettyConnectionInstrumentationFlag enabledOrErrorOnly(boolean b) { return b ? ENABLED : ERROR_ONLY; } } diff --git a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java index be82123486dd..bac89dbe1a6f 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java +++ b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java @@ -5,7 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.netty.v4_0.client; -import static io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag.enabledOrErrorOnly; +import static io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyConnectionInstrumentationFlag.enabledOrErrorOnly; import io.netty.handler.codec.http.HttpResponse; import io.opentelemetry.api.GlobalOpenTelemetry; diff --git a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java index 02acd8423b46..3842aa3eea7b 100644 --- a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java +++ b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyClientSingletons.java @@ -5,7 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.netty.v4_1; -import static io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag.enabledOrErrorOnly; +import static io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyConnectionInstrumentationFlag.enabledOrErrorOnly; import io.netty.handler.codec.http.HttpResponse; import io.opentelemetry.api.GlobalOpenTelemetry; diff --git a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java index 07aadaf99ae3..b9bca46de56a 100644 --- a/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java +++ b/instrumentation/netty/netty-4.1/library/src/main/java/io/opentelemetry/instrumentation/netty/v4_1/NettyClientTelemetryBuilder.java @@ -12,7 +12,7 @@ import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractorBuilder; import io.opentelemetry.instrumentation.netty.v4.common.HttpRequestAndChannel; import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyClientInstrumenterFactory; -import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag; +import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyConnectionInstrumentationFlag; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -112,8 +112,8 @@ public NettyClientTelemetry build() { new NettyClientInstrumenterFactory( openTelemetry, "io.opentelemetry.netty-4.1", - NettyInstrumentationFlag.DISABLED, - NettyInstrumentationFlag.DISABLED, + NettyConnectionInstrumentationFlag.DISABLED, + NettyConnectionInstrumentationFlag.DISABLED, Collections.emptyMap(), emitExperimentalHttpClientMetrics) .createHttpInstrumenter(extractorConfigurer, additionalAttributesExtractors)); diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java index c77f078ecd84..0d36c7de6b4e 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettySingletons.java @@ -15,8 +15,8 @@ import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor; import io.opentelemetry.instrumentation.api.instrumenter.net.PeerServiceAttributesExtractor; import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyClientInstrumenterFactory; +import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyConnectionInstrumentationFlag; import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyConnectionInstrumenter; -import io.opentelemetry.instrumentation.netty.v4.common.internal.client.NettyInstrumentationFlag; import io.opentelemetry.javaagent.bootstrap.internal.CommonConfig; import io.opentelemetry.javaagent.bootstrap.internal.DeprecatedConfigProperties; import io.opentelemetry.javaagent.bootstrap.internal.InstrumentationConfig; @@ -72,9 +72,9 @@ public final class ReactorNettySingletons { GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, connectionTelemetryEnabled - ? NettyInstrumentationFlag.ENABLED - : NettyInstrumentationFlag.DISABLED, - NettyInstrumentationFlag.DISABLED, + ? NettyConnectionInstrumentationFlag.ENABLED + : NettyConnectionInstrumentationFlag.DISABLED, + NettyConnectionInstrumentationFlag.DISABLED, CommonConfig.get().getPeerServiceMapping(), CommonConfig.get().shouldEmitExperimentalHttpClientMetrics()); CONNECTION_INSTRUMENTER = instrumenterFactory.createConnectionInstrumenter(); From e79c943fff839c51ed8a42137cd80e2ee8bef2f9 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Mon, 31 Jul 2023 09:16:32 +0200 Subject: [PATCH 3/3] code review comments --- .../javaagent-unit-tests/build.gradle.kts | 1 + .../v1_0/FailedRequestWithUrlMakerTest.java | 92 +++++++++++++++++++ .../v1_0/FailedRequestWithUrlMaker.java | 15 ++- 3 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMakerTest.java diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/build.gradle.kts b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/build.gradle.kts index 653d5f5fbae1..d1d70c210673 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/build.gradle.kts +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/build.gradle.kts @@ -4,4 +4,5 @@ plugins { dependencies { testImplementation(project(":instrumentation:reactor:reactor-netty:reactor-netty-1.0:javaagent")) + testImplementation("io.projectreactor.netty:reactor-netty-http:1.0.0") } diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMakerTest.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMakerTest.java new file mode 100644 index 000000000000..25ccb781c950 --- /dev/null +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMakerTest.java @@ -0,0 +1,92 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.reactornetty.v1_0; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.when; + +import java.net.InetSocketAddress; +import java.util.function.Supplier; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.ArgumentsProvider; +import org.junit.jupiter.params.provider.ArgumentsSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import reactor.netty.http.client.HttpClientConfig; +import reactor.netty.http.client.HttpClientRequest; + +@ExtendWith(MockitoExtension.class) +class FailedRequestWithUrlMakerTest { + + @Mock HttpClientConfig config; + @Mock HttpClientRequest originalRequest; + + @Test + void shouldUseAbsoluteUri() { + when(config.uri()).thenReturn("https://opentelemetry.io"); + + HttpClientRequest request = FailedRequestWithUrlMaker.create(config, originalRequest); + + assertThat(request.resourceUrl()).isEqualTo("https://opentelemetry.io"); + } + + @ParameterizedTest + @ValueSource(strings = {"https://opentelemetry.io", "https://opentelemetry.io/"}) + void shouldPrependBaseUrl(String baseUrl) { + when(config.baseUrl()).thenReturn(baseUrl); + when(config.uri()).thenReturn("/docs"); + + HttpClientRequest request = FailedRequestWithUrlMaker.create(config, originalRequest); + + assertThat(request.resourceUrl()).isEqualTo("https://opentelemetry.io/docs"); + } + + @Test + void shouldPrependRemoteAddress() { + when(config.baseUrl()).thenReturn("/"); + when(config.uri()).thenReturn("/docs"); + Supplier remoteAddress = + () -> InetSocketAddress.createUnresolved("opentelemetry.io", 8080); + doReturn(remoteAddress).when(config).remoteAddress(); + when(config.isSecure()).thenReturn(true); + + HttpClientRequest request = FailedRequestWithUrlMaker.create(config, originalRequest); + + assertThat(request.resourceUrl()).isEqualTo("https://opentelemetry.io:8080/docs"); + } + + @ParameterizedTest + @ArgumentsSource(DefaultPortsArguments.class) + void shouldSkipDefaultPorts(int port, boolean isSecure) { + when(config.baseUrl()).thenReturn("/"); + when(config.uri()).thenReturn("/docs"); + Supplier remoteAddress = + () -> InetSocketAddress.createUnresolved("opentelemetry.io", port); + doReturn(remoteAddress).when(config).remoteAddress(); + when(config.isSecure()).thenReturn(isSecure); + + HttpClientRequest request = FailedRequestWithUrlMaker.create(config, originalRequest); + + assertThat(request.resourceUrl()) + .isEqualTo((isSecure ? "https" : "http") + "://opentelemetry.io/docs"); + } + + static final class DefaultPortsArguments implements ArgumentsProvider { + + @Override + public Stream provideArguments(ExtensionContext extensionContext) { + return Stream.of(arguments(80, false), arguments(443, true)); + } + } +} diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java index b03a6756d9bb..1b033346674b 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java @@ -54,8 +54,9 @@ private String computeUrlFromConfig() { // use the baseUrl if it was configured String baseUrl = config.baseUrl(); - if (baseUrl != null) { - if (baseUrl.endsWith("/") && uri.startsWith("/")) { + // baseUrl is an actual scheme+host+port base url, and not just "/" + if (baseUrl != null && baseUrl.length() > 1) { + if (baseUrl.endsWith("/")) { baseUrl = baseUrl.substring(0, baseUrl.length() - 1); } return baseUrl + uri; @@ -67,9 +68,7 @@ private String computeUrlFromConfig() { InetSocketAddress inetHostAddress = (InetSocketAddress) hostAddress; return (config.isSecure() ? "https://" : "http://") + inetHostAddress.getHostString() - + ":" - + inetHostAddress.getPort() - + (uri.startsWith("/") ? "" : "/") + + computePortPart(inetHostAddress.getPort()) + uri; } @@ -79,6 +78,12 @@ private String computeUrlFromConfig() { private static boolean isAbsolute(String uri) { return uri != null && !uri.isEmpty() && !uri.startsWith("/"); } + + private String computePortPart(int port) { + boolean defaultPortValue = + (config.isSecure() && port == 443) || (!config.isSecure() && port == 80); + return defaultPortValue ? "" : (":" + port); + } } private FailedRequestWithUrlMaker() {}