fix(tracing): Add sentry-trace header to outgoing http(s) requests in node (#3053)

This brings `@sentry/node` in line with `@sentry/browser`, in that any outgoing request recorded as a span will now correctly include the `sentry-trace` header.

Main Changes:

- Split `integrations/http` utils off into their own file (things were getting unwieldy). `urlToOptions` and `normalizeRequestArgs` are the only actual additions there.

- Handle all allowable inputs to `http(s).request()` and `http(s).get` (we were missing the option of passing a `URL` object) by normalizing those inputs to one of two options, down from ten.

- Fix the types in various places where we had them backwards (requests typed as responses and vice-versa).

- Move our node types to version 10 as a) until two days ago, that version included `method` in the `http.ClientRequest` type whereas version 11 didn't for some reason, and b) even now that that's moot, if we supposedly support node versions back to 6, we shouldn't be using types which didn't exist back then. `v10` is the earliest node version supported by `@types/node`, and is still being actively maintained, so it seemed like a good compromise.

- (The whole point) Add `sentry-trace` header to outgoing requests.

Author: lobsterkatie

package.json

@@ -45,7 +45,7 @@
     "@types/chai": "^4.1.3",
     "@types/jest": "^24.0.11",
     "@types/mocha": "^5.2.0",
-    "@types/node": "^11.13.7",
+    "@types/node": "~10.17.0",
     "@types/sinon": "^7.0.11",
     "chai": "^4.1.2",
     "codecov": "^3.6.5",

packages/node/package.json

@@ -31,10 +31,11 @@
     "@types/cookie": "0.3.2",
     "@types/express": "^4.17.2",
     "@types/lru-cache": "^5.1.0",
-    "@types/node": "^11.13.7",
+    "@types/node": "~10.17.0",
     "eslint": "7.6.0",
     "express": "^4.17.1",
     "jest": "^24.7.1",
+    "nock": "^13.0.5",
     "npm-run-all": "^4.1.2",
     "prettier": "1.19.0",
     "rimraf": "^2.6.3",

packages/node/src/integrations/http.ts

@@ -1,9 +1,18 @@
 import { getCurrentHub } from '@sentry/core';
 import { Integration, Span, Transaction } from '@sentry/types';
-import { fill, parseSemver } from '@sentry/utils';
+import { fill, logger, parseSemver } from '@sentry/utils';
 import * as http from 'http';
 import * as https from 'https';
 
+import {
+  cleanSpanDescription,
+  extractUrl,
+  isSentryRequest,
+  normalizeRequestArgs,
+  RequestMethod,
+  RequestMethodArgs,
+} from './utils/http';
+
 const NODE_VERSION = parseSemver(process.versions.node);
 
 /** http module integration */
@@ -45,7 +54,7 @@ export class Http implements Integration {
       return;
     }
 
-    const wrappedHandlerMaker = _createWrappedHandlerMaker(this._breadcrumbs, this._tracing);
+    const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, this._tracing);
 
     const httpModule = require('http');
     fill(httpModule, 'get', wrappedHandlerMaker);
@@ -62,9 +71,10 @@ export class Http implements Integration {
   }
 }
 
-type OriginalHandler = () => http.ClientRequest;
-type WrappedHandler = (options: string | http.ClientRequestArgs) => http.ClientRequest;
-type WrappedHandlerMaker = (originalHandler: OriginalHandler) => WrappedHandler;
+// for ease of reading below
+type OriginalRequestMethod = RequestMethod;
+type WrappedRequestMethod = RequestMethod;
+type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedRequestMethod;
 
 /**
  * Function which creates a function which creates wrapped versions of internal `request` and `get` calls within `http`
@@ -75,17 +85,22 @@ type WrappedHandlerMaker = (originalHandler: OriginalHandler) => WrappedHandler;
  *
  * @returns A function which accepts the exiting handler and returns a wrapped handler
  */
