Skip to content

Commit 67417e3

Browse files
lauritlaurit
authored
Armeria http client reports wrong protocol (#11334)
1 parent c73e7a6 commit 67417e3

File tree

5 files changed

+143
-14
lines changed

5 files changed

+143
-14
lines changed

‎instrumentation/armeria-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttp2Test.java

+80-5
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55

66
package io.opentelemetry.javaagent.instrumentation.armeria.v1_3;
77

8+
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
9+
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.satisfies;
810
import static org.assertj.core.api.Assertions.assertThat;
911

1012
import com.linecorp.armeria.client.WebClient;
@@ -14,6 +16,12 @@
1416
import com.linecorp.armeria.testing.junit5.server.ServerExtension;
1517
import io.opentelemetry.api.trace.SpanKind;
1618
import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension;
19+
import io.opentelemetry.semconv.ClientAttributes;
20+
import io.opentelemetry.semconv.HttpAttributes;
21+
import io.opentelemetry.semconv.NetworkAttributes;
22+
import io.opentelemetry.semconv.ServerAttributes;
23+
import io.opentelemetry.semconv.UrlAttributes;
24+
import io.opentelemetry.semconv.UserAgentAttributes;
1725
import org.junit.jupiter.api.Test;
1826
import org.junit.jupiter.api.extension.RegisterExtension;
1927

@@ -35,7 +43,7 @@ protected void configure(ServerBuilder sb) {
3543
new ServerExtension() {
3644
@Override
3745
protected void configure(ServerBuilder sb) {
38-
sb.service("/", (ctx, req) -> createWebClient(server1).execute(req));
46+
sb.service("/", (ctx, req) -> createWebClient(server1).get("/"));
3947
}
4048
};
4149

@@ -52,10 +60,77 @@ void testHello() throws Exception {
5260
testing.waitAndAssertTraces(
5361
trace ->
5462
trace.hasSpansSatisfyingExactly(
55-
span -> span.hasName("GET").hasKind(SpanKind.CLIENT).hasNoParent(),
56-
span -> span.hasName("GET /").hasKind(SpanKind.SERVER).hasParent(trace.getSpan(0)),
57-
span -> span.hasName("GET").hasKind(SpanKind.CLIENT).hasParent(trace.getSpan(1)),
5863
span ->
59-
span.hasName("GET /").hasKind(SpanKind.SERVER).hasParent(trace.getSpan(2))));
64+
span.hasName("GET")
65+
.hasKind(SpanKind.CLIENT)
66+
.hasNoParent()
67+
.hasAttributesSatisfyingExactly(
68+
equalTo(UrlAttributes.URL_FULL, server2.httpUri() + "/"),
69+
equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"),
70+
equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200),
71+
equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"),
72+
equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"),
73+
equalTo(ServerAttributes.SERVER_PORT, server2.httpPort()),
74+
equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"),
75+
satisfies(
76+
NetworkAttributes.NETWORK_PEER_PORT,
77+
val -> val.isInstanceOf(Long.class))),
78+
span ->
79+
span.hasName("GET /")
80+
.hasKind(SpanKind.SERVER)
81+
.hasParent(trace.getSpan(0))
82+
.hasAttributesSatisfyingExactly(
83+
equalTo(UrlAttributes.URL_SCHEME, "http"),
84+
equalTo(UrlAttributes.URL_PATH, "/"),
85+
equalTo(HttpAttributes.HTTP_ROUTE, "/"),
86+
equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"),
87+
equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200),
88+
equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"),
89+
equalTo(ClientAttributes.CLIENT_ADDRESS, "127.0.0.1"),
90+
equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"),
91+
equalTo(ServerAttributes.SERVER_PORT, server2.httpPort()),
92+
satisfies(
93+
UserAgentAttributes.USER_AGENT_ORIGINAL,
94+
val -> val.isInstanceOf(String.class)),
95+
equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"),
96+
satisfies(
97+
NetworkAttributes.NETWORK_PEER_PORT,
98+
val -> val.isInstanceOf(Long.class))),
99+
span ->
100+
span.hasName("GET")
101+
.hasKind(SpanKind.CLIENT)
102+
.hasParent(trace.getSpan(1))
103+
.hasAttributesSatisfyingExactly(
104+
equalTo(UrlAttributes.URL_FULL, server1.httpUri() + "/"),
105+
equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"),
106+
equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200),
107+
equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"),
108+
equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"),
109+
equalTo(ServerAttributes.SERVER_PORT, server1.httpPort()),
110+
equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"),
111+
satisfies(
112+
NetworkAttributes.NETWORK_PEER_PORT,
113+
val -> val.isInstanceOf(Long.class))),
114+
span ->
115+
span.hasName("GET /")
116+
.hasKind(SpanKind.SERVER)
117+
.hasParent(trace.getSpan(2))
118+
.hasAttributesSatisfyingExactly(
119+
equalTo(UrlAttributes.URL_SCHEME, "http"),
120+
equalTo(UrlAttributes.URL_PATH, "/"),
121+
equalTo(HttpAttributes.HTTP_ROUTE, "/"),
122+
equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "GET"),
123+
equalTo(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200),
124+
equalTo(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "2"),
125+
equalTo(ClientAttributes.CLIENT_ADDRESS, "127.0.0.1"),
126+
equalTo(ServerAttributes.SERVER_ADDRESS, "127.0.0.1"),
127+
equalTo(ServerAttributes.SERVER_PORT, server1.httpPort()),
128+
satisfies(
129+
UserAgentAttributes.USER_AGENT_ORIGINAL,
130+
val -> val.isInstanceOf(String.class)),
131+
equalTo(NetworkAttributes.NETWORK_PEER_ADDRESS, "127.0.0.1"),
132+
satisfies(
133+
NetworkAttributes.NETWORK_PEER_PORT,
134+
val -> val.isInstanceOf(Long.class)))));
60135
}
61136
}

