Use content type utils where possible (#112)

pavolloffay · web-flow · commit a339bd5747af · 2020-11-13T16:08:52.000+01:00
* Use content type utils where possible

Signed-off-by: Pavol Loffay &lt;p.loffay@gmail.com&gt;

* Add test

Signed-off-by: Pavol Loffay &lt;p.loffay@gmail.com&gt;
diff --git a/instrumentation/okhttp/okhttp-3.0/src/test/java/io/opentelemetry/instrumentation/hypertrace/okhttp/v3_0/OkHttpTracingInterceptorTest.java b/instrumentation/okhttp/okhttp-3.0/src/test/java/io/opentelemetry/instrumentation/hypertrace/okhttp/v3_0/OkHttpTracingInterceptorTest.java
@@ -29,7 +29,6 @@
 import org.hypertrace.agent.testing.AbstractInstrumenterTest;
 import org.hypertrace.agent.testing.TestHttpServer;
 import org.hypertrace.agent.testing.TestHttpServer.GetJsonHandler;
-import org.hypertrace.agent.testing.TestHttpServer.GetPlainTextHandler;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeAll;
@@ -112,34 +111,6 @@ public void getJson() throws Exception {
         clientSpan.getAttributes().get(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY));
   }
 
-  @Test
-  public void getPlainText() throws Exception {
-    Request request =
-        new Builder()
-            .url(String.format("http://localhost:%d/get_plain_text", testHttpServer.port()))
-            .get()
-            .build();
-
-    Response response = client.newCall(request).execute();
-    response.close();
-
-    TEST_WRITER.waitForTraces(1);
-    List<List<SpanData>> traces = TEST_WRITER.getTraces();
-    Assertions.assertEquals(1, traces.size());
-    Assertions.assertEquals(1, traces.get(0).size());
-    SpanData clientSpan = traces.get(0).get(0);
-    Assertions.assertEquals(
-        "test-value",
-        clientSpan
-            .getAttributes()
-            .get(HypertraceSemanticAttributes.httpResponseHeader("test-response-header")));
-    Assertions.assertNull(
-        clientSpan.getAttributes().get(HypertraceSemanticAttributes.HTTP_REQUEST_BODY));
-    Assertions.assertEquals(
-        GetPlainTextHandler.RESPONSE_BODY,
-        clientSpan.getAttributes().get(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY));
-  }
-
   @Test
   public void postUrlEncoded() throws Exception {
     FormBody formBody = new FormBody.Builder().add("foo", "bar").build();
diff --git a/instrumentation/servlet/servlet-2.3/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v2_3/BufferingHttpServletRequest.java b/instrumentation/servlet/servlet-2.3/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v2_3/BufferingHttpServletRequest.java
@@ -32,6 +32,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
 import javax.servlet.http.HttpServletResponse;
+import org.hypertrace.agent.core.ContentTypeUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -192,12 +193,7 @@ private boolean shouldReadContent() {
     if (contentType == null || contentType.isEmpty()) {
       return false;
     }
-    if (contentType.contains("json")
-        || contentType.contains("x-www-form-urlencoded")
-        || contentType.contains("text/plain")) {
-      return true;
-    }
-    return false;
+    return ContentTypeUtils.shouldCapture(contentType);
   }
 
   public Map<String, List<String>> getBufferedParams() {
diff --git a/instrumentation/servlet/servlet-2.3/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v2_3/BufferingHttpServletResponse.java b/instrumentation/servlet/servlet-2.3/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v2_3/BufferingHttpServletResponse.java
@@ -32,6 +32,7 @@
 import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpServletResponseWrapper;
+import org.hypertrace.agent.core.ContentTypeUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -195,12 +196,7 @@ private boolean shouldReadContent() {
     if (contentType == null || contentType.isEmpty()) {
       return false;
     }
-    if (contentType.contains("json")
-        || contentType.contains("x-www-form-urlencoded")
-        || contentType.contains("text/plain")) {
-      return true;
-    }
-    return false;
+    return ContentTypeUtils.shouldCapture(contentType);
   }
 
   public static class BufferingServletOutputStream extends ServletOutputStream {
diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_0/BufferingHttpServletRequest.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_0/BufferingHttpServletRequest.java
@@ -33,6 +33,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
 import javax.servlet.http.HttpServletResponse;
+import org.hypertrace.agent.core.ContentTypeUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -198,12 +199,7 @@ private boolean shouldReadContent() {
     if (contentType == null || contentType.isEmpty()) {
       return false;
     }
-    if (contentType.contains("json")
-        || contentType.contains("x-www-form-urlencoded")
-        || contentType.contains("text/plain")) {
-      return true;
-    }
-    return false;
+    return ContentTypeUtils.shouldCapture(contentType);
   }
 
   public Map<String, List<String>> getBufferedParams() {
diff --git a/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_0/BufferingHttpServletResponse.java b/instrumentation/servlet/servlet-3.0/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_0/BufferingHttpServletResponse.java
@@ -26,6 +26,7 @@
 import javax.servlet.ServletOutputStream;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpServletResponseWrapper;
+import org.hypertrace.agent.core.ContentTypeUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -108,12 +109,7 @@ private boolean shouldReadContent() {
     if (contentType == null || contentType.isEmpty()) {
       return false;
     }
-    if (contentType.contains("json")
-        || contentType.contains("x-www-form-urlencoded")
-        || contentType.contains("text/plain")) {
-      return true;
-    }
-    return false;
+    return ContentTypeUtils.shouldCapture(contentType);
   }
 
   public static class BufferingServletOutputStream extends ServletOutputStream {
diff --git a/instrumentation/servlet/servlet-3.1/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_1/BufferingHttpServletRequest.java b/instrumentation/servlet/servlet-3.1/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_1/BufferingHttpServletRequest.java
@@ -34,6 +34,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
 import javax.servlet.http.HttpServletResponse;
+import org.hypertrace.agent.core.ContentTypeUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -199,12 +200,7 @@ private boolean shouldReadContent() {
     if (contentType == null || contentType.isEmpty()) {
       return false;
     }
-    if (contentType.contains("json")
-        || contentType.contains("x-www-form-urlencoded")
-        || contentType.contains("text/plain")) {
-      return true;
-    }
-    return false;
+    return ContentTypeUtils.shouldCapture(contentType);
   }
 
   public Map<String, List<String>> getBufferedParams() {
diff --git a/instrumentation/servlet/servlet-3.1/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_1/BufferingHttpServletResponse.java b/instrumentation/servlet/servlet-3.1/src/main/java/io/opentelemetry/instrumentation/hypertrace/servlet/v3_1/BufferingHttpServletResponse.java
@@ -27,6 +27,7 @@
 import javax.servlet.WriteListener;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpServletResponseWrapper;
+import org.hypertrace.agent.core.ContentTypeUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -109,12 +110,7 @@ private boolean shouldReadContent() {
     if (contentType == null || contentType.isEmpty()) {
       return false;
     }
-    if (contentType.contains("json")
-        || contentType.contains("x-www-form-urlencoded")
-        || contentType.contains("text/plain")) {
-      return true;
-    }
-    return false;
+    return ContentTypeUtils.shouldCapture(contentType);
   }
 
   public static class BufferingServletOutputStream extends ServletOutputStream {
diff --git a/javaagent-core/src/main/java/org/hypertrace/agent/core/ContentTypeUtils.java b/javaagent-core/src/main/java/org/hypertrace/agent/core/ContentTypeUtils.java
@@ -30,14 +30,9 @@ public static boolean shouldCapture(String contentType) {
     if (contentType == null) {
       return false;
     }
-
     contentType = contentType.toLowerCase();
-    if (contentType.contains("json")
-        || contentType.contains("x-www-form-urlencoded")
-        || contentType.contains("text/plain")) {
-      return true;
-    }
-
-    return false;
+    return contentType.contains("json")
+        || contentType.contains("graphql")
+        || contentType.contains("x-www-form-urlencoded");
   }
 }
diff --git a/javaagent-core/src/test/java/org/hypertrace/agent/core/ContentTypeUtilsTest.java b/javaagent-core/src/test/java/org/hypertrace/agent/core/ContentTypeUtilsTest.java
@@ -0,0 +1,41 @@
+/*
+ * Copyright The Hypertrace Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.hypertrace.agent.core;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+class ContentTypeUtilsTest {
+
+  @Test
+  public void shouldCapture() {
+    String[] valid = {"jSoN", "graphQL", "x-www-forM-urlencoded"};
+    for (String contentType : valid) {
+      Assertions.assertTrue(ContentTypeUtils.shouldCapture(contentType), contentType);
+    }
+  }
+
+  @Test
+  public void shouldNotCapture() {
+    String[] valid = {
+      "text/plain",
+    };
+    for (String contentType : valid) {
+      Assertions.assertFalse(ContentTypeUtils.shouldCapture(contentType), contentType);
+    }
+  }
+}
diff --git a/smoke-tests/src/test/java/org/hypertrace/agent/smoketest/SpringBootSmokeTest.java b/smoke-tests/src/test/java/org/hypertrace/agent/smoketest/SpringBootSmokeTest.java
@@ -20,13 +20,10 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.List;
 import java.util.jar.Attributes;
 import java.util.jar.JarFile;
-import java.util.stream.Collectors;
 import okhttp3.Request;
 import okhttp3.Response;
-import org.hypertrace.agent.core.HypertraceSemanticAttributes;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
@@ -116,18 +113,20 @@ public void get() throws IOException {
                 .map(attribute -> attribute.getValue().getStringValue())
                 .count()
             > 0);
-    List<String> responseBodyAttributes =
-        getSpanStream(traces)
-            .flatMap(span -> span.getAttributesList().stream())
-            .filter(
-                attribute ->
-                    attribute
-                        .getKey()
-                        .contains(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY.getKey()))
-            .map(attribute -> attribute.getValue().getStringValue())
-            .collect(Collectors.toList());
-    Assertions.assertEquals(1, responseBodyAttributes.size());
-    Assertions.assertEquals("Hi!", responseBodyAttributes.get(0));
+    // OTEL BS smoke test app does not have an endpoint that uses content type what we capture
+    // enable once we add smoke tests apps to our build.
+    //    List<String> responseBodyAttributes =
+    //        getSpanStream(traces)
+    //            .flatMap(span -> span.getAttributesList().stream())
+    //            .filter(
+    //                attribute ->
+    //                    attribute
+    //                        .getKey()
+    //                        .contains(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY.getKey()))
+    //            .map(attribute -> attribute.getValue().getStringValue())
+    //            .collect(Collectors.toList());
+    //    Assertions.assertEquals(1, responseBodyAttributes.size());
+    //    Assertions.assertEquals("Hi!", responseBodyAttributes.get(0));
   }
 
   @Test
diff --git a/testing-common/src/main/java/org/hypertrace/agent/testing/TestHttpServer.java b/testing-common/src/main/java/org/hypertrace/agent/testing/TestHttpServer.java
@@ -33,7 +33,6 @@ public void start() throws Exception {
     HandlerList handlerList = new HandlerList();
     handlerList.addHandler(new GetNoContentHandler());
     handlerList.addHandler(new GetJsonHandler());
-    handlerList.addHandler(new GetPlainTextHandler());
     handlerList.addHandler(new PostHandler());
     handlerList.addHandler(new PostRedirect());
     server.setHandler(handlerList);
@@ -103,27 +102,6 @@ public void handle(
     }
   }
 
-  public static class GetPlainTextHandler extends ResponseTestHeadersHandler {
-    public static final String RESPONSE_BODY = "name: james";
-
-    @Override
-    public void handle(
-        String target,
-        Request baseRequest,
-        HttpServletRequest request,
-        HttpServletResponse response)
-        throws IOException {
-      super.handle(target, baseRequest, request, response);
-
-      if (target.equals("/get_plain_text") && "get".equalsIgnoreCase(request.getMethod())) {
-        response.setStatus(200);
-        response.setContentType("tExt/pLain");
-        response.getWriter().print(RESPONSE_BODY);
-        baseRequest.setHandled(true);
-      }
-    }
-  }
-
   static class PostHandler extends ResponseTestHeadersHandler {
     @Override
     public void handle(

Original file line number	Diff line number	Diff line change
`@@ -30,14 +30,9 @@ public static boolean shouldCapture(String contentType) {`
`30`	`30`	`if (contentType == null) {`
`31`	`31`	`return false;`
`32`	`32`	`}`
`33`		`-`
`34`	`33`	`contentType = contentType.toLowerCase();`
`35`		`- if (contentType.contains("json")`
`36`		`- \|\| contentType.contains("x-www-form-urlencoded")`
`37`		`- \|\| contentType.contains("text/plain")) {`
`38`		`- return true;`
`39`		`- }`
`40`		`-`
`41`		`- return false;`
	`34`	`+ return contentType.contains("json")`
	`35`	`+ \|\| contentType.contains("graphql")`
	`36`	`+ \|\| contentType.contains("x-www-form-urlencoded");`
`42`	`37`	`}`
`43`	`38`	`}`