From feb3fb9c5181ea2d9e63e0a71fa88b39271daac6 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 28 Sep 2022 01:45:04 +0300 Subject: [PATCH] Fix akka-http and tomcat10 latest dep tests (#6766) Resolves https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/6760 Resolves https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/6759 --- .../akka/akka-http-10.0/javaagent/build.gradle.kts | 5 ++++- .../src/test/groovy/TomcatServlet3Test.groovy | 2 +- .../src/test/groovy/TomcatServlet5Test.groovy | 2 +- .../tomcat/v10_0/TomcatHandlerTest.groovy | 2 +- .../tomcat/v7_0/TomcatHandlerTest.groovy | 2 +- .../instrumentation/tomcat/common/TomcatHelper.java | 12 ++++++++++++ .../tomcat/common/TomcatHttpAttributesGetter.java | 12 +++++++----- .../tomcat/common/TomcatNetAttributesGetter.java | 8 +++++--- 8 files changed, 32 insertions(+), 13 deletions(-) diff --git a/instrumentation/akka/akka-http-10.0/javaagent/build.gradle.kts b/instrumentation/akka/akka-http-10.0/javaagent/build.gradle.kts index 1d71c79b190e..18be4f537e24 100644 --- a/instrumentation/akka/akka-http-10.0/javaagent/build.gradle.kts +++ b/instrumentation/akka/akka-http-10.0/javaagent/build.gradle.kts @@ -42,7 +42,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().configureEach { diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy index 095b9677b808..93cfeacd8e6b 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy @@ -233,7 +233,7 @@ abstract class TomcatServlet3Test extends AbstractServlet3Test 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 { diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy index 7d03ba845156..a3b061aff9ea 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy @@ -233,7 +233,7 @@ abstract class TomcatServlet5Test extends AbstractServlet5Test 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 { diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy index ec5c46ecf7d5..f640e25dae86 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy @@ -105,7 +105,7 @@ class TomcatHandlerTest extends HttpServerTest 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 { diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy index 96312433a6fe..2f4c06b70deb 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy @@ -105,7 +105,7 @@ class TomcatHandlerTest extends HttpServerTest 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 { diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHelper.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHelper.java index 0cc65cee7547..15d8cd292b2c 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHelper.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHelper.java @@ -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 { protected final Instrumenter instrumenter; @@ -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(); + } } diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHttpAttributesGetter.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHttpAttributesGetter.java index af62bbd853ea..7035a0bcc6e6 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHttpAttributesGetter.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatHttpAttributesGetter.java @@ -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; @@ -22,14 +24,14 @@ public class TomcatHttpAttributesGetter implements HttpServerAttributesGetter 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/")) { diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatNetAttributesGetter.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatNetAttributesGetter.java index 30c8ffb01164..e75d60055224 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatNetAttributesGetter.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatNetAttributesGetter.java @@ -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; @@ -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 @@ -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 @@ -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