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

@@ -30,39 +30,42 @@ function traceHeaders(): { [key: string]: string } {
 
 /**
  * This functions starts a span. If argument passed is of type `Span`, it'll run sampling on it if configured
- * and attach a `SpanRecorder`. If it's of type `SpanContext` and there is already a `Span` on the Scope,
+ * and attach a `SpanList`. If it's of type `SpanContext` and there is already a `Span` on the Scope,
  * the created Span will have a reference to it and become it's child. Otherwise it'll crete a new `Span`.
  *
- * @param span Already constructed span which should be started or properties with which the span should be created
+ * @param spanOrSpanContext Already constructed span or properties with which the span should be created
+ * @param makeRoot This will just create the span as it is and will not attach it to the span on the scope (if there is one).
+ * Under some circumstances, in internal integrations, for example, this is used to make sure they are not interfering with each other.
  */
-function startSpan(spanOrSpanContext?: Span | SpanContext, forceNoChild: boolean = false): Span {
+function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = false): Span {
   // @ts-ignore
-  const that = this as Hub;
-  const scope = that.getScope();
-  const client = that.getClient();
+  const hub = this as Hub;
+  const scope = hub.getScope();
+  const client = hub.getClient();
   let span;
 
-  if (!isSpanInstance(spanOrSpanContext) && !forceNoChild) {
-    if (scope) {
-      const parentSpan = scope.getSpan() as Span;
-      if (parentSpan) {
-        span = parentSpan.child(spanOrSpanContext);
-      }
+  if (!isSpanInstance(spanOrSpanContext) && !makeRoot && scope) {
+    const parentSpan = scope.getSpan() as Span;
+    if (parentSpan) {
+      span = parentSpan.child(spanOrSpanContext);
     }
   }
 
   if (!isSpanInstance(span)) {
-    span = new Span(spanOrSpanContext, that);
+    span = new Span(spanOrSpanContext, hub);
   }
 
-  if (span.sampled === undefined && span.transaction !== undefined) {
+  // We only roll the dice on sampling for "root" spans (transactions) because the childs inherit this state
+  if (span.sampled === undefined && !span.isChildSpan()) {
     const sampleRate = (client && client.getOptions().tracesSampleRate) || 0;
     span.sampled = Math.random() < sampleRate;
   }
 
+  // We only want to create a span list if we sampled the transaction
+  // in case we will discard the span anyway because sampled == false, we safe memory and do not store child spans
   if (span.sampled) {
     const experimentsOptions = (client && client.getOptions()._experiments) || {};
-    span.initFinishedSpans(experimentsOptions.maxSpans as number);
+    span.initSpanList(experimentsOptions.maxSpans as number);
   }
 
   return span;

packages/apm/src/integrations/tracing.ts

@@ -54,15 +54,6 @@ 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
-   */
-  tracesSampleRate: number;
 
   /**
    * The maximum duration of a transaction before it will be discarded. This is for some edge cases where a browser
@@ -109,11 +100,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 +149,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 +174,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 +201,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;
@@ -345,7 +323,7 @@ export class Tracing implements Integration {
          * If an error or unhandled promise occurs, we mark the active transaction as failed
          */
         logger.log(`[Tracing] Global error occured, setting status in transaction: ${SpanStatus.InternalError}`);
-        (Tracing._activeTransaction as SpanClass).setStatus(SpanStatus.InternalError);
+        Tracing._activeTransaction.setStatus(SpanStatus.InternalError);
       }
     }
     addInstrumentationHandler({
@@ -358,31 +336,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
@@ -400,47 +357,22 @@ export class Tracing implements Integration {
       return undefined;
     }
 
-    const span = hub.startSpan(
+    Tracing._activeTransaction = hub.startSpan(
       {
         ...spanContext,
         transaction: name,
       },
       true,
     );
 
-    Tracing._activeTransaction = span;
-
-    // We need to do this workaround here and not use configureScope
-    // Reason being at the time we start the inital transaction we do not have a client bound on the hub yet
-    // therefore configureScope wouldn't be executed and we would miss setting the transaction
-    // tslint:disable-next-line: no-unsafe-any
-    (hub as any).getScope().setSpan(span);
-
     // The reason we do this here is because of cached responses
     // If we start and transaction without an activity it would never finish since there is no activity
     const id = Tracing.pushActivity('idleTransactionStarted');
     setTimeout(() => {
       Tracing.popActivity(id);
     }, (Tracing.options && Tracing.options.idleTimeout) || 100);
 
-    return span;
-  }
-
-  /**
-   * Update transaction
-   * @deprecated
-   */
-  public static updateTransactionName(name: string): void {
-    logger.log('[Tracing] DEPRECATED, use Sentry.configureScope => scope.setTransaction instead', name);
-    const _getCurrentHub = Tracing._getCurrentHub;
-    if (_getCurrentHub) {
-      const hub = _getCurrentHub();
-      if (hub) {
-        hub.configureScope(scope => {
-          scope.setTransaction(name);
-        });
-      }
-    }
+    return Tracing._activeTransaction;
   }
 
   /**
@@ -475,8 +407,8 @@ export class Tracing implements Integration {
 
     // tslint:disable-next-line: completed-docs
     function addSpan(span: SpanClass): void {
-      if (transactionSpan.spanRecorder) {
-        transactionSpan.spanRecorder.finishSpan(span);
+      if (transactionSpan.spanList) {
+        transactionSpan.spanList.finishSpan(span);
       }
     }
 
@@ -566,8 +498,8 @@ export class Tracing implements Integration {
             const resourceName = entry.name.replace(window.location.origin, '');
             if (entry.initiatorType === 'xmlhttprequest' || entry.initiatorType === 'fetch') {
               // We need to update existing spans with new timing info
-              if (transactionSpan.spanRecorder) {
-                transactionSpan.spanRecorder.finishedSpans.map((finishedSpan: SpanClass) => {
+              if (transactionSpan.spanList) {
+                transactionSpan.spanList.finishedSpans.map((finishedSpan: SpanClass) => {
                   if (finishedSpan.description && finishedSpan.description.indexOf(resourceName) !== -1) {
                     finishedSpan.startTimestamp = timeOrigin + startTime;
                     finishedSpan.timestamp = finishedSpan.startTimestamp + duration;
@@ -641,11 +573,9 @@ export class Tracing implements Integration {
       autoPopAfter?: number;
     },
   ): number {
-    if (!Tracing._isEnabled()) {
-      // Tracing is not enabled
-      return 0;
-    }
-    if (!Tracing._activeTransaction) {
+    const activeTransaction = Tracing._activeTransaction;
+
+    if (!activeTransaction) {
       logger.log(`[Tracing] Not pushing activity ${name} since there is no active transaction`);
       return 0;
     }
@@ -657,7 +587,7 @@ export class Tracing implements Integration {
     if (spanContext && _getCurrentHub) {
       const hub = _getCurrentHub();
       if (hub) {
-        const span = hub.startSpan(spanContext);
+        const span = activeTransaction.child(spanContext);
         Tracing._activities[Tracing._currentIndex] = {
           name,
           span,
@@ -689,18 +619,16 @@ 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;
     }
 
     const activity = Tracing._activities[id];
 
     if (activity) {
       logger.log(`[Tracing] popActivity ${activity.name}#${id}`);
-      const span = activity.span as SpanClass;
+      const span = activity.span;
       if (span) {
         if (spanData) {
           Object.keys(spanData).forEach((key: string) => {
@@ -778,7 +706,11 @@ function xhrCallback(handlerData: { [key: string]: any }): void {
   if (activity) {
     const span = activity.span;
     if (span && handlerData.xhr.setRequestHeader) {
-      handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent());
+      try {
+        handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent());
+      } catch (_) {
+        // Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED.
+      }
     }
   }
   // tslint:enable: no-unsafe-any
@@ -839,7 +771,6 @@ function historyCallback(_: { [key: string]: any }): void {
   if (Tracing.options.startTransactionOnLocationChange && global && global.location) {
     Tracing.startIdleTransaction(global.location.href, {
       op: 'navigation',
-      sampled: true,
     });
   }
 }