Skip to content

Commit aae6a14

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 1f3a796 commit aae6a14

File tree

19 files changed

+190
-11
lines changed

19 files changed

+190
-11
lines changed

‎CONTRIBUTING.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,11 @@ able to use it. From the top level of the repo, there are three commands availab
3737
dependencies (`utils`, `core`, `browser`, etc), and all packages which depend on it (currently `gatsby` and `nextjs`))
3838
- `yarn build:dev:watch`, which runs `yarn build:dev` in watch mode (recommended)
3939

40-
You can also run a production build via `yarn build`, which will build everything except for the tarballs for publishing to NPM.
41-
You can use this if you want to bundle Sentry yourself. The build output can be found in the packages `build/` folder, e.g. `packages/browser/build`.
42-
Bundled files can be found in `packages/browser/build/bundles`.
43-
Note that there are no guarantees about the produced file names etc., so make sure to double check which files are generated after upgrading.
40+
You can also run a production build via `yarn build`, which will build everything except for the tarballs for publishing
41+
to NPM. You can use this if you want to bundle Sentry yourself. The build output can be found in the packages `build/`
42+
folder, e.g. `packages/browser/build`. Bundled files can be found in `packages/browser/build/bundles`. Note that there
43+
are no guarantees about the produced file names etc., so make sure to double check which files are generated after
44+
upgrading.
4445

4546
## Testing SDK Packages Locally
4647

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

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

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

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

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

@@ -172,6 +176,13 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined {
172176
? // eslint-disable-next-line deprecation/deprecation
173177
context.scope.getSpan()
174178
: getActiveSpan();
179+
180+
const shouldSkipSpan = context.onlyIfParent && !parentSpan;
181+
182+
if (shouldSkipSpan) {
183+
return undefined;
184+
}
185+
175186
return parentSpan
176187
? // eslint-disable-next-line deprecation/deprecation
177188
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
@@ -318,6 +318,28 @@ describe('startSpan', () => {
318318
expect(getCurrentScope()).toBe(initialScope);
319319
expect(getActiveSpan()).toBe(undefined);
320320
});
321+
322+
describe('onlyIfParent', () => {
323+
it('does not create a span if there is no parent', () => {
324+
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
325+
return span;
326+
});
327+
328+
expect(span).toBeUndefined();
329+
});
330+
331+
it('creates a span if there is a parent', () => {
332+
const span = startSpan({ name: 'parent span' }, () => {
333+
const span = startSpan({ name: 'test span', onlyIfParent: true }, span => {
334+
return span;
335+
});
336+
337+
return span;
338+
});
339+
340+
expect(span).toBeDefined();
341+
});
342+
});
321343
});
322344

323345
describe('startSpanManual', () => {
@@ -381,6 +403,28 @@ describe('startSpanManual', () => {
381403

382404
expect(start).toEqual(1234);
383405
});
406+
407+
describe('onlyIfParent', () => {
408+
it('does not create a span if there is no parent', () => {
409+
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
410+
return span;
411+
});
412+
413+
expect(span).toBeUndefined();
414+
});
415+
416+
it('creates a span if there is a parent', () => {
417+
const span = startSpan({ name: 'parent span' }, () => {
418+
const span = startSpanManual({ name: 'test span', onlyIfParent: true }, span => {
419+
return span;
420+
});
421+
422+
return span;
423+
});
424+
425+
expect(span).toBeDefined();
426+
});
427+
});
384428
});
385429

386430
describe('startInactiveSpan', () => {
@@ -429,6 +473,24 @@ describe('startInactiveSpan', () => {
429473
const span = startInactiveSpan({ name: 'outer', startTime: [1234, 0] });
430474
expect(spanToJSON(span!).start_timestamp).toEqual(1234);
431475
});
476+
477+
describe('onlyIfParent', () => {
478+
it('does not create a span if there is no parent', () => {
479+
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });
480+
481+
expect(span).toBeUndefined();
482+
});
483+
484+
it('creates a span if there is a parent', () => {
485+
const span = startSpan({ name: 'parent span' }, () => {
486+
const span = startInactiveSpan({ name: 'test span', onlyIfParent: true });
487+
488+
return span;
489+
});
490+
491+
expect(span).toBeDefined();
492+
});
493+
});
432494
});
433495

434496
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
@@ -62,6 +62,7 @@ function wrapMakeRequest<TService extends AWSService, TResult>(
6262
req.on('afterBuild', () => {
6363
span = startInactiveSpan({
6464
name: describe(this, operation, params),
65+
onlyIfParent: true,
6566
op: 'http.client',
6667
attributes: {
6768
[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
@@ -110,6 +110,7 @@ function fillGrpcFunction(stub: Stub, serviceIdentifier: string, methodName: str
110110
}
111111
const span = startInactiveSpan({
112112
name: `${callType} ${methodName}`,
113+
onlyIfParent: true,
113114
op: `grpc.${serviceIdentifier}`,
114115
attributes: {
115116
[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
@@ -55,6 +55,7 @@ function wrapRequestFunction(orig: RequestFunction): RequestFunction {
5555
const httpMethod = reqOpts.method || 'GET';
5656
const span = startInactiveSpan({
5757
name: `${httpMethod} ${reqOpts.uri}`,
58+
onlyIfParent: true,
5859
op: `http.client.${identifyService(this.apiEndpoint)}`,
5960
attributes: {
6061
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.serverless',

0 commit comments

Comments
 (0)