fix(tracing): Use Request.headers.append correctly (#3311)

Author: lobsterkatie

packages/tracing/src/browser/request.ts

@@ -58,6 +58,16 @@ export interface FetchData {
   endTimestamp?: number;
 }
 
+type PolymorphicRequestHeaders =
+  | Record<string, string>
+  | Array<[string, string]>
+  // the below is not preicsely the Header type used in Request, but it'll pass duck-typing
+  | {
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      [key: string]: any;
+      append: (key: string, value: string) => void;
+    };
+
 /** Data returned from XHR request */
 export interface XHRData {
   xhr?: {
@@ -187,22 +197,26 @@ export function fetchCallback(
     const request = (handlerData.args[0] = handlerData.args[0] as string | Request);
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const options = (handlerData.args[1] = (handlerData.args[1] as { [key: string]: any }) || {});
-    let headers = options.headers;
+    let headers: PolymorphicRequestHeaders = options.headers;
     if (isInstanceOf(request, Request)) {
       headers = (request as Request).headers;
     }
+    const traceHeaders = span.getTraceHeaders() as Record<string, string>;
     if (headers) {
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-      if (typeof headers.append === 'function') {
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-        headers.append(Object.entries(span.getTraceHeaders()));
+      if ('append' in headers && typeof headers.append === 'function') {
+        headers.append('sentry-trace', traceHeaders['sentry-trace']);
+        if (traceHeaders.tracestate) {
+          headers.append('tracestate', traceHeaders.tracestate);
+        }
       } else if (Array.isArray(headers)) {
-        headers = [...headers, ...Object.entries(span.getTraceHeaders())];
+        // TODO use the nicer version below once we stop supporting Node 6
+        // headers = [...headers, ...Object.entries(traceHeaders)];
+        headers = [...headers, ['sentry-trace', traceHeaders['sentry-trace']], ['tracestate', traceHeaders.tracestate]];
       } else {
-        headers = { ...headers, ...span.getTraceHeaders() };
+        headers = { ...headers, ...traceHeaders };
       }
     } else {
-      headers = span.getTraceHeaders();
+      headers = traceHeaders;
     }
     options.headers = headers;
   }

packages/tracing/test/browser/request.test.ts

@@ -6,6 +6,7 @@ import { Span, SpanStatus, Transaction } from '../../src';
 import { fetchCallback, FetchData, instrumentOutgoingRequests, xhrCallback, XHRData } from '../../src/browser/request';
 import { addExtensionMethods } from '../../src/hubextensions';
 import * as tracingUtils from '../../src/utils';
+import { objectFromEntries } from '../testutils';
 
 // This is a normal base64 regex, modified to reflect that fact that we strip the trailing = or == off
 const stripped_base64 = '([a-zA-Z0-9+/]{4})*([a-zA-Z0-9+/]{2,3})?';
@@ -17,9 +18,29 @@ const TRACESTATE_HEADER_REGEX = new RegExp(
 
 beforeAll(() => {
   addExtensionMethods();
-  // @ts-ignore need to override global Request because it's not in the jest environment (even with an
-  // `@jest-environment jsdom` directive, for some reason)
-  global.Request = {};
+
+  // Add Request to the global scope (necessary because for some reason Request isn't in the jest environment, even with
+  // an `@jest-environment jsdom` directive)
+
+  type MockHeaders = {
+    [key: string]: any;
+    append: (key: string, value: string) => void;
+  };
+
+  class Request {
+    public headers: MockHeaders;
+    constructor() {
+      // We need our headers to act like an object for key-lookup purposes, but also have an append method that adds
+      // items as its siblings. This hack precludes a key named `append`, of course, but for our purposes it's enough.
+      const headers = {} as MockHeaders;
+      headers.append = (key: string, value: any): void => {
+        headers[key] = value;
+      };
+      this.headers = headers;
+    }
+  }
+
+  (global as any).Request = Request;
 });
 
 const hasTracingEnabled = jest.spyOn(tracingUtils, 'hasTracingEnabled');
@@ -57,7 +78,7 @@ describe('instrumentOutgoingRequests', () => {
   });
 });
 
-describe('callbacks', () => {
+describe('fetch and xhr callbacks', () => {
   let hub: Hub;
   let transaction: Transaction;
   const alwaysCreateSpan = () => true;
@@ -193,15 +214,67 @@ describe('callbacks', () => {
       expect(newSpan!.status).toBe(SpanStatus.fromHttpCode(404));
     });
 
-    it('adds tracing headers to fetch requests', () => {
-      // make a local copy so the global one doesn't get mutated
-      const handlerData = { ...fetchHandlerData };
+    describe('adding tracing headers to fetch requests', () => {
+      it('can handle headers added with an `append` method', () => {
+        const handlerData: FetchData = { ...fetchHandlerData, args: [new Request('http://dogs.are.great'), {}] };
 
-      fetchCallback(handlerData, alwaysCreateSpan, {});
+        fetchCallback(handlerData, alwaysCreateSpan, {});
 
-      const headers = (handlerData.args[1].headers as Record<string, string>) || {};
-      expect(headers['sentry-trace']).toBeDefined();
-      expect(headers['tracestate']).toBeDefined();
+        const headers = handlerData.args[1].headers;
+        expect(headers['sentry-trace']).toBeDefined();
+        expect(headers['tracestate']).toBeDefined();
+      });
+
+      it('can handle existing headers in array form', () => {
+        const handlerData = {
+          ...fetchHandlerData,
+          args: [
+            'http://dogs.are.great/',
+            {
+              headers: [
+                ['GREETING_PROTOCOL', 'mutual butt sniffing'],
+                ['TAIL_ACTION', 'wagging'],
+              ],
+            },
+          ],
+        };
+
+        fetchCallback(handlerData, alwaysCreateSpan, {});
+
+        const headers = objectFromEntries((handlerData.args[1] as any).headers);
+        expect(headers['sentry-trace']).toBeDefined();
+        expect(headers['tracestate']).toBeDefined();
+      });
+
+      it('can handle existing headers in object form', () => {
+        const handlerData = {
+          ...fetchHandlerData,
+          args: [
+            'http://dogs.are.great/',
+            {
+              headers: { GREETING_PROTOCOL: 'mutual butt sniffing', TAIL_ACTION: 'wagging' },
+            },
+          ],
+        };
+
+        fetchCallback(handlerData, alwaysCreateSpan, {});
+
+        const headers = (handlerData.args[1] as any).headers;
+        expect(headers['sentry-trace']).toBeDefined();
+        expect(headers['tracestate']).toBeDefined();
+      });
+
+      it('can handle there being no existing headers', () => {
+        // override the value of `args`, even though we're overriding it with the same data, as a means of deep copying
+        // the one part which gets mutated
+        const handlerData = { ...fetchHandlerData, args: ['http://dogs.are.great/', {}] };
+
+        fetchCallback(handlerData, alwaysCreateSpan, {});
+
+        const headers = (handlerData.args[1] as any).headers;
+        expect(headers['sentry-trace']).toBeDefined();
+        expect(headers['tracestate']).toBeDefined();
+      });
     });
   });
 

packages/tracing/test/testutils.ts

@@ -56,3 +56,16 @@ export const testOnlyIfNodeVersionAtLeast = (minVersion: number): jest.It => {
 
   return it;
 };
+
+/** Polyfill for `Object.fromEntries`, for Node < 12 */
+export const objectFromEntries =
+  'fromEntries' in Object
+    ? (Object as { fromEntries: any }).fromEntries
+    : (entries: Array<[string, any]>): Record<string, any> => {
+        const result: Record<string, any> = {};
+        entries.forEach(entry => {
+          const [key, value] = entry;
+          result[key] = value;
+        });
+        return result;
+      };