Skip to content

Commit b8d58be

Browse files
committed
Trace pekko-http requests rejected during parsing
A request with an illegal request-target is rejected by the HTTP parser and answered by HttpServerBluePrint$ControllerStage, which sits below the requestPreparation stage that the server instrumentation wraps. Such a request never becomes an HttpRequest and never reaches the user handler, so no span was produced for the 400 response at all. Instrument ParsingErrorHandler, the only hook that sees the rejected request, and emit a server span for the response it generates. The parser throws before a request-target has been read, so nothing is known about the request itself: the span reports the status code, and the method as _OTHER, but carries no url.path or http.route. The javadsl-returning handle method is a bridge on a scala implementation, so the advice matches the non-bridge declaration instead, which is one method per implementation regardless of which of the two return types it declares. Fixes open-telemetry#5139 for pekko-http.
1 parent 0c16d7c commit b8d58be

6 files changed

Lines changed: 314 additions & 0 deletions

File tree

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server;
7+
8+
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
9+
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasSuperType;
10+
import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
11+
import static net.bytebuddy.matcher.ElementMatchers.isBridge;
12+
import static net.bytebuddy.matcher.ElementMatchers.named;
13+
import static net.bytebuddy.matcher.ElementMatchers.not;
14+
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
15+
16+
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
17+
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
18+
import net.bytebuddy.asm.Advice;
19+
import net.bytebuddy.description.type.TypeDescription;
20+
import net.bytebuddy.matcher.ElementMatcher;
21+
import org.apache.pekko.http.javadsl.model.HttpResponse;
22+
23+
/**
24+
* A request that fails to parse, for example because of an illegal request-target, is answered by
25+
* {@code HttpServerBluePrint$ControllerStage}, which sits below the stage that the server
26+
* instrumentation wraps. The rejected request never becomes an {@code HttpRequest} and never
27+
* reaches the user handler, so the only hook that sees it is the parsing error handler.
28+
*/
29+
class ParsingErrorHandlerInstrumentation implements TypeInstrumentation {
30+
31+
@Override
32+
public ElementMatcher<ClassLoader> classLoaderOptimization() {
33+
return hasClassesNamed("org.apache.pekko.http.ParsingErrorHandler");
34+
}
35+
36+
@Override
37+
public ElementMatcher<TypeDescription> typeMatcher() {
38+
return hasSuperType(named("org.apache.pekko.http.ParsingErrorHandler"));
39+
}
40+
41+
@Override
42+
public void transform(TypeTransformer transformer) {
43+
// a scala implementation returns the more specific scaladsl HttpResponse and gets a bridge
44+
// method for the signature declared on ParsingErrorHandler; skipping the bridge leaves exactly
45+
// one match per implementation, whichever of the two return types it declares
46+
transformer.applyAdviceToMethod(
47+
named("handle").and(not(isAbstract())).and(not(isBridge())).and(takesArguments(4)),
48+
getClass().getName() + "$HandleAdvice");
49+
}
50+
51+
@SuppressWarnings("unused")
52+
public static class HandleAdvice {
53+
54+
@Advice.OnMethodExit(suppress = Throwable.class, inline = false)
55+
public static void onExit(@Advice.Return Object response) {
56+
PekkoHttpParsingErrorSingletons.emitSpan((HttpResponse) response);
57+
}
58+
}
59+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server;
7+
8+
/**
9+
* Marker for a request that pekko-http rejected while parsing it. Such a request never becomes an
10+
* {@code HttpRequest}, so there is nothing to describe it with beyond the response that pekko-http
11+
* synthesized.
12+
*/
13+
final class PekkoHttpParsingError {
14+
15+
static final PekkoHttpParsingError INSTANCE = new PekkoHttpParsingError();
16+
17+
private PekkoHttpParsingError() {}
18+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server;
7+
8+
import static java.util.Collections.emptyList;
9+
10+
import io.opentelemetry.instrumentation.api.semconv.http.HttpServerAttributesGetter;
11+
import java.util.List;
12+
import javax.annotation.Nullable;
13+
import org.apache.pekko.http.javadsl.model.HttpResponse;
14+
15+
/**
16+
* The request failed to parse, so the only thing known about it is the response that pekko-http
17+
* generated. Everything that would come from the request is reported as unknown.
18+
*/
19+
class PekkoHttpParsingErrorAttributesGetter
20+
implements HttpServerAttributesGetter<PekkoHttpParsingError, HttpResponse> {
21+
22+
@Nullable
23+
@Override
24+
public String getHttpRequestMethod(PekkoHttpParsingError request) {
25+
return null;
26+
}
27+
28+
@Override
29+
public List<String> getHttpRequestHeader(PekkoHttpParsingError request, String name) {
30+
return emptyList();
31+
}
32+
33+
@Override
34+
public Integer getHttpResponseStatusCode(
35+
PekkoHttpParsingError request, HttpResponse response, @Nullable Throwable error) {
36+
return response.status().intValue();
37+
}
38+
39+
@Override
40+
public List<String> getHttpResponseHeader(
41+
PekkoHttpParsingError request, HttpResponse response, String name) {
42+
return emptyList();
43+
}
44+
45+
@Nullable
46+
@Override
47+
public String getUrlScheme(PekkoHttpParsingError request) {
48+
return null;
49+
}
50+
51+
@Nullable
52+
@Override
53+
public String getUrlPath(PekkoHttpParsingError request) {
54+
return null;
55+
}
56+
57+
@Nullable
58+
@Override
59+
public String getUrlQuery(PekkoHttpParsingError request) {
60+
return null;
61+
}
62+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.server;
7+
8+
import static io.opentelemetry.instrumentation.api.internal.HttpConstants._OTHER;
9+
import static io.opentelemetry.semconv.HttpAttributes.HTTP_REQUEST_METHOD;
10+
import static java.util.Collections.emptyList;
11+
12+
import io.opentelemetry.context.Context;
13+
import io.opentelemetry.context.propagation.TextMapGetter;
14+
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
15+
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
16+
import io.opentelemetry.javaagent.bootstrap.internal.JavaagentHttpServerInstrumenters;
17+
import io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0.PekkoHttpUtil;
18+
import javax.annotation.Nullable;
19+
import org.apache.pekko.http.javadsl.model.HttpResponse;
20+
21+
/**
22+
* Creates spans for requests that pekko-http rejected while parsing them. These requests never
23+
* reach the user handler, and never become an {@code HttpRequest}, so the regular server
24+
* instrumentation in {@link PekkoHttpServerTracer} does not see them.
25+
*/
26+
public final class PekkoHttpParsingErrorSingletons {
27+
28+
private static final Instrumenter<PekkoHttpParsingError, HttpResponse> INSTRUMENTER =
29+
JavaagentHttpServerInstrumenters.create(
30+
PekkoHttpUtil.instrumentationName(),
31+
new PekkoHttpParsingErrorAttributesGetter(),
32+
NoopTextMapGetter.INSTANCE,
33+
// the request method could not be parsed, so it is reported as unknown
34+
builder ->
35+
builder.addAttributesExtractor(
36+
AttributesExtractor.constant(HTTP_REQUEST_METHOD, _OTHER)));
37+
38+
public static void emitSpan(HttpResponse response) {
39+
Context parentContext = Context.current();
40+
if (!INSTRUMENTER.shouldStart(parentContext, PekkoHttpParsingError.INSTANCE)) {
41+
return;
42+
}
43+
Context context = INSTRUMENTER.start(parentContext, PekkoHttpParsingError.INSTANCE);
44+
INSTRUMENTER.end(context, PekkoHttpParsingError.INSTANCE, response, null);
45+
}
46+
47+
private enum NoopTextMapGetter implements TextMapGetter<PekkoHttpParsingError> {
48+
INSTANCE;
49+
50+
@Override
51+
public Iterable<String> keys(PekkoHttpParsingError carrier) {
52+
return emptyList();
53+
}
54+
55+
@Nullable
56+
@Override
57+
public String get(@Nullable PekkoHttpParsingError carrier, String key) {
58+
return null;
59+
}
60+
}
61+
62+
private PekkoHttpParsingErrorSingletons() {}
63+
}

‎instrumentation/pekko/pekko-http-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/pekkohttp/v1_0/server/PekkoHttpServerInstrumentationModule.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ public List<TypeInstrumentation> typeInstrumentations() {
3535
new HttpPrepareAttributesInstrumentation(),
3636
new HttpServerBluePrintInstrumentation(),
3737
new GraphInterpreterInstrumentation(),
38+
new ParsingErrorHandlerInstrumentation(),
3839
new PekkoHttpServerSourceInstrumentation(),
3940
new Http2ExtServerInstrumentation(),
4041
new Http2RequestParsingInstrumentation());
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0
7+
8+
import io.opentelemetry.api.trace.SpanKind
9+
import io.opentelemetry.instrumentation.testing.junit.{
10+
AgentInstrumentationExtension,
11+
InstrumentationExtension
12+
}
13+
import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo
14+
import io.opentelemetry.sdk.testing.assertj.{SpanDataAssert, TraceAssert}
15+
import io.opentelemetry.semconv.HttpAttributes
16+
import org.apache.pekko.actor.ActorSystem
17+
import org.apache.pekko.http.scaladsl.Http
18+
import org.apache.pekko.http.scaladsl.model.{HttpRequest, HttpResponse}
19+
import org.assertj.core.api.Assertions.assertThat
20+
import org.junit.jupiter.api.Test
21+
import org.junit.jupiter.api.extension.RegisterExtension
22+
23+
import java.io.{BufferedReader, InputStreamReader, OutputStreamWriter}
24+
import java.net.{ServerSocket, Socket}
25+
import java.nio.charset.StandardCharsets
26+
import java.util.function.Consumer
27+
import scala.collection.mutable.ListBuffer
28+
import scala.concurrent.Await
29+
import scala.concurrent.duration._
30+
31+
/** A request that pekko-http rejects while parsing it never becomes an
32+
* HttpRequest and never reaches the user handler, so it is traced through the
33+
* parsing error handler rather than through the regular server
34+
* instrumentation.
35+
*/
36+
class PekkoHttpServerParsingErrorTest {
37+
38+
@RegisterExtension val testing: InstrumentationExtension =
39+
AgentInstrumentationExtension.create()
40+
41+
@Test def illegalRequestTargetIsTraced(): Unit = {
42+
// this test does not share the web server used by the other tests, it needs a server that is
43+
// guaranteed to be running and no other test may terminate its actor system
44+
implicit val system: ActorSystem = ActorSystem("parsing-error-test")
45+
try {
46+
val port = {
47+
val socket = new ServerSocket(0)
48+
try socket.getLocalPort
49+
finally socket.close()
50+
}
51+
val handler: HttpRequest => HttpResponse = _ => HttpResponse()
52+
val binding =
53+
Await.result(
54+
Http().bindAndHandleSync(handler, "localhost", port),
55+
10.seconds
56+
)
57+
58+
try {
59+
val response = send(
60+
port,
61+
"GET /%% HTTP/1.1\r\nHost: localhost:" + port + "\r\n\r\n"
62+
)
63+
assertThat(response.head).contains("400 Bad Request")
64+
65+
testing.waitAndAssertTraces(new Consumer[TraceAssert] {
66+
override def accept(trace: TraceAssert): Unit =
67+
trace.hasSpansSatisfyingExactly(new Consumer[SpanDataAssert] {
68+
override def accept(span: SpanDataAssert): Unit = {
69+
span
70+
.hasName("HTTP")
71+
.hasKind(SpanKind.SERVER)
72+
.hasNoParent()
73+
.hasAttributesSatisfying(
74+
equalTo(HttpAttributes.HTTP_REQUEST_METHOD, "_OTHER"),
75+
equalTo(
76+
HttpAttributes.HTTP_RESPONSE_STATUS_CODE,
77+
java.lang.Long.valueOf(400)
78+
)
79+
)
80+
()
81+
}
82+
})
83+
})
84+
} finally Await.result(binding.unbind(), 10.seconds)
85+
} finally Await.result(system.terminate(), 10.seconds)
86+
}
87+
88+
private def send(port: Int, request: String): List[String] = {
89+
val socket = new Socket("localhost", port)
90+
try {
91+
val out =
92+
new OutputStreamWriter(
93+
socket.getOutputStream,
94+
StandardCharsets.US_ASCII
95+
)
96+
out.write(request)
97+
out.flush()
98+
99+
val in = new BufferedReader(
100+
new InputStreamReader(socket.getInputStream, StandardCharsets.US_ASCII)
101+
)
102+
val lines = ListBuffer[String]()
103+
var line = in.readLine()
104+
while (line != null) {
105+
lines += line
106+
line = in.readLine()
107+
}
108+
lines.toList
109+
} finally socket.close()
110+
}
111+
}

0 commit comments

Comments
 (0)