Skip to content

Commit 1297420

Browse files
andreiborzaclaude
andauthored
ref(opentelemetry): Remove startSpan overrides from async context strategy (#23198)
## What There is now only one implementation of `startSpan`, `startSpanManual` and `startInactiveSpan`: the one in core. The separate OpenTelemetry copies and the ACS overrides for them are removed. - Core asks the async context strategy for the current active span and for making the new span active, so Node answers from the OTel context and the browser keeps using the scope - Core continues incoming (remote) traces itself when the resolved parent is a remote span - Removes `_INTERNAL_startInactiveSpan`, the `SentryTracer` now uses the public `startInactiveSpan` Behavior change: a `scope` passed to the startSpan APIs now resolves the parent from that scope. In Node, a scope without a bound context starts a root span instead of falling back to the ambient active span (consistent with browser behavior and the design of #23222). ## Why The OpenTelemetry copies created spans through core anyway and only added context bookkeeping around it. One shared code path removes the double indirection and a lot of duplicated code. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 9e7cd74 commit 1297420

19 files changed

Lines changed: 213 additions & 495 deletions

File tree

packages/core/src/asyncContext/types.ts

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,7 @@ import type { getTraceData } from '../utils/traceData';
33
import type {
44
continueTrace,
55
isTracingSuppressed,
6-
startInactiveSpan,
76
startNewTrace,
8-
startSpan,
9-
startSpanManual,
107
suppressTracing,
118
withActiveSpan,
129
} from './../tracing/trace';
@@ -63,15 +60,6 @@ export interface AsyncContextStrategy {
6360
// OPTIONAL: Custom tracing methods
6461
// These are used so that we can provide OTEL-based implementations
6562

66-
/** Start an active span. */
67-
startSpan?: typeof startSpan;
68-
69-
/** Start an inactive span. */
70-
startInactiveSpan?: typeof startInactiveSpan;
71-
72-
/** Start an active manual span. */
73-
startSpanManual?: typeof startSpanManual;
74-
7563
/** Get the currently active span. */
7664
getActiveSpan?: typeof getActiveSpan;
7765

packages/core/src/client.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,17 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
648648
*/
649649
public on(hook: 'spanStart', callback: (span: Span) => void): () => void;
650650

651+
/**
652+
* Register a callback that can adjust the scope and the parent span right before a span is
653+
* created. Listeners mutate the passed `spanScope` object. The Node SDK uses this to continue
654+
* the trace of a remote (incoming) parent through the propagation context of a forked scope.
655+
* @returns {() => void} A function that, when executed, removes the registered callback.
656+
*/
657+
public on(
658+
hook: 'prepareSpanScope',
659+
callback: (spanScope: { scope: Scope; parentSpan: Span | undefined }) => void,
660+
): () => void;
661+
651662
/**
652663
* Register a callback before span sampling runs. Receives a `samplingDecision` object argument with a `decision`
653664
* property that can be used to make a sampling decision that will be enforced, before any span sampling runs.
@@ -986,6 +997,9 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
986997
/** Fire a hook whenever a span starts. */
987998
public emit(hook: 'spanStart', span: Span): void;
988999

1000+
/** A hook that is called right before a span is created; listeners mutate the passed object. */
1001+
public emit(hook: 'prepareSpanScope', spanScope: { scope: Scope; parentSpan: Span | undefined }): void;
1002+
9891003
/** A hook that is called every time before a span is sampled. */
9901004
public emit(
9911005
hook: 'beforeSampling',

packages/core/src/tracing/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ export {
2020
export {
2121
startSpan,
2222
startInactiveSpan,
23-
_INTERNAL_startInactiveSpan,
2423
startSpanManual,
2524
continueTrace,
2625
withActiveSpan,

packages/core/src/tracing/trace.ts

Lines changed: 33 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import { sampleSpan } from './sampling';
3939
import { SentryNonRecordingSpan, spanIsNonRecordingSpan } from './sentryNonRecordingSpan';
4040
import { SentrySpan } from './sentrySpan';
4141
import { SPAN_STATUS_ERROR } from './spanstatus';
42-
import { setCapturedScopesOnSpan } from './utils';
42+
import { getCapturedScopesOnSpan, setCapturedScopesOnSpan } from './utils';
4343
import type { Client } from '../client';
4444
import { SUPPRESS_TRACING_KEY } from './constants';
4545

@@ -54,11 +54,6 @@ import { SUPPRESS_TRACING_KEY } from './constants';
5454
* it may just be a non-recording span if the span is not sampled or if tracing is disabled.
5555
*/
5656
export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) => T): T {
57-
const acs = getAcs();
58-
if (acs.startSpan) {
59-
return acs.startSpan(options, callback);
60-
}
61-
6257
const spanArguments = parseSentrySpanArguments(options);
6358
const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options;
6459

@@ -72,7 +67,7 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
7267

7368
return wrapper(() => {
7469
const scope = getCurrentScope();
75-
const parentSpan = getParentSpan(scope, customParentSpan);
70+
const parentSpan = getParentSpan(customScope ?? scope, customParentSpan);
7671
const client = getClient();
7772

7873
const missingRequiredParent = options.onlyIfParent && !parentSpan;
@@ -111,11 +106,6 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
111106
* it may just be a non-recording span if the span is not sampled or if tracing is disabled.
112107
*/
113108
export function startSpanManual<T>(options: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T {
114-
const acs = getAcs();
115-
if (acs.startSpanManual) {
116-
return acs.startSpanManual(options, callback);
117-
}
118-
119109
const spanArguments = parseSentrySpanArguments(options);
120110
const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options;
121111

@@ -127,7 +117,7 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
127117

128118
return wrapper(() => {
129119
const scope = getCurrentScope();
130-
const parentSpan = getParentSpan(scope, customParentSpan);
120+
const parentSpan = getParentSpan(customScope ?? scope, customParentSpan);
131121

132122
const missingRequiredParent = options.onlyIfParent && !parentSpan;
133123
const activeSpan = missingRequiredParent
@@ -162,39 +152,20 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
162152
* it may just be a non-recording span if the span is not sampled or if tracing is disabled.
163153
*/
164154
export function startInactiveSpan(options: StartSpanOptions): Span {
165-
const acs = getAcs();
166-
if (acs.startInactiveSpan) {
167-
return acs.startInactiveSpan(options);
168-
}
169-
170-
return _startInactiveSpanImpl(options);
171-
}
172-
173-
/**
174-
* Internal version of startInactiveSpan that bypasses the ACS check.
175-
* Used by SentryTracerProvider to create spans without triggering recursion
176-
* through ACS overrides.
177-
* @hidden
178-
*/
179-
export function _INTERNAL_startInactiveSpan(options: StartSpanOptions): Span {
180-
return _startInactiveSpanImpl(options);
181-
}
182-
183-
function _startInactiveSpanImpl(options: StartSpanOptions): Span {
184155
const spanArguments = parseSentrySpanArguments(options);
185-
const { forceTransaction, parentSpan: customParentSpan } = options;
156+
const { forceTransaction, parentSpan: customParentSpan, scope: customScope } = options;
186157

187158
// If `options.scope` is defined, we use this as as a wrapper,
188159
// If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
189-
const wrapper = options.scope
190-
? (callback: () => Span) => withScope(options.scope, callback)
160+
const wrapper = customScope
161+
? (callback: () => Span) => withScope(customScope, callback)
191162
: customParentSpan !== undefined
192163
? (callback: () => Span) => withActiveSpan(customParentSpan, callback)
193164
: (callback: () => Span) => callback();
194165

195166
return wrapper(() => {
196167
const scope = getCurrentScope();
197-
const parentSpan = getParentSpan(scope, customParentSpan);
168+
const parentSpan = getParentSpan(customScope ?? scope, customParentSpan);
198169
const client = getClient();
199170

200171
const missingRequiredParent = options.onlyIfParent && !parentSpan;
@@ -342,10 +313,10 @@ function startMissingRequiredParentSpan(scope: Scope, client: Client | undefined
342313
}
343314

344315
function createChildOrRootSpan({
345-
parentSpan,
316+
parentSpan: resolvedParentSpan,
346317
spanArguments,
347318
forceTransaction,
348-
scope,
319+
scope: currentScope,
349320
}: {
350321
parentSpan: Span | undefined;
351322
spanArguments: SentrySpanArguments;
@@ -354,6 +325,16 @@ function createChildOrRootSpan({
354325
}): Span {
355326
const isolationScope = getIsolationScope();
356327

328+
// Listeners can adjust the scope and the parent right before span creation. The Node SDK uses
329+
// this to turn a remote parent (an incoming trace on the ambient OTel context) into a propagation
330+
// context on a forked scope, so the span continues the incoming trace as a root span.
331+
const spanScope: { scope: Scope; parentSpan: Span | undefined } = {
332+
scope: currentScope,
333+
parentSpan: resolvedParentSpan,
334+
};
335+
getClient()?.emit('prepareSpanScope', spanScope);
336+
const { scope, parentSpan } = spanScope;
337+
357338
if (!hasSpansEnabled()) {
358339
const scopePropagationContext = scope.getPropagationContext();
359340
const traceId = parentSpan ? parentSpan.spanContext().traceId : scopePropagationContext.traceId;
@@ -486,6 +467,11 @@ function getAcs(): AsyncContextStrategy {
486467
return getAsyncContextStrategy(carrier);
487468
}
488469

470+
/**
471+
* Runs the callback with the span active. When the async context strategy bridges to an ambient
472+
* context (OTel), activation must go through it so the span lands on that context and
473+
* instrumentation-created child spans nest under it; the scope alone is not consulted there.
474+
*/
489475
function _startRootSpan(
490476
spanArguments: SentrySpanArguments,
491477
scope: Scope,
@@ -666,8 +652,16 @@ function runCallback<T>(span: Span, makeSpanActive: boolean, callback: () => T,
666652
const wrapper = makeSpanActive
667653
? (callback: () => T) => {
668654
return withActiveSpan(span, () => {
655+
const scope = getCurrentScope();
656+
// The fork made by withActiveSpan is based on the ambient scope. Carry over the
657+
// propagation context captured at span creation, which can continue a remote parent's
658+
// trace the ambient scope knows nothing about. For local parents this is a no-op.
659+
const creationScope = getCapturedScopesOnSpan(span).scope;
660+
if (creationScope) {
661+
scope.setPropagationContext(creationScope.getPropagationContext());
662+
}
669663
// Make sure the correct scope is captured on the span, since withActiveSpan forks the scope
670-
setCapturedScopesOnSpan(span, getCurrentScope(), getIsolationScope());
664+
setCapturedScopesOnSpan(span, scope, getIsolationScope());
671665
return callback();
672666
});
673667
}

packages/core/test/lib/tracing/trace.test.ts

Lines changed: 38 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import { SentryNonRecordingSpan } from '../../../src/tracing/sentryNonRecordingS
3131
import { startNewTrace } from '../../../src/tracing/trace';
3232
import type { Event } from '../../../src/types/event';
3333
import type { Span } from '../../../src/types/span';
34-
import type { StartSpanOptions } from '../../../src/types/startSpanOptions';
3534
import { _setSpanForScope } from '../../../src/utils/spanOnScope';
3635
import { getActiveSpan, getRootSpan, getSpanDescendants, spanIsSampled } from '../../../src/utils/spanUtils';
3736
import { getDefaultTestClientOptions, TestClient } from '../../mocks/client';
@@ -888,30 +887,6 @@ describe('startSpan', () => {
888887
});
889888
});
890889
});
891-
892-
it('uses implementation from ACS, if it exists', () => {
893-
const staticSpan = new SentrySpan({ spanId: 'aha', sampled: true });
894-
895-
const carrier = getMainCarrier();
896-
897-
const customFn = vi.fn((_options: StartSpanOptions, callback: (span: Span) => string) => {
898-
callback(staticSpan);
899-
return 'aha';
900-
}) as typeof startSpan;
901-
902-
const acs = {
903-
...getAsyncContextStrategy(carrier),
904-
startSpan: customFn,
905-
};
906-
setAsyncContextStrategy(acs);
907-
908-
const result = startSpan({ name: 'GET users/[id]' }, span => {
909-
expect(span).toEqual(staticSpan);
910-
return 'oho?';
911-
});
912-
913-
expect(result).toBe('aha');
914-
});
915890
});
916891

917892
describe('startSpanManual', () => {
@@ -1406,30 +1381,6 @@ describe('startSpanManual', () => {
14061381
});
14071382
});
14081383
});
1409-
1410-
it('uses implementation from ACS, if it exists', () => {
1411-
const staticSpan = new SentrySpan({ spanId: 'aha', sampled: true });
1412-
1413-
const carrier = getMainCarrier();
1414-
1415-
const customFn = vi.fn((_options: StartSpanOptions, callback: (span: Span) => string) => {
1416-
callback(staticSpan);
1417-
return 'aha';
1418-
}) as unknown as typeof startSpanManual;
1419-
1420-
const acs = {
1421-
...getAsyncContextStrategy(carrier),
1422-
startSpanManual: customFn,
1423-
};
1424-
setAsyncContextStrategy(acs);
1425-
1426-
const result = startSpanManual({ name: 'GET users/[id]' }, span => {
1427-
expect(span).toEqual(staticSpan);
1428-
return 'oho?';
1429-
});
1430-
1431-
expect(result).toBe('aha');
1432-
});
14331384
});
14341385

14351386
describe('startInactiveSpan', () => {
@@ -1446,6 +1397,42 @@ describe('startInactiveSpan', () => {
14461397
client.init();
14471398
});
14481399

1400+
it('includes the scope the span was started on when finished', async () => {
1401+
const beforeSendTransaction = vi.fn(event => event);
1402+
1403+
const options = getDefaultTestClientOptions({ tracesSampleRate: 1, beforeSendTransaction });
1404+
client = new TestClient(options);
1405+
setCurrentClient(client);
1406+
client.init();
1407+
1408+
let span: Span | undefined;
1409+
1410+
withScope(scope => {
1411+
scope.setTag('scope', 1);
1412+
span = startInactiveSpan({ name: 'my-span' });
1413+
// The span captures the scope it was started on, so later mutations of that scope
1414+
// are reflected on the transaction.
1415+
scope.setTag('scope_after_span', 2);
1416+
});
1417+
1418+
withScope(scope => {
1419+
scope.setTag('scope', 2);
1420+
span?.end();
1421+
});
1422+
1423+
await client.flush();
1424+
1425+
expect(beforeSendTransaction).toHaveBeenCalledTimes(1);
1426+
expect(beforeSendTransaction).toHaveBeenCalledWith(
1427+
expect.objectContaining({
1428+
// The span-start scope is captured, including `scope_after_span` (set on the same scope
1429+
// after span start), but not `scope: 2` (a different scope active at `end()`).
1430+
tags: { scope: 1, scope_after_span: 2 },
1431+
}),
1432+
expect.anything(),
1433+
);
1434+
});
1435+
14491436
it('returns a non recording span if tracing is disabled', () => {
14501437
const options = getDefaultTestClientOptions({});
14511438
client = new TestClient(options);
@@ -1897,25 +1884,6 @@ describe('startInactiveSpan', () => {
18971884
expect(childSpans).toContain(innerSpan);
18981885
});
18991886
});
1900-
1901-
it('uses implementation from ACS, if it exists', () => {
1902-
const staticSpan = new SentrySpan({ spanId: 'aha', sampled: true });
1903-
1904-
const carrier = getMainCarrier();
1905-
1906-
const customFn = vi.fn((_options: StartSpanOptions) => {
1907-
return staticSpan;
1908-
}) as unknown as typeof startInactiveSpan;
1909-
1910-
const acs = {
1911-
...getAsyncContextStrategy(carrier),
1912-
startInactiveSpan: customFn,
1913-
};
1914-
setAsyncContextStrategy(acs);
1915-
1916-
const result = startInactiveSpan({ name: 'GET users/[id]' });
1917-
expect(result).toBe(staticSpan);
1918-
});
19191887
});
19201888

19211889
describe('continueTrace', () => {
@@ -2378,6 +2346,8 @@ describe('span hooks', () => {
23782346
beforeEach(() => {
23792347
resetGlobals();
23802348

2349+
setAsyncContextStrategy(undefined);
2350+
23812351
const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
23822352
client = new TestClient(options);
23832353
setCurrentClient(client);

packages/node/src/sdk/client.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,11 @@ import {
1111
SDK_VERSION,
1212
ServerRuntimeClient,
1313
} from '@sentry/core';
14-
import { type AsyncLocalStorageLookup, type SentryTracerProvider } from '@sentry/opentelemetry';
14+
import {
15+
type AsyncLocalStorageLookup,
16+
registerPrepareSpanScope,
17+
type SentryTracerProvider,
18+
} from '@sentry/opentelemetry';
1519
import { isMainThread, threadId } from 'worker_threads';
1620
import { DEBUG_BUILD } from '../debug-build';
1721
import type { NodeClientOptions } from '../types';
@@ -72,6 +76,10 @@ export class NodeClient extends ServerRuntimeClient<NodeClientOptions> {
7276
// provider path produce OTel spans that never reach `SentrySpan`, so the strategy is simply never
7377
// consulted for them.
7478
_INTERNAL_setDeferSegmentSpanCapture(this);
79+
80+
// Same constructor anchoring as above: every client must continue incoming (remote) traces,
81+
// also manually constructed ones that never run `initOtel`.
82+
registerPrepareSpanScope(this);
7583
}
7684

7785
/** Get the OTEL tracer. */

0 commit comments

Comments
 (0)