make beforeNavigate always return transaction

Author: lobsterkatie

packages/tracing/src/browser/browsertracing.ts

@@ -61,10 +61,15 @@ 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;
 
@@ -187,22 +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 expandedContext = {
       ...context,
       ...getHeaderContext(),
       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;
+    // 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();
-    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/test/browser/browsertracing.test.ts

@@ -177,14 +177,14 @@ describe('BrowserTracing', () => {
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
 
-      it('does not create a transaction if it returns undefined', () => {
+      it('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);
       });