-function _createWrappedHandlerMaker(breadcrumbsEnabled: boolean, tracingEnabled: boolean): WrappedHandlerMaker {
-  return function wrappedHandlerMaker(originalHandler: OriginalHandler): WrappedHandler {
-    return function wrappedHandler(
-      this: typeof http | typeof https,
-      options: string | http.ClientRequestArgs,
-    ): http.ClientRequest {
-      const requestUrl = extractUrl(options);
-
-      // we don't want to record requests to Sentry as either breadcrumbs or spans, so just use the original handler
+function _createWrappedRequestMethodFactory(
+  breadcrumbsEnabled: boolean,
+  tracingEnabled: boolean,
+): WrappedRequestMethodFactory {
+  return function wrappedRequestMethodFactory(originalRequestMethod: OriginalRequestMethod): WrappedRequestMethod {
+    return function wrappedMethod(this: typeof http | typeof https, ...args: RequestMethodArgs): http.ClientRequest {
+      // eslint-disable-next-line @typescript-eslint/no-this-alias
+      const httpModule = this;
+
+      const requestArgs = normalizeRequestArgs(args);
+      const requestOptions = requestArgs[0];
+      const requestUrl = extractUrl(requestOptions);
+
+      // we don't want to record requests to Sentry as either breadcrumbs or spans, so just use the original method
       if (isSentryRequest(requestUrl)) {
-        return originalHandler.apply(this, arguments);
+        return originalRequestMethod.apply(httpModule, requestArgs);
       }
 
       let span: Span | undefined;
@@ -96,32 +111,43 @@ function _createWrappedHandlerMaker(breadcrumbsEnabled: boolean, tracingEnabled:
         transaction = scope.getTransaction();
         if (transaction) {
           span = transaction.startChild({
-            description: `${typeof options === 'string' || !options.method ? 'GET' : options.method} ${requestUrl}`,
+            description: `${requestOptions.method || 'GET'} ${requestUrl}`,
             op: 'request',
           });
+
+          const sentryTraceHeader = span.toTraceparent();
+          logger.log(`[Tracing] Adding sentry-trace header to outgoing request: ${sentryTraceHeader}`);
+          requestOptions.headers = { ...requestOptions.headers, 'sentry-trace': sentryTraceHeader };
         }
       }
 
       // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-      return originalHandler
-        .apply(this, arguments)
-        .once('response', function(this: http.IncomingMessage, res: http.ServerResponse): void {
+      return originalRequestMethod
+        .apply(httpModule, requestArgs)
+        .once('response', function(this: http.ClientRequest, res: http.IncomingMessage): void {
+          // eslint-disable-next-line @typescript-eslint/no-this-alias
+          const req = this;
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('response', requestUrl, this, res);
+            addRequestBreadcrumb('response', requestUrl, req, res);
           }
           if (tracingEnabled && span) {
-            span.setHttpStatus(res.statusCode);
-            cleanDescription(options, this, span);
+            if (res.statusCode) {
+              span.setHttpStatus(res.statusCode);
+            }
+            span.description = cleanSpanDescription(span.description, requestOptions, req);
             span.finish();
           }
         })
-        .once('error', function(this: http.IncomingMessage): void {
+        .once('error', function(this: http.ClientRequest): void {
+          // eslint-disable-next-line @typescript-eslint/no-this-alias
+          const req = this;
+
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('error', requestUrl, this);
+            addRequestBreadcrumb('error', requestUrl, req);
           }
           if (tracingEnabled && span) {
             span.setHttpStatus(500);
-            cleanDescription(options, this, span);
+            span.description = cleanSpanDescription(span.description, requestOptions, req);
             span.finish();
           }
         });
@@ -132,7 +158,7 @@ function _createWrappedHandlerMaker(breadcrumbsEnabled: boolean, tracingEnabled:
 /**
  * Captures Breadcrumb based on provided request/response pair
  */
-function addRequestBreadcrumb(event: string, url: string, req: http.IncomingMessage, res?: http.ServerResponse): void {
+function addRequestBreadcrumb(event: string, url: string, req: http.ClientRequest, res?: http.IncomingMessage): void {
   if (!getCurrentHub().getIntegration(Http)) {
     return;
   }
@@ -154,72 +180,3 @@ function addRequestBreadcrumb(event: string, url: string, req: http.IncomingMess
     },
   );
 }
-
-/**
- * Assemble a URL to be used for breadcrumbs and spans.
- *
- * @param url URL string or object containing the component parts
- * @returns Fully-formed URL
- */
-export function extractUrl(url: string | http.ClientRequestArgs): string {
-  if (typeof url === 'string') {
-    return url;
-  }
-  const protocol = url.protocol || '';
-  const hostname = url.hostname || url.host || '';
-  // Don't log standard :80 (http) and :443 (https) ports to reduce the noise
-  const port = !url.port || url.port === 80 || url.port === 443 ? '' : `:${url.port}`;
-  const path = url.path ? url.path : '/';
-
-  // internal routes end up with too many slashes
-  return `${protocol}//${hostname}${port}${path}`.replace('///', '/');
-}
-
-/**
- * Handle an edge case with urls in the span description. Runs just before the span closes because it relies on
- * data from the response object.
- *
- * @param requestOptions Configuration data for the request
- * @param response Response object
- * @param span Span representing the request
- */
-function cleanDescription(
-  requestOptions: string | http.ClientRequestArgs,
-  response: http.IncomingMessage,
-  span: Span,
-): void {
-  // There are some libraries which don't pass the request protocol in the options object, so attempt to retrieve it
-  // from the response and run the URL processing again. We only do this in the presence of a (non-empty) host value,
-  // because if we're missing both, it's likely we're dealing with an internal route, in which case we don't want to be
-  // jamming a random `http:` on the front of it.
-  if (typeof requestOptions !== 'string' && !Object.keys(requestOptions).includes('protocol') && requestOptions.host) {
-    // Neither http.IncomingMessage nor any of its ancestors have an `agent` property in their type definitions, and
-    // http.Agent doesn't have a `protocol` property in its type definition. Nonetheless, at least one request library
-    // (superagent) arranges things that way, so might as well give it a shot.
-    try {
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
-      requestOptions.protocol = (response as any).agent.protocol;
-      span.description = `${requestOptions.method || 'GET'} ${extractUrl(requestOptions)}`;
-    } catch (error) {
-      // well, we tried
-    }
-  }
-}
-
-/**
- * Checks whether given url points to Sentry server
- * @param url url to verify
- */
-function isSentryRequest(url: string): boolean {
-  const client = getCurrentHub().getClient();
-  if (!url || !client) {
-    return false;
-  }
-
-  const dsn = client.getDsn();
-  if (!dsn) {
-    return false;
-  }
-
-  return url.indexOf(dsn.host) !== -1;
-}

packages/node/src/integrations/utils/http.ts

@@ -0,0 +1,154 @@
+import { getCurrentHub } from '@sentry/core';
+import * as http from 'http';
+import { URL } from 'url';
+
+/**
+ * Checks whether given url points to Sentry server
+ * @param url url to verify
+ */
+export function isSentryRequest(url: string): boolean {
+  const dsn = getCurrentHub()
+    .getClient()
+    ?.getDsn();
+  return dsn ? url.includes(dsn.host) : false;
+}
+
+/**
+ * Assemble a URL to be used for breadcrumbs and spans.
+ *
+ * @param requestOptions RequestOptions object containing the component parts for a URL
+ * @returns Fully-formed URL
+ */
+export function extractUrl(requestOptions: RequestOptions): string {
+  const protocol = requestOptions.protocol || '';
+  const hostname = requestOptions.hostname || requestOptions.host || '';
+  // Don't log standard :80 (http) and :443 (https) ports to reduce the noise
+  const port =
+    !requestOptions.port || requestOptions.port === 80 || requestOptions.port === 443 ? '' : `:${requestOptions.port}`;
+  const path = requestOptions.path ? requestOptions.path : '/';
+
+  return `${protocol}//${hostname}${port}${path}`;
+}
+
+/**
+ * Handle various edge cases in the span description (for spans representing http(s) requests).
+ *
+ * @param description current `description` property of the span representing the request
+ * @param requestOptions Configuration data for the request
+ * @param Request Request object
+ *
+ * @returns The cleaned description
+ */
+export function cleanSpanDescription(
+  description: string | undefined,
+  requestOptions: RequestOptions,
+  request: http.ClientRequest,
+): string | undefined {
+  // nothing to clean
+  if (!description) {
+    return description;
+  }
+
+  // eslint-disable-next-line prefer-const
+  let [method, requestUrl] = description.split(' ');
+
+  // superagent sticks the protocol in a weird place (we check for host because if both host *and* protocol are missing,
+  // we're likely dealing with an internal route and this doesn't apply)
+  if (requestOptions.host && !requestOptions.protocol) {
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
+    requestOptions.protocol = (request as any)?.agent?.protocol; // worst comes to worst, this is undefined and nothing changes
+    requestUrl = extractUrl(requestOptions);
+  }
+
+  // internal routes can end up starting with a triple slash rather than a single one
+  if (requestUrl?.startsWith('///')) {
+    requestUrl = requestUrl.slice(2);
+  }
+
+  return `${method} ${requestUrl}`;
+}
+
+// the node types are missing a few properties which node's `urlToOptions` function spits out
+export type RequestOptions = http.RequestOptions & { hash?: string; search?: string; pathname?: string; href?: string };
+type RequestCallback = (response: http.IncomingMessage) => void;
+export type RequestMethodArgs =
+  | [RequestOptions | string | URL, RequestCallback?]
+  | [string | URL, RequestOptions, RequestCallback?];
+export type RequestMethod = (...args: RequestMethodArgs) => http.ClientRequest;
+
+/**
+ * Convert a URL object into a RequestOptions object.
+ *
+ * Copied from Node's internals (where it's used in http(s).request() and http(s).get()), modified only to use the
+ * RequestOptions type above.
+ *
+ * See https://github.com/nodejs/node/blob/master/lib/internal/url.js.
+ */
+export function urlToOptions(url: URL): RequestOptions {
+  const options: RequestOptions = {
+    protocol: url.protocol,
+    hostname:
+      typeof url.hostname === 'string' && url.hostname.startsWith('[') ? url.hostname.slice(1, -1) : url.hostname,
+    hash: url.hash,
+    search: url.search,
+    pathname: url.pathname,
+    path: `${url.pathname || ''}${url.search || ''}`,
+    href: url.href,
+  };
+  if (url.port !== '') {
+    options.port = Number(url.port);
+  }
+  if (url.username || url.password) {
+    options.auth = `${url.username}:${url.password}`;
+  }
+  return options;
+}
+
+/**
+ * Normalize inputs to `http(s).request()` and `http(s).get()`.
+ *
+ * Legal inputs to `http(s).request()` and `http(s).get()` can take one of ten forms:
+ *     [ RequestOptions | string | URL ],
+ *     [ RequestOptions | string | URL, RequestCallback ],
+ *     [ string | URL, RequestOptions ], and
+ *     [ string | URL, RequestOptions, RequestCallback ].
+ *
+ * This standardizes to one of two forms: [ RequestOptions ] and [ RequestOptions, RequestCallback ]. A similar thing is
+ * done as the first step of `http(s).request()` and `http(s).get()`; this just does it early so that we can interact
+ * with the args in a standard way.
+ *
+ * @param requestArgs The inputs to `http(s).request()` or `http(s).get()`, as an array.
+ *
+ * @returns Equivalent args of the form [ RequestOptions ] or [ RequestOptions, RequestCallback ].
+ */
+export function normalizeRequestArgs(
+  requestArgs: RequestMethodArgs,
+): [RequestOptions] | [RequestOptions, RequestCallback] {
+  let callback, requestOptions;
+
+  // pop off the callback, if there is one
+  if (typeof requestArgs[requestArgs.length - 1] === 'function') {
+    callback = requestArgs.pop() as RequestCallback;
+  }
+
+  // create a RequestOptions object of whatever's at index 0
+  if (typeof requestArgs[0] === 'string') {
+    requestOptions = urlToOptions(new URL(requestArgs[0]));
+  } else if (requestArgs[0] instanceof URL) {
+    requestOptions = urlToOptions(requestArgs[0]);
+  } else {
+    requestOptions = requestArgs[0];
+  }
+
+  // if the options were given separately from the URL, fold them in
+  if (requestArgs.length === 2) {
+    requestOptions = { ...requestOptions, ...requestArgs[1] };
+  }
+
+  // return args in standardized form
+  if (callback) {
+    return [requestOptions, callback];
+  } else {
+    return [requestOptions];
+  }
+}