From caa7e4426a7a15d6133b01c24f06a55e1be4d7b5 Mon Sep 17 00:00:00 2001
From: Laplie Anderson <laplie.anderson@datadoghq.com>
Date: Tue, 9 Jul 2019 15:08:08 -0400
Subject: [PATCH] Passing tests.  Modify escaping of spaces in urls

---
 .../GoogleHttpClientDecorator.java            |  6 +-
 .../GoogleHttpClientInstrumentation.java      |  9 +--
 .../test/groovy/GoogleHttpClientTest.groovy   | 67 +++++++++++++++++++
 3 files changed, 77 insertions(+), 5 deletions(-)
 create mode 100644 dd-java-agent/instrumentation/google-http-client/src/test/groovy/GoogleHttpClientTest.groovy

diff --git a/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientDecorator.java b/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientDecorator.java
index bebdddcf8c..4a6d03c1e5 100644
--- a/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientDecorator.java
+++ b/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientDecorator.java
@@ -16,7 +16,11 @@ public class GoogleHttpClientDecorator extends HttpClientDecorator<HttpRequest,
 
   @Override
   protected URI url(final HttpRequest httpRequest) throws URISyntaxException {
-    return httpRequest.getUrl().toURI();
+    // Google uses %20 (space) instead of "+" for spaces in the fragment
+    // Add "+" back for consistency with the other http client instrumentations
+    final String url = httpRequest.getUrl().build();
+    final String fixedUrl = url.replaceAll("%20", "+");
+    return new URI(fixedUrl);
   }
 
   @Override
diff --git a/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientInstrumentation.java b/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientInstrumentation.java
index b0e692d6b4..dba8cae9da 100644
--- a/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientInstrumentation.java
+++ b/dd-java-agent/instrumentation/google-http-client/src/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientInstrumentation.java
@@ -19,12 +19,12 @@ import io.opentracing.propagation.Format;
 import io.opentracing.propagation.TextMap;
 import io.opentracing.tag.Tags;
 import io.opentracing.util.GlobalTracer;
+import java.util.Iterator;
+import java.util.Map;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
-import java.util.Iterator;
-import java.util.Map;
 
 @AutoService(Instrumenter.class)
 public class GoogleHttpClientInstrumentation extends Instrumenter.Default {
@@ -44,7 +44,7 @@ public class GoogleHttpClientInstrumentation extends Instrumenter.Default {
       "datadog.trace.agent.decorator.BaseDecorator",
       "datadog.trace.agent.decorator.ClientDecorator",
       "datadog.trace.agent.decorator.HttpClientDecorator",
-      packageName + ".GoogleHttpClientClientDecorator",
+      packageName + ".GoogleHttpClientDecorator",
       getClass().getName() + "$GoogleHttpClientAdvice",
       getClass().getName() + "$HeadersInjectAdapter"
     };
@@ -87,13 +87,14 @@ public class GoogleHttpClientInstrumentation extends Instrumenter.Default {
 
           // If HttpRequest.setThrowExceptionOnExecuteError is set to false, there are no exceptions
           // for a failed request.  Thus, check the response code
-          if (!response.isSuccessStatusCode()) {
+          if (response != null && !response.isSuccessStatusCode()) {
             Tags.ERROR.set(span, true);
             span.log(singletonMap(Fields.MESSAGE, response.getStatusMessage()));
           }
           DECORATE.onError(span, throwable);
 
           DECORATE.beforeFinish(span);
+          span.finish();
         } finally {
           scope.close();
           CallDepthThreadLocalMap.reset(HttpRequest.class);
diff --git a/dd-java-agent/instrumentation/google-http-client/src/test/groovy/GoogleHttpClientTest.groovy b/dd-java-agent/instrumentation/google-http-client/src/test/groovy/GoogleHttpClientTest.groovy
new file mode 100644
index 0000000000..944b72dfaf
--- /dev/null
+++ b/dd-java-agent/instrumentation/google-http-client/src/test/groovy/GoogleHttpClientTest.groovy
@@ -0,0 +1,67 @@
+import com.google.api.client.http.GenericUrl
+import com.google.api.client.http.HttpRequest
+import com.google.api.client.http.HttpResponse
+import com.google.api.client.http.javanet.NetHttpTransport
+import datadog.trace.agent.test.base.HttpClientTest
+import datadog.trace.api.DDSpanTypes
+import datadog.trace.instrumentation.googlehttpclient.GoogleHttpClientDecorator
+import spock.lang.Shared
+
+class GoogleHttpClientTest extends HttpClientTest<GoogleHttpClientDecorator> {
+
+  @Shared
+  def requestFactory = new NetHttpTransport().createRequestFactory();
+
+  @Override
+  int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
+    doRequest(method, uri, headers, callback, false);
+  }
+
+  int doRequest(String method, URI uri, Map<String, String> headers, Closure callback, boolean throwExceptionOnError) {
+    GenericUrl genericUrl = new GenericUrl(uri)
+
+    HttpRequest request = requestFactory.buildRequest(method, genericUrl, null)
+    request.getHeaders().putAll(headers)
+    request.setThrowExceptionOnExecuteError(throwExceptionOnError)
+
+    HttpResponse response = request.execute()
+    callback?.call()
+
+    return response.getStatusCode()
+  }
+
+  @Override
+  GoogleHttpClientDecorator decorator() {
+    return GoogleHttpClientDecorator.DECORATE
+  }
+
+  @Override
+  boolean testRedirects() {
+    // Circular redirects don't throw an exception with Google Http Client
+    return false
+  }
+
+  def "error traces when exception is not thrown"() {
+    given:
+    def uri = server.address.resolve("/error")
+
+    when:
+    def status = doRequest(method, uri)
+
+    then:
+    status == 500
+    assertTraces(2) {
+      server.distributedRequestTrace(it, 0, trace(1).last())
+      trace(1, size(1)) {
+        span(0) {
+          resourceName "$method $uri.path"
+          spanType DDSpanTypes.HTTP_CLIENT
+          errored true
+        }
+      }
+    }
+
+    where:
+    method = "GET"
+  }
+}