From b07beb044b697e237e6492bc172baf568297debe Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 11 Dec 2022 15:17:35 -0800 Subject: [PATCH] Fix NullPointerException when uri is null --- ...eactorNettyHttpClientAttributesGetter.java | 8 +- ...ReactorNettyNetClientAttributesGetter.java | 8 +- .../v1_0/ReactorNettyBaseUrlOnlyTest.java | 123 ++++++++++++++++++ 3 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyBaseUrlOnlyTest.java 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 b20577946a51..bc045b334ca8 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 @@ -25,6 +25,12 @@ public String url(HttpClientConfig request) { // use the baseUrl if it was configured String baseUrl = request.baseUrl(); + + if (uri == null) { + // internally reactor netty appends "/" to the baseUrl + return baseUrl.endsWith("/") ? baseUrl : baseUrl + "/"; + } + if (baseUrl != null) { if (baseUrl.endsWith("/") && uri.startsWith("/")) { baseUrl = baseUrl.substring(0, baseUrl.length() - 1); @@ -48,7 +54,7 @@ public String url(HttpClientConfig request) { } private static boolean isAbsolute(String uri) { - return uri.startsWith("http://") || uri.startsWith("https://"); + return uri != null && !uri.isEmpty() && !uri.startsWith("/"); } @Nullable diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyNetClientAttributesGetter.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyNetClientAttributesGetter.java index 3ea477c3e5bd..35b265dfdda8 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyNetClientAttributesGetter.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyNetClientAttributesGetter.java @@ -56,7 +56,7 @@ private static String getHost(HttpClientConfig request) { String baseUrl = request.baseUrl(); String uri = request.uri(); - if (baseUrl != null && uri.startsWith("/")) { + if (baseUrl != null && !isAbsolute(uri)) { return UrlParser.getHost(baseUrl); } else { return UrlParser.getHost(uri); @@ -68,10 +68,14 @@ private static Integer getPort(HttpClientConfig request) { String baseUrl = request.baseUrl(); String uri = request.uri(); - if (baseUrl != null && uri.startsWith("/")) { + if (baseUrl != null && !isAbsolute(uri)) { return UrlParser.getPort(baseUrl); } else { return UrlParser.getPort(uri); } } + + private static boolean isAbsolute(String uri) { + return uri != null && !uri.isEmpty() && !uri.startsWith("/"); + } } diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyBaseUrlOnlyTest.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyBaseUrlOnlyTest.java new file mode 100644 index 000000000000..392e0c84f55f --- /dev/null +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyBaseUrlOnlyTest.java @@ -0,0 +1,123 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.reactornetty.v1_0; + +import static io.opentelemetry.api.trace.SpanKind.CLIENT; +import static io.opentelemetry.api.trace.SpanKind.INTERNAL; +import static io.opentelemetry.api.trace.SpanKind.SERVER; +import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo; +import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies; +import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.HttpFlavorValues.HTTP_1_1; +import static org.assertj.core.api.Assertions.assertThat; + +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.trace.SpanBuilder; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.test.server.http.RequestContextGetter; +import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension; +import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import io.opentelemetry.testing.internal.armeria.common.HttpData; +import io.opentelemetry.testing.internal.armeria.common.HttpResponse; +import io.opentelemetry.testing.internal.armeria.common.HttpStatus; +import io.opentelemetry.testing.internal.armeria.common.ResponseHeaders; +import io.opentelemetry.testing.internal.armeria.common.ResponseHeadersBuilder; +import io.opentelemetry.testing.internal.armeria.server.ServerBuilder; +import io.opentelemetry.testing.internal.armeria.testing.junit5.server.ServerExtension; +import org.assertj.core.api.AbstractLongAssert; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import reactor.netty.http.client.HttpClient; + +class ReactorNettyBaseUrlOnlyTest { + + @RegisterExtension + static final InstrumentationExtension testing = AgentInstrumentationExtension.create(); + + static ServerExtension server; + + @BeforeAll + static void setUp() { + server = + new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) { + sb.service( + "/base/", + (ctx, req) -> { + ResponseHeadersBuilder headers = ResponseHeaders.builder(HttpStatus.OK); + + SpanBuilder span = + GlobalOpenTelemetry.getTracer("test") + .spanBuilder("test-http-server") + .setSpanKind(SERVER) + .setParent( + GlobalOpenTelemetry.getPropagators() + .getTextMapPropagator() + .extract(Context.current(), ctx, RequestContextGetter.INSTANCE)); + + span.startSpan().end(); + + return HttpResponse.of(headers.build(), HttpData.ofAscii("Hello.")); + }); + } + }; + server.start(); + } + + @AfterAll + static void tearDown() { + server.stop(); + } + + @Test + void testSuccessfulRequest() { + HttpClient httpClient = HttpClient.create(); + String uri = "http://localhost:" + server.httpPort() + "/base"; + + int responseCode = + testing.runWithSpan( + "parent", + () -> + httpClient + .baseUrl(uri) + .get() + .responseSingle( + (resp, content) -> { + // Make sure to consume content since that's when we close the span. + return content.map(unused -> resp); + }) + .block() + .status() + .code()); + + assertThat(responseCode).isEqualTo(200); + + testing.waitAndAssertTraces( + trace -> + trace.hasSpansSatisfyingExactlyInAnyOrder( + span -> span.hasName("parent").hasKind(INTERNAL).hasNoParent(), + span -> + span.hasName("HTTP GET") + .hasKind(CLIENT) + .hasParent(trace.getSpan(0)) + .hasAttributesSatisfyingExactly( + equalTo(SemanticAttributes.HTTP_METHOD, "GET"), + equalTo(SemanticAttributes.HTTP_URL, uri + "/"), + equalTo(SemanticAttributes.HTTP_FLAVOR, HTTP_1_1), + equalTo(SemanticAttributes.HTTP_STATUS_CODE, 200), + satisfies( + SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH, + AbstractLongAssert::isNotNegative), + equalTo(SemanticAttributes.NET_PEER_NAME, "localhost"), + equalTo(SemanticAttributes.NET_PEER_PORT, server.httpPort()), + equalTo(SemanticAttributes.NET_SOCK_PEER_ADDR, "127.0.0.1")), + span -> + span.hasName("test-http-server").hasKind(SERVER).hasParent(trace.getSpan(1)))); + } +}