fix: inject context http headers early if expect header exists (#766)

PR-URL: #766
googleapis · Jun 2, 2018 · bc877a5 · bc877a5
1 parent 2978728
commit bc877a5
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 3 deletions.
diff --git a/src/plugins/plugin-http.ts b/src/plugins/plugin-http.ts
@@ -42,6 +42,16 @@ function getSpanName(options: ClientRequestArgs|url.URL) {
   return options.hostname || options.host || 'localhost';
 }
 
+/**
+ * Returns whether the Expect header is on the given options object.
+ * @param options Options for http.request.
+ */
+function hasExpectHeader(options: ClientRequestArgs|url.URL): boolean {
+  return !!(
+      (options as ClientRequestArgs).headers &&
+      (options as ClientRequestArgs).headers!.Expect);
+}
+
 function extractUrl(
     options: ClientRequestArgs|url.URL, fallbackProtocol: string) {
   let path;
@@ -111,6 +121,24 @@ function makeRequestTrace(
     span.addLabel(api.labels.HTTP_METHOD_LABEL_KEY, method);
     span.addLabel(api.labels.HTTP_URL_LABEL_KEY, uri);
 
+    // If outgoing request headers contain the "Expect" header, the returned
+    // ClientRequest will throw an error if any new headers are added. For this
+    // reason, only in this scenario, we opt to clone the options object to
+    // inject the trace context header instead of using ClientRequest#setHeader.
+    // (We don't do this generally because cloning the options object is an
+    // expensive operation.)
+    let traceHeaderPreinjected = false;
+    if (hasExpectHeader(options)) {
+      traceHeaderPreinjected = true;
+      // "Clone" the options object -- but don't deep-clone anything except for
+      // headers.
+      options = Object.assign({}, options) as ClientRequestArgs;
+      options.headers = Object.assign({}, options.headers);
+      // Inject the trace context header.
+      options.headers[api.constants.TRACE_CONTEXT_HEADER_NAME] =
+          span.getTraceContext();
+    }
+
     const req = request(options, (res) => {
       api.wrapEmitter(res);
       let numBytes = 0;
@@ -144,13 +172,17 @@ function makeRequestTrace(
       }
     });
     api.wrapEmitter(req);
-    req.setHeader(
-        api.constants.TRACE_CONTEXT_HEADER_NAME, span.getTraceContext());
     req.on('error', error => {
       span.addLabel(api.labels.ERROR_DETAILS_NAME, error.name);
       span.addLabel(api.labels.ERROR_DETAILS_MESSAGE, error.message);
       span.endSpan();
     });
+    // Inject the trace context header, but only if it wasn't already injected
+    // earlier.
+    if (!traceHeaderPreinjected) {
+      req.setHeader(
+          api.constants.TRACE_CONTEXT_HEADER_NAME, span.getTraceContext());
+    }
     return req;
   };
 }

diff --git a/test/plugins/test-trace-http.ts b/test/plugins/test-trace-http.ts
@@ -183,11 +183,24 @@ for (const nodule of Object.keys(servers) as Array<keyof typeof servers>) {
             const req = http.request(
                 {port, rejectUnauthorized: false},
                 waitForResponse.handleResponse);
-            await wait(DEFAULT_SPAN_DURATION / 2);
             req.end();
             await waitForResponse.done;
           }
         },
+        {
+          description: 'calling http.get with Expect header',
+          fn: async () => {
+            const waitForResponse = new WaitForResponse();
+            const req = http.get(
+                {
+                  port,
+                  rejectUnauthorized: false,
+                  headers: {Expect: '100-continue'}
+                },
+                waitForResponse.handleResponse);
+            await waitForResponse.done;
+          }
+        },
         {
           description: 'calling http.get, but timing out and emitting an error',
           fn: async () => {