feat(node): Move tracing options to Http integration (#6191)

timfish · web-flow · commit 23c19bfba045 · 2022-11-15T09:37:31.000+01:00
Since both `tracing` and `shouldCreateSpanForRequest` effectively control whether tracing is enabled and have opposing defaults (`tracing: false` and `shouldCreateSpanForRequest: _ =&gt; true`), this patch makes `tracing: TracingOptions | boolean` so its not a breaking change and the options don't end up conflicting. 

```ts
interface TracingOptions {
  tracePropagationTargets?: TracePropagationTargets;
  shouldCreateSpanForRequest?: (url: string) =&gt; boolean;
}

interface HttpOptions {
  breadcrumbs?: boolean;
  tracing?: TracingOptions | boolean;
}
```
diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts
@@ -129,7 +129,7 @@ function addServerIntegrations(options: NextjsOptions): void {
   if (hasTracingEnabled(options)) {
     const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true });
     integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, {
-      _tracing: true,
+      _tracing: {},
     });
   }
 
diff --git a/packages/nextjs/test/index.server.test.ts b/packages/nextjs/test/index.server.test.ts
@@ -169,7 +169,7 @@ describe('Server init()', () => {
         const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');
 
         expect(httpIntegration).toBeDefined();
-        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
+        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
       });
 
       it('adds `Http` integration with tracing enabled if `tracesSampler` is set', () => {
@@ -179,7 +179,7 @@ describe('Server init()', () => {
         const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');
 
         expect(httpIntegration).toBeDefined();
-        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
+        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
       });
 
       it('does not add `Http` integration if tracing not enabled in SDK', () => {
@@ -201,7 +201,7 @@ describe('Server init()', () => {
         const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');
 
         expect(httpIntegration).toBeDefined();
-        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
+        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
       });
 
       it('forces `_tracing = true` if `tracesSampler` is set', () => {
@@ -214,7 +214,7 @@ describe('Server init()', () => {
         const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');
 
         expect(httpIntegration).toBeDefined();
-        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: true }));
+        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: {} }));
       });
 
       it('does not force `_tracing = true` if tracing not enabled in SDK', () => {
@@ -226,7 +226,7 @@ describe('Server init()', () => {
         const httpIntegration = findIntegrationByName(nodeInitOptions.integrations, 'Http');
 
         expect(httpIntegration).toBeDefined();
-        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: false }));
+        expect(httpIntegration).toEqual(expect.objectContaining({ _tracing: undefined }));
       });
     });
   });
diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts
@@ -1,5 +1,5 @@
 import { getCurrentHub, Hub } from '@sentry/core';
