Next iteration of span API

kamilogorek · kamilogorek · commit 0a83eae5a8a6 · 2019-11-01T11:27:38.000+01:00
diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts
@@ -397,75 +397,18 @@ export class Hub implements HubInterface {
   /**
    * @inheritDoc
    */
-  public startSpan(spanContext?: SpanContext, bindOnScope: boolean = false): Span {
+  public startSpan(spanContext?: SpanContext): Span {
     const top = this.getStackTop();
 
-    const simpleNewSpan = new Span(spanContext);
-    if (top.scope && top.client) {
-      if (bindOnScope) {
-        top.scope.setSpan(simpleNewSpan);
-        return simpleNewSpan;
-      }
+    if (top.scope) {
       const span = top.scope.getSpan();
+
       if (span) {
         return span.newSpan(spanContext);
       }
     }
 
-    return simpleNewSpan;
-  }
-
-  /**
-   * @inheritDoc
-   */
-  public finishSpan(span?: Span): string | undefined {
-    const top = this.getStackTop();
-    let passedSpan = span;
-
-    // If the passed span is undefined we try to get the span from the scope and finish it.
-    if (passedSpan === undefined) {
-      if (top.scope && top.client) {
-        const scopeSpan = top.scope.getSpan();
-        if (scopeSpan) {
-          passedSpan = scopeSpan;
-        }
-      }
-    }
-
-    if (passedSpan === undefined) {
-      logger.warn('There was no Span on the Scope and none was passed, do nothing.');
-      // We will do nothing since nothing was passed and there is no Span on the scope.
-      return undefined;
-    }
-
-    if (!passedSpan.timestamp) {
-      passedSpan.finish();
-    }
-
-    if (!passedSpan.transaction) {
-      return undefined;
-    }
-
-    if (!top.client) {
-      return undefined;
-    }
-
-    // TODO: if sampled do what?
-
-    const finishedSpans = passedSpan.finishedSpans.filter(s => s !== passedSpan);
-
-    const eventId = this.captureEvent({
-      contexts: { trace: passedSpan.getTraceContext() },
-      spans: finishedSpans.length > 0 ? finishedSpans : undefined,
-      start_timestamp: passedSpan.startTimestamp,
-      timestamp: passedSpan.timestamp,
-      transaction: passedSpan.transaction,
-      type: 'transaction',
-    });
-
-    // After sending we reset the finishedSpans array
-    passedSpan.finishedSpans = [];
-    return eventId;
+    return new Span(spanContext);
   }
 }
 
diff --git a/packages/hub/src/span.ts b/packages/hub/src/span.ts
@@ -1,12 +1,18 @@
 import { Span as SpanInterface, SpanContext } from '@sentry/types';
 import { timestampWithMs, uuid4 } from '@sentry/utils';
+import { getCurrentHub, Hub } from './hub';
 
 export const TRACEPARENT_REGEXP = /^[ \t]*([0-9a-f]{32})?-?([0-9a-f]{16})?-?([01])?[ \t]*$/;
 
 /**
  * Span contains all data about a span
  */
 export class Span implements SpanInterface, SpanContext {
+  /**
+   * @inheritDoc
+   */
+  private readonly _hub: Hub = getCurrentHub();
+
   /**
    * @inheritDoc
    */
@@ -67,7 +73,11 @@ export class Span implements SpanInterface, SpanContext {
    */
   public finishedSpans: Span[] = [];
 
-  public constructor(spanContext?: SpanContext) {
+  public constructor(spanContext?: SpanContext, hub?: Hub) {
+    if (hub instanceof Hub) {
+      this._hub = hub;
+    }
+
     if (!spanContext) {
       return this;
     }
@@ -147,9 +157,32 @@ export class Span implements SpanInterface, SpanContext {
   /**
    * Sets the finish timestamp on the current span
    */
-  public finish(): void {
+  public finish(): string | undefined {
+    // Don't allow for finishing more than once
+    if (typeof this.timestamp === 'number') {
+      return undefined;
+    }
+
     this.timestamp = timestampWithMs();
     this.finishedSpans.push(this);
+
+    // Don't send non-transaction spans
+    if (typeof this.transaction !== 'string') {
+      return undefined;
+    }
+
+    // TODO: if sampled do what?
+    const finishedSpans = this.finishedSpans.filter(s => s !== this);
+    this.finishedSpans = [];
+
+    return this._hub.captureEvent({
+      contexts: { trace: this.getTraceContext() },
+      spans: finishedSpans.length > 0 ? finishedSpans : undefined,
+      start_timestamp: this.startTimestamp,
+      timestamp: this.timestamp,
+      transaction: this.transaction,
+      type: 'transaction',
+    });
   }
 
   /**
diff --git a/packages/hub/test/hub.test.ts b/packages/hub/test/hub.test.ts
@@ -290,51 +290,16 @@ describe('Hub', () => {
         expect(span._spanId).toBeTruthy();
       });
 
-      test('bindOnScope', () => {
+      test('inherits from parent span', () => {
         const myScope = new Scope();
         const hub = new Hub(clientFn, myScope);
-        const span = hub.startSpan({}, true) as any;
-        expect((myScope as any)._span).toBe(span);
-      });
-    });
-
-    describe('finish', () => {
-      test('simple', () => {
-        const hub = new Hub(clientFn);
-        const span = hub.startSpan() as any;
-        expect(span.timestamp).toBeUndefined();
-        expect(hub.finishSpan(span)).toBeUndefined();
-        expect(span.timestamp).toBeGreaterThan(1);
-      });
-
-      test('finish a scope span without transaction', () => {
-        const myScope = new Scope();
-        const hub = new Hub(clientFn, myScope);
-        const spy = jest.spyOn(hub as any, 'captureEvent');
-        const span = hub.startSpan({}, true) as any;
-        expect(hub.finishSpan(span)).toBeUndefined();
-        expect(spy).not.toHaveBeenCalled();
-      });
-
-      test('finish a scope span with transaction', () => {
-        const myScope = new Scope();
-        const hub = new Hub(clientFn, myScope);
-        const spy = jest.spyOn(hub as any, 'captureEvent');
-        const span = hub.startSpan({ transaction: 'test' }, true) as any;
-        expect(hub.finishSpan(span)).toBeDefined();
-        expect(spy).toHaveBeenCalled();
-        expect(spy.mock.calls[0][0].spans).toBeUndefined();
-      });
-
-      test('finish a scope span with transaction + child span', () => {
-        const myScope = new Scope();
-        const hub = new Hub(clientFn, myScope);
-        const spy = jest.spyOn(hub as any, 'captureEvent');
-        const span = hub.startSpan({ transaction: 'test' }, true) as any;
-        hub.finishSpan(hub.startSpan());
-        expect(hub.finishSpan(span)).toBeDefined();
-        expect(spy).toHaveBeenCalled();
-        expect(spy.mock.calls[0][0].spans).toHaveLength(1);
+        const parentSpan = hub.startSpan({}) as any;
+        expect(parentSpan._parentId).toBeFalsy();
+        hub.configureScope(scope => {
+          scope.setSpan(parentSpan);
+        });
+        const span = hub.startSpan({}) as any;
+        expect(span._parentSpanId).toBeTruthy();
       });
     });
   });
diff --git a/packages/hub/test/scope.test.ts b/packages/hub/test/scope.test.ts
@@ -1,7 +1,7 @@
 import { Event, EventHint, Severity } from '@sentry/types';
 import { getGlobalObject } from '@sentry/utils';
 
-import { addGlobalEventProcessor, Scope } from '../src';
+import { addGlobalEventProcessor, Scope, Span } from '../src';
 
 describe('Scope', () => {
   afterEach(() => {
@@ -73,9 +73,6 @@ describe('Scope', () => {
       scope.addBreadcrumb({ message: 'test' }, 100);
       expect((scope as any)._breadcrumbs[0]).toHaveProperty('message', 'test');
     });
-  });
-
-  describe('level', () => {
     test('set', () => {
       const scope = new Scope();
       scope.setLevel(Severity.Critical);
@@ -111,6 +108,21 @@ describe('Scope', () => {
     });
   });
 
+  describe('span', () => {
+    test('set', () => {
+      const scope = new Scope();
+      const span = new Span({});
+      scope.setSpan(span);
+      expect((scope as any)._span).toEqual(span);
+    });
+    test('unset', () => {
+      const scope = new Scope();
+      scope.setSpan(new Span({}));
+      scope.setSpan();
+      expect((scope as any)._span).toEqual(undefined);
+    });
+  });
+
   test('chaining', () => {
     const scope = new Scope();
     scope.setLevel(Severity.Critical).setUser({ id: '1' });
diff --git a/packages/hub/test/span.test.ts b/packages/hub/test/span.test.ts
@@ -1,6 +1,14 @@
-import { Span, TRACEPARENT_REGEXP } from '../src';
+import { Span, TRACEPARENT_REGEXP, Hub, Scope } from '../src';
 
 describe('Span', () => {
+  let hub: Hub;
+
+  beforeEach(() => {
+    const clientFn: any = jest.fn();
+    const myScope = new Scope();
+    hub = new Hub(clientFn, myScope);
+  });
+
   describe('newSpan', () => {
     test('simple', () => {
       const span = new Span({ sampled: true });
@@ -9,6 +17,13 @@ describe('Span', () => {
       expect((span2 as any)._traceId).toBe((span as any)._traceId);
       expect((span2 as any).sampled).toBe((span as any).sampled);
     });
+
+    test.only('gets currentHub', () => {
+      const span = new Span({});
+      const span2 = span.newSpan();
+      expect((span as any)._hub).toBeInstanceOf(Hub);
+      expect((span2 as any)._hub).toBeInstanceOf(Hub);
+    });
   });
 
   describe('toTraceparent', () => {
@@ -74,4 +89,38 @@ describe('Span', () => {
       expect(serialized).toHaveProperty('trace_id', 'c');
     });
   });
+
+  describe('finish', () => {
+    test('simple', () => {
+      const span = new Span({});
+      expect(span.timestamp).toBeUndefined();
+      span.finish();
+      expect(span.timestamp).toBeGreaterThan(1);
+    });
+
+    test('finish a scope span without transaction', () => {
+      const spy = jest.spyOn(hub as any, 'captureEvent');
+      const span = new Span({}, hub);
+      span.finish();
+      expect(spy).not.toHaveBeenCalled();
+    });
+
+    test('finish a scope span with transaction', () => {
+      const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+      const span = new Span({ transaction: 'test' }, hub);
+      span.finish();
+      expect(spy).toHaveBeenCalled();
+      expect(spy.mock.calls[0][0].spans).toBeUndefined();
+    });
+
+    test('finish a scope span with transaction + child span', () => {
+      const spy = jest.spyOn(hub as any, 'captureEvent') as any;
+      const parentSpan = new Span({ transaction: 'test' }, hub);
+      const childSpan = parentSpan.newSpan();
+      childSpan.finish();
+      parentSpan.finish();
+      expect(spy).toHaveBeenCalled();
+      expect(spy.mock.calls[0][0].spans).toHaveLength(1);
+    });
+  });
 });
