From 707afd6ad16d72ebc3f575bf7822bbc088738445 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 16 Jul 2024 13:47:47 +0200 Subject: [PATCH] fix(tracing): Ensure you can pass `null` as `parentSpan` in `startSpan*` (#12928) Noticed this while writing docs, this makes this a bit harder to understand. With this change you can say that `parentSpan` behaves the same and accepts the same as `withActiveSpan`. See https://github.com/getsentry/sentry-docs/pull/10729 --- packages/core/src/tracing/trace.ts | 6 ++-- packages/core/test/lib/tracing/trace.test.ts | 25 +++++++++++++++ packages/opentelemetry/src/trace.ts | 4 +-- packages/opentelemetry/test/trace.test.ts | 32 ++++++++++++++++++++ packages/types/src/startSpanOptions.ts | 3 +- 5 files changed, 64 insertions(+), 6 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index cdb2efd7221c..0cb98082c590 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -157,7 +157,7 @@ export function startInactiveSpan(options: StartSpanOptions): Span { // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` const wrapper = options.scope ? (callback: () => Span) => withScope(options.scope, callback) - : customParentSpan + : customParentSpan !== undefined ? (callback: () => Span) => withActiveSpan(customParentSpan, callback) : (callback: () => Span) => callback(); @@ -445,8 +445,8 @@ function getParentSpan(scope: Scope): SentrySpan | undefined { return span; } -function getActiveSpanWrapper(parentSpan?: Span): (callback: () => T) => T { - return parentSpan +function getActiveSpanWrapper(parentSpan: Span | undefined | null): (callback: () => T) => T { + return parentSpan !== undefined ? (callback: () => T) => { return withActiveSpan(parentSpan, callback); } diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 33b8e0572835..fe58ce6f9f7d 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -282,6 +282,14 @@ describe('startSpan', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass parentSpan=null', () => { + startSpan({ name: 'GET users/[id]' }, () => { + startSpan({ name: 'GET users/[id]', parentSpan: null }, span => { + expect(spanToJSON(span).parent_span_id).toBe(undefined); + }); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); client = new TestClient(options); @@ -693,6 +701,15 @@ describe('startSpanManual', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass parentSpan=null', () => { + startSpan({ name: 'GET users/[id]' }, () => { + startSpanManual({ name: 'child', parentSpan: null }, span => { + expect(spanToJSON(span).parent_span_id).toBe(undefined); + span.end(); + }); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); client = new TestClient(options); @@ -1014,6 +1031,14 @@ describe('startInactiveSpan', () => { expect(getActiveSpan()).toBeUndefined(); }); + it('allows to pass parentSpan=null', () => { + startSpan({ name: 'outer' }, () => { + const span = startInactiveSpan({ name: 'GET users/[id]', parentSpan: null }); + expect(spanToJSON(span).parent_span_id).toBe(undefined); + span.end(); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); client = new TestClient(options); diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index 5ea5381a2db3..6f9fe5dad6d1 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -286,8 +286,8 @@ export function continueTrace(options: Parameters[0 }); } -function getActiveSpanWrapper(parentSpan?: Span | SentrySpan): (callback: () => T) => T { - return parentSpan +function getActiveSpanWrapper(parentSpan: Span | SentrySpan | undefined | null): (callback: () => T) => T { + return parentSpan !== undefined ? (callback: () => T) => { // We cast this, because the OTEL Span has a few more methods than our Span interface // TODO: Add these missing methods to the Span interface diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index d3ac52327bb6..2332fd1ced05 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -325,6 +325,16 @@ describe('trace', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass parentSpan=null', () => { + startSpan({ name: 'GET users/[id' }, () => { + startSpan({ name: 'child', parentSpan: null }, span => { + // Due to the way we propagate the scope in OTEL, + // the parent_span_id is not actually undefined here, but comes from the propagation context + expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId); + }); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const client = getClient()!; const transactionEvents: Event[] = []; @@ -577,6 +587,17 @@ describe('trace', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass parentSpan=null', () => { + startSpan({ name: 'outer' }, () => { + const span = startInactiveSpan({ name: 'test span', parentSpan: null }); + + // Due to the way we propagate the scope in OTEL, + // the parent_span_id is not actually undefined here, but comes from the propagation context + expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId); + span.end(); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const client = getClient()!; const transactionEvents: Event[] = []; @@ -856,6 +877,17 @@ describe('trace', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass parentSpan=null', () => { + startSpan({ name: 'outer' }, () => { + startSpanManual({ name: 'GET users/[id]', parentSpan: null }, span => { + // Due to the way we propagate the scope in OTEL, + // the parent_span_id is not actually undefined here, but comes from the propagation context + expect(spanToJSON(span).parent_span_id).toBe(getCurrentScope().getPropagationContext().spanId); + span.end(); + }); + }); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const client = getClient()!; const transactionEvents: Event[] = []; diff --git a/packages/types/src/startSpanOptions.ts b/packages/types/src/startSpanOptions.ts index 89e523f6c922..35d5326e32f3 100644 --- a/packages/types/src/startSpanOptions.ts +++ b/packages/types/src/startSpanOptions.ts @@ -20,8 +20,9 @@ export interface StartSpanOptions { /** * If provided, make the new span a child of this span. * If this is not provided, the new span will be a child of the currently active span. + * If this is set to `null`, the new span will have no parent span. */ - parentSpan?: Span; + parentSpan?: Span | null; /** * If set to true, this span will be forced to be treated as a transaction in the Sentry UI, if possible and applicable.