code review comments

open-telemetry · Aug 1, 2023 · e79c943 · e79c943
1 parent 25eb375
commit e79c943
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 5 deletions.
diff --git a/...rumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent-unit-tests/build.gradle.kts b/...rumentation/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/...ntelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMakerTest.java b/...ntelemetry/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<InetSocketAddress> 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<InetSocketAddress> 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<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+      return Stream.of(arguments(80, false), arguments(443, true));
+    }
+  }
+}
diff --git a/.../opentelemetry/javaagent/instrumentation/reactornetty/v1_0/FailedRequestWithUrlMaker.java b/.../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() {}