‎instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/internal/ArmeriaHttpClientAttributesGetter.java

+44-6
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import com.linecorp.armeria.common.SessionProtocol;
1212
import com.linecorp.armeria.common.logging.RequestLog;
1313
import io.opentelemetry.instrumentation.api.semconv.http.HttpClientAttributesGetter;
14+
import java.lang.reflect.Method;
1415
import java.net.InetSocketAddress;
1516
import java.util.List;
1617
import javax.annotation.Nullable;
@@ -23,6 +24,19 @@ public enum ArmeriaHttpClientAttributesGetter
2324
implements HttpClientAttributesGetter<RequestContext, RequestLog> {
2425
INSTANCE;
2526

27+
private static final ClassValue<Method> authorityMethodCache =
28+
new ClassValue<Method>() {
29+
@Nullable
30+
@Override
31+
protected Method computeValue(Class<?> type) {
32+
try {
33+
return type.getMethod("authority");
34+
} catch (NoSuchMethodException e) {
35+
return null;
36+
}
37+
}
38+
};
39+
2640
@Override
2741
public String getHttpRequestMethod(RequestContext ctx) {
2842
return ctx.method().name();
@@ -33,10 +47,16 @@ public String getUrlFull(RequestContext ctx) {
3347
HttpRequest request = request(ctx);
3448
StringBuilder uri = new StringBuilder();
3549
String scheme = request.scheme();
50+
if (scheme == null) {
51+
String name = ctx.sessionProtocol().uriText();
52+
if ("http".equals(name) || "https".equals(name)) {
53+
scheme = name;
54+
}
55+
}
3656
if (scheme != null) {
3757
uri.append(scheme).append("://");
3858
}
39-
String authority = request.authority();
59+
String authority = authority(ctx);
4060
if (authority != null) {
4161
uri.append(authority);
4262
}
@@ -73,15 +93,15 @@ public String getNetworkProtocolName(RequestContext ctx, @Nullable RequestLog re
7393

7494
@Override
7595
public String getNetworkProtocolVersion(RequestContext ctx, @Nullable RequestLog requestLog) {
76-
SessionProtocol protocol = ctx.sessionProtocol();
96+
SessionProtocol protocol =
97+
requestLog != null ? requestLog.sessionProtocol() : ctx.sessionProtocol();
7798
return protocol.isMultiplex() ? "2" : "1.1";
7899
}
79100

80101
@Nullable
81102
@Override
82103
public String getServerAddress(RequestContext ctx) {
83-
HttpRequest request = request(ctx);
84-
String authority = request.authority();
104+
String authority = authority(ctx);
85105
if (authority == null) {
86106
return null;
87107
}
@@ -92,8 +112,7 @@ public String getServerAddress(RequestContext ctx) {
92112
@Nullable
93113
@Override
94114
public Integer getServerPort(RequestContext ctx) {
95-
HttpRequest request = request(ctx);
96-
String authority = request.authority();
115+
String authority = authority(ctx);
97116
if (authority == null) {
98117
return null;
99118
}
@@ -115,6 +134,25 @@ public InetSocketAddress getNetworkPeerInetSocketAddress(
115134
return RequestContextAccess.remoteAddress(ctx);
116135
}
117136

137+
@Nullable
138+
private static String authority(RequestContext ctx) {
139+
// newer armeria versions expose authority through DefaultClientRequestContext#authority
140+
// we are using this method as it provides default values based on endpoint
141+
// in older versions armeria wraps the request, and we can get the same default values through
142+
// the request
143+
Method method = authorityMethodCache.get(ctx.getClass());
144+
if (method != null) {
145+
try {
146+
return (String) method.invoke(ctx);
147+
} catch (Exception e) {
148+
return null;
149+
}
150+
}
151+
152+
HttpRequest request = request(ctx);
153+
return request.authority();
154+
}
155+
118156
private static HttpRequest request(RequestContext ctx) {
119157
HttpRequest request = ctx.request();
120158
if (request == null) {

‎instrumentation/armeria-1.3/testing/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.java

+9
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,15 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
111111
optionsBuilder.disableTestReusedRequest();
112112
// can only use methods from HttpMethod enum
113113
optionsBuilder.disableTestNonStandardHttpMethod();
114+
// armeria uses http/2
115+
optionsBuilder.setHttpProtocolVersion(
116+
uri -> {
117+
String uriString = uri.toString();
118+
if (uriString.equals("http://localhost:61/") || uriString.equals("https://192.0.2.1/")) {
119+
return "1.1";
120+
}
121+
return "2";
122+
});
114123
}
115124

116125
@Test

‎testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java

+4-2
Original file line numberDiff line numberDiff line change
@@ -972,8 +972,10 @@ SpanDataAssert assertClientSpan(
972972
.doesNotContainKey(NetworkAttributes.NETWORK_TYPE)
973973
.doesNotContainKey(NetworkAttributes.NETWORK_PROTOCOL_NAME);
974974
if (httpClientAttributes.contains(NetworkAttributes.NETWORK_PROTOCOL_VERSION)) {
975-
// TODO(anuraaga): Support HTTP/2
976-
assertThat(attrs).containsEntry(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "1.1");
975+
assertThat(attrs)
976+
.containsEntry(
977+
NetworkAttributes.NETWORK_PROTOCOL_VERSION,
978+
options.getHttpProtocolVersion().apply(uri));
977979
}
978980

979981
if (httpClientAttributes.contains(ServerAttributes.SERVER_ADDRESS)) {

‎testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestOptions.java

+6-1
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,8 @@ public boolean isLowLevelInstrumentation() {
8989

9090
public abstract boolean getTestNonStandardHttpMethod();
9191

92+
public abstract Function<URI, String> getHttpProtocolVersion();
93+
9294
static Builder builder() {
9395
return new AutoValue_HttpClientTestOptions.Builder().withDefaults();
9496
}
@@ -116,7 +118,8 @@ default Builder withDefaults() {
116118
.setTestCallback(true)
117119
.setTestCallbackWithParent(true)
118120
.setTestErrorWithCallback(true)
119-
.setTestNonStandardHttpMethod(true);
121+
.setTestNonStandardHttpMethod(true)
122+
.setHttpProtocolVersion(uri -> "1.1");
120123
}
121124

122125
Builder setHttpAttributes(Function<URI, Set<AttributeKey<?>>> value);
@@ -157,6 +160,8 @@ default Builder withDefaults() {
157160

158161
Builder setTestNonStandardHttpMethod(boolean value);
159162

163+
Builder setHttpProtocolVersion(Function<URI, String> value);
164+
160165
@CanIgnoreReturnValue
161166
default Builder disableTestWithClientParent() {
162167
return setTestWithClientParent(false);

0 commit comments

Comments
 (0)