Skip to content

Commit 7bf1776

Browse files
kcsurapanenikcsurapaneni
committed
PR comments addressed
1 parent 4fd797f commit 7bf1776

File tree

8 files changed

+45
-177
lines changed

8 files changed

+45
-177
lines changed

‎instrumentation/activej-http-6.0/javaagent/README.md

-50
This file was deleted.

‎instrumentation/activej-http-6.0/javaagent/build.gradle.kts

+2-4
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,12 @@ muzzle {
88
module.set("activej-http")
99

1010
versions.set("[6.0,)")
11+
assertInverse.set(true)
1112
}
1213
}
1314

1415
dependencies {
15-
constraints {
16-
implementation("io.activej:activej-http:6.0-rc2")
17-
}
18-
implementation("io.activej:activej-http")
16+
library("io.activej:activej-http:6.0-rc2")
1917
}
2018

2119
otelJava {

‎instrumentation/activej-http-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerConnectionInstrumentation.java

+14-37
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package io.opentelemetry.javaagent.instrumentation.activejhttp;
77

88
import static io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge.currentContext;
9+
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
910
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasSuperType;
1011
import static io.opentelemetry.javaagent.instrumentation.activejhttp.ActivejHttpServerConnectionSingletons.instrumenter;
1112
import static net.bytebuddy.matcher.ElementMatchers.isInterface;
@@ -16,11 +17,9 @@
1617
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
1718

1819
import io.activej.http.AsyncServlet;
19-
import io.activej.http.HttpHeaders;
2020
import io.activej.http.HttpRequest;
2121
import io.activej.http.HttpResponse;
2222
import io.activej.promise.Promise;
23-
import io.opentelemetry.api.trace.Span;
2423
import io.opentelemetry.context.Context;
2524
import io.opentelemetry.context.Scope;
2625
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
@@ -36,6 +35,11 @@ public ElementMatcher<TypeDescription> typeMatcher() {
3635
return hasSuperType(named("io.activej.http.AsyncServlet")).and(not(isInterface()));
3736
}
3837

38+
@Override
39+
public ElementMatcher<ClassLoader> classLoaderOptimization() {
40+
return hasClassesNamed("io.activej.http.AsyncServlet");
41+
}
42+
3943
@Override
4044
public void transform(TypeTransformer transformer) {
4145
transformer.applyAdviceToMethod(
@@ -67,48 +71,21 @@ public static void methodEnter(
6771
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
6872
public static void methodExit(
6973
@Advice.This AsyncServlet asyncServlet,
70-
@Advice.Return(readOnly = false) Promise<HttpResponse> responsePromise,
74+
@Advice.Return Promise<HttpResponse> responsePromise,
7175
@Advice.Thrown Throwable throwable,
7276
@Advice.Local("otelContext") Context context,
7377
@Advice.Local("otelScope") Scope scope,
7478
@Advice.Local("httpRequest") HttpRequest httpRequest) {
75-
if (context == null || scope == null || httpRequest == null) {
79+
if (scope == null) {
7680
return;
7781
}
78-
String traceId = Span.fromContext(context).getSpanContext().getTraceId();
79-
String spanId = Span.fromContext(context).getSpanContext().getSpanId();
80-
String traceFlags =
81-
Span.fromContext(context).getSpanContext().getTraceFlags().asHex().substring(0, 2);
82-
String traceparent = String.format("00-%s-%s-%s", traceId, spanId, traceFlags);
83-
8482
scope.close();
85-
String traceparentHeader = "traceparent";
86-
if (responsePromise != null) {
87-
HttpResponse httpResponse = responsePromise.getResult();
88-
Throwable error = throwable;
89-
if (responsePromise.isException()) {
90-
error = responsePromise.getException();
91-
}
92-
httpResponse = ActivejHttpServerHelper.createResponse(error, traceparent, httpResponse);
93-
instrumenter().end(context, httpRequest, httpResponse, error);
94-
responsePromise = Promise.of(httpResponse);
95-
} else if (throwable != null) {
96-
HttpResponse httpResponse =
97-
HttpResponse.builder()
98-
.withCode(500)
99-
.withPlainText(throwable.getMessage())
100-
.withHeader(HttpHeaders.of(traceparentHeader), traceparent)
101-
.build();
102-
instrumenter().end(context, httpRequest, httpResponse, throwable);
103-
responsePromise = Promise.of(httpResponse);
104-
} else {
105-
HttpResponse httpResponse =
106-
HttpResponse.notFound404()
107-
.withHeader(HttpHeaders.of(traceparentHeader), traceparent)
108-
.build();
109-
instrumenter().end(context, httpRequest, httpResponse, throwable);
110-
responsePromise = Promise.of(httpResponse);
111-
}
83+
instrumenter()
84+
.end(
85+
context,
86+
httpRequest,
87+
responsePromise == null ? null : responsePromise.getResult(),
88+
throwable);
11289
}
11390
}
11491
}

‎instrumentation/activej-http-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerConnectionInstrumentationModule.java

-6
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import static java.util.Collections.singletonList;
99

1010
import com.google.auto.service.AutoService;
11-
import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder;
1211
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
1312
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1413
import java.util.List;
@@ -24,9 +23,4 @@ public ActivejHttpServerConnectionInstrumentationModule() {
2423
public List<TypeInstrumentation> typeInstrumentations() {
2524
return singletonList(new ActivejHttpServerConnectionInstrumentation());
2625
}
27-
28-
@Override
29-
public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) {
30-
helperResourceBuilder.register(ActivejHttpServerHelper.class.getName());
31-
}
3226
}

‎instrumentation/activej-http-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerHelper.java

-44
This file was deleted.

‎instrumentation/activej-http-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerHttpAttributesGetter.java

+3-3
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public String getUrlScheme(HttpRequest request) {
4444

4545
@Override
4646
public String getUrlPath(HttpRequest request) {
47-
return UrlParser.of(request.getFullUrl()).getPath();
47+
return request.getPath();
4848
}
4949

5050
@Override
@@ -54,13 +54,13 @@ public String getUrlQuery(HttpRequest request) {
5454

5555
@Override
5656
public String getNetworkProtocolName(HttpRequest request, @Nullable HttpResponse httpResponse) {
57-
return UrlParser.of(request.getFullUrl()).getProtocol().name();
57+
return ActivejHttpServerUtil.getNetworkProtocolName(request);
5858
}
5959

6060
@Override
6161
public String getNetworkProtocolVersion(
6262
HttpRequest request, @Nullable HttpResponse httpResponse) {
63-
return request.getVersion().name();
63+
return ActivejHttpServerUtil.getNetworkProtocolVersion(request.getVersion());
6464
}
6565

6666
@Override

‎instrumentation/activej-http-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerUtil.java

+26-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import io.activej.http.HttpHeaders;
1010
import io.activej.http.HttpRequest;
1111
import io.activej.http.HttpResponse;
12+
import io.activej.http.HttpVersion;
1213
import java.util.Arrays;
1314
import java.util.Collections;
1415
import java.util.List;
@@ -47,6 +48,30 @@ static List<String> getHttpResponseHeader(
4748
if (headerValue == null) {
4849
return Collections.emptyList();
4950
}
50-
return Arrays.stream(headerValue.split(",")).collect(Collectors.toList());
51+
return Arrays.stream(headerValue.split(",")).toList();
52+
}
53+
54+
static String getNetworkProtocolVersion(HttpVersion version) {
55+
switch (version) {
56+
case HTTP_0_9:
57+
return "0.9";
58+
case HTTP_1_0:
59+
return "1.0";
60+
case HTTP_1_1:
61+
return "1.1";
62+
case HTTP_2_0:
63+
return "2.0";
64+
}
65+
return "unknown";
66+
}
67+
68+
static String getNetworkProtocolName(HttpRequest request) {
69+
if (request.getVersion() == HttpVersion.HTTP_0_9
70+
|| request.getVersion() == HttpVersion.HTTP_1_0
71+
|| request.getVersion() == HttpVersion.HTTP_1_1
72+
|| request.getVersion() == HttpVersion.HTTP_2_0) {
73+
return "http";
74+
}
75+
return null;
5176
}
5277
}

‎instrumentation/activej-http-6.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/activejhttp/ActivejRoutingServletTest.java

-32
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,12 @@
1111
import static io.opentelemetry.semconv.HttpAttributes.HTTP_RESPONSE_STATUS_CODE;
1212
import static io.opentelemetry.semconv.UrlAttributes.URL_PATH;
1313
import static java.nio.charset.StandardCharsets.UTF_8;
14-
import static org.assertj.core.api.Assertions.assertThat;
1514
import static org.junit.jupiter.api.Assertions.assertEquals;
1615
import static org.junit.jupiter.api.Assertions.assertTrue;
1716

1817
import io.activej.eventloop.Eventloop;
1918
import io.activej.http.AsyncServlet;
2019
import io.activej.http.HttpError;
21-
import io.activej.http.HttpHeaders;
2220
import io.activej.http.HttpRequest;
2321
import io.activej.http.HttpResponse;
2422
import io.activej.http.RoutingServlet;
@@ -95,33 +93,6 @@ void testRequestNotFoundFlow() throws Exception {
9593
equalTo(HTTP_RESPONSE_STATUS_CODE, 404))));
9694
}
9795

