Skip to content

Commit 236e350

Browse files
mydeamydea
committed
feat(core): Ensure startSpan() can handle spans that require parent
This introduces a new `onlyIfParent` option to the start-span APIs, which, if set, will ensure to not create a span if there is no parent. This PR also uses this option in a few places, esp. for http.client spans, to ensure we do not capture these if there is no ongoing transaction.
1 parent 07a2264 commit 236e350

File tree

18 files changed

+206
-35
lines changed

18 files changed

+206
-35
lines changed

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,9 @@ export function startSpan<T>(context: StartSpanOptions, callback: (span: Span |
8080
// eslint-disable-next-line deprecation/deprecation
8181
const parentSpan = scope.getSpan();
8282

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

@@ -128,7 +130,9 @@ export function startSpanManual<T>(
128130
// eslint-disable-next-line deprecation/deprecation
129131
const parentSpan = scope.getSpan();
130132

131-
const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx);
133+
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
134+
const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx);
135+
132136
// eslint-disable-next-line deprecation/deprecation
133137
scope.setSpan(activeSpan);
134138

@@ -174,6 +178,12 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
174178
context.scope.getSpan()
175179
: getActiveSpan();
176180

181+
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
182+
183+
if (shouldSkipSpan) {
184+
return undefined;
185+
}
186+
177187
if (parentSpan) {
178188
// eslint-disable-next-line deprecation/deprecation
179189
return parentSpan.startChild(ctx);

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,28 @@ describe('startSpan', () => {
335335
});
336336
});
337337
});
338+
339+
describe('onlyIfParent', () => {
340+
it('does not create a span if there is no parent', () => {
341+
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
342+
return span;
343+
});
344+
345+
expect(span).toBeUndefined();
346+
});
347+
348+
it('creates a span if there is a parent', () => {
349+
const span = startSpan({ name: 'parent span' }, () => {
350+
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
351+
return span;
352+
});
353+
354+
return span;
355+
});
356+
357+
expect(span).toBeDefined();
358+
});
359+
});
338360
});
339361

340362
describe('startSpanManual', () => {
@@ -415,6 +437,28 @@ describe('startSpanManual', () => {
415437
});
416438
});
417439
});
440+
441+
describe('onlyIfParent', () => {
442+
it('does not create a span if there is no parent', () => {
443+
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
444+
return span;
445+
});
446+
447+
expect(span).toBeUndefined();
448+
});
449+
450+
it('creates a span if there is a parent', () => {
451+
const span = startSpan({ name: 'parent span' }, () => {
452+
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
453+
return span;
454+
});
455+
456+
return span;
457+
});
458+
459+
expect(span).toBeDefined();
460+
});
461+
});
418462
});
419463

420464
describe('startInactiveSpan', () => {
@@ -479,6 +523,24 @@ describe('startInactiveSpan', () => {
479523
span?.end();
480524
});
481525
});
526+
527+
describe('onlyIfParent', () => {
528+
it('does not create a span if there is no parent', () => {
529+
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });
530+
531+
expect(span).toBeUndefined();
532+
});
533+
534+
it('creates a span if there is a parent', () => {
535+
const span = startSpan({ name: 'parent span' }, () => {
536+
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });
537+
538+
return span;
539+
});
540+
541+
expect(span).toBeDefined();
542+
});
543+
});
482544
});
483545

