From f9ae0d818babab4b7150197543b681134a3142fa Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 19 Aug 2019 15:50:14 -0700 Subject: [PATCH 1/8] chore: add basic test for OpenCensus spans --- google-http-client/pom.xml | 23 +++-- .../client/http/HttpRequestTracingTest.java | 91 +++++++++++++++++++ pom.xml | 10 ++ 3 files changed, 118 insertions(+), 6 deletions(-) create mode 100644 google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java diff --git a/google-http-client/pom.xml b/google-http-client/pom.xml index fe8e384d4..45d97fea3 100644 --- a/google-http-client/pom.xml +++ b/google-http-client/pom.xml @@ -84,6 +84,19 @@ com.google.guava guava + + com.google.j2objc + j2objc-annotations + + + io.opencensus + opencensus-api + + + io.opencensus + opencensus-contrib-http-util + + com.google.guava guava-testlib @@ -104,17 +117,15 @@ mockito-all test - - com.google.j2objc - j2objc-annotations - io.opencensus - opencensus-api + opencensus-impl + test io.opencensus - opencensus-contrib-http-util + opencensus-testing + test diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java new file mode 100644 index 000000000..63a808b4a --- /dev/null +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java @@ -0,0 +1,91 @@ +package com.google.api.client.http; + +import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpResponse; +import io.opencensus.common.Functions; +import io.opencensus.testing.export.TestHandler; +import io.opencensus.trace.AttributeValue; +import io.opencensus.trace.MessageEvent; +import io.opencensus.trace.Status; +import io.opencensus.trace.Tracing; +import io.opencensus.trace.config.TraceParams; +import io.opencensus.trace.export.SpanData; +import io.opencensus.trace.samplers.Samplers; +import org.junit.*; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static com.google.api.client.http.OpenCensusUtils.SPAN_NAME_HTTP_REQUEST_EXECUTE; +import static org.junit.Assert.*; + +public class HttpRequestTracingTest { + private static final TestHandler testHandler = new TestHandler(); + + @Before + public void setupTestTracer() { + Tracing.getExportComponent().getSpanExporter().registerHandler("test", testHandler); + TraceParams params = + Tracing.getTraceConfig() + .getActiveTraceParams() + .toBuilder() + .setSampler(Samplers.alwaysSample()) + .build(); + Tracing.getTraceConfig().updateActiveTraceParams(params); + } + + @After + public void teardownTestTracer() { + Tracing.getExportComponent().getSpanExporter().unregisterHandler("test"); + } + + @Test(timeout = 20_000L) + public void testExecute_spanClosureOnException() throws IOException { + MockLowLevelHttpResponse mockResponse = new MockLowLevelHttpResponse() + .setStatusCode(200); + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpResponse(mockResponse) + .build(); + HttpRequest request = new HttpRequestFactory(transport, null) + .buildGetRequest(new GenericUrl("https://google.com/")); + HttpResponse response = request.execute(); + + List spans = testHandler.waitForExport(1); + assertEquals(1, spans.size()); + SpanData span = spans.get(0); + + // Ensure the span name is set + assertEquals(SPAN_NAME_HTTP_REQUEST_EXECUTE, span.getName()); + + // Ensure we have basic span attributes + assertAttributeEquals(span, "http.path", "/"); + assertAttributeEquals(span, "http.host", "google.com"); + assertAttributeEquals(span, "http.url", "https://google.com/"); + assertAttributeEquals(span, "http.method", "GET"); + + // Ensure we have a single annotation for starting the first attempt + assertEquals(1, span.getAnnotations().getEvents().size()); + + // Ensure we have 2 message events, SENT and RECEIVED + assertEquals(2, span.getMessageEvents().getEvents().size()); + assertEquals(MessageEvent.Type.SENT, span.getMessageEvents().getEvents().get(0).getEvent().getType()); + assertEquals(MessageEvent.Type.RECEIVED, span.getMessageEvents().getEvents().get(1).getEvent().getType()); + + // Ensure we correctly record the span status as OK + assertEquals(Status.OK, span.getStatus()); + } + + void assertAttributeEquals(SpanData span, String attributeName, String expectedValue) { + Object attributeValue = span.getAttributes().getAttributeMap().get(attributeName); + assertNotNull("expected span to contain attribute: " + attributeName, attributeValue); + assertTrue(attributeValue instanceof AttributeValue); + String value = ((AttributeValue) attributeValue).match( + Functions.returnToString(), + Functions.returnToString(), + Functions.returnToString(), + Functions.returnToString(), + Functions.returnNull()); + assertEquals(expectedValue, value); + } +} diff --git a/pom.xml b/pom.xml index d7d5e8b69..74d26649c 100644 --- a/pom.xml +++ b/pom.xml @@ -242,6 +242,16 @@ opencensus-contrib-http-util ${project.opencensus.version} + + io.opencensus + opencensus-impl + ${project.opencensus.version} + + + io.opencensus + opencensus-testing + ${project.opencensus.version} + From f758535ea8f5013fd475a48ba075fea4be3c90fb Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 19 Aug 2019 16:46:29 -0700 Subject: [PATCH 2/8] chore: add failing test for closing spans on IOExceptions --- .../client/http/HttpRequestTracingTest.java | 50 +++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java index 63a808b4a..31728f159 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java @@ -1,6 +1,7 @@ package com.google.api.client.http; import com.google.api.client.testing.http.MockHttpTransport; +import com.google.api.client.testing.http.MockLowLevelHttpRequest; import com.google.api.client.testing.http.MockLowLevelHttpResponse; import io.opencensus.common.Functions; import io.opencensus.testing.export.TestHandler; @@ -15,7 +16,6 @@ import java.io.IOException; import java.util.List; -import java.util.Map; import static com.google.api.client.http.OpenCensusUtils.SPAN_NAME_HTTP_REQUEST_EXECUTE; import static org.junit.Assert.*; @@ -41,7 +41,7 @@ public void teardownTestTracer() { } @Test(timeout = 20_000L) - public void testExecute_spanClosureOnException() throws IOException { + public void executeCreatesSpan() throws IOException { MockLowLevelHttpResponse mockResponse = new MockLowLevelHttpResponse() .setStatusCode(200); HttpTransport transport = new MockHttpTransport.Builder() @@ -49,8 +49,9 @@ public void testExecute_spanClosureOnException() throws IOException { .build(); HttpRequest request = new HttpRequestFactory(transport, null) .buildGetRequest(new GenericUrl("https://google.com/")); - HttpResponse response = request.execute(); + request.execute(); + // This call blocks - we set a timeout on this test to ensure we don't wait forever List spans = testHandler.waitForExport(1); assertEquals(1, spans.size()); SpanData span = spans.get(0); @@ -76,6 +77,49 @@ public void testExecute_spanClosureOnException() throws IOException { assertEquals(Status.OK, span.getStatus()); } + @Test(timeout = 20_000L) + public void executeExceptionCreatesSpan() throws IOException { + HttpTransport transport = new MockHttpTransport.Builder() + .setLowLevelHttpRequest(new MockLowLevelHttpRequest() { + @Override + public LowLevelHttpResponse execute() throws IOException { + throw new IOException("some IOException"); + } + }) + .build(); + HttpRequest request = new HttpRequestFactory(transport, null) + .buildGetRequest(new GenericUrl("https://google.com/")); + + try { + request.execute(); + fail("expected to throw an IOException"); + } catch (IOException expected) { + } + + // This call blocks - we set a timeout on this test to ensure we don't wait forever + List spans = testHandler.waitForExport(1); + assertEquals(1, spans.size()); + SpanData span = spans.get(0); + + // Ensure the span name is set + assertEquals(SPAN_NAME_HTTP_REQUEST_EXECUTE, span.getName()); + + // Ensure we have basic span attributes + assertAttributeEquals(span, "http.path", "/"); + assertAttributeEquals(span, "http.host", "google.com"); + assertAttributeEquals(span, "http.url", "https://google.com/"); + assertAttributeEquals(span, "http.method", "GET"); + + // Ensure we have a single annotation for starting the first attempt + assertEquals(1, span.getAnnotations().getEvents().size()); + + // Ensure we have 2 message events, SENT and RECEIVED + assertEquals(1, span.getMessageEvents().getEvents().size()); + assertEquals(MessageEvent.Type.SENT, span.getMessageEvents().getEvents().get(0).getEvent().getType()); + + // Ensure we correctly record the span status as OK + assertEquals(Status.UNKNOWN, span.getStatus()); } + void assertAttributeEquals(SpanData span, String attributeName, String expectedValue) { Object attributeValue = span.getAttributes().getAttributeMap().get(attributeName); assertNotNull("expected span to contain attribute: " + attributeName, attributeValue); From 0763216235a8e01babfb95a602a6d2f42e720dc2 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 19 Aug 2019 16:53:10 -0700 Subject: [PATCH 3/8] fix: close SENT span if the IOException was not handled before rethrowing --- .../src/main/java/com/google/api/client/http/HttpRequest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java index 57adecde5..7fb1699e5 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java @@ -1013,6 +1013,7 @@ public HttpResponse execute() throws IOException { if (!retryOnExecuteIOException && (ioExceptionHandler == null || !ioExceptionHandler.handleIOException(this, retryRequest))) { + span.end(OpenCensusUtils.getEndSpanOptions(response == null ? null : response.getStatusCode())); throw e; } // Save the exception in case the retries do not work and we need to re-throw it later. From 7d5ee76bdddd3abd6901d5db75e9d7a1243f7ebd Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 28 Aug 2019 07:08:30 -0700 Subject: [PATCH 4/8] relax curl logger tests for added opencensus header --- .../api/client/http/HttpRequestTest.java | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java index f024e05cb..17d4535fe 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java @@ -1222,13 +1222,9 @@ public void testExecute_curlLogger() throws Exception { for (String message : recorder.messages()) { if (message.startsWith("curl")) { found = true; - assertEquals( - "curl -v --compressed -H 'Accept-Encoding: gzip' -H 'User-Agent: " - + "Google-HTTP-Java-Client/" - + HttpRequest.VERSION - + " (gzip)" - + "' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c'", - message); + assert(message.contains("curl -v --compressed -H 'Accept-Encoding: gzip'")); + assert(message.contains("-H 'User-Agent: Google-HTTP-Java-Client/" + HttpRequest.VERSION + " (gzip)'")); + assert(message.contains("' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c'")); } } assertTrue(found); @@ -1253,16 +1249,13 @@ public void testExecute_curlLoggerWithContentEncoding() throws Exception { .execute(); boolean found = false; - final String expectedCurlLog = - "curl -v --compressed -X POST -H 'Accept-Encoding: gzip' " - + "-H 'User-Agent: " - + HttpRequest.USER_AGENT_SUFFIX - + "' -H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip' " - + "-d '@-' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c' << $$$"; for (String message : recorder.messages()) { if (message.startsWith("curl")) { found = true; - assertEquals(expectedCurlLog, message); + assert(message.contains("curl -v --compressed -X POST -H 'Accept-Encoding: gzip'")); + assert(message.contains("-H 'User-Agent: " + HttpRequest.USER_AGENT_SUFFIX + "'")); + assert(message.contains("-H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip'")); + assert(message.contains("-d '@-' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c' << $$$")); } } assertTrue(found); From b06a0db92de0d732c6e9bed574eea435e869a719 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 28 Aug 2019 07:34:40 -0700 Subject: [PATCH 5/8] fix: address PR comments --- .../com/google/api/client/http/HttpRequest.java | 3 ++- .../google/api/client/http/OpenCensusUtils.java | 7 +++++++ .../google/api/client/http/HttpRequestTest.java | 14 +++++++------- .../api/client/http/HttpRequestTracingTest.java | 13 +++++++++++++ 4 files changed, 29 insertions(+), 8 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java index 7fb1699e5..c53c10e07 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java +++ b/google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java @@ -1013,7 +1013,8 @@ public HttpResponse execute() throws IOException { if (!retryOnExecuteIOException && (ioExceptionHandler == null || !ioExceptionHandler.handleIOException(this, retryRequest))) { - span.end(OpenCensusUtils.getEndSpanOptions(response == null ? null : response.getStatusCode())); + // static analysis shows response is always null here + span.end(OpenCensusUtils.getEndSpanOptions(null)); throw e; } // Save the exception in case the retries do not work and we need to re-throw it later. diff --git a/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java b/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java index 5b2c9d41e..4f2968aa1 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java +++ b/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java @@ -179,6 +179,13 @@ public static EndSpanOptions getEndSpanOptions(@Nullable Integer statusCode) { return builder.build(); } + static EndSpanOptions getEndSpanOptionsForResponse(@Nullable HttpResponse response) { + if (response != null) { + return getEndSpanOptions(response.getStatusCode()); + } + return getEndSpanOptions(null); + } + /** * Records a new message event which contains the size of the request content. Note that the size * represents the message size in application layer, i.e., content-length. diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java index 17d4535fe..66b6449eb 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTest.java @@ -1222,9 +1222,9 @@ public void testExecute_curlLogger() throws Exception { for (String message : recorder.messages()) { if (message.startsWith("curl")) { found = true; - assert(message.contains("curl -v --compressed -H 'Accept-Encoding: gzip'")); - assert(message.contains("-H 'User-Agent: Google-HTTP-Java-Client/" + HttpRequest.VERSION + " (gzip)'")); - assert(message.contains("' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c'")); + assertTrue(message.contains("curl -v --compressed -H 'Accept-Encoding: gzip'")); + assertTrue(message.contains("-H 'User-Agent: Google-HTTP-Java-Client/" + HttpRequest.VERSION + " (gzip)'")); + assertTrue(message.contains("' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c'")); } } assertTrue(found); @@ -1252,10 +1252,10 @@ public void testExecute_curlLoggerWithContentEncoding() throws Exception { for (String message : recorder.messages()) { if (message.startsWith("curl")) { found = true; - assert(message.contains("curl -v --compressed -X POST -H 'Accept-Encoding: gzip'")); - assert(message.contains("-H 'User-Agent: " + HttpRequest.USER_AGENT_SUFFIX + "'")); - assert(message.contains("-H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip'")); - assert(message.contains("-d '@-' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c' << $$$")); + assertTrue(message.contains("curl -v --compressed -X POST -H 'Accept-Encoding: gzip'")); + assertTrue(message.contains("-H 'User-Agent: " + HttpRequest.USER_AGENT_SUFFIX + "'")); + assertTrue(message.contains("-H 'Content-Type: text/plain; charset=UTF-8' -H 'Content-Encoding: gzip'")); + assertTrue(message.contains("-d '@-' -- 'http://google.com/#q=a'\"'\"'b'\"'\"'c' << $$$")); } } assertTrue(found); diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java index 31728f159..65d9d211b 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java @@ -1,3 +1,16 @@ +/* + * Copyright 2019 Google LLC + * + * 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 com.google.api.client.http; import com.google.api.client.testing.http.MockHttpTransport; From ae89bc066b2aeced27d4c5e4000811a2cf5aa74d Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 28 Aug 2019 07:36:04 -0700 Subject: [PATCH 6/8] chore: remove unused helper --- .../java/com/google/api/client/http/OpenCensusUtils.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java b/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java index 4f2968aa1..5b2c9d41e 100644 --- a/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java +++ b/google-http-client/src/main/java/com/google/api/client/http/OpenCensusUtils.java @@ -179,13 +179,6 @@ public static EndSpanOptions getEndSpanOptions(@Nullable Integer statusCode) { return builder.build(); } - static EndSpanOptions getEndSpanOptionsForResponse(@Nullable HttpResponse response) { - if (response != null) { - return getEndSpanOptions(response.getStatusCode()); - } - return getEndSpanOptions(null); - } - /** * Records a new message event which contains the size of the request content. Note that the size * represents the message size in application layer, i.e., content-length. From 855db7ccd6711fdecb94321edb5d63313b8761f2 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 28 Aug 2019 07:44:08 -0700 Subject: [PATCH 7/8] fix: dependency warning for opencensus-impl --- google-http-client/pom.xml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/google-http-client/pom.xml b/google-http-client/pom.xml index 45d97fea3..f748898e4 100644 --- a/google-http-client/pom.xml +++ b/google-http-client/pom.xml @@ -16,6 +16,18 @@ + + + + org.apache.maven.plugins + maven-dependency-plugin + 3.1.1 + + io.opencensus:opencensus-impl + + + + maven-javadoc-plugin From 7e2f2c715e0afd227144e3c748db868d37fbb02d Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Wed, 28 Aug 2019 09:27:25 -0700 Subject: [PATCH 8/8] chore: cleanup PR comments --- .../api/client/http/HttpRequestTracingTest.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java index 65d9d211b..5d89f0350 100644 --- a/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java +++ b/google-http-client/src/test/java/com/google/api/client/http/HttpRequestTracingTest.java @@ -25,13 +25,18 @@ import io.opencensus.trace.config.TraceParams; import io.opencensus.trace.export.SpanData; import io.opencensus.trace.samplers.Samplers; -import org.junit.*; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; import java.io.IOException; import java.util.List; import static com.google.api.client.http.OpenCensusUtils.SPAN_NAME_HTTP_REQUEST_EXECUTE; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; public class HttpRequestTracingTest { private static final TestHandler testHandler = new TestHandler(); @@ -86,7 +91,7 @@ public void executeCreatesSpan() throws IOException { assertEquals(MessageEvent.Type.SENT, span.getMessageEvents().getEvents().get(0).getEvent().getType()); assertEquals(MessageEvent.Type.RECEIVED, span.getMessageEvents().getEvents().get(1).getEvent().getType()); - // Ensure we correctly record the span status as OK + // Ensure we record the span status as OK assertEquals(Status.OK, span.getStatus()); } @@ -130,7 +135,7 @@ public LowLevelHttpResponse execute() throws IOException { assertEquals(1, span.getMessageEvents().getEvents().size()); assertEquals(MessageEvent.Type.SENT, span.getMessageEvents().getEvents().get(0).getEvent().getType()); - // Ensure we correctly record the span status as OK + // Ensure we record the span status as UNKNOWN assertEquals(Status.UNKNOWN, span.getStatus()); } void assertAttributeEquals(SpanData span, String attributeName, String expectedValue) {