98-
@Test
99-
void testRequestErrorFlow() throws Exception {
100-
101-
AsyncServlet asyncServlet = request -> Promise.ofException(HttpError.ofCode(500));
102-
103-
RoutingServlet routingServlet =
104-
RoutingServlet.builder(eventloop).with(GET, "/error", asyncServlet).build();
105-
106-
String url = "http://some-test.com/error";
107-
HttpRequest httpRequest = HttpRequest.get(url).build();
108-
check(routingServlet.serve(httpRequest), "HTTP code 500", 500);
109-
110-
UrlParser urlParser = UrlParser.of(url);
111-
112-
testing.waitAndAssertTraces(
113-
trace ->
114-
trace.hasSpansSatisfyingExactly(
115-
span ->
116-
span.hasName("GET /error")
117-
.hasNoParent()
118-
.hasKind(SpanKind.SERVER)
119-
.hasAttributesSatisfying(
120-
equalTo(URL_PATH, urlParser.getPath()),
121-
equalTo(HTTP_REQUEST_METHOD, "GET"),
122-
equalTo(HTTP_RESPONSE_STATUS_CODE, 500))));
123-
}
124-
12596
private static void check(Promise<HttpResponse> promise, String expectedBody, int expectedCode) {
12697
assertTrue(promise.isComplete());
12798
if (promise.isResult() && !promise.isException()) {
@@ -131,8 +102,5 @@ private static void check(Promise<HttpResponse> promise, String expectedBody, in
131102
} else {
132103
assertEquals(expectedCode, ((HttpError) promise.getException()).getCode());
133104
}
134-
assertThat(promise.getResult().getHeader(HttpHeaders.of("traceparent")))
135-
.isNotNull()
136-
.isNotBlank();
137105
}
138106
}

0 commit comments

Comments
 (0)