Skip to content

Commit 368bfaa

Browse files
AbhiPrasadAbhiPrasad
authored
Revert "feat(core): Add parentSpan option to startSpan* APIs (#12567)"
This reverts commit 0019309.
1 parent 31700c9 commit 368bfaa

File tree

5 files changed

+139
-287
lines changed

5 files changed

+139
-287
lines changed

‎packages/core/src/tracing/trace.ts

Lines changed: 89 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
/* eslint-disable max-lines */
2-
31
import type { ClientOptions, Scope, SentrySpanArguments, Span, SpanTimeInput, StartSpanOptions } from '@sentry/types';
42
import { generatePropagationContext, logger, propagationContextFromHeaders } from '@sentry/utils';
53
import type { AsyncContextStrategy } from '../asyncContext/types';
@@ -34,47 +32,40 @@ const SUPPRESS_TRACING_KEY = '__SENTRY_SUPPRESS_TRACING__';
3432
* You'll always get a span passed to the callback,
3533
* it may just be a non-recording span if the span is not sampled or if tracing is disabled.
3634
*/
37-
export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) => T): T {
35+
export function startSpan<T>(context: StartSpanOptions, callback: (span: Span) => T): T {
3836
const acs = getAcs();
3937
if (acs.startSpan) {
40-
return acs.startSpan(options, callback);
38+
return acs.startSpan(context, callback);
4139
}
4240

43-
const spanArguments = parseSentrySpanArguments(options);
44-
const { forceTransaction, parentSpan: customParentSpan } = options;
45-
46-
return withScope(options.scope, () => {
47-
// If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
48-
const wrapper = getActiveSpanWrapper<T>(customParentSpan);
49-
50-
return wrapper(() => {
51-
const scope = getCurrentScope();
52-
const parentSpan = getParentSpan(scope);
53-
54-
const shouldSkipSpan = options.onlyIfParent && !parentSpan;
55-
const activeSpan = shouldSkipSpan
56-
? new SentryNonRecordingSpan()
57-
: createChildOrRootSpan({
58-
parentSpan,
59-
spanArguments,
60-
forceTransaction,
61-
scope,
62-
});
63-
64-
_setSpanForScope(scope, activeSpan);
65-
66-
return handleCallbackErrors(
67-
() => callback(activeSpan),
68-
() => {
69-
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
70-
const { status } = spanToJSON(activeSpan);
71-
if (activeSpan.isRecording() && (!status || status === 'ok')) {
72-
activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
73-
}
74-
},
75-
() => activeSpan.end(),
76-
);
77-
});
41+
const spanContext = normalizeContext(context);
42+
43+
return withScope(context.scope, scope => {
44+
const parentSpan = getParentSpan(scope);
45+
46+
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
47+
const activeSpan = shouldSkipSpan
48+
? new SentryNonRecordingSpan()
49+
: createChildOrRootSpan({
50+
parentSpan,
51+
spanContext,
52+
forceTransaction: context.forceTransaction,
53+
scope,
54+
});
55+
56+
_setSpanForScope(scope, activeSpan);
57+
58+
return handleCallbackErrors(
59+
() => callback(activeSpan),
60+
() => {
61+
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
62+
const { status } = spanToJSON(activeSpan);
63+
if (activeSpan.isRecording() && (!status || status === 'ok')) {
64+
activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
65+
}
66+
},
67+
() => activeSpan.end(),
68+
);
7869
});
7970
}
8071

@@ -88,50 +79,43 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
8879
* You'll always get a span passed to the callback,
8980
* it may just be a non-recording span if the span is not sampled or if tracing is disabled.
9081
*/
91-
export function startSpanManual<T>(options: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T {
82+
export function startSpanManual<T>(context: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T {
9283
const acs = getAcs();
9384
if (acs.startSpanManual) {
94-
return acs.startSpanManual(options, callback);
85+
return acs.startSpanManual(context, callback);
9586
}
9687

97-
const spanArguments = parseSentrySpanArguments(options);
98-
const { forceTransaction, parentSpan: customParentSpan } = options;
99-
100-
return withScope(options.scope, () => {
101-
// If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
102-
const wrapper = getActiveSpanWrapper<T>(customParentSpan);
103-
104-
return wrapper(() => {
105-
const scope = getCurrentScope();
106-
const parentSpan = getParentSpan(scope);
107-
108-
const shouldSkipSpan = options.onlyIfParent && !parentSpan;
109-
const activeSpan = shouldSkipSpan
110-
? new SentryNonRecordingSpan()
111-
: createChildOrRootSpan({
112-
parentSpan,
113-
spanArguments,
114-
forceTransaction,
115-
scope,
116-
});
117-
118-
_setSpanForScope(scope, activeSpan);
119-
120-
function finishAndSetSpan(): void {
121-
activeSpan.end();
122-
}
123-
124-
return handleCallbackErrors(
125-
() => callback(activeSpan, finishAndSetSpan),
126-
() => {
127-
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
128-
const { status } = spanToJSON(activeSpan);
129-
if (activeSpan.isRecording() && (!status || status === 'ok')) {
130-
activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
131-
}
132-
},
133-
);
134-
});
88+
const spanContext = normalizeContext(context);
89+
90+
return withScope(context.scope, scope => {
91+
const parentSpan = getParentSpan(scope);
92+
93+
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
94+
const activeSpan = shouldSkipSpan
95+
? new SentryNonRecordingSpan()
96+
: createChildOrRootSpan({
97+
parentSpan,
98+
spanContext,
99+
forceTransaction: context.forceTransaction,
100+
scope,
101+
});
102+
103+
_setSpanForScope(scope, activeSpan);
104+
105+
function finishAndSetSpan(): void {
106+
activeSpan.end();
107+
}
108+
109+
return handleCallbackErrors(
110+
() => callback(activeSpan, finishAndSetSpan),
111+
() => {
112+
// Only update the span status if it hasn't been changed yet, and the span is not yet finished
113+
const { status } = spanToJSON(activeSpan);
114+
if (activeSpan.isRecording() && (!status || status === 'ok')) {
115+
activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' });
116+
}
117+
},
118+
);
135119
});
136120
}
137121

@@ -144,39 +128,28 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
144128
* This function will always return a span,
145129
* it may just be a non-recording span if the span is not sampled or if tracing is disabled.
146130
*/
147-
export function startInactiveSpan(options: StartSpanOptions): Span {
131+
export function startInactiveSpan(context: StartSpanOptions): Span {
148132
const acs = getAcs();
149133
if (acs.startInactiveSpan) {
150-
return acs.startInactiveSpan(options);
134+
return acs.startInactiveSpan(context);
151135
}
152136

153-
const spanArguments = parseSentrySpanArguments(options);
154-
const { forceTransaction, parentSpan: customParentSpan } = options;
137+
const spanContext = normalizeContext(context);
155138

156-
// If `options.scope` is defined, we use this as as a wrapper,
157-
// If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan`
158-
const wrapper = options.scope
159-
? (callback: () => Span) => withScope(options.scope, callback)
160-
: customParentSpan
161-
? (callback: () => Span) => withActiveSpan(customParentSpan, callback)
162-
: (callback: () => Span) => callback();
139+
const scope = context.scope || getCurrentScope();
140+
const parentSpan = getParentSpan(scope);
163141

164-
return wrapper(() => {
165-
const scope = getCurrentScope();
166-
const parentSpan = getParentSpan(scope);
142+
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
167143

168-
const shouldSkipSpan = options.onlyIfParent && !parentSpan;
169-
170-
if (shouldSkipSpan) {
171-
return new SentryNonRecordingSpan();
172-
}
144+
if (shouldSkipSpan) {
145+
return new SentryNonRecordingSpan();
146+
}
173147

174-
return createChildOrRootSpan({
175-
parentSpan,
176-
spanArguments,
177-
forceTransaction,
178-
scope,
179-
});
148+
return createChildOrRootSpan({
149+
parentSpan,
150+
spanContext,
151+
forceTransaction: context.forceTransaction,
152+
scope,
180153
});
181154
}
182155

@@ -266,12 +239,12 @@ export function startNewTrace<T>(callback: () => T): T {
266239

267240
function createChildOrRootSpan({
268241
parentSpan,
269-
spanArguments,
242+
spanContext,
270243
forceTransaction,
271244
scope,
272245
}: {
273246
parentSpan: SentrySpan | undefined;
274-
spanArguments: SentrySpanArguments;
247+
spanContext: SentrySpanArguments;
275248
forceTransaction?: boolean;
276249
scope: Scope;
277250
}): Span {
@@ -283,7 +256,7 @@ function createChildOrRootSpan({
283256

284257
let span: Span;
285258
if (parentSpan && !forceTransaction) {
286-
span = _startChildSpan(parentSpan, scope, spanArguments);
259+
span = _startChildSpan(parentSpan, scope, spanContext);
287260
addChildSpanToSpan(parentSpan, span);
288261
} else if (parentSpan) {
289262
// If we forced a transaction but have a parent span, make sure to continue from the parent span, not the scope
@@ -295,7 +268,7 @@ function createChildOrRootSpan({
295268
{
296269
traceId,
297270
parentSpanId,
298-
...spanArguments,
271+
...spanContext,
299272
},
300273
scope,
301274
parentSampled,
@@ -317,7 +290,7 @@ function createChildOrRootSpan({
317290
{
318291
traceId,
319292
parentSpanId,
320-
...spanArguments,
293+
...spanContext,
321294
},
322295
scope,
323296
parentSampled,
@@ -339,17 +312,19 @@ function createChildOrRootSpan({
339312
* This converts StartSpanOptions to SentrySpanArguments.
340313
* For the most part (for now) we accept the same options,
341314
* but some of them need to be transformed.
315+
*
316+
* Eventually the StartSpanOptions will be more aligned with OpenTelemetry.
342317
*/
343-
function parseSentrySpanArguments(options: StartSpanOptions): SentrySpanArguments {
344-
const exp = options.experimental || {};
318+
function normalizeContext(context: StartSpanOptions): SentrySpanArguments {
319+
const exp = context.experimental || {};
345320
const initialCtx: SentrySpanArguments = {
346321
isStandalone: exp.standalone,
347-
...options,
322+
...context,
348323
};
349324

350-
if (options.startTime) {
325+
if (context.startTime) {
351326
const ctx: SentrySpanArguments & { startTime?: SpanTimeInput } = { ...initialCtx };
352-
ctx.startTimestamp = spanTimeInputToSeconds(options.startTime);
327+
ctx.startTimestamp = spanTimeInputToSeconds(context.startTime);
353328
delete ctx.startTime;
354329
return ctx;
355330
}
@@ -444,11 +419,3 @@ function getParentSpan(scope: Scope): SentrySpan | undefined {
444419

445420
return span;
446421
}
447-
448-
function getActiveSpanWrapper<T>(parentSpan?: Span): (callback: () => T) => T {
449-
return parentSpan
450-
? (callback: () => T) => {
451-
return withActiveSpan(parentSpan, callback);
452-
}
453-
: (callback: () => T) => callback();
454-
}

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

Lines changed: 2 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -271,17 +271,6 @@ describe('startSpan', () => {
271271
expect(getActiveSpan()).toBe(undefined);
272272
});
273273

274-
it('allows to pass a parentSpan', () => {
275-
const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' });
276-
277-
startSpan({ name: 'GET users/[id]', parentSpan }, span => {
278-
expect(getActiveSpan()).toBe(span);
279-
expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
280-
});
281-
282-
expect(getActiveSpan()).toBe(undefined);
283-
});
284-
285274
it('allows to force a transaction with forceTransaction=true', async () => {
286275
const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
287276
client = new TestClient(options);
@@ -664,13 +653,13 @@ describe('startSpanManual', () => {
664653
const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true });
665654
_setSpanForScope(manualScope, parentSpan);
666655

667-
startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => {
656+
startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => {
668657
expect(getCurrentScope()).not.toBe(initialScope);
669658
expect(getCurrentScope()).toBe(manualScope);
670659
expect(getActiveSpan()).toBe(span);
671660
expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
672661

673-
span.end();
662+
finish();
674663

675664
// Is still the active span
676665
expect(getActiveSpan()).toBe(span);
@@ -680,19 +669,6 @@ describe('startSpanManual', () => {
680669
expect(getActiveSpan()).toBe(undefined);
681670
});
682671

683-
it('allows to pass a parentSpan', () => {
684-
const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' });
685-
686-
startSpanManual({ name: 'GET users/[id]', parentSpan }, span => {
687-
expect(getActiveSpan()).toBe(span);
688-
expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
689-
690-
span.end();
691-
});
692-
693-
expect(getActiveSpan()).toBe(undefined);
694-
});
695-
696672
it('allows to force a transaction with forceTransaction=true', async () => {
697673
const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
698674
client = new TestClient(options);
@@ -1001,19 +977,6 @@ describe('startInactiveSpan', () => {
1001977
expect(getActiveSpan()).toBeUndefined();
1002978
});
1003979

1004-
it('allows to pass a parentSpan', () => {
1005-
const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' });
1006-
1007-
const span = startInactiveSpan({ name: 'GET users/[id]', parentSpan });
1008-
1009-
expect(spanToJSON(span).parent_span_id).toBe('parent-span-id');
1010-
expect(getActiveSpan()).toBe(undefined);
1011-
1012-
span.end();
1013-
1014-
expect(getActiveSpan()).toBeUndefined();
1015-
});
1016-
1017980
it('allows to force a transaction with forceTransaction=true', async () => {
1018981
const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 });
1019982
client = new TestClient(options);

0 commit comments

Comments
 (0)