484546
describe('continueTrace', () => {

‎packages/opentelemetry/src/trace.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import type { Span, Tracer } from '@opentelemetry/api';
2+
import { context } from '@opentelemetry/api';
23
import { SpanStatusCode, trace } from '@opentelemetry/api';
4+
import { suppressTracing } from '@opentelemetry/core';
35
import { SDK_VERSION, handleCallbackErrors } from '@sentry/core';
46
import type { Client } from '@sentry/types';
57

@@ -22,7 +24,11 @@ export function startSpan<T>(spanContext: OpenTelemetrySpanContext, callback: (s
2224

2325
const { name } = spanContext;
2426

25-
return tracer.startActiveSpan(name, spanContext, span => {
27+
const activeCtx = context.active();
28+
const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx);
29+
const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;
30+
31+
return tracer.startActiveSpan(name, spanContext, ctx, span => {
2632
_applySentryAttributesToSpan(span, spanContext);
2733

2834
return handleCallbackErrors(
@@ -49,7 +55,11 @@ export function startSpanManual<T>(spanContext: OpenTelemetrySpanContext, callba
4955

5056
const { name } = spanContext;
5157

52-
return tracer.startActiveSpan(name, spanContext, span => {
58+
const activeCtx = context.active();
59+
const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx);
60+
const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;
61+
62+
return tracer.startActiveSpan(name, spanContext, ctx, span => {
5363
_applySentryAttributesToSpan(span, spanContext);
5464

5565
return handleCallbackErrors(
@@ -81,7 +91,11 @@ export function startInactiveSpan(spanContext: OpenTelemetrySpanContext): Span {
8191

8292
const { name } = spanContext;
8393

84-
const span = tracer.startSpan(name, spanContext);
94+
const activeCtx = context.active();
95+
const shouldSkipSpan = spanContext.onlyIfParent && !trace.getSpan(activeCtx);
96+
const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;
97+
98+
const span = tracer.startSpan(name, spanContext, ctx);
8599

86100
_applySentryAttributesToSpan(span, spanContext);
87101

‎packages/opentelemetry/src/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ export interface OpenTelemetrySpanContext {
1414
origin?: SpanOrigin;
1515
source?: TransactionSource;
1616
scope?: Scope;
17+
onlyIfParent?: boolean;
1718

1819
// Base SpanOptions we support
1920
attributes?: Attributes;

‎packages/opentelemetry/test/trace.test.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { Span } from '@opentelemetry/api';
22
import { SpanKind } from '@opentelemetry/api';
33
import { TraceFlags, context, trace } from '@opentelemetry/api';
44
import type { ReadableSpan } from '@opentelemetry/sdk-trace-base';
5+
import { Span as SpanClass } from '@opentelemetry/sdk-trace-base';
56
import type { PropagationContext } from '@sentry/types';
67

78
import { getClient } from '../src/custom/hub';
@@ -260,6 +261,28 @@ describe('trace', () => {
260261
},
261262
);
262263
});
264+
265+
describe('onlyIfParent', () => {
266+
it('does not create a span if there is no parent', () => {
267+
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
268+
return span;
269+
});
270+
271+
expect(span).not.toBeInstanceOf(SpanClass);
272+
});
273+
274+
it('creates a span if there is a parent', () => {
275+
const span = startSpan({ name: 'parent span' }, () => {
276+
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
277+
return span;
278+
});
279+
280+
return span;
281+
});
282+
283+
expect(span).toBeInstanceOf(SpanClass);
284+
});
285+
});
263286
});
264287

265288
describe('startInactiveSpan', () => {
@@ -349,6 +372,24 @@ describe('trace', () => {
349372
});
350373
expect(getSpanKind(span)).toEqual(SpanKind.CLIENT);
351374
});
375+
376+
describe('onlyIfParent', () => {
377+
it('does not create a span if there is no parent', () => {
378+
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });
379+
380+
expect(span).not.toBeInstanceOf(SpanClass);
381+
});
382+
383+
it('creates a span if there is a parent', () => {
384+
const span = startSpan({ name: 'parent span' }, () => {
385+
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });
386+
387+
return span;
388+
});
389+
390+
expect(span).toBeInstanceOf(SpanClass);
391+
});
392+
});
352393
});
353394

354395
describe('startSpanManual', () => {
@@ -419,6 +460,28 @@ describe('trace', () => {
419460
);
420461
});
421462
});
463+
464+
describe('onlyIfParent', () => {
465+
it('does not create a span if there is no parent', () => {
466+
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
467+
return span;
468+
});
469+
470+
expect(span).not.toBeInstanceOf(SpanClass);
471+
});
472+
473+
it('creates a span if there is a parent', () => {
474+
const span = startSpan({ name: 'parent span' }, () => {
475+
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
476+
return span;
477+
});
478+
479+
return span;
480+
});
481+
482+
expect(span).toBeInstanceOf(SpanClass);
483+
});
484+
});
422485
});
423486

