From b82ef996069f0bc29f9b80bc90c438854e6fbb91 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 21 Dec 2023 13:28:10 +0100 Subject: [PATCH 1/2] feat(core): Ensure `startSpan` & `startSpanManual` fork scope --- packages/core/src/tracing/trace.ts | 112 +++++++++---------- packages/core/test/lib/tracing/trace.test.ts | 82 +++++++++++++- 2 files changed, 136 insertions(+), 58 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 52c915c30189..a81187139a5c 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -2,7 +2,7 @@ import type { TransactionContext } from '@sentry/types'; import { dropUndefinedKeys, isThenable, logger, tracingContextFromHeaders } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; -import { getCurrentScope } from '../exports'; +import { getCurrentScope, withScope } from '../exports'; import type { Hub } from '../hub'; import { getCurrentHub } from '../hub'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; @@ -89,42 +89,42 @@ export function trace( export function startSpan(context: TransactionContext, callback: (span: Span | undefined) => T): T { const ctx = normalizeContext(context); - const hub = getCurrentHub(); - const scope = getCurrentScope(); - const parentSpan = scope.getSpan(); + return withScope(scope => { + const hub = getCurrentHub(); + const parentSpan = scope.getSpan(); - const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx); - scope.setSpan(activeSpan); - - function finishAndSetSpan(): void { - activeSpan && activeSpan.end(); - scope.setSpan(parentSpan); - } + const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx); + scope.setSpan(activeSpan); - let maybePromiseResult: T; - try { - maybePromiseResult = callback(activeSpan); - } catch (e) { - activeSpan && activeSpan.setStatus('internal_error'); - finishAndSetSpan(); - throw e; - } + function finishAndSetSpan(): void { + activeSpan && activeSpan.finish(); + } - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then( - () => { - finishAndSetSpan(); - }, - () => { - activeSpan && activeSpan.setStatus('internal_error'); - finishAndSetSpan(); - }, - ); - } else { - finishAndSetSpan(); - } - - return maybePromiseResult; + let maybePromiseResult: T; + try { + maybePromiseResult = callback(activeSpan); + } catch (e) { + activeSpan && activeSpan.setStatus('internal_error'); + finishAndSetSpan(); + throw e; + } + + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then( + () => { + finishAndSetSpan(); + }, + () => { + activeSpan && activeSpan.setStatus('internal_error'); + finishAndSetSpan(); + }, + ); + } else { + finishAndSetSpan(); + } + + return maybePromiseResult; + }); } /** @@ -149,33 +149,33 @@ export function startSpanManual( ): T { const ctx = normalizeContext(context); - const hub = getCurrentHub(); - const scope = getCurrentScope(); - const parentSpan = scope.getSpan(); + return withScope(scope => { + const hub = getCurrentHub(); + const parentSpan = scope.getSpan(); - const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx); - scope.setSpan(activeSpan); + const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx); + scope.setSpan(activeSpan); - function finishAndSetSpan(): void { - activeSpan && activeSpan.end(); - scope.setSpan(parentSpan); - } + function finishAndSetSpan(): void { + activeSpan && activeSpan.finish(); + } - let maybePromiseResult: T; - try { - maybePromiseResult = callback(activeSpan, finishAndSetSpan); - } catch (e) { - activeSpan && activeSpan.setStatus('internal_error'); - throw e; - } - - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then(undefined, () => { + let maybePromiseResult: T; + try { + maybePromiseResult = callback(activeSpan, finishAndSetSpan); + } catch (e) { activeSpan && activeSpan.setStatus('internal_error'); - }); - } + throw e; + } - return maybePromiseResult; + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then(undefined, () => { + activeSpan && activeSpan.setStatus('internal_error'); + }); + } + + return maybePromiseResult; + }); } /** diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index a9ea12ef89c0..86c5a33e984e 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -1,5 +1,6 @@ -import { Hub, addTracingExtensions, makeMain } from '../../../src'; -import { continueTrace, startSpan } from '../../../src/tracing'; +import type { Span } from '@sentry/types'; +import { Hub, addTracingExtensions, getCurrentScope, makeMain } from '../../../src'; +import { continueTrace, startInactiveSpan, startSpan, startSpanManual } from '../../../src/tracing'; import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; beforeAll(() => { @@ -80,6 +81,18 @@ describe('startSpan', () => { expect(ref.status).toEqual(isError ? 'internal_error' : undefined); }); + it('creates & finishes span', async () => { + let _span: Span | undefined; + startSpan({ name: 'GET users/[id]' }, span => { + expect(span).toBeDefined(); + expect(span?.endTimestamp).toBeUndefined(); + _span = span; + }); + + expect(_span).toBeDefined(); + expect(_span?.endTimestamp).toBeDefined(); + }); + it('allows traceparent information to be overriden', async () => { let ref: any = undefined; client.on('finishTransaction', transaction => { @@ -168,6 +181,71 @@ describe('startSpan', () => { expect(ref.spanRecorder.spans).toHaveLength(2); expect(ref.spanRecorder.spans[1].op).toEqual('db.query'); }); + + it('forks the scope', () => { + const initialScope = getCurrentScope(); + + startSpan({ name: 'GET users/[id]' }, span => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope().getSpan()).toBe(span); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(initialScope.getSpan()).toBe(undefined); + }); + }); +}); + +describe('startSpanManual', () => { + it('creates & finishes span', async () => { + startSpanManual({ name: 'GET users/[id]' }, (span, finish) => { + expect(span).toBeDefined(); + expect(span?.endTimestamp).toBeUndefined(); + finish(); + expect(span?.endTimestamp).toBeDefined(); + }); + }); + + it('forks the scope automatically', () => { + const initialScope = getCurrentScope(); + + startSpanManual({ name: 'GET users/[id]' }, (span, finish) => { + expect(getCurrentScope()).not.toBe(initialScope); + expect(getCurrentScope().getSpan()).toBe(span); + + finish(); + + // Is still the active span + expect(getCurrentScope().getSpan()).toBe(span); + }); + + expect(getCurrentScope()).toBe(initialScope); + expect(initialScope.getSpan()).toBe(undefined); + }); +}); + +describe('startInactiveSpan', () => { + it('creates & finishes span', async () => { + const span = startInactiveSpan({ name: 'GET users/[id]' }); + + expect(span).toBeDefined(); + expect(span?.endTimestamp).toBeUndefined(); + + span?.finish(); + + expect(span?.endTimestamp).toBeDefined(); + }); + + it('does not set span on scope', () => { + const initialScope = getCurrentScope(); + + const span = startInactiveSpan({ name: 'GET users/[id]' }); + + expect(initialScope.getSpan()).toBeUndefined(); + + span?.finish(); + + expect(initialScope.getSpan()).toBeUndefined(); }); }); From ef204bc89733e31474bc09b50249f321e34a49c8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 21 Dec 2023 16:04:17 +0100 Subject: [PATCH 2/2] fixes for end --- packages/core/src/tracing/trace.ts | 4 ++-- packages/core/test/lib/tracing/trace.test.ts | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index a81187139a5c..db6e34aa487e 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -97,7 +97,7 @@ export function startSpan(context: TransactionContext, callback: (span: Span scope.setSpan(activeSpan); function finishAndSetSpan(): void { - activeSpan && activeSpan.finish(); + activeSpan && activeSpan.end(); } let maybePromiseResult: T; @@ -157,7 +157,7 @@ export function startSpanManual( scope.setSpan(activeSpan); function finishAndSetSpan(): void { - activeSpan && activeSpan.finish(); + activeSpan && activeSpan.end(); } let maybePromiseResult: T; diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 86c5a33e984e..30eac02c881f 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -231,7 +231,7 @@ describe('startInactiveSpan', () => { expect(span).toBeDefined(); expect(span?.endTimestamp).toBeUndefined(); - span?.finish(); + span?.end(); expect(span?.endTimestamp).toBeDefined(); }); @@ -241,9 +241,10 @@ describe('startInactiveSpan', () => { const span = startInactiveSpan({ name: 'GET users/[id]' }); + expect(span).toBeDefined(); expect(initialScope.getSpan()).toBeUndefined(); - span?.finish(); + span?.end(); expect(initialScope.getSpan()).toBeUndefined(); });