opentelemetry-http: fix url extraction

servicetalk-opentelemetry-http/build.gradle

@@ -50,6 +50,7 @@ dependencies {
   testImplementation "io.opentelemetry:opentelemetry-sdk-trace"
   testImplementation "io.opentelemetry:opentelemetry-sdk-testing"
   testImplementation "io.opentelemetry.semconv:opentelemetry-semconv"
+  testImplementation "org.hamcrest:hamcrest:$hamcrestVersion"
   testImplementation "org.junit.jupiter:junit-jupiter-api"
   testImplementation "org.junit.jupiter:junit-jupiter-params"
   testImplementation "org.assertj:assertj-core:$assertJCoreVersion"

servicetalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetter.java

@@ -78,7 +78,6 @@ public List<String> getHttpResponseHeader(
     }
 
     @Override
-    @Nullable
     public final String getNetworkProtocolName(
             final HttpRequestMetaData request, @Nullable final HttpResponseMetaData response) {
         return HTTP_SCHEME;
@@ -117,35 +116,34 @@ private static final class ClientGetter extends ServiceTalkHttpAttributesGetter
         @Override
         @Nullable
         public String getUrlFull(final HttpRequestMetaData request) {
+            String requestTarget = request.requestTarget();
+            if (requestTarget.startsWith("https://") || requestTarget.startsWith("http://")) {
+                // request target is already absolute-form: just return it.
+                return requestTarget;
+            }
+
+            // in this case the request target is most likely origin-form so we need to convert it to absolute-form.
             HostAndPort effectiveHostAndPort = request.effectiveHostAndPort();
             if (effectiveHostAndPort == null) {
+                // we cant create the authority so we must just return.
                 return null;
             }
             String scheme = request.scheme();
             if (scheme == null) {
-                scheme = HTTP_SCHEME;
+                scheme = effectiveHostAndPort.port() == 443 ? HTTPS_SCHEME : HTTP_SCHEME;
             }
-            String requestTarget = request.requestTarget();
-            StringBuilder sb = new StringBuilder(
-                    scheme.length() + 3 +
-                    effectiveHostAndPort.hostName().length() +
-                    ((effectiveHostAndPort.port()) >= 0 ? 5 : 0) +
-                    requestTarget.length());
-            sb.append(scheme)
-                    .append("://")
-                    .append(effectiveHostAndPort.hostName());
-            if (effectiveHostAndPort.port() >= 0) {
-                sb.append(':')
-                        .append(effectiveHostAndPort.port());
+            String authority = effectiveHostAndPort.hostName();
+            if (!isDefaultPort(scheme, effectiveHostAndPort.port())) {
+                authority = authority + ':' + effectiveHostAndPort.port();
             }
-            sb.append(requestTarget);
-            return sb.toString();
+            String authoritySeparator = requestTarget.startsWith("/") ? "" : "/";
+            return scheme + "://" + authority + authoritySeparator + requestTarget;
         }
 
         @Override
         @Nullable
         public String getServerAddress(final HttpRequestMetaData request) {
-            final HostAndPort effectiveHostAndPort = request.effectiveHostAndPort();
+            HostAndPort effectiveHostAndPort = request.effectiveHostAndPort();
             return effectiveHostAndPort != null ? effectiveHostAndPort.hostName() : null;
         }
 
@@ -168,6 +166,10 @@ public Integer getServerPort(HttpRequestMetaData metaData) {
             }
             return null;
         }
+
+        private static boolean isDefaultPort(String scheme, int port) {
+            return port < 1 || HTTPS_SCHEME.equals(scheme) && port == 443 || HTTP_SCHEME.equals(scheme) && port == 80;
+        }
     }
 
     private static final class ServerGetter extends ServiceTalkHttpAttributesGetter

servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpRequestFilterTest.java → servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpRequesterFilterTest.java

@@ -18,12 +18,14 @@
 
 import io.servicetalk.client.api.TransportObserverConnectionFactoryFilter;
 import io.servicetalk.http.api.HttpClient;
+import io.servicetalk.http.api.HttpRequest;
 import io.servicetalk.http.api.HttpResponse;
 import io.servicetalk.http.api.HttpServerBuilder;
 import io.servicetalk.http.netty.HttpServers;
 import io.servicetalk.log4j2.mdc.utils.LoggerStringWriter;
 import io.servicetalk.transport.api.ConnectionInfo;
 import io.servicetalk.transport.api.ConnectionObserver;
+import io.servicetalk.transport.api.HostAndPort;
 import io.servicetalk.transport.api.ServerContext;
 import io.servicetalk.transport.api.TransportObserver;
 import io.servicetalk.transport.netty.internal.NoopTransportObserver;
@@ -45,6 +47,7 @@
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.slf4j.Logger;
@@ -59,8 +62,10 @@
 import static io.opentelemetry.semconv.HttpAttributes.HTTP_REQUEST_METHOD;
 import static io.opentelemetry.semconv.NetworkAttributes.NETWORK_PROTOCOL_NAME;
 import static io.opentelemetry.semconv.NetworkAttributes.NETWORK_PROTOCOL_VERSION;
+import static io.opentelemetry.semconv.UrlAttributes.URL_FULL;
 import static io.servicetalk.concurrent.api.Single.succeeded;
 import static io.servicetalk.concurrent.internal.TestTimeoutConstants.CI;
+import static io.servicetalk.http.api.HttpHeaderNames.HOST;
 import static io.servicetalk.http.netty.HttpClients.forSingleAddress;
 import static io.servicetalk.log4j2.mdc.utils.LoggerStringWriter.assertContainsMdcPair;
 import static io.servicetalk.opentelemetry.http.AbstractOpenTelemetryFilter.DEFAULT_OPTIONS;
@@ -76,7 +81,7 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 @ExtendWith(MockitoExtension.class)
-class OpenTelemetryHttpRequestFilterTest {
+class OpenTelemetryHttpRequesterFilterTest {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
     private static final int SLEEP_TIME = CI ? 500 : 100;
@@ -102,7 +107,8 @@ void testInjectWithNoParent() throws Exception {
         OpenTelemetry openTelemetry = otelTesting.getOpenTelemetry();
         try (ServerContext context = buildServer(openTelemetry, false)) {
             try (HttpClient client = forSingleAddress(serverHostAndPort(context))
-                .appendClientFilter(new OpenTelemetryHttpRequesterFilter(openTelemetry, "testClient", DEFAULT_OPTIONS))
+                .appendClientFilter(new OpenTelemetryHttpRequesterFilter(openTelemetry, "testClient",
+                        DEFAULT_OPTIONS))
                 .appendClientFilter(new TestTracingClientLoggerFilter(TRACING_TEST_LOG_LINE_PREFIX)).build()) {
                 HttpResponse response = client.request(client.get(requestUrl)).toFuture().get();
                 TestSpanState serverSpanState = response.payloadBody(SPAN_STATE_SERIALIZER);
@@ -132,7 +138,8 @@ void testInjectWithAParent() throws Exception {
         OpenTelemetry openTelemetry = otelTesting.getOpenTelemetry();
         try (ServerContext context = buildServer(openTelemetry, true)) {
             try (HttpClient client = forSingleAddress(serverHostAndPort(context))
-                .appendClientFilter(new OpenTelemetryHttpRequesterFilter(openTelemetry, "testClient", DEFAULT_OPTIONS))
+                .appendClientFilter(new OpenTelemetryHttpRequesterFilter(openTelemetry, "testClient",
+                        DEFAULT_OPTIONS))
                 .appendClientFilter(new TestTracingClientLoggerFilter(TRACING_TEST_LOG_LINE_PREFIX)).build()) {
                 HttpResponse response = client.request(client.get(requestUrl)).toFuture().get();
                 TestSpanState serverSpanState = response.payloadBody(SPAN_STATE_SERIALIZER);
@@ -176,47 +183,61 @@ void testInjectWithAParent() throws Exception {
         }
     }
 
-    @Test
-    void testInjectWithAParentCreated() throws Exception {
-        final String requestUrl = "/path/to/resource";
+    @ParameterizedTest() // TODO: set the name
+    @CsvSource({"false, false", "false, true", "true, false", "true, true"})
+    void testInjectWithAParentCreated(boolean absoluteForm, boolean withHostHeader) throws Exception {
         OpenTelemetry openTelemetry = otelTesting.getOpenTelemetry();
         try (ServerContext context = buildServer(openTelemetry, true)) {
-            try (HttpClient client = forSingleAddress(serverHostAndPort(context))
-                .appendClientFilter(new OpenTelemetryHttpRequesterFilter(openTelemetry, "testClient", DEFAULT_OPTIONS))
-                .appendClientFilter(new TestTracingClientLoggerFilter(TRACING_TEST_LOG_LINE_PREFIX)).build()) {
+            HostAndPort serverHostAndPort = serverHostAndPort(context);
+            final String requestPath = "/path/to/resource";
+            final String requestUrl = absoluteForm ? fullUrl(serverHostAndPort, requestPath) : requestPath;
+            try (HttpClient client = forSingleAddress(serverHostAndPort)
+                    .appendClientFilter(new OpenTelemetryHttpRequesterFilter(openTelemetry, "testClient",
+                            DEFAULT_OPTIONS))
+                    .appendClientFilter(new TestTracingClientLoggerFilter(TRACING_TEST_LOG_LINE_PREFIX)).build()) {
 
                 Span span = otelTesting.getOpenTelemetry().getTracer("io.serviceTalk").spanBuilder("/")
-                    .setSpanKind(SpanKind.INTERNAL)
-                    .setAttribute("component", "serviceTalk")
-                    .startSpan();
+                        .setSpanKind(SpanKind.INTERNAL)
+                        .setAttribute("component", "serviceTalk")
+                        .startSpan();
                 TestSpanState serverSpanState;
                 try (Scope scope = span.makeCurrent()) {
                     LOGGER.info("making span={} current", span);
-                    HttpResponse response = client.request(client.get(requestUrl)).toFuture().get();
+                    HttpRequest request = client.get(requestUrl);
+                    if (withHostHeader) {
+                        request.setHeader(HOST, serverHostAndPort.hostName() + ":" + serverHostAndPort.port());
+                    }
+                    HttpResponse response = client.request(request).toFuture().get();
                     serverSpanState = response.payloadBody(SPAN_STATE_SERIALIZER);
                 } finally {
                     span.end();
                 }
-                    verifyTraceIdPresentInLogs(loggerStringWriter.stableAccumulated(1000), requestUrl,
+                verifyTraceIdPresentInLogs(loggerStringWriter.stableAccumulated(1000), requestPath,
                         serverSpanState.getTraceId(), serverSpanState.getSpanId(),
                         TRACING_TEST_LOG_LINE_PREFIX);
-                    assertThat(otelTesting.getSpans()).hasSize(3);
-                    assertThat(otelTesting.getSpans()).extracting("traceId")
+                assertThat(otelTesting.getSpans()).hasSize(3);
+                assertThat(otelTesting.getSpans()).extracting("traceId")
                         .containsExactly(serverSpanState.getTraceId(), serverSpanState.getTraceId(),
-                            serverSpanState.getTraceId());
-                    assertThat(otelTesting.getSpans()).extracting("spanId")
+                                serverSpanState.getTraceId());
+                assertThat(otelTesting.getSpans()).extracting("spanId")
                         .containsAnyOf(serverSpanState.getSpanId());
-                    otelTesting.assertTraces()
+                otelTesting.assertTraces()
                         .hasTracesSatisfyingExactly(ta ->
-                            ta.hasTraceId(serverSpanState.getTraceId()));
+                                ta.hasTraceId(serverSpanState.getTraceId()));
 
-                    otelTesting.assertTraces()
+                otelTesting.assertTraces()
                         .hasTracesSatisfyingExactly(ta -> {
                             assertThat(ta.getSpan(0).getAttributes().get(AttributeKey.stringKey("component")))
                                     .isEqualTo("serviceTalk");
                             assertThat(ta.getSpan(1).getParentSpanId()).isEqualTo(ta.getSpan(0).getSpanId());
+                            if (absoluteForm || withHostHeader) {
+                                assertThat(ta.getSpan(1).getAttributes().get(URL_FULL)).isEqualTo(
+                                        fullUrl(serverHostAndPort, requestPath));
+                            } else {
+                                assertThat(ta.getSpan(1).getAttributes().get(URL_FULL)).isNull();
+                            }
                             assertThat(ta.getSpan(1).getAttributes().get(NETWORK_PROTOCOL_NAME))
-                                .isNull(); // only needs to be set if != http
+                                    .isNull(); // only needs to be set if != http
                             assertThat(ta.getSpan(2).getParentSpanId()).isEqualTo(ta.getSpan(1).getSpanId());
                         });
             }
@@ -453,4 +474,8 @@ static void verifyTraceIdPresentInLogs(String logs, String requestPath, String t
             assertTrue(foundMatch, "could not find log line with prefix: " + prefix);
         }
     }
+
+    private static String fullUrl(HostAndPort serverHostAndPort, String requestTarget) {
+        return "http://" + serverHostAndPort.hostName() + ":" + serverHostAndPort.port() + requestTarget;
+    }
 }

servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServiceFilterTest.java

@@ -95,7 +95,7 @@
 import static io.servicetalk.http.netty.AsyncContextHttpFilterVerifier.verifyServerFilterAsyncContextVisibility;
 import static io.servicetalk.http.netty.HttpClients.forSingleAddress;
 import static io.servicetalk.opentelemetry.http.AbstractOpenTelemetryFilter.DEFAULT_OPTIONS;
-import static io.servicetalk.opentelemetry.http.OpenTelemetryHttpRequestFilterTest.verifyTraceIdPresentInLogs;
+import static io.servicetalk.opentelemetry.http.OpenTelemetryHttpRequesterFilterTest.verifyTraceIdPresentInLogs;
 import static io.servicetalk.opentelemetry.http.TestUtils.SPAN_STATE_SERIALIZER;
 import static io.servicetalk.opentelemetry.http.TestUtils.TRACING_TEST_LOG_LINE_PREFIX;
 import static io.servicetalk.transport.netty.internal.AddressUtils.localAddress;

servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetterTest.java

@@ -0,0 +1,93 @@
+/*
+ * Copyright © 2025 Apple Inc. and the ServiceTalk project authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.servicetalk.opentelemetry.http;
+
+import io.servicetalk.http.api.DefaultHttpHeadersFactory;
+import io.servicetalk.http.api.HttpRequestMetaData;
+import io.servicetalk.http.api.HttpRequestMethod;
+
+import org.junit.jupiter.api.Test;
+
+import static io.servicetalk.http.api.HttpHeaderNames.HOST;
+import static io.servicetalk.http.api.HttpProtocolVersion.HTTP_1_1;
+import static io.servicetalk.http.api.HttpRequestMetaDataFactory.newRequestMetaData;
+import static io.servicetalk.opentelemetry.http.ServiceTalkHttpAttributesGetter.CLIENT_INSTANCE;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.nullValue;
+
+class ServiceTalkHttpAttributesGetterTest {
+
+    @Test
+    void clientUrlExtractionNoHostAndPort() {
+        String pathQueryFrag = "/foo?bar=baz#frag";
+        HttpRequestMetaData request = newRequest(pathQueryFrag);
+        assertThat(CLIENT_INSTANCE.getUrlFull(request), nullValue());
+    }
+
+    @Test
+    void clientUrlExtractionHostHeader() {
+        String pathQueryFrag = "/foo?bar=baz#frag";
+        HttpRequestMetaData request = newRequest(pathQueryFrag);
+        request.addHeader(HOST, "myservice");
+        assertThat(CLIENT_INSTANCE.getUrlFull(request), equalTo("http://myservice" + pathQueryFrag));
+    }
+
+    @Test
+    void clientUrlExtractionNoLeadingSlashPath() {
+        String pathQueryFrag = "foo";
+        HttpRequestMetaData request = newRequest(pathQueryFrag);
+        request.addHeader(HOST, "myservice:8080");
+        assertThat(CLIENT_INSTANCE.getUrlFull(request), equalTo("http://myservice:8080/" + pathQueryFrag));
+    }
+
+    @Test
+    void clientUrlExtractionHostAndPortHttpNonDefaultScheme() {
+        String pathQueryFrag = "/foo?bar=baz#frag";
+        HttpRequestMetaData request = newRequest(pathQueryFrag);
+        request.addHeader(HOST, "myservice:8080");
+        assertThat(CLIENT_INSTANCE.getUrlFull(request), equalTo("http://myservice:8080" + pathQueryFrag));
+    }
+
+    @Test
+    void clientUrlExtractionHostAndPortHttpDefaultScheme() {
+        String pathQueryFrag = "/foo?bar=baz#frag";
+        HttpRequestMetaData request = newRequest(pathQueryFrag);
+        request.addHeader(HOST, "myservice:80");
+        assertThat(CLIENT_INSTANCE.getUrlFull(request), equalTo("http://myservice" + pathQueryFrag));
+    }
+
+    @Test
+    void clientUrlExtractionHostAndPortHttpsDefaultScheme() {
+        String pathQueryFrag = "/foo?bar=baz#frag";
+        HttpRequestMetaData request = newRequest(pathQueryFrag);
+        request.addHeader(HOST, "myservice:443");
+        assertThat(CLIENT_INSTANCE.getUrlFull(request), equalTo("https://myservice" + pathQueryFrag));
+    }
+
+    @Test
+    void clientAbsoluteUrl() {
+        String requestTarget = "https://myservice/foo?bar=baz#frag";
+        HttpRequestMetaData request = newRequest(requestTarget);
+        request.addHeader(HOST, "badservice"); // should be unused
+        assertThat(CLIENT_INSTANCE.getUrlFull(request), equalTo(requestTarget));
+    }
+
+    private static HttpRequestMetaData newRequest(String requestTarget) {
+        return newRequestMetaData(HTTP_1_1, HttpRequestMethod.GET, requestTarget,
+                DefaultHttpHeadersFactory.INSTANCE.newHeaders());
+    }
+}