From e79c943fff839c51ed8a42137cd80e2ee8bef2f9 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Mon, 31 Jul 2023 09:16:32 +0200 Subject: [PATCH] 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() {}