getsentry · lobsterkatie · Sep 18, 2020 · Sep 18, 2020
diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts
@@ -214,6 +214,7 @@ describe('tracingHandler', () => {
 
     const transaction = (res as any).__sentry_transaction;
 
+    // since we have no tracesSampler defined, the default behavior (inherit if possible) applies
     expect(transaction.traceId).toEqual('12312012123120121231201212312012');
     expect(transaction.parentSpanId).toEqual('1121201211212012');
     expect(transaction.sampled).toEqual(false);

diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts
@@ -79,6 +79,17 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
   ): void;
 }
 
+const DEFAULT_BROWSER_TRACING_OPTIONS = {
+  beforeNavigate: defaultBeforeNavigate,
+  idleTimeout: DEFAULT_IDLE_TIMEOUT,
+  markBackgroundTransactions: true,
+  maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS,
+  routingInstrumentation: defaultRoutingInstrumentation,
+  startTransactionOnLocationChange: true,
+  startTransactionOnPageLoad: true,
+  ...defaultRequestInstrumentionOptions,
+};
+
 /**
  * The Browser Tracing integration automatically instruments browser pageload/navigation
  * actions as transactions, and captures requests, metrics and errors as spans.
@@ -93,16 +104,7 @@ export class BrowserTracing implements Integration {
   public static id: string = 'BrowserTracing';
 
   /** Browser Tracing integration options */
