fix(apm): Sampling of traces

CHANGELOG.md

@@ -4,6 +4,10 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
+- [apm] fix: Sampling of traces (#2500)
+- [apm] ref: Remove status from tags in transaction (#2497)
+- [browser] fix: Respect breadcrumbs sentry:false option (#2499)
+
 ## 5.14.2
 
 - [apm] fix: Use Performance API for timings when available, including Web Workers (#2492)

packages/apm/src/hubextensions.ts

@@ -60,10 +60,11 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean
     span.sampled = Math.random() < sampleRate;
   }
 
-  if (span.sampled) {
-    const experimentsOptions = (client && client.getOptions()._experiments) || {};
-    span.initFinishedSpans(experimentsOptions.maxSpans as number);
-  }
+  // We always want to record spans independent from the sample rate.
+  // The Span tree should be built regardless and the top level sample rate shouldn't determine if we create
+  // child spans.
+  const experimentsOptions = (client && client.getOptions()._experiments) || {};
+  span.initFinishedSpans(experimentsOptions.maxSpans as number);
 
   return span;
 }

packages/apm/src/integrations/tracing.ts

@@ -54,15 +54,12 @@ interface TracingOptions {
    * Default: true
    */
   startTransactionOnLocationChange: boolean;
+
   /**
-   * Sample to determine if the Integration should instrument anything. The decision will be taken once per load
-   * on initalization.
-   * 0 = 0% chance of instrumenting
-   * 1 = 100% change of instrumenting
-   *
-   * Default: 1
+   * @deprecated Use tracesSampleRate in the SDK options
+   * This is a noop
    */
-  tracesSampleRate: number;
+  tracesSampleRate?: number;
 
   /**
    * The maximum duration of a transaction before it will be discarded. This is for some edge cases where a browser
@@ -109,11 +106,6 @@ export class Tracing implements Integration {
    */
   public static id: string = 'Tracing';
 
-  /**
-   * Is Tracing enabled, this will be determined once per pageload.
-   */
-  private static _enabled?: boolean;
-
   /** JSDoc */
   public static options: TracingOptions;
 
@@ -163,7 +155,6 @@ export class Tracing implements Integration {
       startTransactionOnLocationChange: true,
       traceFetch: true,
       traceXHR: true,
-      tracesSampleRate: 1,
       tracingOrigins: defaultTracingOrigins,
     };
     // NOTE: Logger doesn't work in contructors, as it's initialized after integrations instances
@@ -189,16 +180,11 @@ export class Tracing implements Integration {
       logger.warn(`[Tracing] We added a reasonable default for you: ${defaultTracingOrigins}`);
     }
 
-    if (!Tracing._isEnabled()) {
-      return;
-    }
-
     // Starting our inital pageload transaction
     if (global.location && global.location.href) {
       // `${global.location.href}` will be used a temp transaction name
       Tracing.startIdleTransaction(global.location.href, {
         op: 'pageload',
-        sampled: true,
       });
     }
 
@@ -221,17 +207,15 @@ export class Tracing implements Integration {
         return event;
       }
 
-      if (Tracing._isEnabled()) {
-        const isOutdatedTransaction =
-          event.timestamp &&
-          event.start_timestamp &&
-          (event.timestamp - event.start_timestamp > Tracing.options.maxTransactionDuration ||
-            event.timestamp - event.start_timestamp < 0);
+      const isOutdatedTransaction =
+        event.timestamp &&
+        event.start_timestamp &&
+        (event.timestamp - event.start_timestamp > Tracing.options.maxTransactionDuration ||
+          event.timestamp - event.start_timestamp < 0);
 
-        if (Tracing.options.maxTransactionDuration !== 0 && event.type === 'transaction' && isOutdatedTransaction) {
-          logger.log('[Tracing] Discarded transaction since it maxed out maxTransactionDuration');
-          return null;
-        }
+      if (Tracing.options.maxTransactionDuration !== 0 && event.type === 'transaction' && isOutdatedTransaction) {
+        logger.log('[Tracing] Discarded transaction since it maxed out maxTransactionDuration');
+        return null;
       }
 
       return event;
@@ -358,31 +342,10 @@ export class Tracing implements Integration {
     });
   }
 
-  /**
-   * Is tracing enabled
-   */
-  private static _isEnabled(): boolean {
-    if (Tracing._enabled !== undefined) {
-      return Tracing._enabled;
-    }
-    // This happens only in test cases where the integration isn't initalized properly
-    // tslint:disable-next-line: strict-type-predicates
-    if (!Tracing.options || typeof Tracing.options.tracesSampleRate !== 'number') {
-      return false;
-    }
-    Tracing._enabled = Math.random() > Tracing.options.tracesSampleRate ? false : true;
-    return Tracing._enabled;
-  }
-
   /**
    * Starts a Transaction waiting for activity idle to finish
    */
   public static startIdleTransaction(name: string, spanContext?: SpanContext): Span | undefined {
-    if (!Tracing._isEnabled()) {
-      // Tracing is not enabled
-      return undefined;
-    }
-
     // If we already have an active transaction it means one of two things
     // a) The user did rapid navigation changes and didn't wait until the transaction was finished
     // b) A activity wasn't popped correctly and therefore the transaction is stalling
@@ -641,10 +604,6 @@ export class Tracing implements Integration {
       autoPopAfter?: number;
     },
   ): number {
-    if (!Tracing._isEnabled()) {
-      // Tracing is not enabled
-      return 0;
-    }
     if (!Tracing._activeTransaction) {
       logger.log(`[Tracing] Not pushing activity ${name} since there is no active transaction`);
       return 0;
@@ -689,10 +648,8 @@ export class Tracing implements Integration {
    */
   public static popActivity(id: number, spanData?: { [key: string]: any }): void {
     // The !id is on purpose to also fail with 0
-    // Since 0 is returned by push activity in case tracing is not enabled
-    // or there is no active transaction
-    if (!Tracing._isEnabled() || !id) {
-      // Tracing is not enabled
+    // Since 0 is returned by push activity in case there is no active transaction
+    if (!id) {
       return;
     }
 
@@ -839,7 +796,6 @@ function historyCallback(_: { [key: string]: any }): void {
   if (Tracing.options.startTransactionOnLocationChange && global && global.location) {
     Tracing.startIdleTransaction(global.location.href, {
       op: 'navigation',
-      sampled: true,
     });
   }
 }

packages/apm/src/span.ts

@@ -287,11 +287,8 @@ export class Span implements SpanInterface, SpanContext {
       return undefined;
     }
 
-    if (this.sampled === undefined) {
-      // At this point a `sampled === undefined` should have already been
-      // resolved to a concrete decision. If `sampled` is `undefined`, it's
-      // likely that somebody used `Sentry.startSpan(...)` on a
-      // non-transaction span and later decided to make it a transaction.
+    if (this.sampled !== true) {
+      // At this point if `sampled !== true` we want to discard the transaction.
       logger.warn('Discarding transaction Span without sampling decision');
       return undefined;
     }

packages/apm/test/hub.test.ts

@@ -1,9 +1,9 @@
+import { BrowserClient } from '@sentry/browser';
 import { Hub, Scope } from '@sentry/hub';
 
 import { addExtensionMethods } from '../src/hubextensions';
 
 addExtensionMethods();
-const clientFn: any = jest.fn();
 
 describe('Hub', () => {
   afterEach(() => {
@@ -12,16 +12,33 @@ describe('Hub', () => {
   });
 
   describe('spans', () => {
+    describe('sampling', () => {
+      test('set tracesSampleRate 0', () => {
+        const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 }));
+        const span = hub.startSpan() as any;
+        expect(span.sampled).toBeUndefined();
+      });
+      test('set tracesSampleRate 0 on transaction', () => {
+        const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 }));
+        const span = hub.startSpan({ transaction: 'foo' }) as any;
+        expect(span.sampled).toBe(false);
+      });
+      test('set tracesSampleRate 1', () => {
+        const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
+        const span = hub.startSpan({ transaction: 'foo' }) as any;
+        expect(span.sampled).toBeTruthy();
+      });
+    });
     describe('start', () => {
       test('simple', () => {
-        const hub = new Hub(clientFn);
+        const hub = new Hub(new BrowserClient());
         const span = hub.startSpan() as any;
         expect(span._spanId).toBeTruthy();
       });
 
       test('inherits from parent span', () => {
         const myScope = new Scope();
-        const hub = new Hub(clientFn, myScope);
+        const hub = new Hub(new BrowserClient(), myScope);
         const parentSpan = hub.startSpan({}) as any;
         expect(parentSpan._parentId).toBeFalsy();
         hub.configureScope(scope => {

packages/types/src/options.ts

@@ -78,7 +78,14 @@ export interface Options {
   /** A global sample rate to apply to all events (0 - 1). */
   sampleRate?: number;
 
-  /** A global sample rate to apply to all transactions (0 - 1). */
+  /**
+   * Sample rate to determine transaction/span sampling.
+   *
+   * 0 = 0% chance of instrumenting
+   * 1 = 100% change of instrumenting
+   *
+   * Default: 1
+   */
   tracesSampleRate?: number;
 
   /** Attaches stacktraces to pure capture message / log integrations */