only run beforeNavigate if provided by user

Author: lobsterkatie

packages/tracing/src/browser/browsertracing.ts

@@ -13,7 +13,7 @@ import {
   registerRequestInstrumentation,
   RequestInstrumentationOptions,
 } from './request';
-import { defaultBeforeNavigate, defaultRoutingInstrumentation } from './router';
+import { defaultRoutingInstrumentation } from './router';
 
 export const DEFAULT_MAX_TRANSACTION_DURATION_SECONDS = 600;
 
@@ -61,12 +61,17 @@ 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`, though that option may
+   * disappear in the future.
+   *
+   * @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;
+  beforeNavigate?(context: TransactionContext): TransactionContext | undefined;
 
   /**
    * Instrumentation that creates routing change transactions. By default creates
@@ -80,7 +85,6 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
 }
 
 const DEFAULT_BROWSER_TRACING_OPTIONS = {
-  beforeNavigate: defaultBeforeNavigate,
   idleTimeout: DEFAULT_IDLE_TIMEOUT,
   markBackgroundTransactions: true,
   maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS,
@@ -188,21 +192,25 @@ 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 ctx = beforeNavigate({
+    const expandedContext = {
       ...context,
       ...getHeaderContext(),
       trimEnd: true,
-    });
+    };
+    const modifiedContext = typeof beforeNavigate === 'function' ? beforeNavigate(expandedContext) : expandedContext;
 
-    if (ctx === undefined) {
-      logger.log(`[Tracing] Did not create ${context.op} idleTransaction due to beforeNavigate`);
-      return undefined;
+    // TODO (kmclb): for backwards compatibility reasons, beforeNavigate can return undefined to "drop" the transaction
+    // (prevent it from being sent to Sentry). This can be removed in V6, after which time modifiedContext and
+    // finalContext will be one and the same.
+    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();
-    logger.log(`[Tracing] starting ${ctx.op} idleTransaction on scope`);
-    const idleTransaction = startIdleTransaction(hub, ctx, idleTimeout, true);
+    logger.log(`[Tracing] Starting ${finalContext.op} transaction on scope`);
+    const idleTransaction = startIdleTransaction(hub, finalContext, idleTimeout, true);
     idleTransaction.registerBeforeFinishCallback((transaction, endTimestamp) => {
       this._metrics.addPerformanceEntries(transaction);
       adjustTransactionDuration(secToMs(maxTransactionDuration), transaction, endTimestamp);

packages/tracing/src/browser/router.ts

@@ -54,8 +54,3 @@ export function defaultRoutingInstrumentation<T extends TransactionType>(
     });
   }
 }
-
-/** default implementation of Browser Tracing before navigate */
-export function defaultBeforeNavigate(context: TransactionContext): TransactionContext | undefined {
-  return context;
-}

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

@@ -76,7 +76,6 @@ describe('BrowserTracing', () => {
     const browserTracing = createBrowserTracing();
 
     expect(browserTracing.options).toEqual({
-      beforeNavigate: expect.any(Function),
       idleTimeout: DEFAULT_IDLE_TIMEOUT,
       markBackgroundTransactions: true,
       maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS,
@@ -177,14 +176,14 @@ describe('BrowserTracing', () => {
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
 
-      it('does not create a transaction if it returns undefined', () => {
+      it('creates an unsampled transaction 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);
       });

packages/tracing/test/browser/router.test.ts

@@ -1,6 +1,6 @@
 import { JSDOM } from 'jsdom';
 
-import { defaultBeforeNavigate, defaultRoutingInstrumentation } from '../../src/browser/router';
+import { defaultRoutingInstrumentation } from '../../src/browser/router';
 
 let mockChangeHistory: ({ to, from }: { to: string; from?: string }) => void = () => undefined;
 let addInstrumentationHandlerType: string = '';
@@ -15,13 +15,6 @@ jest.mock('@sentry/utils', () => {
   };
 });
 
-describe('defaultBeforeNavigate()', () => {
-  it('returns a context', () => {
-    const ctx = { name: 'testing', status: 'ok' };
-    expect(defaultBeforeNavigate(ctx)).toBe(ctx);
-  });
-});
-
 describe('defaultRoutingInstrumentation', () => {
   const mockFinish = jest.fn();
   const startTransaction = jest.fn().mockReturnValue({ finish: mockFinish });

packages/tracing/test/hub.test.ts

@@ -22,7 +22,7 @@ describe('Hub', () => {
   });
 
   describe('getTransaction()', () => {
-    it('should find a transaction which has been set on the scope', () => {
+    it('should find a sampled transaction which has been set on the scope', () => {
       const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
       const transaction = hub.startTransaction({ name: 'dogpark' });
       hub.configureScope(scope => {
@@ -32,6 +32,16 @@ describe('Hub', () => {
       expect(hub.getScope()?.getTransaction()).toBe(transaction);
     });
 
+    it('should find an unsampled transaction which has been set on the scope', () => {
+      const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
+      const transaction = hub.startTransaction({ name: 'dogpark', sampled: false });
+      hub.configureScope(scope => {
+        scope.setSpan(transaction);
+      });
+
+      expect(hub.getScope()?.getTransaction()).toBe(transaction);
+    });
+
     it("should not find an open transaction if it's not on the scope", () => {
       const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
       hub.startTransaction({ name: 'dogpark' });