424487
describe('trace (tracing disabled)', () => {

‎packages/react/src/profiler.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ class Profiler extends React.Component<ProfilerProps> {
5757

5858
this._mountSpan = startInactiveSpan({
5959
name: `<${name}>`,
60+
onlyIfParent: true,
6061
op: REACT_MOUNT_OP,
6162
origin: 'auto.ui.react.profiler',
6263
attributes: { 'ui.component_name': name },
@@ -83,6 +84,7 @@ class Profiler extends React.Component<ProfilerProps> {
8384
this._updateSpan = withActiveSpan(this._mountSpan, () => {
8485
return startInactiveSpan({
8586
name: `<${this.props.name}>`,
87+
onlyIfParent: true,
8688
op: REACT_UPDATE_OP,
8789
origin: 'auto.ui.react.profiler',
8890
startTimestamp: now,
@@ -115,6 +117,7 @@ class Profiler extends React.Component<ProfilerProps> {
115117
const startTimestamp = spanToJSON(this._mountSpan).timestamp;
116118
withActiveSpan(this._mountSpan, () => {
117119
const renderSpan = startInactiveSpan({
120+
onlyIfParent: true,
118121
name: `<${name}>`,
119122
op: REACT_RENDER_OP,
120123
origin: 'auto.ui.react.profiler',
@@ -187,6 +190,7 @@ function useProfiler(
187190

188191
return startInactiveSpan({
189192
name: `<${name}>`,
193+
onlyIfParent: true,
190194
op: REACT_MOUNT_OP,
191195
origin: 'auto.ui.react.profiler',
192196
attributes: { 'ui.component_name': name },
@@ -205,6 +209,7 @@ function useProfiler(
205209

206210
const renderSpan = startInactiveSpan({
207211
name: `<${name}>`,
212+
onlyIfParent: true,
208213
op: REACT_RENDER_OP,
209214
origin: 'auto.ui.react.profiler',
210215
startTimestamp,

‎packages/react/test/profiler.test.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ describe('withProfiler', () => {
7474
expect(mockStartInactiveSpan).toHaveBeenCalledTimes(1);
7575
expect(mockStartInactiveSpan).toHaveBeenLastCalledWith({
7676
name: `<${UNKNOWN_COMPONENT}>`,
77+
onlyIfParent: true,
7778
op: REACT_MOUNT_OP,
7879
origin: 'auto.ui.react.profiler',
7980
attributes: { 'ui.component_name': 'unknown' },
@@ -92,6 +93,7 @@ describe('withProfiler', () => {
9293
expect(mockStartInactiveSpan).toHaveBeenCalledTimes(2);
9394
expect(mockStartInactiveSpan).toHaveBeenLastCalledWith({
9495
name: `<${UNKNOWN_COMPONENT}>`,
96+
onlyIfParent: true,
9597
op: REACT_RENDER_OP,
9698
origin: 'auto.ui.react.profiler',
9799
startTimestamp: undefined,
@@ -125,6 +127,7 @@ describe('withProfiler', () => {
125127
expect(mockStartInactiveSpan).toHaveBeenLastCalledWith({
126128
attributes: { 'ui.react.changed_props': ['num'], 'ui.component_name': 'unknown' },
127129
name: `<${UNKNOWN_COMPONENT}>`,
130+
onlyIfParent: true,
128131
op: REACT_UPDATE_OP,
129132
origin: 'auto.ui.react.profiler',
130133
startTimestamp: expect.any(Number),
@@ -136,6 +139,7 @@ describe('withProfiler', () => {
136139
expect(mockStartInactiveSpan).toHaveBeenLastCalledWith({
137140
attributes: { 'ui.react.changed_props': ['num'], 'ui.component_name': 'unknown' },
138141
name: `<${UNKNOWN_COMPONENT}>`,
142+
onlyIfParent: true,
139143
op: REACT_UPDATE_OP,
140144
origin: 'auto.ui.react.profiler',
141145
startTimestamp: expect.any(Number),
@@ -175,6 +179,7 @@ describe('useProfiler()', () => {
175179
expect(mockStartInactiveSpan).toHaveBeenCalledTimes(1);
176180
expect(mockStartInactiveSpan).toHaveBeenLastCalledWith({
177181
name: '<Example>',
182+
onlyIfParent: true,
178183
op: REACT_MOUNT_OP,
179184
origin: 'auto.ui.react.profiler',
180185
attributes: { 'ui.component_name': 'Example' },
@@ -199,6 +204,7 @@ describe('useProfiler()', () => {
199204
expect(mockStartInactiveSpan).toHaveBeenLastCalledWith(
200205
expect.objectContaining({
201206
name: '<Example>',
207+
onlyIfParent: true,
202208
op: REACT_RENDER_OP,
203209
origin: 'auto.ui.react.profiler',
204210
attributes: { 'ui.component_name': 'Example' },

‎packages/serverless/src/awsservices.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ function wrapMakeRequest<TService extends AWSService, TResult>(
7171
req.on('afterBuild', () => {
7272
span = startInactiveSpan({
7373
name: describe(this, operation, params),
74+
onlyIfParent: true,
7475
op: 'http.client',
7576
attributes: {
7677
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',

‎packages/serverless/src/google-cloud-grpc.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ function fillGrpcFunction(stub: Stub, serviceIdentifier: string, methodName: str
122122
}
123123
const span = startInactiveSpan({
124124
name: `${callType} ${methodName}`,
125+
onlyIfParent: true,
125126
op: `grpc.${serviceIdentifier}`,
126127
attributes: {
127128
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.grpc.serverless',

‎packages/serverless/src/google-cloud-http.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ function wrapRequestFunction(orig: RequestFunction): RequestFunction {
6464
const span = SETUP_CLIENTS.has(getClient() as Client)
6565
? startInactiveSpan({
6666
name: `${httpMethod} ${reqOpts.uri}`,
67+
onlyIfParent: true,
6768
op: `http.client.${identifyService(this.apiEndpoint)}`,
6869
attributes: {
6970
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',

‎packages/serverless/test/awsservices.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ describe('awsServicesIntegration', () => {
6161
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',
6262
},
6363
name: 'aws.s3.getObject foo',
64+
onlyIfParent: true,
6465
});
6566

6667
expect(mockSpanEnd).toHaveBeenCalledTimes(1);
@@ -84,6 +85,7 @@ describe('awsServicesIntegration', () => {
8485
expect(mockStartInactiveSpan).toBeCalledWith({
8586
op: 'http.client',
8687
name: 'aws.s3.getObject foo',
88+
onlyIfParent: true,
8789
attributes: {
8890
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',
8991
},
@@ -114,6 +116,7 @@ describe('awsServicesIntegration', () => {
114116
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',
115117
},
116118
name: 'aws.lambda.invoke foo',
119+
onlyIfParent: true,
117120
});
118121
expect(mockSpanEnd).toHaveBeenCalledTimes(1);
119122
});

0 commit comments

Comments
 (0)