-import { EventProcessor, Integration, Span } from '@sentry/types';
+import { EventProcessor, Integration, Span, TracePropagationTargets } from '@sentry/types';
 import {
   dynamicSamplingContextToSentryBaggageHeader,
   fill,
@@ -11,7 +11,6 @@ import * as http from 'http';
 import * as https from 'https';
 
 import { NodeClient } from '../client';
-import { NodeClientOptions } from '../types';
 import {
   cleanSpanDescription,
   extractUrl,
@@ -23,6 +22,39 @@ import {
 
 const NODE_VERSION = parseSemver(process.versions.node);
 
+interface TracingOptions {
+  /**
+   * List of strings/regex controlling to which outgoing requests
+   * the SDK will attach tracing headers.
+   *
+   * By default the SDK will attach those headers to all outgoing
+   * requests. If this option is provided, the SDK will match the
+   * request URL of outgoing requests against the items in this
+   * array, and only attach tracing headers if a match was found.
+   */
+  tracePropagationTargets?: TracePropagationTargets;
+
+  /**
+   * Function determining whether or not to create spans to track outgoing requests to the given URL.
+   * By default, spans will be created for all outgoing requests.
+   */
+  shouldCreateSpanForRequest?: (url: string) => boolean;
+}
+
+interface HttpOptions {
+  /**
+   * Whether breadcrumbs should be recorded for requests
+   * Defaults to true
+   */
+  breadcrumbs?: boolean;
+
+  /**
+   * Whether tracing spans should be created for requests
+   * Defaults to false
+   */
+  tracing?: TracingOptions | boolean;
+}
+
 /**
  * The http module integration instruments Node's internal http module. It creates breadcrumbs, transactions for outgoing
  * http requests and attaches trace data when tracing is enabled via its `tracing` option.
@@ -38,22 +70,15 @@ export class Http implements Integration {
    */
   public name: string = Http.id;
 
-  /**
-   * @inheritDoc
-   */
   private readonly _breadcrumbs: boolean;
+  private readonly _tracing: TracingOptions | undefined;
 
   /**
    * @inheritDoc
    */
-  private readonly _tracing: boolean;
-
-  /**
-   * @inheritDoc
-   */
-  public constructor(options: { breadcrumbs?: boolean; tracing?: boolean } = {}) {
+  public constructor(options: HttpOptions = {}) {
     this._breadcrumbs = typeof options.breadcrumbs === 'undefined' ? true : options.breadcrumbs;
-    this._tracing = typeof options.tracing === 'undefined' ? false : options.tracing;
+    this._tracing = !options.tracing ? undefined : options.tracing === true ? {} : options.tracing;
   }
 
   /**
@@ -76,7 +101,11 @@ export class Http implements Integration {
       return;
     }
 
-    const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, this._tracing, clientOptions);
+    // TODO (v8): `tracePropagationTargets` and `shouldCreateSpanForRequest` will be removed from clientOptions
+    // and we will no longer have to do this optional merge, we can just pass `this._tracing` directly.
+    const tracingOptions = this._tracing ? { ...clientOptions, ...this._tracing } : undefined;
+
+    const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions);
 
     // eslint-disable-next-line @typescript-eslint/no-var-requires
     const httpModule = require('http');
@@ -111,37 +140,36 @@ type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedR
  */
 function _createWrappedRequestMethodFactory(
   breadcrumbsEnabled: boolean,
-  tracingEnabled: boolean,
-  options: NodeClientOptions | undefined,
+  tracingOptions: TracingOptions | undefined,
 ): WrappedRequestMethodFactory {
   // We're caching results so we don't have to recompute regexp every time we create a request.
   const createSpanUrlMap: Record<string, boolean> = {};
   const headersUrlMap: Record<string, boolean> = {};
 
   const shouldCreateSpan = (url: string): boolean => {
-    if (options?.shouldCreateSpanForRequest === undefined) {
+    if (tracingOptions?.shouldCreateSpanForRequest === undefined) {
       return true;
     }
 
     if (createSpanUrlMap[url]) {
       return createSpanUrlMap[url];
     }
 
-    createSpanUrlMap[url] = options.shouldCreateSpanForRequest(url);
+    createSpanUrlMap[url] = tracingOptions.shouldCreateSpanForRequest(url);
 
     return createSpanUrlMap[url];
   };
 
   const shouldAttachTraceData = (url: string): boolean => {
-    if (options?.tracePropagationTargets === undefined) {
+    if (tracingOptions?.tracePropagationTargets === undefined) {
       return true;
     }
 
     if (headersUrlMap[url]) {
       return headersUrlMap[url];
     }
 
-    headersUrlMap[url] = options.tracePropagationTargets.some(tracePropagationTarget =>
+    headersUrlMap[url] = tracingOptions.tracePropagationTargets.some(tracePropagationTarget =>
       isMatchingPattern(url, tracePropagationTarget),
     );
 
@@ -167,7 +195,7 @@ function _createWrappedRequestMethodFactory(
 
       const scope = getCurrentHub().getScope();
 
-      if (scope && tracingEnabled && shouldCreateSpan(requestUrl)) {
+      if (scope && tracingOptions && shouldCreateSpan(requestUrl)) {
         parentSpan = scope.getSpan();
 
         if (parentSpan) {
@@ -235,7 +263,7 @@ function _createWrappedRequestMethodFactory(
           if (breadcrumbsEnabled) {
             addRequestBreadcrumb('response', requestUrl, req, res);
           }
-          if (tracingEnabled && requestSpan) {
+          if (requestSpan) {
             if (res.statusCode) {
               requestSpan.setHttpStatus(res.statusCode);
             }
@@ -250,7 +278,7 @@ function _createWrappedRequestMethodFactory(
           if (breadcrumbsEnabled) {
             addRequestBreadcrumb('error', requestUrl, req);
           }
-          if (tracingEnabled && requestSpan) {
+          if (requestSpan) {
             requestSpan.setHttpStatus(500);
             requestSpan.description = cleanSpanDescription(requestSpan.description, requestOptions, req);
             requestSpan.finish();
diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts
@@ -6,22 +6,39 @@ export interface BaseNodeOptions {
   /** Sets an optional server name (device name) */
   serverName?: string;
 
-  // We have this option separately in both, the node options and the browser options, so that we can have different JSDoc
-  // comments, since this has different behaviour in the Browser and Node SDKs.
+  // TODO (v8): Remove this in v8
   /**
-   * List of strings/regex controlling to which outgoing requests
-   * the SDK will attach tracing headers.
-   *
-   * By default the SDK will attach those headers to all outgoing
-   * requests. If this option is provided, the SDK will match the
-   * request URL of outgoing requests against the items in this
-   * array, and only attach tracing headers if a match was found.
+   * @deprecated Moved to constructor options of the `Http` integration.
+   * @example
+   * ```js
+   * Sentry.init({
+   *   integrations: [
+   *     new Sentry.Integrations.Http({
+   *       tracing: {
+   *         tracePropagationTargets: ['api.site.com'],
+   *       }
+   *     });
+   *   ],
+   * });
+   * ```
    */
   tracePropagationTargets?: TracePropagationTargets;
 
+  // TODO (v8): Remove this in v8
   /**
-   * Function determining whether or not to create spans to track outgoing requests to the given URL.
-   * By default, spans will be created for all outgoing requests.
+   * @deprecated Moved to constructor options of the `Http` integration.
+   * @example
+   * ```js
+   * Sentry.init({
+   *   integrations: [
+   *     new Sentry.Integrations.Http({
+   *       tracing: {
+   *         shouldCreateSpanForRequest: (url: string) => false,
+   *       }
+   *     });
+   *   ],
+   * });
+   * ```
    */
   shouldCreateSpanForRequest?(url: string): boolean;
 
diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts

Original file line number	Diff line number	Diff line change
`@@ -129,7 +129,7 @@ function addServerIntegrations(options: NextjsOptions): void {`
`129`	`129`	`if (hasTracingEnabled(options)) {`
`130`	`130`	`const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true });`
`131`	`131`	`integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, {`
`132`		`- _tracing: true,`
	`132`	`+ _tracing: {},`
`133`	`133`	`});`
`134`	`134`	`}`
`135`	`135`