From 0a700a30d523c68b085c19c8d1df6fc74bfd712e Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 10 May 2024 17:39:14 +0300 Subject: [PATCH 1/3] Armeria http client reports wrong protocol --- .../armeria/v1_3/ArmeriaHttp2Test.java | 83 ++++++++++++++++++- .../ArmeriaHttpClientAttributesGetter.java | 3 +- 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java b/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java index 9025514ae7fd..d97612d773e2 100644 --- a/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java +++ b/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.instrumentation.armeria.v1_3; +import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo; +import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies; import static org.assertj.core.api.Assertions.assertThat; import com.linecorp.armeria.client.WebClient; @@ -14,6 +16,12 @@ import com.linecorp.armeria.testing.junit5.server.ServerExtension; import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension; +import io.opentelemetry.semconv.ClientAttributes; +import io.opentelemetry.semconv.HttpAttributes; +import io.opentelemetry.semconv.NetworkAttributes; +import io.opentelemetry.semconv.ServerAttributes; +import io.opentelemetry.semconv.UrlAttributes; +import io.opentelemetry.semconv.UserAgentAttributes; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -52,10 +60,77 @@ void testHello() throws Exception { testing.waitAndAssertTraces( trace -> trace.hasSpansSatisfyingExactly( - span -> span.hasName("GET").hasKind(SpanKind.CLIENT).hasNoParent(), - span -> span.hasName("GET /").hasKind(SpanKind.SERVER).hasParent(trace.getSpan(0)), - span -> span.hasName("GET").hasKind(SpanKind.CLIENT).hasParent(trace.getSpan(1)), span -> - span.hasName("GET /").hasKind(SpanKind.SERVER).hasParent(trace.getSpan(2)))); + span.hasName("GET") + .hasKind(SpanKind.CLIENT) + .hasNoParent() + .hasAttributesSatisfyingExactly( + equalTo(UrlAttributes.URL_FULL, server2.httpUri() + "/"), + equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"), + equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200), + equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"), + equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"), + equalTo(ServerAttributes.SERVER_PORT, server2.httpPort()), + equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"), + satisfies( + NetworkAttributes.NETWORK_PEER_PORT, + val -> val.isInstanceOf(Long.class))), + span -> + span.hasName("GET /") + .hasKind(SpanKind.SERVER) + .hasParent(trace.getSpan(0)) + .hasAttributesSatisfyingExactly( + equalTo(UrlAttributes.URL_SCHEME, "http"), + equalTo(UrlAttributes.URL_PATH, "/"), + equalTo(HttpAttributes.HTTP_ROUTE, "/"), + equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"), + equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200), + equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"), + equalTo(ClientAttributes.CLIENT_ADDRESS, "127.0.0.1"), + equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"), + equalTo(ServerAttributes.SERVER_PORT, server2.httpPort()), + satisfies( + UserAgentAttributes.USER_AGENT_ORIGINAL, + val -> val.isInstanceOf(String.class)), + equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"), + satisfies( + NetworkAttributes.NETWORK_PEER_PORT, + val -> val.isInstanceOf(Long.class))), + span -> + span.hasName("GET") + .hasKind(SpanKind.CLIENT) + .hasParent(trace.getSpan(1)) + .hasAttributesSatisfyingExactly( + equalTo(UrlAttributes.URL_FULL, server1.httpUri() + "/"), + equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"), + equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200), + equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"), + equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"), + equalTo(ServerAttributes.SERVER_PORT, server1.httpPort()), + equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"), + satisfies( + NetworkAttributes.NETWORK_PEER_PORT, + val -> val.isInstanceOf(Long.class))), + span -> + span.hasName("GET /") + .hasKind(SpanKind.SERVER) + .hasParent(trace.getSpan(2)) + .hasAttributesSatisfyingExactly( + equalTo(UrlAttributes.URL_SCHEME, "http"), + equalTo(UrlAttributes.URL_PATH, "/"), + equalTo(HttpAttributes.HTTP_ROUTE, "/"), + equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"), + equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200), + equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"), + equalTo(ClientAttributes.CLIENT_ADDRESS, "127.0.0.1"), + equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"), + equalTo(ServerAttributes.SERVER_PORT, server1.httpPort()), + satisfies( + UserAgentAttributes.USER_AGENT_ORIGINAL, + val -> val.isInstanceOf(String.class)), + equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"), + satisfies( + NetworkAttributes.NETWORK_PEER_PORT, + val -> val.isInstanceOf(Long.class))))); } } diff --git a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java index b6e5e961a45b..3102045054a8 100644 --- a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java +++ b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java @@ -73,7 +73,8 @@ public String getNetworkProtocolName(RequestContext ctx, @Nullable RequestLog re @Override public String getNetworkProtocolVersion(RequestContext ctx, @Nullable RequestLog requestLog) { - SessionProtocol protocol = ctx.sessionProtocol(); + SessionProtocol protocol = + requestLog != null ? requestLog.sessionProtocol() : ctx.sessionProtocol(); return protocol.isMultiplex() ? "2" : "1.1"; } From 561a89c5e4dd28cd8405c5e054a06b189ecb35a6 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Sat, 11 May 2024 12:00:08 +0300 Subject: [PATCH 2/3] armeria client uses http/2 --- .../armeria/v1_3/AbstractArmeriaHttpClientTest.java | 9 +++++++++ .../testing/junit/http/AbstractHttpClientTest.java | 6 ++++-- .../testing/junit/http/HttpClientTestOptions.java | 7 ++++++- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/instrumentation/armeria-1.3/testing/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.java b/instrumentation/armeria-1.3/testing/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.java index 804189cb66e5..19a6ea1110dc 100644 --- a/instrumentation/armeria-1.3/testing/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.java +++ b/instrumentation/armeria-1.3/testing/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.java @@ -111,6 +111,15 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) { optionsBuilder.disableTestReusedRequest(); // can only use methods from HttpMethod enum optionsBuilder.disableTestNonStandardHttpMethod(); + // armeria uses http/2 + optionsBuilder.setHttpProtocolVersion( + uri -> { + String uriString = uri.toString(); + if (uriString.equals("http://localhost:61/") || uriString.equals("https://192.0.2.1/")) { + return "1.1"; + } + return "2"; + }); } @Test diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java index c00f90fab716..02250b349771 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java @@ -972,8 +972,10 @@ SpanDataAssert assertClientSpan( .doesNotContainKey(NetworkAttributes.NETWORK_TYPE) .doesNotContainKey(NetworkAttributes.NETWORK_PROTOCOL_NAME); if (httpClientAttributes.contains(NetworkAttributes.NETWORK_PROTOCOL_VERSION)) { - // TODO(anuraaga): Support HTTP/2 - assertThat(attrs).containsEntry(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "1.1"); + assertThat(attrs) + .containsEntry( + NetworkAttributes.NETWORK_PROTOCOL_VERSION, + options.getHttpProtocolVersion().apply(uri)); } if (httpClientAttributes.contains(ServerAttributes.SERVER_ADDRESS)) { diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestOptions.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestOptions.java index dd1cb84f4531..3762e0344102 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestOptions.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestOptions.java @@ -89,6 +89,8 @@ public boolean isLowLevelInstrumentation() { public abstract boolean getTestNonStandardHttpMethod(); + public abstract Function getHttpProtocolVersion(); + static Builder builder() { return new AutoValue_HttpClientTestOptions.Builder().withDefaults(); } @@ -116,7 +118,8 @@ default Builder withDefaults() { .setTestCallback(true) .setTestCallbackWithParent(true) .setTestErrorWithCallback(true) - .setTestNonStandardHttpMethod(true); + .setTestNonStandardHttpMethod(true) + .setHttpProtocolVersion(uri -> "1.1"); } Builder setHttpAttributes(Function>> value); @@ -157,6 +160,8 @@ default Builder withDefaults() { Builder setTestNonStandardHttpMethod(boolean value); + Builder setHttpProtocolVersion(Function value); + @CanIgnoreReturnValue default Builder disableTestWithClientParent() { return setTestWithClientParent(false); From 6a8d885eb0f1aba0367c910271e4e1a18e6805f4 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 13 May 2024 11:27:22 +0300 Subject: [PATCH 3/3] fix tests --- .../armeria/v1_3/ArmeriaHttp2Test.java | 2 +- .../ArmeriaHttpClientAttributesGetter.java | 47 +++++++++++++++++-- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java b/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java index d97612d773e2..4351adc47c4c 100644 --- a/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java +++ b/instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java @@ -43,7 +43,7 @@ protected void configure(ServerBuilder sb) { new ServerExtension() { @Override protected void configure(ServerBuilder sb) { - sb.service("/", (ctx, req) -> createWebClient(server1).execute(req)); + sb.service("/", (ctx, req) -> createWebClient(server1).get("/")); } }; diff --git a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java index 3102045054a8..ec6d00b8d6e2 100644 --- a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java +++ b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java @@ -11,6 +11,7 @@ import com.linecorp.armeria.common.SessionProtocol; import com.linecorp.armeria.common.logging.RequestLog; import io.opentelemetry.instrumentation.api.semconv.http.HttpClientAttributesGetter; +import java.lang.reflect.Method; import java.net.InetSocketAddress; import java.util.List; import javax.annotation.Nullable; @@ -23,6 +24,19 @@ public enum ArmeriaHttpClientAttributesGetter implements HttpClientAttributesGetter { INSTANCE; + private static final ClassValue authorityMethodCache = + new ClassValue() { + @Nullable + @Override + protected Method computeValue(Class type) { + try { + return type.getMethod("authority"); + } catch (NoSuchMethodException e) { + return null; + } + } + }; + @Override public String getHttpRequestMethod(RequestContext ctx) { return ctx.method().name(); @@ -33,10 +47,16 @@ public String getUrlFull(RequestContext ctx) { HttpRequest request = request(ctx); StringBuilder uri = new StringBuilder(); String scheme = request.scheme(); + if (scheme == null) { + String name = ctx.sessionProtocol().uriText(); + if ("http".equals(name) || "https".equals(name)) { + scheme = name; + } + } if (scheme != null) { uri.append(scheme).append("://"); } - String authority = request.authority(); + String authority = authority(ctx); if (authority != null) { uri.append(authority); } @@ -81,8 +101,7 @@ public String getNetworkProtocolVersion(RequestContext ctx, @Nullable RequestLog @Nullable @Override public String getServerAddress(RequestContext ctx) { - HttpRequest request = request(ctx); - String authority = request.authority(); + String authority = authority(ctx); if (authority == null) { return null; } @@ -93,8 +112,7 @@ public String getServerAddress(RequestContext ctx) { @Nullable @Override public Integer getServerPort(RequestContext ctx) { - HttpRequest request = request(ctx); - String authority = request.authority(); + String authority = authority(ctx); if (authority == null) { return null; } @@ -116,6 +134,25 @@ public InetSocketAddress getNetworkPeerInetSocketAddress( return RequestContextAccess.remoteAddress(ctx); } + @Nullable + private static String authority(RequestContext ctx) { + // newer armeria versions expose authority through DefaultClientRequestContext#authority + // we are using this method as it provides default values based on endpoint + // in older versions armeria wraps the request, and we can get the same default values through + // the request + Method method = authorityMethodCache.get(ctx.getClass()); + if (method != null) { + try { + return (String) method.invoke(ctx); + } catch (Exception e) { + return null; + } + } + + HttpRequest request = request(ctx); + return request.authority(); + } + private static HttpRequest request(RequestContext ctx) { HttpRequest request = ctx.request(); if (request == null) {