-  public options: BrowserTracingOptions = {
-    beforeNavigate: defaultBeforeNavigate,
-    idleTimeout: DEFAULT_IDLE_TIMEOUT,
-    markBackgroundTransactions: true,
-    maxTransactionDuration: DEFAULT_MAX_TRANSACTION_DURATION_SECONDS,
-    routingInstrumentation: defaultRoutingInstrumentation,
-    startTransactionOnLocationChange: true,
-    startTransactionOnPageLoad: true,
-    ...defaultRequestInstrumentionOptions,
-  };
+  public options: BrowserTracingOptions;
 
   /**
    * @inheritDoc
@@ -130,7 +132,7 @@ export class BrowserTracing implements Integration {
     }
 
     this.options = {
-      ...this.options,
+      ...DEFAULT_BROWSER_TRACING_OPTIONS,
       ..._options,
       tracingOrigins,
     };
@@ -179,7 +181,7 @@ export class BrowserTracing implements Integration {
   /** Create routing idle transaction. */
   private _createRouteTransaction(context: TransactionContext): TransactionType | undefined {
     if (!this._getCurrentHub) {
-      logger.warn(`[Tracing] Did not create ${context.op} idleTransaction due to invalid _getCurrentHub`);
+      logger.warn(`[Tracing] Did not create ${context.op} transaction because _getCurrentHub is invalid.`);
       return undefined;
     }
 

diff --git a/packages/tracing/src/browser/router.ts b/packages/tracing/src/browser/router.ts
@@ -1,11 +1,10 @@
 import { Transaction as TransactionType, TransactionContext } from '@sentry/types';
 import { addInstrumentationHandler, getGlobalObject, logger } from '@sentry/utils';
 
-// type StartTransaction
 const global = getGlobalObject<Window>();
 
 /**
- * Creates a default router based on
+ * Default function implementing pageload and navigation transactions
  */
 export function defaultRoutingInstrumentation<T extends TransactionType>(
   startTransaction: (context: TransactionContext) => T | undefined,
@@ -28,24 +27,24 @@ export function defaultRoutingInstrumentation<T extends TransactionType>(
     addInstrumentationHandler({
       callback: ({ to, from }: { to: string; from?: string }) => {
         /**
-         * This early return is there to account for some cases where navigation transaction
-         * starts right after long running pageload. We make sure that if `from` is undefined
-         * and that a valid `startingURL` exists, we don't unnecessarily create a navigation transaction.
+         * This early return is there to account for some cases where a navigation transaction starts right after
+         * long-running pageload. We make sure that if `from` is undefined and a valid `startingURL` exists, we don't
+         * create an uneccessary navigation transaction.
          *
-         * This was hard to duplicate, but this behavior stopped as soon as this fix
-         * was applied. This issue might also only be caused in certain development environments
-         * where the usage of a hot module reloader is causing errors.
+         * This was hard to duplicate, but this behavior stopped as soon as this fix was applied. This issue might also
+         * only be caused in certain development environments where the usage of a hot module reloader is causing
+         * errors.
          */
         if (from === undefined && startingUrl && startingUrl.indexOf(to) !== -1) {
           startingUrl = undefined;
           return;
         }
+
         if (from !== to) {
           startingUrl = undefined;
           if (activeTransaction) {
-            logger.log(`[Tracing] finishing current idleTransaction with op: ${activeTransaction.op}`);
-            // We want to finish all current ongoing idle transactions as we
-            // are navigating to a new page.
+            logger.log(`[Tracing] Finishing current transaction with op: ${activeTransaction.op}`);
+            // If there's an open transaction on the scope, we need to finish it before creating an new one.
             activeTransaction.finish();
           }
           activeTransaction = startTransaction({ name: global.location.pathname, op: 'navigation' });

diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts
@@ -106,9 +106,9 @@ function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext
 
   // at this point we know we're keeping the transaction, whether because of an inherited decision or because it got
   // lucky with the dice roll
-  const experimentsOptions = options._experiments || {};
-  transaction.initSpanRecorder(experimentsOptions.maxSpans as number);
+  transaction.initSpanRecorder(options._experiments?.maxSpans as number);
 
+  logger.log(`[Tracing] starting ${transaction.op} transaction - ${transaction.name}`);
   return transaction;
 }
 /**
@@ -187,6 +187,10 @@ function isValidSampleRate(rate: unknown): boolean {
  * The Hub.startTransaction method delegates to this method to do its work, passing the Hub instance in as `this`.
  * Exists as a separate function so that it can be injected into the class as an "extension method."
  *
+ * @param this: The Hub starting the transaction
+ * @param transactionContext: Data used to configure the transaction
+ * @param CustomSamplingContext: Optional data to be provided to the `tracesSampler` function (if any)
+ *
  * @returns The new transaction
  *
  * @see {@link Hub.startTransaction}

diff --git a/packages/tracing/src/utils.ts b/packages/tracing/src/utils.ts
@@ -43,16 +43,9 @@ export function extractTraceparentData(traceparent: string): TraceparentData | u
   return undefined;
 }
 
-/** Grabs active transaction off scope */
+/** Grabs active transaction off scope, if any */
 export function getActiveTransaction<T extends Transaction>(hub: Hub = getCurrentHub()): T | undefined {
-  if (hub) {
-    const scope = hub.getScope();
-    if (scope) {
-      return scope.getTransaction() as T | undefined;
-    }
-  }
-
-  return undefined;
+  return hub?.getScope()?.getTransaction() as T | undefined;
 }
 
 /**

diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts
@@ -133,7 +133,7 @@ describe('BrowserTracing', () => {
         expect(inst.options.tracingOrigins).toEqual(defaultRequestInstrumentionOptions.tracingOrigins);
       });
 
-      it('warns and uses default tracing origins if array not given', () => {
+      it('warns and uses default tracing origins if empty array given', () => {
         const inst = createBrowserTracing(true, {
           routingInstrumentation: customRoutingInstrumentation,
           tracingOrigins: [],
@@ -189,18 +189,18 @@ describe('BrowserTracing', () => {
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
 
-      it('can be a custom value', () => {
+      it('can override default context values', () => {
         const mockBeforeNavigation = jest.fn(ctx => ({
           ...ctx,
-          op: 'something-else',
+          op: 'not-pageload',
         }));
         createBrowserTracing(true, {
           beforeNavigate: mockBeforeNavigation,
           routingInstrumentation: customRoutingInstrumentation,
         });
         const transaction = getActiveTransaction(hub) as IdleTransaction;
         expect(transaction).toBeDefined();
-        expect(transaction.op).toBe('something-else');
+        expect(transaction.op).toBe('not-pageload');
 
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
@@ -308,7 +308,7 @@ describe('BrowserTracing', () => {
         mockChangeHistory = () => undefined;
       });
 
-      it('it is not created automatically', () => {
+      it('it is not created automatically at startup', () => {
         createBrowserTracing(true);
         jest.runAllTimers();
 

diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts
@@ -283,7 +283,7 @@ describe('Hub', () => {
       it('should propagate sampling decision to child spans', () => {
         const hub = new Hub(new BrowserClient({ tracesSampleRate: Math.random() }));
         const transaction = hub.startTransaction({ name: 'dogpark' });
-        const child = transaction.startChild({ op: 'test' });
+        const child = transaction.startChild({ op: 'ball.chase' });
 
         expect(child.sampled).toBe(transaction.sampled);
       });

diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts
@@ -96,7 +96,7 @@ export interface Scope {
   getSpan(): Span | undefined;
 
   /**
-   * Returns the `Transaction` if there is one
+   * Returns the `Transaction` attached to the scope (if there is one)
    */
   getTransaction(): Transaction | undefined;