Skip to content

Commit 7798c67

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 214bfb3 commit 7798c67

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
@@ -49,12 +49,17 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
4949
* @param options Configuration options for this SDK.
5050
*/
5151
public constructor(options: BrowserClientOptions) {
52+
const opts = {
53+
// We default this to true, as it is the safer scenario
54+
parentSpanIsAlwaysRootSpan: true,
55+
...options,
56+
};
5257
const sdkSource = WINDOW.SENTRY_SDK_SOURCE || getSDKSource();
53-
applySdkMetadata(options, 'browser', ['browser'], sdkSource);
58+
applySdkMetadata(opts, 'browser', ['browser'], sdkSource);
5459

55-
super(options);
60+
super(opts);
5661

57-
if (options.sendClientReports && WINDOW.document) {
62+
if (opts.sendClientReports && WINDOW.document) {
5863
WINDOW.document.addEventListener('visibilitychange', () => {
5964
if (WINDOW.document.visibilityState === 'hidden') {
6065
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, Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types';
2-
32
import { propagationContextFromHeaders } from '@sentry/utils';
43
import type { AsyncContextStrategy } from '../asyncContext';
54
import { getMainCarrier } from '../asyncContext';
@@ -10,13 +9,7 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '../semanticAttributes';
109
import { handleCallbackErrors } from '../utils/handleCallbackErrors';
1110
import { hasTracingEnabled } from '../utils/hasTracingEnabled';
1211
import { _getSpanForScope, _setSpanForScope } from '../utils/spanOnScope';
13-
import {
14-
addChildSpanToSpan,
15-
getActiveSpan,
16-
spanIsSampled,
17-
spanTimeInputToSeconds,
18-
spanToJSON,
19-
} from '../utils/spanUtils';
12+
import { addChildSpanToSpan, getRootSpan, spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';
2013
import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
2114
import { sampleTransaction } from './sampling';
2215
import { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
@@ -44,7 +37,7 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span) =
4437
const spanContext = normalizeContext(context);
4538

4639
return withScope(context.scope, scope => {
47-
const parentSpan = _getSpanForScope(scope) as SentrySpan | undefined;
40+
const parentSpan = getParentSpan(scope);
4841

4942
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
5043
const activeSpan = shouldSkipSpan
@@ -91,7 +84,7 @@ export function startSpanManual<T>(context: StartSpanOptions, callback: (span: S
9184
const spanContext = normalizeContext(context);
9285

9386
return withScope(context.scope, scope => {
94-
const parentSpan = _getSpanForScope(scope) as SentrySpan | undefined;
87+
const parentSpan = getParentSpan(scope);
9588

9689
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
9790
const activeSpan = shouldSkipSpan
@@ -138,18 +131,16 @@ export function startInactiveSpan(context: StartSpanOptions): Span {
138131
}
139132

140133
const spanContext = normalizeContext(context);
141-
const parentSpan = context.scope
142-
? (_getSpanForScope(context.scope) as SentrySpan | undefined)
143-
: (getActiveSpan() as SentrySpan | undefined);
134+
135+
const scope = context.scope || getCurrentScope();
136+
const parentSpan = getParentSpan(scope);
144137

145138
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
146139

147140
if (shouldSkipSpan) {
148141
return new SentryNonRecordingSpan();
149142
}
150143

151-
const scope = context.scope || getCurrentScope();
152-
153144
return createChildSpanOrTransaction({
154145
parentSpan,
155146
spanContext,
@@ -324,3 +315,19 @@ function _startTransaction(transactionContext: TransactionContext): Transaction
324315

325316
return transaction;
326317
}
318+
319+
function getParentSpan(scope: Scope): SentrySpan | undefined {
320+
const span = _getSpanForScope(scope) as SentrySpan | undefined;
321+
322+
if (!span) {
323+
return undefined;
324+
}
325+
326+
const client = getClient();
327+
const options: Partial<ClientOptions> = client ? client.getOptions() : {};
328+
if (options.parentSpanIsAlwaysRootSpan) {
329+
return getRootSpan(span) as SentrySpan;
330+
}
331+
332+
return span as SentrySpan;
333+
}

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

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

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

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

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

‎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)