From 1008aef1692aa7332d679ad553f582a6d9da83c8 Mon Sep 17 00:00:00 2001 From: siyuniu-ms Date: Fri, 24 Feb 2023 15:05:03 -0800 Subject: [PATCH] refactor, rename --- ...tputStreamSnippetInjectionHelperTest.java} | 20 ++++++++--------- .../SnippetInjectingResponseWrapperTest.java | 22 +++++++++---------- ...lAfter.html => afterSnippetInjection.html} | 0 ...html => afterSnippetInjectionChinese.html} | 0 ...rigin.html => beforeSnippetInjection.html} | 0 ...tml => beforeSnippetInjectionChinese.html} | 0 .../servlet-3.0/javaagent/build.gradle.kts | 1 - .../Servlet3OutputStreamWriteBytesAdvice.java | 2 +- ...OutputStreamWriteBytesAndOffsetAdvice.java | 2 +- .../OutputStreamSnippetInjectionHelper.java | 2 +- .../test/groovy/AbstractServlet3Test.groovy | 1 - .../src/test/groovy/TestServlet3.groovy | 4 +--- .../servlet/ExperimentalSnippetHolder.java | 2 +- .../ServletOutputStreamInstrumentation.java | 16 +++++++------- .../testing/junit/http/ServerEndpoint.java | 4 ++-- 15 files changed, 36 insertions(+), 40 deletions(-) rename instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/{InjectionTest.java => OutputStreamSnippetInjectionHelperTest.java} (88%) rename instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/{staticHtmlAfter.html => afterSnippetInjection.html} (100%) rename instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/{staticHtmlChineseAfter.html => afterSnippetInjectionChinese.html} (100%) rename instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/{staticHtmlOrigin.html => beforeSnippetInjection.html} (100%) rename instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/{staticHtmlChineseOrigin.html => beforeSnippetInjectionChinese.html} (100%) diff --git a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/InjectionTest.java b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/OutputStreamSnippetInjectionHelperTest.java similarity index 88% rename from instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/InjectionTest.java rename to instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/OutputStreamSnippetInjectionHelperTest.java index 0b8f3bdc501d..bb719deb43bd 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/InjectionTest.java +++ b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/OutputStreamSnippetInjectionHelperTest.java @@ -17,13 +17,13 @@ import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; -class InjectionTest { +class OutputStreamSnippetInjectionHelperTest { @Test void testInjectionForStringContainHeadTag() throws IOException { String testSnippet = "\n "; - String original = readFile("staticHtmlOrigin.html"); - String correct = readFile("staticHtmlAfter.html"); + String original = readFile("beforeSnippetInjection.html"); + String correct = readFile("afterSnippetInjection.html"); byte[] originalBytes = original.getBytes(StandardCharsets.UTF_8); SnippetInjectingResponseWrapper response = mock(SnippetInjectingResponseWrapper.class); when(response.isCommitted()).thenReturn(false); @@ -40,7 +40,7 @@ public void write(int b) throws IOException { } }; OutputStreamSnippetInjectionHelper helper = new OutputStreamSnippetInjectionHelper(testSnippet); - boolean injected = helper.handleWrite(originalBytes, 0, originalBytes.length, obj, sp); + boolean injected = helper.handleWrite(obj, sp, originalBytes, 0, originalBytes.length); assertThat(obj.getHeadTagBytesSeen()).isEqualTo(-1); assertThat(injected).isEqualTo(true); writer.flush(); @@ -54,8 +54,8 @@ public void write(int b) throws IOException { @Disabled void testInjectionForChinese() throws IOException { String testSnippet = "\n "; - String original = readFile("staticHtmlChineseOrigin.html"); - String correct = readFile("staticHtmlChineseAfter.html"); + String original = readFile("beforeSnippetInjectionChinese.html"); + String correct = readFile("afterSnippetInjectionChinese.html"); byte[] originalBytes = original.getBytes(StandardCharsets.UTF_8); SnippetInjectingResponseWrapper response = mock(SnippetInjectingResponseWrapper.class); @@ -73,7 +73,7 @@ public void write(int b) throws IOException { } }; OutputStreamSnippetInjectionHelper helper = new OutputStreamSnippetInjectionHelper(testSnippet); - boolean injected = helper.handleWrite(originalBytes, 0, originalBytes.length, obj, sp); + boolean injected = helper.handleWrite(obj, sp, originalBytes, 0, originalBytes.length); assertThat(obj.getHeadTagBytesSeen()).isEqualTo(-1); assertThat(injected).isEqualTo(true); writer.flush(); @@ -103,7 +103,7 @@ public void write(int b) throws IOException { } }; OutputStreamSnippetInjectionHelper helper = new OutputStreamSnippetInjectionHelper(testSnippet); - boolean injected = helper.handleWrite(originalBytes, 0, originalBytes.length, obj, sp); + boolean injected = helper.handleWrite(obj, sp, originalBytes, 0, originalBytes.length); assertThat(obj.getHeadTagBytesSeen()).isEqualTo(0); assertThat(injected).isEqualTo(false); writer.flush(); @@ -132,7 +132,7 @@ public void write(int b) throws IOException { }; OutputStreamSnippetInjectionHelper helper = new OutputStreamSnippetInjectionHelper(testSnippet); boolean injected = - helper.handleWrite(originalFirstPartBytes, 0, originalFirstPartBytes.length, obj, sp); + helper.handleWrite(obj, sp, originalFirstPartBytes, 0, originalFirstPartBytes.length); writer.flush(); String result = writer.toString(); @@ -150,7 +150,7 @@ public void write(int b) throws IOException { + ""; byte[] originalSecondPartBytes = originalSecondPart.getBytes(StandardCharsets.UTF_8); injected = - helper.handleWrite(originalSecondPartBytes, 0, originalSecondPartBytes.length, obj, sp); + helper.handleWrite(obj, sp, originalSecondPartBytes, 0, originalSecondPartBytes.length); assertThat(obj.getHeadTagBytesSeen()).isEqualTo(-1); assertThat(injected).isEqualTo(true); String correctSecondPart = diff --git a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/SnippetInjectingResponseWrapperTest.java b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/SnippetInjectingResponseWrapperTest.java index 69d78c1ddfd9..437a446cf7d1 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/SnippetInjectingResponseWrapperTest.java +++ b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/SnippetInjectingResponseWrapperTest.java @@ -23,8 +23,8 @@ class SnippetInjectingResponseWrapperTest { @Test void testInjectToTextHtml() throws IOException { - String original = readFile("staticHtmlOrigin.html"); - String correct = readFile("staticHtmlAfter.html"); + String original = readFile("beforeSnippetInjection.html"); + String correct = readFile("afterSnippetInjection.html"); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getContentType()).thenReturn("text/html"); when(response.getStatus()).thenReturn(200); @@ -47,8 +47,8 @@ void testInjectToTextHtml() throws IOException { @Disabled void testInjectToChineseTextHtml() throws IOException { - String original = readFile("staticHtmlChineseOrigin.html"); - String correct = readFile("staticHtmlChineseAfter.html"); + String original = readFile("beforeSnippetInjectionChinese.html"); + String correct = readFile("afterSnippetInjectionChinese.html"); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getContentType()).thenReturn("text/html"); @@ -69,7 +69,7 @@ void testInjectToChineseTextHtml() throws IOException { @Test void shouldNotInjectToTextHtml() throws IOException { - String original = readFile("staticHtmlOrigin.html"); + String original = readFile("beforeSnippetInjection.html"); StringWriter writer = new StringWriter(); HttpServletResponse response = mock(HttpServletResponse.class); @@ -94,8 +94,8 @@ void shouldNotInjectToTextHtml() throws IOException { @Test void testWriteInt() throws IOException { - String original = readFile("staticHtmlOrigin.html"); - String correct = readFile("staticHtmlAfter.html"); + String original = readFile("beforeSnippetInjection.html"); + String correct = readFile("afterSnippetInjection.html"); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getContentType()).thenReturn("text/html"); @@ -120,8 +120,8 @@ void testWriteInt() throws IOException { @Test void testWriteCharArray() throws IOException { - String original = readFile("staticHtmlChineseOrigin.html"); - String correct = readFile("staticHtmlChineseAfter.html"); + String original = readFile("beforeSnippetInjectionChinese.html"); + String correct = readFile("afterSnippetInjectionChinese.html"); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getContentType()).thenReturn("text/html"); when(response.getStatus()).thenReturn(200); @@ -145,8 +145,8 @@ void testWriteCharArray() throws IOException { @Test void testWriteWithOffset() throws IOException { - String original = readFile("staticHtmlChineseOrigin.html"); - String correct = readFile("staticHtmlChineseAfter.html"); + String original = readFile("beforeSnippetInjectionChinese.html"); + String correct = readFile("afterSnippetInjectionChinese.html"); String extraBuffer = "this buffer should not be print out"; original = extraBuffer + original; HttpServletResponse response = mock(HttpServletResponse.class); diff --git a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlAfter.html b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/afterSnippetInjection.html similarity index 100% rename from instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlAfter.html rename to instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/afterSnippetInjection.html diff --git a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlChineseAfter.html b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/afterSnippetInjectionChinese.html similarity index 100% rename from instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlChineseAfter.html rename to instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/afterSnippetInjectionChinese.html diff --git a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlOrigin.html b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/beforeSnippetInjection.html similarity index 100% rename from instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlOrigin.html rename to instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/beforeSnippetInjection.html diff --git a/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlChineseOrigin.html b/instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/beforeSnippetInjectionChinese.html similarity index 100% rename from instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/staticHtmlChineseOrigin.html rename to instrumentation/servlet/servlet-3.0/javaagent-unit-tests/src/test/resources/beforeSnippetInjectionChinese.html diff --git a/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts b/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts index e9d9f0a6d093..ee6fcecfeb37 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts +++ b/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts @@ -27,7 +27,6 @@ dependencies { testLibrary("org.eclipse.jetty:jetty-server:8.0.0.v20110901") testLibrary("org.eclipse.jetty:jetty-servlet:8.0.0.v20110901") - // TODO (trask?) test against tomcat 7 (servlet 3.0) testLibrary("org.apache.tomcat.embed:tomcat-embed-core:8.0.41") testLibrary("org.apache.tomcat.embed:tomcat-embed-jasper:8.0.41") diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAdvice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAdvice.java index b07fc3b14bd4..d17e8e551224 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAdvice.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAdvice.java @@ -29,6 +29,6 @@ public static boolean methodEnter( // if it returns false, then it means nothing was written to the servletOutputStream and the // original method call should be executed return !getSnippetInjectionHelper() - .handleWrite(write, 0, write.length, state, servletOutputStream); + .handleWrite(state, servletOutputStream, write, 0, write.length); } } diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAndOffsetAdvice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAndOffsetAdvice.java index a9969cfe7eef..e2c0a1823f42 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAndOffsetAdvice.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3OutputStreamWriteBytesAndOffsetAdvice.java @@ -30,6 +30,6 @@ public static boolean methodEnter( // call (see skipOn above) // if it returns false, then it means nothing was written to the servletOutputStream and the // original method call should be executed - return !getSnippetInjectionHelper().handleWrite(write, off, len, state, servletOutputStream); + return !getSnippetInjectionHelper().handleWrite(state, servletOutputStream, write, off, len); } } diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/OutputStreamSnippetInjectionHelper.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/OutputStreamSnippetInjectionHelper.java index 6e87ce65fb2d..e88cb15ab9b9 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/OutputStreamSnippetInjectionHelper.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/snippet/OutputStreamSnippetInjectionHelper.java @@ -29,7 +29,7 @@ public OutputStreamSnippetInjectionHelper(String snippet) { * true, and would write the original bytes when the return value is false. */ public boolean handleWrite( - byte[] original, int off, int length, InjectionState state, OutputStream out) + InjectionState state, OutputStream out, byte[] original, int off, int length) throws IOException { if (state.isHeadTagWritten()) { return false; diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy index a79fe7a8c1d8..aadfefed9952 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy @@ -109,7 +109,6 @@ abstract class AbstractServlet3Test extends HttpServerTest\n" + "\n" + diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TestServlet3.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TestServlet3.groovy index 2002be3a88f1..e992ea8ba97b 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TestServlet3.groovy +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TestServlet3.groovy @@ -228,13 +228,11 @@ class TestServlet3 { resp.writer.print(endpoint.body) throw new ServletException(endpoint.body) case HTML_PRINT_WRITER: - // intentionally testing HTML_PRINT_WRITER and HTML_SERVLET_OUTPUT_STREAM with different order of setting status and contentType - resp.status = endpoint.status + // intentionally testing setting status before contentType here to cover that case somewhere resp.status = endpoint.status resp.contentType = "text/html" resp.writer.print(endpoint.body) break case HTML_SERVLET_OUTPUT_STREAM: - // intentionally testing HTML_PRINT_WRITER and HTML_SERVLET_OUTPUT_STREAM with different order of setting status and contentType resp.contentType = "text/html" resp.status = endpoint.status resp.getOutputStream().print(endpoint.body) diff --git a/instrumentation/servlet/servlet-common/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/ExperimentalSnippetHolder.java b/instrumentation/servlet/servlet-common/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/ExperimentalSnippetHolder.java index dcf787748f6f..acc944ffa4b1 100644 --- a/instrumentation/servlet/servlet-common/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/ExperimentalSnippetHolder.java +++ b/instrumentation/servlet/servlet-common/bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/servlet/ExperimentalSnippetHolder.java @@ -7,7 +7,7 @@ public class ExperimentalSnippetHolder { - private static String snippet = ""; + private static volatile String snippet = ""; public static void setSnippet(String snippet) { ExperimentalSnippetHolder.snippet = snippet; diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletOutputStreamInstrumentation.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletOutputStreamInstrumentation.java index df80666d71d1..a545f3874b7a 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletOutputStreamInstrumentation.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletOutputStreamInstrumentation.java @@ -20,18 +20,18 @@ public class ServletOutputStreamInstrumentation implements TypeInstrumentation { private final String basePackageName; - private final String writeBytesAndOffsetClassName; - private final String writeBytesClassName; + private final String writeBytesAndOffsetAdviceClassName; + private final String writeBytesAdviceClassName; private final String writeIntAdviceClassName; public ServletOutputStreamInstrumentation( String basePackageName, - String writeBytesAndOffsetClassName, - String writeBytesClassName, + String writeBytesAndOffsetAdviceClassName, + String writeBytesAdviceClassName, String writeIntAdviceClassName) { this.basePackageName = basePackageName; - this.writeBytesAndOffsetClassName = writeBytesAndOffsetClassName; - this.writeBytesClassName = writeBytesClassName; + this.writeBytesAndOffsetAdviceClassName = writeBytesAndOffsetAdviceClassName; + this.writeBytesAdviceClassName = writeBytesAdviceClassName; this.writeIntAdviceClassName = writeIntAdviceClassName; } @@ -54,10 +54,10 @@ public void transform(TypeTransformer transformer) { .and(takesArgument(1, int.class)) .and(takesArgument(2, int.class)) .and(isPublic()), - writeBytesAndOffsetClassName); + writeBytesAndOffsetAdviceClassName); transformer.applyAdviceToMethod( named("write").and(takesArguments(1)).and(takesArgument(0, byte[].class)).and(isPublic()), - writeBytesClassName); + writeBytesAdviceClassName); transformer.applyAdviceToMethod( named("write").and(takesArguments(1)).and(takesArgument(0, int.class)).and(isPublic()), writeIntAdviceClassName); diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java index 53895d00f1f8..78751a47f7b3 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/ServerEndpoint.java @@ -33,7 +33,7 @@ public enum ServerEndpoint { AUTH_ERROR("basicsecured/endpoint", 401, null), INDEXED_CHILD("child", 200, ""), HTML_PRINT_WRITER( - "HTML_PRINT_WRITER", + "htmlPrintWriter", 200, "\n" + "\n" @@ -46,7 +46,7 @@ public enum ServerEndpoint { + "\n" + ""), HTML_SERVLET_OUTPUT_STREAM( - "HTML_SERVLET_OUTPUT_STREAM", + "htmlServletOutputStream", 200, "\n" + "\n"