fix(tracing): Clean up sampling decision inheritance (#2921)

Fixes a number of problems with propagation of sampling decisions from parent to child when the parent is in one service and the child is in another. See PR description for full details.

Author: lobsterkatie

packages/tracing/src/browser/browsertracing.ts

@@ -61,10 +61,14 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
   markBackgroundTransactions: boolean;
 
   /**
-   * beforeNavigate is called before a pageload/navigation transaction is created and allows for users
-   * to set custom transaction context. Default behavior is to return `window.location.pathname`.
+   * beforeNavigate is called before a pageload/navigation transaction is created and allows users to modify transaction
+   * context data, or drop the transaction entirely (by setting `sampled = false` in the context).
    *
-   * If undefined is returned, a pageload/navigation transaction will not be created.
+   * Note: For legacy reasons, transactions can also be dropped by returning `undefined`.
+   *
+   * @param context: The context data which will be passed to `startTransaction` by default
+   *
+   * @returns A (potentially) modified context object, with `sampled = false` if the transaction should be dropped.
    */
   beforeNavigate?(context: TransactionContext): TransactionContext | undefined;
 
@@ -187,22 +191,26 @@ export class BrowserTracing implements Integration {
     // eslint-disable-next-line @typescript-eslint/unbound-method
     const { beforeNavigate, idleTimeout, maxTransactionDuration } = this.options;
 
-    // if beforeNavigate returns undefined, we should not start a transaction.
+    const parentContextFromHeader = context.op === 'pageload' ? getHeaderContext() : undefined;
+
     const expandedContext = {
       ...context,
-      ...getHeaderContext(),
+      ...parentContextFromHeader,
       trimEnd: true,
     };
     const modifiedContext = typeof beforeNavigate === 'function' ? beforeNavigate(expandedContext) : expandedContext;
 
-    if (modifiedContext === undefined) {
-      logger.log(`[Tracing] Did not create ${context.op} idleTransaction due to beforeNavigate`);
-      return undefined;
+    // For backwards compatibility reasons, beforeNavigate can return undefined to "drop" the transaction (prevent it
+    // from being sent to Sentry).
+    const finalContext = modifiedContext === undefined ? { ...expandedContext, sampled: false } : modifiedContext;
+
+    if (finalContext.sampled === false) {
+      logger.log(`[Tracing] Will not send ${finalContext.op} transaction because of beforeNavigate.`);
     }
 
     const hub = this._getCurrentHub();
-    const idleTransaction = startIdleTransaction(hub, modifiedContext, idleTimeout, true);
-    logger.log(`[Tracing] Starting ${modifiedContext.op} transaction on scope`);
+    const idleTransaction = startIdleTransaction(hub, finalContext, idleTimeout, true);
+    logger.log(`[Tracing] Starting ${finalContext.op} transaction on scope`);
     idleTransaction.registerBeforeFinishCallback((transaction, endTimestamp) => {
       this._metrics.addPerformanceEntries(transaction);
       adjustTransactionDuration(secToMs(maxTransactionDuration), transaction, endTimestamp);

packages/tracing/src/browser/request.ts

@@ -1,7 +1,8 @@
+import { getCurrentHub } from '@sentry/hub';
 import { addInstrumentationHandler, isInstanceOf, isMatchingPattern } from '@sentry/utils';
 
 import { Span } from '../span';
-import { getActiveTransaction } from '../utils';
+import { getActiveTransaction, hasTracingEnabled } from '../utils';
 
 export const DEFAULT_TRACING_ORIGINS = ['localhost', /^\//];
 
@@ -142,7 +143,13 @@ export function fetchCallback(
   shouldCreateSpan: (url: string) => boolean,
   spans: Record<string, Span>,
 ): void {
-  if (!handlerData.fetchData || !shouldCreateSpan(handlerData.fetchData.url)) {
+  const currentClientOptions = getCurrentHub()
+    .getClient()
+    ?.getOptions();
+  if (
+    !(currentClientOptions && hasTracingEnabled(currentClientOptions)) ||
+    !(handlerData.fetchData && shouldCreateSpan(handlerData.fetchData.url))
+  ) {
     return;
   }
 
@@ -209,19 +216,18 @@ export function xhrCallback(
   shouldCreateSpan: (url: string) => boolean,
   spans: Record<string, Span>,
 ): void {
-  if (!handlerData || !handlerData.xhr || !handlerData.xhr.__sentry_xhr__) {
+  const currentClientOptions = getCurrentHub()
+    .getClient()
+    ?.getOptions();
+  if (
+    !(currentClientOptions && hasTracingEnabled(currentClientOptions)) ||
+    !(handlerData.xhr && handlerData.xhr.__sentry_xhr__ && shouldCreateSpan(handlerData.xhr.__sentry_xhr__.url)) ||
+    handlerData.xhr.__sentry_own_request__
+  ) {
     return;
   }
 
   const xhr = handlerData.xhr.__sentry_xhr__;
-  if (!shouldCreateSpan(xhr.url)) {
-    return;
-  }
-
-  // We only capture complete, non-sentry requests
-  if (handlerData.xhr.__sentry_own_request__) {
-    return;
-  }
 
   // check first if the request has finished and is tracked by an existing span which should now end
   if (handlerData.endTimestamp && handlerData.xhr.__sentry_xhr_span_id__) {

packages/tracing/src/hubextensions.ts

@@ -62,6 +62,11 @@ function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext
     return transaction;
   }
 
+  // if the user has forced a sampling decision by passing a `sampled` value in their transaction context, go with that
+  if (transaction.sampled !== undefined) {
+    return transaction;
+  }
+
   // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of these should
   // work; prefer the hook if so
   const sampleRate =

packages/tracing/test/browser/browsertracing.test.ts

@@ -7,6 +7,7 @@ import {
   BrowserTracing,
   BrowserTracingOptions,
   DEFAULT_MAX_TRANSACTION_DURATION_SECONDS,
+  getHeaderContext,
   getMetaContent,
 } from '../../src/browser/browsertracing';
 import { defaultRequestInstrumentionOptions } from '../../src/browser/request';
@@ -177,14 +178,15 @@ describe('BrowserTracing', () => {
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
 
-      it('does not create a transaction if it returns undefined', () => {
+      // TODO add this back in once getTransaction() returns sampled = false transactions, too
+      it.skip('creates a transaction with sampled = false if it returns undefined', () => {
         const mockBeforeNavigation = jest.fn().mockReturnValue(undefined);
         createBrowserTracing(true, {
           beforeNavigate: mockBeforeNavigation,
           routingInstrumentation: customRoutingInstrumentation,
         });
         const transaction = getActiveTransaction(hub) as IdleTransaction;
-        expect(transaction).not.toBeDefined();
+        expect(transaction.sampled).toBe(false);
 
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
@@ -379,5 +381,67 @@ describe('BrowserTracing', () => {
         expect(metaTagValue).toBe(content);
       });
     });
+
+    describe('getHeaderContext', () => {
+      it('correctly parses a valid sentry-trace meta header', () => {
+        document.head.innerHTML = `<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-0">`;
+
+        const headerContext = getHeaderContext();
+
+        expect(headerContext).toBeDefined();
+        expect(headerContext!.traceId).toEqual('12312012123120121231201212312012');
+        expect(headerContext!.parentSpanId).toEqual('1121201211212012');
+        expect(headerContext!.parentSampled).toEqual(false);
+      });
+
+      it('returns undefined if the header is malformed', () => {
+        document.head.innerHTML = `<meta name="sentry-trace" content="12312012-112120121-0">`;
+
+        const headerContext = getHeaderContext();
+
+        expect(headerContext).toBeUndefined();
+      });
+
+      it("returns undefined if the header isn't there", () => {
+        document.head.innerHTML = `<meta name="dogs" content="12312012123120121231201212312012-1121201211212012-0">`;
+
+        const headerContext = getHeaderContext();
+
+        expect(headerContext).toBeUndefined();
+      });
+    });
+
+    describe('using the data', () => {
+      // TODO add this back in once getTransaction() returns sampled = false transactions, too
+      it.skip('uses the data for pageload transactions', () => {
+        // make sampled false here, so we can see that it's being used rather than the tracesSampleRate-dictated one
+        document.head.innerHTML = `<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-0">`;
+
+        // pageload transactions are created as part of the BrowserTracing integration's initialization
+        createBrowserTracing(true);
+        const transaction = getActiveTransaction(hub) as IdleTransaction;
+
+        expect(transaction).toBeDefined();
+        expect(transaction.op).toBe('pageload');
+        expect(transaction.traceId).toEqual('12312012123120121231201212312012');
+        expect(transaction.parentSpanId).toEqual('1121201211212012');
+        expect(transaction.sampled).toBe(false);
+      });
+
+      it('ignores the data for navigation transactions', () => {
+        mockChangeHistory = () => undefined;
+        document.head.innerHTML = `<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-0">`;
+
+        createBrowserTracing(true);
+
+        mockChangeHistory({ to: 'here', from: 'there' });
+        const transaction = getActiveTransaction(hub) as IdleTransaction;
+
+        expect(transaction).toBeDefined();
+        expect(transaction.op).toBe('navigation');
+        expect(transaction.traceId).not.toEqual('12312012123120121231201212312012');
+        expect(transaction.parentSpanId).toBeUndefined();
+      });
+    });
   });
 });

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

@@ -20,6 +20,7 @@ beforeAll(() => {
   global.Request = {};
 });
 
+const hasTracingEnabled = jest.spyOn(tracingUtils, 'hasTracingEnabled');
 const addInstrumentationHandler = jest.spyOn(utils, 'addInstrumentationHandler');
 const setRequestHeader = jest.fn();
 
@@ -108,6 +109,30 @@ describe('callbacks', () => {
       expect(spans).toEqual({});
     });
 
+    it('does not add fetch request spans if tracing is disabled', () => {
+      hasTracingEnabled.mockReturnValueOnce(false);
+      const spans = {};
+
+      fetchCallback(fetchHandlerData, alwaysCreateSpan, spans);
+      expect(spans).toEqual({});
+    });
+
+    it('does not add fetch request headers if tracing is disabled', () => {
+      hasTracingEnabled.mockReturnValueOnce(false);
+
+      // make a local copy so the global one doesn't get mutated
+      const handlerData: FetchData = {
+        args: ['http://dogs.are.great/', {}],
+        fetchData: { url: 'http://dogs.are.great/', method: 'GET' },
+        startTimestamp: 1353501072000,
+      };
+
+      fetchCallback(handlerData, alwaysCreateSpan, {});
+
+      const headers = (handlerData.args[1].headers as Record<string, string>) || {};
+      expect(headers['sentry-trace']).not.toBeDefined();
+    });
+
     it('creates and finishes fetch span on active transaction', () => {
       const spans = {};
 
@@ -174,6 +199,22 @@ describe('callbacks', () => {
       expect(spans).toEqual({});
     });
 
+    it('does not add xhr request spans if tracing is disabled', () => {
+      hasTracingEnabled.mockReturnValueOnce(false);
+      const spans = {};
+
+      xhrCallback(xhrHandlerData, alwaysCreateSpan, spans);
+      expect(spans).toEqual({});
+    });
+
+    it('does not add xhr request headers if tracing is disabled', () => {
+      hasTracingEnabled.mockReturnValueOnce(false);
+
+      xhrCallback(xhrHandlerData, alwaysCreateSpan, {});
+
+      expect(setRequestHeader).not.toHaveBeenCalled();
+    });
+
     it('adds sentry-trace header to XHR requests', () => {
       xhrCallback(xhrHandlerData, alwaysCreateSpan, {});