From f35e2e02fa19715cc62d7c57e1e4c2c4034b8791 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 29 Nov 2023 11:53:10 +0100 Subject: [PATCH 1/3] Implement error.type in spring-webflux and reactor-netty --- .../http/HttpCommonAttributesGetter.java | 2 +- ...eactorNettyHttpClientAttributesGetter.java | 24 ++++----- .../AbstractReactorNettyHttpClientTest.java | 2 +- .../webflux/v5_0/client/WebClientHelper.java | 4 -- .../v5_3/SpringWebfluxTelemetryBuilder.java | 38 +++++--------- .../internal/ClientInstrumenterFactory.java | 8 +-- ...ClientExperimentalAttributesExtractor.java | 43 ---------------- .../WebClientHttpAttributesGetter.java | 12 +++++ ...pringWebfluxClientInstrumentationTest.java | 49 +++++++++++++++++++ 9 files changed, 90 insertions(+), 92 deletions(-) delete mode 100644 instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientExperimentalAttributesExtractor.java diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpCommonAttributesGetter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpCommonAttributesGetter.java index 49b1687a7ee8..e3342f3b86d9 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpCommonAttributesGetter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpCommonAttributesGetter.java @@ -76,7 +76,7 @@ public interface HttpCommonAttributesGetter { */ @Nullable default String getErrorType( - REQUEST request, @Nullable RESPONSE response, @Nullable Throwable throwable) { + REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error) { return null; } } diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyHttpClientAttributesGetter.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyHttpClientAttributesGetter.java index 495e5065624f..2aa7a163d351 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyHttpClientAttributesGetter.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyHttpClientAttributesGetter.java @@ -72,13 +72,15 @@ public String getNetworkProtocolVersion( @Nullable @Override public String getServerAddress(HttpClientRequest request) { - return getHost(request); + String resourceUrl = request.resourceUrl(); + return resourceUrl == null ? null : UrlParser.getHost(resourceUrl); } @Nullable @Override public Integer getServerPort(HttpClientRequest request) { - return getPort(request); + String resourceUrl = request.resourceUrl(); + return resourceUrl == null ? null : UrlParser.getPort(resourceUrl); } @Nullable @@ -99,14 +101,14 @@ public InetSocketAddress getNetworkPeerInetSocketAddress( } @Nullable - private static String getHost(HttpClientRequest request) { - String resourceUrl = request.resourceUrl(); - return resourceUrl == null ? null : UrlParser.getHost(resourceUrl); - } - - @Nullable - private static Integer getPort(HttpClientRequest request) { - String resourceUrl = request.resourceUrl(); - return resourceUrl == null ? null : UrlParser.getPort(resourceUrl); + @Override + public String getErrorType( + HttpClientRequest request, @Nullable HttpClientResponse response, @Nullable Throwable error) { + // if both response and error are null it means the request has been cancelled -- see the + // ConnectionWrapper class + if (response == null && error == null) { + return "cancelled"; + } + return null; } } 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 255ee15d7bcb..93927998494f 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 @@ -310,7 +310,7 @@ void shouldEndSpanOnMonoTimeout() { equalTo(SemanticAttributes.URL_FULL, uri.toString()), equalTo(SemanticAttributes.SERVER_ADDRESS, "localhost"), equalTo(SemanticAttributes.SERVER_PORT, uri.getPort()), - equalTo(HttpAttributes.ERROR_TYPE, "_OTHER")), + equalTo(HttpAttributes.ERROR_TYPE, "cancelled")), span -> span.hasName("test-http-server") .hasKind(SpanKind.SERVER) diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/client/WebClientHelper.java b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/client/WebClientHelper.java index 03f50954ae70..3d77440012a3 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/client/WebClientHelper.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webflux/v5_0/client/WebClientHelper.java @@ -14,7 +14,6 @@ import io.opentelemetry.instrumentation.spring.webflux.v5_3.internal.WebClientHttpAttributesGetter; import io.opentelemetry.instrumentation.spring.webflux.v5_3.internal.WebClientTracingFilter; import io.opentelemetry.javaagent.bootstrap.internal.CommonConfig; -import io.opentelemetry.javaagent.bootstrap.internal.InstrumentationConfig; import java.util.List; import org.springframework.web.reactive.function.client.ClientRequest; import org.springframework.web.reactive.function.client.ClientResponse; @@ -35,9 +34,6 @@ public final class WebClientHelper { HttpClientPeerServiceAttributesExtractor.create( WebClientHttpAttributesGetter.INSTANCE, CommonConfig.get().getPeerServiceResolver())), - InstrumentationConfig.get() - .getBoolean( - "otel.instrumentation.spring-webflux.experimental-span-attributes", false), CommonConfig.get().shouldEmitExperimentalHttpClientTelemetry()); public static void addFilter(List exchangeFilterFunctions) { diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java index 59068361a28c..8908ab2972eb 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/SpringWebfluxTelemetryBuilder.java @@ -53,9 +53,8 @@ public final class SpringWebfluxTelemetryBuilder { clientExtractorConfigurer = builder -> {}; private Consumer> clientSpanNameExtractorConfigurer = builder -> {}; - private boolean captureExperimentalSpanAttributes = false; - private boolean emitExperimentalHttpClientMetrics = false; - private boolean emitExperimentalHttpServerMetrics = false; + private boolean emitExperimentalHttpClientTelemetry = false; + private boolean emitExperimentalHttpServerTelemetry = false; SpringWebfluxTelemetryBuilder(OpenTelemetry openTelemetry) { this.openTelemetry = openTelemetry; @@ -100,18 +99,6 @@ public SpringWebfluxTelemetryBuilder setCapturedClientResponseHeaders( return this; } - /** - * Sets whether experimental attributes should be set to spans. These attributes may be changed or - * removed in the future, so only enable this if you know you do not require attributes filled by - * this instrumentation to be stable across versions. - */ - @CanIgnoreReturnValue - public SpringWebfluxTelemetryBuilder setCaptureExperimentalSpanAttributes( - boolean captureExperimentalSpanAttributes) { - this.captureExperimentalSpanAttributes = captureExperimentalSpanAttributes; - return this; - } - /** * Adds an additional {@link AttributesExtractor} to invoke to set attributes to instrumented * items. @@ -178,26 +165,26 @@ public SpringWebfluxTelemetryBuilder setKnownMethods(Set knownMethods) { /** * Configures the instrumentation to emit experimental HTTP client metrics. * - * @param emitExperimentalHttpClientMetrics {@code true} if the experimental HTTP client metrics + * @param emitExperimentalHttpClientTelemetry {@code true} if the experimental HTTP client metrics * are to be emitted. */ @CanIgnoreReturnValue - public SpringWebfluxTelemetryBuilder setEmitExperimentalHttpClientMetrics( - boolean emitExperimentalHttpClientMetrics) { - this.emitExperimentalHttpClientMetrics = emitExperimentalHttpClientMetrics; + public SpringWebfluxTelemetryBuilder setEmitExperimentalHttpClientTelemetry( + boolean emitExperimentalHttpClientTelemetry) { + this.emitExperimentalHttpClientTelemetry = emitExperimentalHttpClientTelemetry; return this; } /** * Configures the instrumentation to emit experimental HTTP server metrics. * - * @param emitExperimentalHttpServerMetrics {@code true} if the experimental HTTP server metrics + * @param emitExperimentalHttpServerTelemetry {@code true} if the experimental HTTP server metrics * are to be emitted. */ @CanIgnoreReturnValue - public SpringWebfluxTelemetryBuilder setEmitExperimentalHttpServerMetrics( - boolean emitExperimentalHttpServerMetrics) { - this.emitExperimentalHttpServerMetrics = emitExperimentalHttpServerMetrics; + public SpringWebfluxTelemetryBuilder setEmitExperimentalHttpServerTelemetry( + boolean emitExperimentalHttpServerTelemetry) { + this.emitExperimentalHttpServerTelemetry = emitExperimentalHttpServerTelemetry; return this; } @@ -213,8 +200,7 @@ public SpringWebfluxTelemetry build() { clientExtractorConfigurer, clientSpanNameExtractorConfigurer, clientAdditionalExtractors, - captureExperimentalSpanAttributes, - emitExperimentalHttpClientMetrics); + emitExperimentalHttpClientTelemetry); Instrumenter serverInstrumenter = buildServerInstrumenter(); @@ -234,7 +220,7 @@ private Instrumenter buildServerInstrument .addAttributesExtractors(serverAdditionalExtractors) .addContextCustomizer(httpServerRouteBuilder.build()) .addOperationMetrics(HttpServerMetrics.get()); - if (emitExperimentalHttpServerMetrics) { + if (emitExperimentalHttpServerTelemetry) { builder .addAttributesExtractor(HttpExperimentalAttributesExtractor.create(getter)) .addOperationMetrics(HttpServerExperimentalMetrics.get()); diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/ClientInstrumenterFactory.java b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/ClientInstrumenterFactory.java index 454c5c574084..262faa170141 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/ClientInstrumenterFactory.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/ClientInstrumenterFactory.java @@ -40,8 +40,7 @@ public static Instrumenter create( extractorConfigurer, Consumer> spanNameExtractorConfigurer, List> additionalExtractors, - boolean captureExperimentalSpanAttributes, - boolean emitExperimentalHttpClientMetrics) { + boolean emitExperimentalHttpClientTelemetry) { WebClientHttpAttributesGetter httpAttributesGetter = WebClientHttpAttributesGetter.INSTANCE; @@ -61,10 +60,7 @@ public static Instrumenter create( .addAttributesExtractors(additionalExtractors) .addOperationMetrics(HttpClientMetrics.get()); - if (captureExperimentalSpanAttributes) { - clientBuilder.addAttributesExtractor(new WebClientExperimentalAttributesExtractor()); - } - if (emitExperimentalHttpClientMetrics) { + if (emitExperimentalHttpClientTelemetry) { clientBuilder .addAttributesExtractor(HttpExperimentalAttributesExtractor.create(httpAttributesGetter)) .addOperationMetrics(HttpClientExperimentalMetrics.get()); diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientExperimentalAttributesExtractor.java b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientExperimentalAttributesExtractor.java deleted file mode 100644 index e915efedb180..000000000000 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientExperimentalAttributesExtractor.java +++ /dev/null @@ -1,43 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.spring.webflux.v5_3.internal; - -import static io.opentelemetry.api.common.AttributeKey.stringKey; - -import io.opentelemetry.api.common.AttributeKey; -import io.opentelemetry.api.common.AttributesBuilder; -import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; -import javax.annotation.Nullable; -import org.springframework.web.reactive.function.client.ClientRequest; -import org.springframework.web.reactive.function.client.ClientResponse; - -final class WebClientExperimentalAttributesExtractor - implements AttributesExtractor { - - private static final AttributeKey SPRING_WEBFLUX_EVENT = - stringKey("spring-webflux.event"); - private static final AttributeKey SPRING_WEBFLUX_MESSAGE = - stringKey("spring-webflux.message"); - - @Override - public void onStart(AttributesBuilder attributes, Context parentContext, ClientRequest request) {} - - @Override - public void onEnd( - AttributesBuilder attributes, - Context context, - ClientRequest request, - @Nullable ClientResponse response, - @Nullable Throwable error) { - - // no response and no error means that the request has been cancelled - if (response == null && error == null) { - attributes.put(SPRING_WEBFLUX_EVENT, "cancelled"); - attributes.put(SPRING_WEBFLUX_MESSAGE, "The subscription was cancelled"); - } - } -} diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientHttpAttributesGetter.java b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientHttpAttributesGetter.java index 9ca3a37ea15f..80c75f68a61d 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientHttpAttributesGetter.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/v5_3/internal/WebClientHttpAttributesGetter.java @@ -59,4 +59,16 @@ public String getServerAddress(ClientRequest request) { public Integer getServerPort(ClientRequest request) { return request.url().getPort(); } + + @Nullable + @Override + public String getErrorType( + ClientRequest request, @Nullable ClientResponse response, @Nullable Throwable error) { + // if both response and error are null it means the request has been cancelled -- see the + // WebClientTracingFilter class + if (response == null && error == null) { + return "cancelled"; + } + return null; + } } diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java b/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java index 7d01bef49355..c029c924ccb0 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java @@ -5,20 +5,29 @@ package io.opentelemetry.instrumentation.spring.webflux.client; +import static io.opentelemetry.api.trace.SpanKind.CLIENT; +import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo; +import static java.util.Collections.emptyMap; import static java.util.Objects.requireNonNull; +import static org.assertj.core.api.Assertions.catchThrowable; import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.instrumentation.api.instrumenter.http.internal.HttpAttributes; 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.trace.data.StatusData; import io.opentelemetry.semconv.SemanticAttributes; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; import java.net.URI; +import java.time.Duration; import java.util.HashSet; import java.util.Map; import java.util.Set; +import org.junit.jupiter.api.Test; import org.springframework.http.HttpMethod; import org.springframework.web.reactive.function.client.ClientResponse; import org.springframework.web.reactive.function.client.WebClient; @@ -142,4 +151,44 @@ private static int getStatusCode(ClientResponse response) { throw new AssertionError(e); } } + + @Test + void shouldEndSpanOnMonoTimeout() { + URI uri = resolveAddress("/read-timeout"); + Throwable thrown = + catchThrowable( + () -> + testing.runWithSpan( + "parent", + () -> + buildRequest("GET", uri, emptyMap()) + .exchange() + // apply Mono timeout that is way shorter than HTTP request timeout + .timeout(Duration.ofSeconds(1)) + .block())); + + testing.waitAndAssertTraces( + trace -> + trace.hasSpansSatisfyingExactly( + span -> + span.hasName("parent") + .hasKind(SpanKind.INTERNAL) + .hasNoParent() + .hasStatus(StatusData.error()) + .hasException(thrown), + span -> + span.hasName("GET") + .hasKind(CLIENT) + .hasParent(trace.getSpan(0)) + .hasAttributesSatisfyingExactly( + equalTo(SemanticAttributes.HTTP_REQUEST_METHOD, "GET"), + equalTo(SemanticAttributes.URL_FULL, uri.toString()), + equalTo(SemanticAttributes.SERVER_ADDRESS, "localhost"), + equalTo(SemanticAttributes.SERVER_PORT, uri.getPort()), + equalTo(HttpAttributes.ERROR_TYPE, "cancelled")), + span -> + span.hasName("test-http-server") + .hasKind(SpanKind.SERVER) + .hasParent(trace.getSpan(1)))); + } } From 20ba35d659ddffa873cdef129575193008a3ebd6 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 5 Dec 2023 12:18:11 +0100 Subject: [PATCH 2/3] docs --- .../spring-webflux/spring-webflux-5.3/library/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/README.md b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/README.md index 0b177cdcc6cf..a3c7f07aff7c 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/README.md +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.3/library/README.md @@ -45,6 +45,11 @@ interface. a `WebFilter` and using the OpenTelemetry Reactor instrumentation to ensure context is passed around correctly. +### Web client instrumentation + +The `WebClient` instrumentation will emit the `error.type` attribute with value `cancelled` whenever +an outgoing HTTP request is cancelled. + ### Setup Here is how to set up client and server instrumentation respectively: From d4c1965dfcd1e33b9bf86bfd2c24f9d521455d8d Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 5 Dec 2023 13:03:52 +0100 Subject: [PATCH 3/3] fix after rebase --- .../client/AbstractSpringWebfluxClientInstrumentationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java b/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java index c029c924ccb0..5c9a09c59113 100644 --- a/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java +++ b/instrumentation/spring/spring-webflux/spring-webflux-5.3/testing/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/AbstractSpringWebfluxClientInstrumentationTest.java @@ -13,7 +13,7 @@ import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.trace.SpanKind; -import io.opentelemetry.instrumentation.api.instrumenter.http.internal.HttpAttributes; +import io.opentelemetry.instrumentation.api.semconv.http.internal.HttpAttributes; import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest; import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult; import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;