Skip to content

Commit 4592d98

Browse files
mydeamydea
committed
feat(browser): Create spans as children of root span by default
You can opt out of this by setting `parentSpanIsAlwaysRootSpan=false` in your client options. update nested span handling in svelte
1 parent 15d6cb1 commit 4592d98

File tree

6 files changed

+204
-46
lines changed

6 files changed

+204
-46
lines changed

‎packages/browser/src/client.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,17 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
5252
* @param options Configuration options for this SDK.
5353
*/
5454
public constructor(options: BrowserClientOptions) {
55+
const opts = {
56+
// We default this to true, as it is the safer scenario
57+
parentSpanIsAlwaysRootSpan: true,
58+
...options,
59+
};
5560
const sdkSource = WINDOW.SENTRY_SDK_SOURCE || getSDKSource();
56-
applySdkMetadata(options, 'browser', ['browser'], sdkSource);
61+
applySdkMetadata(opts, 'browser', ['browser'], sdkSource);
5762

58-
super(options);
63+
super(opts);
5964

60-
if (options.sendClientReports && WINDOW.document) {
65+
if (opts.sendClientReports && WINDOW.document) {
6166
WINDOW.document.addEventListener('visibilitychange', () => {
6267
if (WINDOW.document.visibilityState === 'hidden') {
6368
this._flushOutcomes();

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

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import type { ClientOptions, Scope, SentrySpanArguments, Span, SpanTimeInput, StartSpanOptions } from '@sentry/types';
2-
32
import { propagationContextFromHeaders } from '@sentry/utils';
43
import type { AsyncContextStrategy } from '../asyncContext';
54
import { getMainCarrier } from '../asyncContext';
@@ -11,13 +10,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE
1110
import { handleCallbackErrors } from '../utils/handleCallbackErrors';
1211
import { hasTracingEnabled } from '../utils/hasTracingEnabled';
1312
import { _getSpanForScope, _setSpanForScope } from '../utils/spanOnScope';
14-
import {
15-
addChildSpanToSpan,
16-
getActiveSpan,
17-
spanIsSampled,
18-
spanTimeInputToSeconds,
19-
spanToJSON,
20-
} from '../utils/spanUtils';
13+
import { addChildSpanToSpan, getRootSpan, spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';
2114
import { freezeDscOnSpan, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
2215
import { logSpanStart } from './logSpans';
2316
import { sampleSpan } from './sampling';
@@ -47,7 +40,7 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span) =
4740
const spanContext = normalizeContext(context);
4841

4942
return withScope(context.scope, scope => {
50-
const parentSpan = _getSpanForScope(scope) as SentrySpan | undefined;
43+
const parentSpan = getParentSpan(scope);
5144

5245
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
5346
const activeSpan = shouldSkipSpan
@@ -94,7 +87,7 @@ export function startSpanManual<T>(context: StartSpanOptions, callback: (span: S
9487
const spanContext = normalizeContext(context);
9588

9689
return withScope(context.scope, scope => {
97-
const parentSpan = _getSpanForScope(scope) as SentrySpan | undefined;
90+
const parentSpan = getParentSpan(scope);
9891

9992
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
10093
const activeSpan = shouldSkipSpan
@@ -141,18 +134,16 @@ export function startInactiveSpan(context: StartSpanOptions): Span {
141134
}
142135

143136
const spanContext = normalizeContext(context);
144-
const parentSpan = context.scope
145-
? (_getSpanForScope(context.scope) as SentrySpan | undefined)
146-
: (getActiveSpan() as SentrySpan | undefined);
137+
138+
const scope = context.scope || getCurrentScope();
139+
const parentSpan = getParentSpan(scope);
147140

148141
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
149142

150143
if (shouldSkipSpan) {
151144
return new SentryNonRecordingSpan();
152145
}
153146

154-
const scope = context.scope || getCurrentScope();
155-
156147
return createChildSpanOrTransaction({
157148
parentSpan,
158149
spanContext,
@@ -381,3 +372,19 @@ function _startChildSpan(parentSpan: Span, scope: Scope, spanArguments: SentrySp
381372

382373
return childSpan;
383374
}
375+
376+
function getParentSpan(scope: Scope): SentrySpan | undefined {
377+
const span = _getSpanForScope(scope) as SentrySpan | undefined;
378+
379+
if (!span) {
380+
return undefined;
381+
}
382+
383+
const client = getClient();
384+
const options: Partial<ClientOptions> = client ? client.getOptions() : {};
385+
if (options.parentSpanIsAlwaysRootSpan) {
386+
return getRootSpan(span) as SentrySpan;
387+
}
388+
389+
return span;
390+
}

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

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,48 @@ describe('startSpan', () => {
417417
});
418418
});
419419

420+
describe('parentSpanIsAlwaysRootSpan', () => {
421+
it('creates a span as child of root span if parentSpanIsAlwaysRootSpan=true', () => {
422+
const options = getDefaultTestClientOptions({
423+
tracesSampleRate: 1,
424+
parentSpanIsAlwaysRootSpan: true,
425+
});
426+
client = new TestClient(options);
427+
setCurrentClient(client);
428+
client.init();
429+
430+
startSpan({ name: 'parent span' }, span => {
431+
expect(spanToJSON(span).parent_span_id).toBe(undefined);
432+
startSpan({ name: 'child span' }, childSpan => {
433+
expect(spanToJSON(childSpan).parent_span_id).toBe(span.spanContext().spanId);
434+
startSpan({ name: 'grand child span' }, grandChildSpan => {
435+
expect(spanToJSON(grandChildSpan).parent_span_id).toBe(span.spanContext().spanId);
436+
});
437+
});
438+
});
439+
});
440+
441+
it('does not creates a span as child of root span if parentSpanIsAlwaysRootSpan=false', () => {
442+
const options = getDefaultTestClientOptions({
443+
tracesSampleRate: 1,
444+
parentSpanIsAlwaysRootSpan: false,
445+
});
446+
client = new TestClient(options);
447+
setCurrentClient(client);
448+
client.init();
449+
450+
startSpan({ name: 'parent span' }, span => {
451+
expect(spanToJSON(span).parent_span_id).toBe(undefined);
452+
startSpan({ name: 'child span' }, childSpan => {
453+
expect(spanToJSON(childSpan).parent_span_id).toBe(span.spanContext().spanId);
454+
startSpan({ name: 'grand child span' }, grandChildSpan => {
455+
expect(spanToJSON(grandChildSpan).parent_span_id).toBe(childSpan.spanContext().spanId);
456+
});
457+
});
458+
});
459+
});
460+
});
461+
420462
it('samples with a tracesSampler', () => {
421463
const tracesSampler = jest.fn(() => {
422464
return true;
@@ -751,6 +793,54 @@ describe('startSpanManual', () => {
751793
});
752794
});
753795

796+
describe('parentSpanIsAlwaysRootSpan', () => {
797+
it('creates a span as child of root span if parentSpanIsAlwaysRootSpan=true', () => {
798+
const options = getDefaultTestClientOptions({
799+
tracesSampleRate: 1,
800+
parentSpanIsAlwaysRootSpan: true,
801+
});
802+
client = new TestClient(options);
803+
setCurrentClient(client);
804+
client.init();
805+
806+
startSpanManual({ name: 'parent span' }, span => {
807+
expect(spanToJSON(span).parent_span_id).toBe(undefined);
808+
startSpanManual({ name: 'child span' }, childSpan => {
809+
expect(spanToJSON(childSpan).parent_span_id).toBe(span.spanContext().spanId);
810+
startSpanManual({ name: 'grand child span' }, grandChildSpan => {
811+
expect(spanToJSON(grandChildSpan).parent_span_id).toBe(span.spanContext().spanId);
812+
grandChildSpan.end();
813+
});
814+
childSpan.end();
815+
});
816+
span.end();
817+
});
818+
});
819+
820+
it('does not creates a span as child of root span if parentSpanIsAlwaysRootSpan=false', () => {
821+
const options = getDefaultTestClientOptions({
822+
tracesSampleRate: 1,
823+
parentSpanIsAlwaysRootSpan: false,
824+
});
825+
client = new TestClient(options);
826+
setCurrentClient(client);
827+
client.init();
828+
829+
startSpanManual({ name: 'parent span' }, span => {
830+
expect(spanToJSON(span).parent_span_id).toBe(undefined);
831+
startSpanManual({ name: 'child span' }, childSpan => {
832+
expect(spanToJSON(childSpan).parent_span_id).toBe(span.spanContext().spanId);
833+
startSpanManual({ name: 'grand child span' }, grandChildSpan => {
834+
expect(spanToJSON(grandChildSpan).parent_span_id).toBe(childSpan.spanContext().spanId);
835+
grandChildSpan.end();
836+
});
837+
childSpan.end();
838+
});
839+
span.end();
840+
});
841+
});
842+
});
843+
754844
it('sets a child span reference on the parent span', () => {
755845
expect.assertions(1);
756846
startSpan({ name: 'outer' }, (outerSpan: any) => {
@@ -995,6 +1085,64 @@ describe('startInactiveSpan', () => {
9951085
});
9961086
});
9971087

1088+
describe('parentSpanIsAlwaysRootSpan', () => {
1089+
it('creates a span as child of root span if parentSpanIsAlwaysRootSpan=true', () => {
1090+
const options = getDefaultTestClientOptions({
1091+
tracesSampleRate: 1,
1092+
parentSpanIsAlwaysRootSpan: true,
1093+
});
1094+
client = new TestClient(options);
1095+
setCurrentClient(client);
1096+
client.init();
1097+
1098+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1099+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(undefined);
1100+
1101+
startSpan({ name: 'parent span' }, span => {
1102+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1103+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(span.spanContext().spanId);
1104+
1105+
startSpan({ name: 'child span' }, () => {
1106+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1107+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(span.spanContext().spanId);
1108+
1109+
startSpan({ name: 'grand child span' }, () => {
1110+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1111+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(span.spanContext().spanId);
1112+
});
1113+
});
1114+
});
1115+
});
1116+
1117+
it('does not creates a span as child of root span if parentSpanIsAlwaysRootSpan=false', () => {
1118+
const options = getDefaultTestClientOptions({
1119+
tracesSampleRate: 1,
1120+
parentSpanIsAlwaysRootSpan: false,
1121+
});
1122+
client = new TestClient(options);
1123+
setCurrentClient(client);
1124+
client.init();
1125+
1126+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1127+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(undefined);
1128+
1129+
startSpan({ name: 'parent span' }, span => {
1130+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1131+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(span.spanContext().spanId);
1132+
1133+
startSpan({ name: 'child span' }, childSpan => {
1134+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1135+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(childSpan.spanContext().spanId);
1136+
1137+
startSpan({ name: 'grand child span' }, grandChildSpan => {
1138+
const inactiveSpan = startInactiveSpan({ name: 'inactive span' });
1139+
expect(spanToJSON(inactiveSpan).parent_span_id).toBe(grandChildSpan.spanContext().spanId);
1140+
});
1141+
});
1142+
});
1143+
});
1144+
});
1145+
9981146
it('includes the scope at the time the span was started when finished', async () => {
9991147
const beforeSendTransaction = jest.fn(event => event);
10001148

‎packages/svelte/src/performance.ts

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import type { Span } from '@sentry/types';
33
import { afterUpdate, beforeUpdate, onMount } from 'svelte';
44
import { current_component } from 'svelte/internal';
55

6-
import { getRootSpan, startInactiveSpan, withActiveSpan } from '@sentry/core';
6+
import { startInactiveSpan } from '@sentry/core';
77
import { DEFAULT_COMPONENT_NAME, UI_SVELTE_INIT, UI_SVELTE_UPDATE } from './constants';
88
import type { TrackComponentOptions } from './types';
99

@@ -34,17 +34,16 @@ export function trackComponent(options?: TrackComponentOptions): void {
3434
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
3535
const componentName = `<${customComponentName || current_component.constructor.name || DEFAULT_COMPONENT_NAME}>`;
3636

37-
let initSpan: Span | undefined = undefined;
3837
if (mergedOptions.trackInit) {
39-
initSpan = recordInitSpan(componentName);
38+
recordInitSpan(componentName);
4039
}
4140

4241
if (mergedOptions.trackUpdates) {
43-
recordUpdateSpans(componentName, initSpan);
42+
recordUpdateSpans(componentName);
4443
}
4544
}
4645

47-
function recordInitSpan(componentName: string): Span | undefined {
46+
function recordInitSpan(componentName: string): void {
4847
const initSpan = startInactiveSpan({
4948
onlyIfParent: true,
5049
op: UI_SVELTE_INIT,
@@ -55,35 +54,21 @@ function recordInitSpan(componentName: string): Span | undefined {
5554
onMount(() => {
5655
initSpan.end();
5756
});
58-
59-
return initSpan;
6057
}
6158

62-
function recordUpdateSpans(componentName: string, initSpan?: Span): void {
59+
function recordUpdateSpans(componentName: string): void {
6360
let updateSpan: Span | undefined;
6461
beforeUpdate(() => {
65-
// We need to get the active transaction again because the initial one could
66-
// already be finished or there is currently no transaction going on.
62+
// If there is no active span, we skip
6763
const activeSpan = getActiveSpan();
6864
if (!activeSpan) {
6965
return;
7066
}
7167

72-
// If we are initializing the component when the update span is started, we start it as child
73-
// of the init span. Else, we start it as a child of the transaction.
74-
const parentSpan =
75-
initSpan && initSpan.isRecording() && getRootSpan(initSpan) === getRootSpan(activeSpan)
76-
? initSpan
77-
: getRootSpan(activeSpan);
78-
79-
if (!parentSpan) return;
80-
81-
updateSpan = withActiveSpan(parentSpan, () => {
82-
return startInactiveSpan({
83-
op: UI_SVELTE_UPDATE,
84-
name: componentName,
85-
attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.svelte' },
86-
});
68+
updateSpan = startInactiveSpan({
69+
op: UI_SVELTE_UPDATE,
70+
name: componentName,
71+
attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.svelte' },
8772
});
8873
});
8974

‎packages/svelte/test/performance.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ describe('Sentry.trackComponent()', () => {
3333
});
3434
});
3535

36-
it('creates nested init and update spans on component initialization', async () => {
36+
it('creates init and update spans on component initialization', async () => {
3737
startSpan({ name: 'outer' }, span => {
3838
expect(span).toBeDefined();
3939
render(DummyComponent, { props: { options: {} } });
@@ -73,7 +73,7 @@ describe('Sentry.trackComponent()', () => {
7373
description: '<Dummy$>',
7474
op: 'ui.svelte.update',
7575
origin: 'auto.ui.svelte',
76-
parent_span_id: initSpanId,
76+
parent_span_id: rootSpanId,
7777
span_id: expect.any(String),
7878
start_timestamp: expect.any(Number),
7979
timestamp: expect.any(Number),
@@ -128,7 +128,7 @@ describe('Sentry.trackComponent()', () => {
128128
description: '<Dummy$>',
129129
op: 'ui.svelte.update',
130130
origin: 'auto.ui.svelte',
131-
parent_span_id: initSpanId,
131+
parent_span_id: rootSpanId,
132132
span_id: expect.any(String),
133133
start_timestamp: expect.any(Number),
134134
timestamp: expect.any(Number),

‎packages/types/src/options.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,19 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
9494
*/
9595
enableTracing?: boolean;
9696

97+
/**
98+
* If this is enabled, any spans started will always have their parent be the active root span,
99+
* if there is any active span.
100+
*
101+
* This is necessary because in some environments (e.g. browser),
102+
* we cannot guarantee an accurate active span.
103+
* Because we cannot properly isolate execution environments,
104+
* you may get wrong results when using e.g. nested `startSpan()` calls.
105+
*
106+
* To solve this, in these environments we'll by default enable this option.
107+
*/
108+
parentSpanIsAlwaysRootSpan?: boolean;
109+
97110
/**
98111
* Initial data to populate scope.
99112
*/

0 commit comments

Comments
 (0)