diff --git a/servicetalk-opentelemetry-http/build.gradle b/servicetalk-opentelemetry-http/build.gradle index 2785b87b89..5cc1c94952 100755 --- a/servicetalk-opentelemetry-http/build.gradle +++ b/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" diff --git a/servicetalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetter.java b/servicetalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetter.java index 132ab32753..51635e7d9b 100644 --- a/servicetalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetter.java +++ b/servicetalk-opentelemetry-http/src/main/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetter.java @@ -78,7 +78,6 @@ public List getHttpResponseHeader( } @Override - @Nullable public final String getNetworkProtocolName( final HttpRequestMetaData request, @Nullable final HttpResponseMetaData response) { return HTTP_SCHEME; @@ -117,35 +116,35 @@ 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; + // Note that this is best effort guessing: we cannot know if the connection is actually secure. + 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 +167,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 diff --git a/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpRequestFilterTest.java b/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpRequesterFilterTest.java similarity index 88% rename from servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpRequestFilterTest.java rename to servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpRequesterFilterTest.java index 098cdcc9f0..c77eea53d2 100755 --- a/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpRequestFilterTest.java +++ b/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(name = "{displayName} [{index}]: absoluteForm={0}, withHostHeader={1}") + @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; + } } diff --git a/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServiceFilterTest.java b/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServiceFilterTest.java index 2d2f44d58d..8eed3e1d45 100644 --- a/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/OpenTelemetryHttpServiceFilterTest.java +++ b/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; diff --git a/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetterTest.java b/servicetalk-opentelemetry-http/src/test/java/io/servicetalk/opentelemetry/http/ServiceTalkHttpAttributesGetterTest.java new file mode 100644 index 0000000000..8bf0343760 --- /dev/null +++ b/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()); + } +}