Skip to content

Fix akka-http and tomcat10 latest dep tests #6766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ dependencies {
testInstrumentation(project(":instrumentation:akka:akka-actor-fork-join-2.5:javaagent"))

latestDepTestLibrary("com.typesafe.akka:akka-http_2.13:+")
latestDepTestLibrary("com.typesafe.akka:akka-stream_2.13:+")
// FIXME: latest akka 2.7.0-M2 isn't compatible with latest akka-http
// change back to latestDepTestLibrary("com.typesafe.akka:akka-stream_2.13:+") when there is a
// new release of akka-http
latestDepTestLibrary("com.typesafe.akka:akka-stream_2.13:2.7.0-M1")
}

tasks.withType<Test>().configureEach {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ abstract class TomcatServlet3Test extends AbstractServlet3Test<Tomcat, Context>
class ErrorHandlerValve extends ErrorReportValve {
@Override
protected void report(Request request, Response response, Throwable t) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.setErrorReported()) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.isError()) {
return
}
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ abstract class TomcatServlet5Test extends AbstractServlet5Test<Tomcat, Context>
class ErrorHandlerValve extends ErrorReportValve {
@Override
protected void report(Request request, Response response, Throwable t) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.setErrorReported()) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.isError()) {
return
}
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class TomcatHandlerTest extends HttpServerTest<Tomcat> implements AgentTestTrait
class ErrorHandlerValve extends ErrorReportValve {
@Override
protected void report(Request request, Response response, Throwable t) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.setErrorReported()) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.isError()) {
return
}
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class TomcatHandlerTest extends HttpServerTest<Tomcat> implements AgentTestTrait
class ErrorHandlerValve extends ErrorReportValve {
@Override
protected void report(Request request, Response response, Throwable t) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.setErrorReported()) {
if (response.getStatus() < 400 || response.getContentWritten() > 0 || !response.isError()) {
return
}
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.opentelemetry.javaagent.instrumentation.servlet.ServletHelper;
import org.apache.coyote.Request;
import org.apache.coyote.Response;
import org.apache.tomcat.util.buf.MessageBytes;

public class TomcatHelper<REQUEST, RESPONSE> {
protected final Instrumenter<Request, Response> instrumenter;
Expand Down Expand Up @@ -66,4 +67,15 @@ public void attachResponseToRequest(Request request, Response response) {
servletHelper.setAsyncListenerResponse(servletRequest, servletResponse);
}
}

static String messageBytesToString(MessageBytes messageBytes) {
// on tomcat 10.1.0 MessageBytes.toString() has a side effect. Calling it caches the string
// value and changes type of the MessageBytes from T_BYTES to T_STR which breaks request
// processing in CoyoteAdapter.postParseRequest when it is called on MessageBytes from
// request.requestURI().
if (messageBytes.getType() == MessageBytes.T_BYTES) {
return messageBytes.getByteChunk().toString();
}
return messageBytes.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

package io.opentelemetry.javaagent.instrumentation.tomcat.common;

import static io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatHelper.messageBytesToString;

import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerAttributesGetter;
import java.util.Collections;
import java.util.List;
Expand All @@ -17,14 +19,14 @@ public class TomcatHttpAttributesGetter implements HttpServerAttributesGetter<Re

@Override
public String method(Request request) {
return request.method().toString();
return messageBytesToString(request.method());
}

@Override
@Nullable
public String target(Request request) {
String target = request.requestURI().toString();
String queryString = request.queryString().toString();
String target = messageBytesToString(request.requestURI());
String queryString = messageBytesToString(request.queryString());
if (queryString != null) {
target += "?" + queryString;
}
Expand All @@ -35,7 +37,7 @@ public String target(Request request) {
@Nullable
public String scheme(Request request) {
MessageBytes schemeMessageBytes = request.scheme();
return schemeMessageBytes.isNull() ? "http" : schemeMessageBytes.toString();
return schemeMessageBytes.isNull() ? "http" : messageBytesToString(schemeMessageBytes);
}

@Override
Expand All @@ -46,7 +48,7 @@ public List<String> requestHeader(Request request, String name) {
@Override
@Nullable
public String flavor(Request request) {
String flavor = request.protocol().toString();
String flavor = messageBytesToString(request.protocol());
if (flavor != null) {
// remove HTTP/ prefix to comply with semantic conventions
if (flavor.startsWith("HTTP/")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

package io.opentelemetry.javaagent.instrumentation.tomcat.common;

import static io.opentelemetry.javaagent.instrumentation.tomcat.common.TomcatHelper.messageBytesToString;

import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import javax.annotation.Nullable;
Expand All @@ -22,7 +24,7 @@ public String transport(Request request) {
@Nullable
@Override
public String hostName(Request request) {
return request.serverName().toString();
return messageBytesToString(request.serverName());
}

@Override
Expand All @@ -34,7 +36,7 @@ public Integer hostPort(Request request) {
@Nullable
public String sockPeerAddr(Request request) {
request.action(ActionCode.REQ_HOST_ADDR_ATTRIBUTE, request);
return request.remoteAddr().toString();
return messageBytesToString(request.remoteAddr());
}

@Override
Expand All @@ -48,7 +50,7 @@ public Integer sockPeerPort(Request request) {
@Override
public String sockHostAddr(Request request) {
request.action(ActionCode.REQ_LOCAL_ADDR_ATTRIBUTE, request);
return request.localAddr().toString();
return messageBytesToString(request.localAddr());
}

@Nullable
Expand Down