Skip to content

Commit

Permalink
fix: Fork scope and keep async context within startSpan and `startS…
Browse files Browse the repository at this point in the history
…panManual` (#10413)
  • Loading branch information
lforst authored Jan 30, 2024
1 parent 71b8ac1 commit 9fe67aa
Show file tree
Hide file tree
Showing 2 changed files with 202 additions and 47 deletions.
99 changes: 52 additions & 47 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/ut
import { DEBUG_BUILD } from '../debug-build';
import { getCurrentScope, withScope } from '../exports';
import type { Hub } from '../hub';
import { runWithAsyncContext } from '../hub';
import { getIsolationScope } from '../hub';
import { getCurrentHub } from '../hub';
import { handleCallbackErrors } from '../utils/handleCallbackErrors';
Expand Down Expand Up @@ -74,31 +75,33 @@ export function trace<T>(
export function startSpan<T>(context: StartSpanOptions, callback: (span: Span | undefined) => T): T {
const ctx = normalizeContext(context);

return withScope(context.scope, scope => {
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();
return runWithAsyncContext(() => {
return withScope(context.scope, scope => {
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();

const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx);
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx);

// eslint-disable-next-line deprecation/deprecation
scope.setSpan(activeSpan);

return handleCallbackErrors(
() => callback(activeSpan),
() => {
// Only update the span status if it hasn't been changed yet
if (activeSpan) {
const { status } = spanToJSON(activeSpan);
if (!status || status === 'ok') {
activeSpan.setStatus('internal_error');
// eslint-disable-next-line deprecation/deprecation
scope.setSpan(activeSpan);

return handleCallbackErrors(
() => callback(activeSpan),
() => {
// Only update the span status if it hasn't been changed yet
if (activeSpan) {
const { status } = spanToJSON(activeSpan);
if (!status || status === 'ok') {
activeSpan.setStatus('internal_error');
}
}
}
},
() => activeSpan && activeSpan.end(),
);
},
() => activeSpan && activeSpan.end(),
);
});
});
}

Expand All @@ -124,34 +127,36 @@ export function startSpanManual<T>(
): T {
const ctx = normalizeContext(context);

return withScope(context.scope, scope => {
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();

const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx);
return runWithAsyncContext(() => {
return withScope(context.scope, scope => {
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();
// eslint-disable-next-line deprecation/deprecation
const parentSpan = scope.getSpan();

// eslint-disable-next-line deprecation/deprecation
scope.setSpan(activeSpan);
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx);

function finishAndSetSpan(): void {
activeSpan && activeSpan.end();
}

return handleCallbackErrors(
() => callback(activeSpan, finishAndSetSpan),
() => {
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
if (activeSpan && activeSpan.isRecording()) {
const { status } = spanToJSON(activeSpan);
if (!status || status === 'ok') {
activeSpan.setStatus('internal_error');
// eslint-disable-next-line deprecation/deprecation
scope.setSpan(activeSpan);

function finishAndSetSpan(): void {
activeSpan && activeSpan.end();
}

return handleCallbackErrors(
() => callback(activeSpan, finishAndSetSpan),
() => {
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
if (activeSpan && activeSpan.isRecording()) {
const { status } = spanToJSON(activeSpan);
if (!status || status === 'ok') {
activeSpan.setStatus('internal_error');
}
}
}
},
);
},
);
});
});
}

Expand Down
150 changes: 150 additions & 0 deletions packages/node/test/performance.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
import { setAsyncContextStrategy, setCurrentClient, startSpan, startSpanManual } from '@sentry/core';
import type { TransactionEvent } from '@sentry/types';
import { NodeClient, defaultStackParser } from '../src';
import { setNodeAsyncContextStrategy } from '../src/async';
import { getDefaultNodeClientOptions } from './helper/node-client-options';

const dsn = 'https://[email protected]/4291';

beforeAll(() => {
setNodeAsyncContextStrategy();
});

afterAll(() => {
setAsyncContextStrategy(undefined);
});

describe('startSpan()', () => {
it('should correctly separate spans when called after one another with interwoven timings', async () => {
const transactionEventPromise = new Promise<TransactionEvent>(resolve => {
setCurrentClient(
new NodeClient(
getDefaultNodeClientOptions({
stackParser: defaultStackParser,
tracesSampleRate: 1,
beforeSendTransaction: event => {
resolve(event);
return null;
},
dsn,
}),
),
);
});

startSpan({ name: 'first' }, () => {
return new Promise<void>(resolve => {
setTimeout(resolve, 500);
});
});

startSpan({ name: 'second' }, () => {
return new Promise<void>(resolve => {
setTimeout(resolve, 250);
});
});

const transactionEvent = await transactionEventPromise;

// Any transaction events happening shouldn't have any child spans
expect(transactionEvent.spans).toStrictEqual([]);
});

it('should correctly nest spans when called within one another', async () => {
const transactionEventPromise = new Promise<TransactionEvent>(resolve => {
setCurrentClient(
new NodeClient(
getDefaultNodeClientOptions({
stackParser: defaultStackParser,
tracesSampleRate: 1,
beforeSendTransaction: event => {
resolve(event);
return null;
},
dsn,
}),
),
);
});

startSpan({ name: 'first' }, () => {
startSpan({ name: 'second' }, () => undefined);
});

const transactionEvent = await transactionEventPromise;

expect(transactionEvent.spans).toContainEqual(expect.objectContaining({ description: 'second' }));
});
});

describe('startSpanManual()', () => {
it('should correctly separate spans when called after one another with interwoven timings', async () => {
const transactionEventPromise = new Promise<TransactionEvent>(resolve => {
setCurrentClient(
new NodeClient(
getDefaultNodeClientOptions({
stackParser: defaultStackParser,
tracesSampleRate: 1,
beforeSendTransaction: event => {
resolve(event);
return null;
},
dsn,
}),
),
);
});

startSpanManual({ name: 'first' }, span => {
return new Promise<void>(resolve => {
setTimeout(() => {
span?.end();
resolve();
}, 500);
});
});

startSpanManual({ name: 'second' }, span => {
return new Promise<void>(resolve => {
setTimeout(() => {
span?.end();
resolve();
}, 500);
});
});

const transactionEvent = await transactionEventPromise;

// Any transaction events happening shouldn't have any child spans
expect(transactionEvent.spans).toStrictEqual([]);
});

it('should correctly nest spans when called within one another', async () => {
const transactionEventPromise = new Promise<TransactionEvent>(resolve => {
setCurrentClient(
new NodeClient(
getDefaultNodeClientOptions({
stackParser: defaultStackParser,
tracesSampleRate: 1,
beforeSendTransaction: event => {
resolve(event);
return null;
},
dsn,
}),
),
);
});

startSpanManual({ name: 'first' }, span1 => {
startSpanManual({ name: 'second' }, span2 => {
span2?.end();
});
span1?.end();
});

const transactionEvent = await transactionEventPromise;

expect(transactionEvent.spans).toContainEqual(expect.objectContaining({ description: 'second' }));
});
});

0 comments on commit 9fe67aa

Please sign in to comment.