fix(apm): Sampling of traces

packages/apm/src/hubextensions.ts

@@ -30,7 +30,7 @@ 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 spanOrSpanContext Already constructed span or properties with which the span should be created
@@ -61,11 +61,12 @@ function startSpan(spanOrSpanContext?: Span | SpanContext, makeRoot: boolean = f
     span.sampled = Math.random() < sampleRate;
   }
 
-  // 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);
+  // 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.initSpanList(experimentsOptions.maxSpans as number);
+  }
 
   return span;
 }

packages/apm/src/integrations/tracing.ts

@@ -407,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);
       }
     }
 
@@ -498,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;

packages/apm/src/span.ts

@@ -16,12 +16,12 @@ export const TRACEPARENT_REGEXP = new RegExp(
 /**
  * Keeps track of finished spans for a given transaction
  */
-class SpanRecorder {
+class SpanList {
   private readonly _maxlen: number;
   private _openSpanCount: number = 0;
   public finishedSpans: Span[] = [];
 
-  public constructor(maxlen: number) {
+  public constructor(maxlen: number = 1000) {
     this._maxlen = maxlen;
   }
 
@@ -31,11 +31,11 @@ class SpanRecorder {
    * trace tree (i.e.the first n spans with the smallest
    * start_timestamp).
    */
-  public startSpan(span: Span): void {
-    this._openSpanCount += 1;
+  public add(span: Span): void {
     if (this._openSpanCount > this._maxlen) {
-      span.spanRecorder = undefined;
+      span.spanList = undefined;
     }
+    this._openSpanCount++;
   }
 
   /**
@@ -119,8 +119,13 @@ export class Span implements SpanInterface, SpanContext {
   /**
    * List of spans that were finalized
    */
-  public spanRecorder?: SpanRecorder;
+  public spanList?: SpanList;
 
+  /**
+   * You should never call the custructor manually, always use `hub.startSpan()`.
+   * @internal
+   * @hideconstructor
+   */
   public constructor(spanContext?: SpanContext, hub?: Hub) {
     if (isInstanceOf(hub, Hub)) {
       this._hub = hub as Hub;
@@ -167,11 +172,11 @@ export class Span implements SpanInterface, SpanContext {
    * Attaches SpanRecorder to the span itself
    * @param maxlen maximum number of spans that can be recorded
    */
-  public initFinishedSpans(maxlen: number = 1000): void {
-    if (!this.spanRecorder) {
-      this.spanRecorder = new SpanRecorder(maxlen);
+  public initSpanList(maxlen: number = 1000): void {
+    if (!this.spanList) {
+      this.spanList = new SpanList(maxlen);
     }
-    this.spanRecorder.startSpan(this);
+    this.spanList.add(this);
   }
 
   /**
@@ -187,7 +192,10 @@ export class Span implements SpanInterface, SpanContext {
       traceId: this._traceId,
     });
 
-    span.spanRecorder = this.spanRecorder;
+    span.spanList = this.spanList;
+    if (span.spanList) {
+      span.spanList.add(span);
+    }
 
     return span;
   }
@@ -283,19 +291,21 @@ export class Span implements SpanInterface, SpanContext {
 
     this.timestamp = timestampWithMs();
 
-    if (this.spanRecorder === undefined) {
+    // This happens if a span was initiated outside of `hub.startSpan`
+    // Also if the span was sampled (sampled = false) in `hub.startSpan` already
+    if (this.spanList === undefined) {
       return undefined;
     }
 
-    this.spanRecorder.finishSpan(this);
+    this.spanList.finishSpan(this);
 
     if (this.sampled !== true) {
       // At this point if `sampled !== true` we want to discard the transaction.
       logger.warn('Discarding transaction Span because it was span.sampled !== true');
       return undefined;
     }
 
-    const finishedSpans = this.spanRecorder ? this.spanRecorder.finishedSpans.filter(s => s !== this) : [];
+    const finishedSpans = this.spanList ? this.spanList.finishedSpans.filter(s => s !== this) : [];
 
     if (trimEnd && finishedSpans.length > 0) {
       this.timestamp = finishedSpans.reduce((prev: Span, current: Span) => {

packages/apm/test/hub.test.ts

@@ -13,21 +13,27 @@ describe('Hub', () => {
 
   describe('spans', () => {
     describe('sampling', () => {
-      test('set tracesSampleRate 0', () => {
+      test('set tracesSampleRate 0 root span', () => {
         const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 }));
         const span = hub.startSpan() as any;
-        expect(span.sampled).toBeUndefined();
+        expect(span.sampled).toBe(false);
       });
       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', () => {
+      test('set tracesSampleRate 1 on transaction', () => {
         const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
         const span = hub.startSpan({ transaction: 'foo' }) as any;
         expect(span.sampled).toBeTruthy();
       });
+      test('set tracesSampleRate should be propergated to children', () => {
+        const hub = new Hub(new BrowserClient({ tracesSampleRate: 0 }));
+        const span = hub.startSpan() as any;
+        const child = span.child({ op: 1 });
+        expect(child.sampled).toBeFalsy();
+      });
     });
     describe('start', () => {
       test('simple', () => {

packages/apm/test/span.test.ts

@@ -1,3 +1,4 @@
+import { BrowserClient } from '@sentry/browser';
 import { Hub, Scope } from '@sentry/hub';
 import { SpanStatus } from '@sentry/types';
 
@@ -7,9 +8,8 @@ describe('Span', () => {
   let hub: Hub;
 
   beforeEach(() => {
-    const clientFn: any = jest.fn();
     const myScope = new Scope();
-    hub = new Hub(clientFn, myScope);
+    hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }), myScope);
   });
 
   describe('newSpan', () => {
@@ -90,6 +90,15 @@ describe('Span', () => {
       expect((span as any)._hub).toBeInstanceOf(Hub);
       expect((span2 as any)._hub).toBeInstanceOf(Hub);
     });
+
+    test('inherit span list', () => {
+      const span = new Span({ sampled: true });
+      const span2 = span.child();
+      const span3 = span.child();
+      span3.finish();
+      expect(span.spanList).toBe(span2.spanList);
+      expect(span.spanList).toBe(span3.spanList);
+    });
   });
 
   describe('toTraceparent', () => {
@@ -183,50 +192,108 @@ describe('Span', () => {
       expect(span.timestamp).toBeGreaterThan(1);
     });
 
-    test('finish a span without transaction', () => {
-      const spy = jest.spyOn(hub as any, 'captureEvent');
-      const span = new Span({}, hub);
-      span.finish();
-      expect(spy).not.toHaveBeenCalled();
-    });
+    describe('hub.startSpan', () => {
+      test('finish a span', () => {
+        const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+        const span = hub.startSpan();
+        span.finish();
+        expect(spy).toHaveBeenCalled();
+        expect(spy.mock.calls[0][0].spans).toHaveLength(0);
+        expect(spy.mock.calls[0][0].timestamp).toBeTruthy();
+        expect(spy.mock.calls[0][0].start_timestamp).toBeTruthy();
+        expect(spy.mock.calls[0][0].contexts.trace).toEqual(span.getTraceContext());
+      });
 
-    test('finish a span with transaction', () => {
-      const spy = jest.spyOn(hub as any, 'captureEvent') as any;
-      const span = new Span({ transaction: 'test', sampled: false }, hub);
-      span.initFinishedSpans();
-      span.finish();
-      expect(spy.mock.calls[0][0].spans).toHaveLength(0);
-      expect(spy.mock.calls[0][0].contexts.trace).toEqual(span.getTraceContext());
-    });
+      test('finish a span with transaction + child span', () => {
+        const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+        const parentSpan = hub.startSpan();
+        const childSpan = parentSpan.child();
+        childSpan.finish();
+        parentSpan.finish();
+        expect(spy).toHaveBeenCalled();
+        expect(spy.mock.calls[0][0].spans).toHaveLength(1);
+        expect(spy.mock.calls[0][0].contexts.trace).toEqual(parentSpan.getTraceContext());
+      });
 
-    test('finish a span with transaction + child span', () => {
-      const spy = jest.spyOn(hub as any, 'captureEvent') as any;
-      const parentSpan = new Span({ transaction: 'test', sampled: false }, hub);
-      parentSpan.initFinishedSpans();
-      const childSpan = parentSpan.child();
-      childSpan.finish();
-      parentSpan.finish();
-      expect(spy.mock.calls[0][0].spans).toHaveLength(1);
-      expect(spy.mock.calls[0][0].contexts.trace).toEqual(parentSpan.getTraceContext());
-    });
+      test("finish a child span shouldn't trigger captureEvent", () => {
+        const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+        const parentSpan = hub.startSpan();
+        const childSpan = parentSpan.child();
+        childSpan.finish();
+        expect(spy).not.toHaveBeenCalled();
+      });
 
-    test('finish a span with another one on the scope shouldnt override contexts.trace', () => {
-      const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+      test('finish a span with another one on the scope should add the span and not call captureEvent', () => {
+        const spy = jest.spyOn(hub as any, 'captureEvent') as any;
 
-      const spanOne = new Span({ transaction: 'testOne', sampled: false }, hub);
-      spanOne.initFinishedSpans();
-      const childSpanOne = spanOne.child();
-      childSpanOne.finish();
-      hub.configureScope(scope => {
-        scope.setSpan(spanOne);
+        const spanOne = hub.startSpan();
+        const childSpanOne = spanOne.child();
+        childSpanOne.finish();
+
+        hub.configureScope(scope => {
+          scope.setSpan(spanOne);
+        });
+
+        const spanTwo = hub.startSpan();
+        spanTwo.finish();
+
+        expect(spy).not.toHaveBeenCalled();
+        expect((spanOne as any).spanList.finishedSpans).toHaveLength(2);
       });
 
-      const spanTwo = new Span({ transaction: 'testTwo', sampled: false }, hub);
-      spanTwo.initFinishedSpans();
-      spanTwo.finish();
+      test("finish a span with another one on the scope shouldn't override contexts.trace", () => {
+        const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+
+        const spanOne = hub.startSpan();
+        const childSpanOne = spanOne.child();
+        childSpanOne.finish();
+
+        hub.configureScope(scope => {
+          scope.setSpan(spanOne);
+        });
+
+        const spanTwo = hub.startSpan();
+        spanTwo.finish();
+        spanOne.finish();
 
-      expect(spy.mock.calls[0][0].spans).toHaveLength(0);
-      expect(spy.mock.calls[0][0].contexts.trace).toEqual(spanTwo.getTraceContext());
+        expect(spy).toHaveBeenCalled();
+        expect(spy.mock.calls[0][0].spans).toHaveLength(2);
+        expect(spy.mock.calls[0][0].contexts.trace).toEqual(spanOne.getTraceContext());
+      });
+
+      test('span child limit', () => {
+        const _hub = new Hub(
+          new BrowserClient({
+            _experiments: { maxSpans: 3 },
+            tracesSampleRate: 1,
+          }),
+        );
+        const spy = jest.spyOn(_hub as any, 'captureEvent') as any;
+        const span = _hub.startSpan();
+        for (let i = 0; i < 10; i++) {
+          const child = span.child();
+          child.finish();
+        }
+        span.finish();
+        expect(spy.mock.calls[0][0].spans).toHaveLength(3);
+      });
+
+      test('if we sampled the parent (transaction) we do not want any childs', () => {
+        const _hub = new Hub(
+          new BrowserClient({
+            tracesSampleRate: 0,
+          }),
+        );
+        const spy = jest.spyOn(_hub as any, 'captureEvent') as any;
+        const span = _hub.startSpan();
+        for (let i = 0; i < 10; i++) {
+          const child = span.child();
+          child.finish();
+        }
+        span.finish();
+        expect((span as any).spanList).toBeUndefined();
+        expect(spy).not.toHaveBeenCalled();
+      });
     });
   });
 

packages/types/src/hub.ts

@@ -174,7 +174,7 @@ export interface Hub {
 
   /**
    * 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