From 772fdac12f2b508ddc349d33007212751727bab5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 19 Jun 2024 14:57:39 +0200 Subject: [PATCH] feat(core): Add `parentSpan` option to `startSpan*` APIs --- packages/core/src/tracing/trace.ts | 211 +++++++++++-------- packages/core/test/lib/tracing/trace.test.ts | 41 +++- packages/opentelemetry/src/trace.ts | 119 ++++++----- packages/opentelemetry/test/trace.test.ts | 47 +++++ packages/types/src/startSpanOptions.ts | 8 +- 5 files changed, 287 insertions(+), 139 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index e34c2c1a62d3..cdb2efd7221c 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines */ + import type { ClientOptions, Scope, SentrySpanArguments, Span, SpanTimeInput, StartSpanOptions } from '@sentry/types'; import { generatePropagationContext, logger, propagationContextFromHeaders } from '@sentry/utils'; import type { AsyncContextStrategy } from '../asyncContext/types'; @@ -32,40 +34,47 @@ const SUPPRESS_TRACING_KEY = '__SENTRY_SUPPRESS_TRACING__'; * You'll always get a span passed to the callback, * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ -export function startSpan(context: StartSpanOptions, callback: (span: Span) => T): T { +export function startSpan(options: StartSpanOptions, callback: (span: Span) => T): T { const acs = getAcs(); if (acs.startSpan) { - return acs.startSpan(context, callback); + return acs.startSpan(options, callback); } - const spanContext = normalizeContext(context); - - return withScope(context.scope, scope => { - const parentSpan = getParentSpan(scope); - - const shouldSkipSpan = context.onlyIfParent && !parentSpan; - const activeSpan = shouldSkipSpan - ? new SentryNonRecordingSpan() - : createChildOrRootSpan({ - parentSpan, - spanContext, - forceTransaction: context.forceTransaction, - scope, - }); - - _setSpanForScope(scope, activeSpan); - - return handleCallbackErrors( - () => callback(activeSpan), - () => { - // Only update the span status if it hasn't been changed yet, and the span is not yet finished - const { status } = spanToJSON(activeSpan); - if (activeSpan.isRecording() && (!status || status === 'ok')) { - activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); - } - }, - () => activeSpan.end(), - ); + const spanArguments = parseSentrySpanArguments(options); + const { forceTransaction, parentSpan: customParentSpan } = options; + + return withScope(options.scope, () => { + // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` + const wrapper = getActiveSpanWrapper(customParentSpan); + + return wrapper(() => { + const scope = getCurrentScope(); + const parentSpan = getParentSpan(scope); + + const shouldSkipSpan = options.onlyIfParent && !parentSpan; + const activeSpan = shouldSkipSpan + ? new SentryNonRecordingSpan() + : createChildOrRootSpan({ + parentSpan, + spanArguments, + forceTransaction, + scope, + }); + + _setSpanForScope(scope, activeSpan); + + return handleCallbackErrors( + () => callback(activeSpan), + () => { + // Only update the span status if it hasn't been changed yet, and the span is not yet finished + const { status } = spanToJSON(activeSpan); + if (activeSpan.isRecording() && (!status || status === 'ok')) { + activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + } + }, + () => activeSpan.end(), + ); + }); }); } @@ -79,43 +88,50 @@ export function startSpan(context: StartSpanOptions, callback: (span: Span) = * You'll always get a span passed to the callback, * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ -export function startSpanManual(context: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T { +export function startSpanManual(options: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T { const acs = getAcs(); if (acs.startSpanManual) { - return acs.startSpanManual(context, callback); + return acs.startSpanManual(options, callback); } - const spanContext = normalizeContext(context); - - return withScope(context.scope, scope => { - const parentSpan = getParentSpan(scope); - - const shouldSkipSpan = context.onlyIfParent && !parentSpan; - const activeSpan = shouldSkipSpan - ? new SentryNonRecordingSpan() - : createChildOrRootSpan({ - parentSpan, - spanContext, - forceTransaction: context.forceTransaction, - scope, - }); - - _setSpanForScope(scope, activeSpan); - - function finishAndSetSpan(): void { - activeSpan.end(); - } - - return handleCallbackErrors( - () => callback(activeSpan, finishAndSetSpan), - () => { - // Only update the span status if it hasn't been changed yet, and the span is not yet finished - const { status } = spanToJSON(activeSpan); - if (activeSpan.isRecording() && (!status || status === 'ok')) { - activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); - } - }, - ); + const spanArguments = parseSentrySpanArguments(options); + const { forceTransaction, parentSpan: customParentSpan } = options; + + return withScope(options.scope, () => { + // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` + const wrapper = getActiveSpanWrapper(customParentSpan); + + return wrapper(() => { + const scope = getCurrentScope(); + const parentSpan = getParentSpan(scope); + + const shouldSkipSpan = options.onlyIfParent && !parentSpan; + const activeSpan = shouldSkipSpan + ? new SentryNonRecordingSpan() + : createChildOrRootSpan({ + parentSpan, + spanArguments, + forceTransaction, + scope, + }); + + _setSpanForScope(scope, activeSpan); + + function finishAndSetSpan(): void { + activeSpan.end(); + } + + return handleCallbackErrors( + () => callback(activeSpan, finishAndSetSpan), + () => { + // Only update the span status if it hasn't been changed yet, and the span is not yet finished + const { status } = spanToJSON(activeSpan); + if (activeSpan.isRecording() && (!status || status === 'ok')) { + activeSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + } + }, + ); + }); }); } @@ -128,28 +144,39 @@ export function startSpanManual(context: StartSpanOptions, callback: (span: S * This function will always return a span, * it may just be a non-recording span if the span is not sampled or if tracing is disabled. */ -export function startInactiveSpan(context: StartSpanOptions): Span { +export function startInactiveSpan(options: StartSpanOptions): Span { const acs = getAcs(); if (acs.startInactiveSpan) { - return acs.startInactiveSpan(context); + return acs.startInactiveSpan(options); } - const spanContext = normalizeContext(context); + const spanArguments = parseSentrySpanArguments(options); + const { forceTransaction, parentSpan: customParentSpan } = options; - const scope = context.scope || getCurrentScope(); - const parentSpan = getParentSpan(scope); + // If `options.scope` is defined, we use this as as a wrapper, + // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` + const wrapper = options.scope + ? (callback: () => Span) => withScope(options.scope, callback) + : customParentSpan + ? (callback: () => Span) => withActiveSpan(customParentSpan, callback) + : (callback: () => Span) => callback(); - const shouldSkipSpan = context.onlyIfParent && !parentSpan; + return wrapper(() => { + const scope = getCurrentScope(); + const parentSpan = getParentSpan(scope); - if (shouldSkipSpan) { - return new SentryNonRecordingSpan(); - } + const shouldSkipSpan = options.onlyIfParent && !parentSpan; + + if (shouldSkipSpan) { + return new SentryNonRecordingSpan(); + } - return createChildOrRootSpan({ - parentSpan, - spanContext, - forceTransaction: context.forceTransaction, - scope, + return createChildOrRootSpan({ + parentSpan, + spanArguments, + forceTransaction, + scope, + }); }); } @@ -239,12 +266,12 @@ export function startNewTrace(callback: () => T): T { function createChildOrRootSpan({ parentSpan, - spanContext, + spanArguments, forceTransaction, scope, }: { parentSpan: SentrySpan | undefined; - spanContext: SentrySpanArguments; + spanArguments: SentrySpanArguments; forceTransaction?: boolean; scope: Scope; }): Span { @@ -256,7 +283,7 @@ function createChildOrRootSpan({ let span: Span; if (parentSpan && !forceTransaction) { - span = _startChildSpan(parentSpan, scope, spanContext); + span = _startChildSpan(parentSpan, scope, spanArguments); addChildSpanToSpan(parentSpan, span); } else if (parentSpan) { // If we forced a transaction but have a parent span, make sure to continue from the parent span, not the scope @@ -268,7 +295,7 @@ function createChildOrRootSpan({ { traceId, parentSpanId, - ...spanContext, + ...spanArguments, }, scope, parentSampled, @@ -290,7 +317,7 @@ function createChildOrRootSpan({ { traceId, parentSpanId, - ...spanContext, + ...spanArguments, }, scope, parentSampled, @@ -312,19 +339,17 @@ function createChildOrRootSpan({ * This converts StartSpanOptions to SentrySpanArguments. * For the most part (for now) we accept the same options, * but some of them need to be transformed. - * - * Eventually the StartSpanOptions will be more aligned with OpenTelemetry. */ -function normalizeContext(context: StartSpanOptions): SentrySpanArguments { - const exp = context.experimental || {}; +function parseSentrySpanArguments(options: StartSpanOptions): SentrySpanArguments { + const exp = options.experimental || {}; const initialCtx: SentrySpanArguments = { isStandalone: exp.standalone, - ...context, + ...options, }; - if (context.startTime) { + if (options.startTime) { const ctx: SentrySpanArguments & { startTime?: SpanTimeInput } = { ...initialCtx }; - ctx.startTimestamp = spanTimeInputToSeconds(context.startTime); + ctx.startTimestamp = spanTimeInputToSeconds(options.startTime); delete ctx.startTime; return ctx; } @@ -419,3 +444,11 @@ function getParentSpan(scope: Scope): SentrySpan | undefined { return span; } + +function getActiveSpanWrapper(parentSpan?: Span): (callback: () => T) => T { + return parentSpan + ? (callback: () => T) => { + return withActiveSpan(parentSpan, callback); + } + : (callback: () => T) => callback(); +} diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 47c709ced1dd..33b8e0572835 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -271,6 +271,17 @@ describe('startSpan', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass a parentSpan', () => { + const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' }); + + startSpan({ name: 'GET users/[id]', parentSpan }, span => { + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); + }); + + expect(getActiveSpan()).toBe(undefined); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); client = new TestClient(options); @@ -653,13 +664,13 @@ describe('startSpanManual', () => { const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); _setSpanForScope(manualScope, parentSpan); - startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { + startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => { expect(getCurrentScope()).not.toBe(initialScope); expect(getCurrentScope()).toBe(manualScope); expect(getActiveSpan()).toBe(span); expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); - finish(); + span.end(); // Is still the active span expect(getActiveSpan()).toBe(span); @@ -669,6 +680,19 @@ describe('startSpanManual', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass a parentSpan', () => { + const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' }); + + startSpanManual({ name: 'GET users/[id]', parentSpan }, span => { + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); + + span.end(); + }); + + expect(getActiveSpan()).toBe(undefined); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); client = new TestClient(options); @@ -977,6 +1001,19 @@ describe('startInactiveSpan', () => { expect(getActiveSpan()).toBeUndefined(); }); + it('allows to pass a parentSpan', () => { + const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true, name: 'parent-span' }); + + const span = startInactiveSpan({ name: 'GET users/[id]', parentSpan }); + + expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); + expect(getActiveSpan()).toBe(undefined); + + span.end(); + + expect(getActiveSpan()).toBeUndefined(); + }); + 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 b7d60ab117ad..5ea5381a2db3 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -12,7 +12,7 @@ import { handleCallbackErrors, spanToJSON, } from '@sentry/core'; -import type { Client, Scope } from '@sentry/types'; +import type { Client, Scope, Span as SentrySpan } from '@sentry/types'; import { continueTraceAsRemoteSpan, makeTraceState } from './propagator'; import type { OpenTelemetryClient, OpenTelemetrySpanContext } from './types'; @@ -32,27 +32,32 @@ import { getSamplingDecision } from './utils/getSamplingDecision'; export function startSpan(options: OpenTelemetrySpanContext, callback: (span: Span) => T): T { const tracer = getTracer(); - const { name } = options; + const { name, parentSpan: customParentSpan } = options; - const activeCtx = getContext(options.scope, options.forceTransaction); - const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); - const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` + const wrapper = getActiveSpanWrapper(customParentSpan); - const spanContext = getSpanContext(options); + return wrapper(() => { + const activeCtx = getContext(options.scope, options.forceTransaction); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); + const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; - return tracer.startActiveSpan(name, spanContext, ctx, span => { - _applySentryAttributesToSpan(span, options); + const spanOptions = getSpanOptions(options); + + return tracer.startActiveSpan(name, spanOptions, ctx, span => { + _applySentryAttributesToSpan(span, options); - return handleCallbackErrors( - () => callback(span), - () => { - // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses - if (spanToJSON(span).status === undefined) { - span.setStatus({ code: SpanStatusCode.ERROR }); - } - }, - () => span.end(), - ); + return handleCallbackErrors( + () => callback(span), + () => { + // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses + if (spanToJSON(span).status === undefined) { + span.setStatus({ code: SpanStatusCode.ERROR }); + } + }, + () => span.end(), + ); + }); }); } @@ -72,26 +77,31 @@ export function startSpanManual( ): T { const tracer = getTracer(); - const { name } = options; + const { name, parentSpan: customParentSpan } = options; - const activeCtx = getContext(options.scope, options.forceTransaction); - const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); - const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` + const wrapper = getActiveSpanWrapper(customParentSpan); - const spanContext = getSpanContext(options); + return wrapper(() => { + const activeCtx = getContext(options.scope, options.forceTransaction); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); + const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; - return tracer.startActiveSpan(name, spanContext, ctx, span => { - _applySentryAttributesToSpan(span, options); + const spanOptions = getSpanOptions(options); - return handleCallbackErrors( - () => callback(span, () => span.end()), - () => { - // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses - if (spanToJSON(span).status === undefined) { - span.setStatus({ code: SpanStatusCode.ERROR }); - } - }, - ); + return tracer.startActiveSpan(name, spanOptions, ctx, span => { + _applySentryAttributesToSpan(span, options); + + return handleCallbackErrors( + () => callback(span, () => span.end()), + () => { + // Only set the span status to ERROR when there wasn't any status set before, in order to avoid stomping useful span statuses + if (spanToJSON(span).status === undefined) { + span.setStatus({ code: SpanStatusCode.ERROR }); + } + }, + ); + }); }); } @@ -107,19 +117,24 @@ export function startSpanManual( export function startInactiveSpan(options: OpenTelemetrySpanContext): Span { const tracer = getTracer(); - const { name } = options; + const { name, parentSpan: customParentSpan } = options; - const activeCtx = getContext(options.scope, options.forceTransaction); - const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); - const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` + const wrapper = getActiveSpanWrapper(customParentSpan); - const spanContext = getSpanContext(options); + return wrapper(() => { + const activeCtx = getContext(options.scope, options.forceTransaction); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); + const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; - const span = tracer.startSpan(name, spanContext, ctx); + const spanOptions = getSpanOptions(options); - _applySentryAttributesToSpan(span, options); + const span = tracer.startSpan(name, spanOptions, ctx); - return span; + _applySentryAttributesToSpan(span, options); + + return span; + }); } /** @@ -149,7 +164,7 @@ function _applySentryAttributesToSpan(span: Span, options: OpenTelemetrySpanCont } } -function getSpanContext(options: OpenTelemetrySpanContext): SpanOptions { +function getSpanOptions(options: OpenTelemetrySpanContext): SpanOptions { const { startTime, attributes, kind } = options; // OTEL expects timestamps in ms, not seconds @@ -188,7 +203,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi sampled: propagationContext.sampled, }); - const spanContext: SpanContext = { + const spanOptions: SpanContext = { traceId: propagationContext.traceId, spanId: propagationContext.parentSpanId || propagationContext.spanId, isRemote: true, @@ -197,7 +212,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi }; // Add remote parent span context, - return trace.setSpanContext(ctx, spanContext); + return trace.setSpanContext(ctx, spanOptions); } // if we have no scope or client, we just return the context as-is @@ -230,7 +245,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi sampled, }); - const spanContext: SpanContext = { + const spanOptions: SpanContext = { traceId, spanId, isRemote: true, @@ -238,7 +253,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi traceState, }; - const ctxWithSpanContext = trace.setSpanContext(ctxWithoutSpan, spanContext); + const ctxWithSpanContext = trace.setSpanContext(ctxWithoutSpan, spanOptions); return ctxWithSpanContext; } @@ -270,3 +285,13 @@ export function continueTrace(options: Parameters[0 return continueTraceAsRemoteSpan(context.active(), options, callback); }); } + +function getActiveSpanWrapper(parentSpan?: Span | SentrySpan): (callback: () => T) => T { + return parentSpan + ? (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 + return withActiveSpan(parentSpan as Span, callback); + } + : (callback: () => T) => callback(); +} diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 2da8c7a8b511..6fd4ada4dc46 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -310,6 +310,21 @@ describe('trace', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass a parentSpan', () => { + let parentSpan: Span; + + startSpanManual({ name: 'detached' }, span => { + parentSpan = span; + }); + + startSpan({ name: 'GET users/[id]', parentSpan: parentSpan! }, span => { + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe(parentSpan.spanContext().spanId); + }); + + expect(getActiveSpan()).toBe(undefined); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const client = getClient()!; const transactionEvents: Event[] = []; @@ -549,6 +564,21 @@ describe('trace', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass a parentSpan', () => { + let parentSpan: Span; + + startSpanManual({ name: 'detached' }, span => { + parentSpan = span; + }); + + const span = startInactiveSpan({ name: 'GET users/[id]', parentSpan: parentSpan! }); + + expect(getActiveSpan()).toBe(undefined); + expect(spanToJSON(span).parent_span_id).toBe(parentSpan!.spanContext().spanId); + + expect(getActiveSpan()).toBe(undefined); + }); + it('allows to force a transaction with forceTransaction=true', async () => { const client = getClient()!; const transactionEvents: Event[] = []; @@ -813,6 +843,23 @@ describe('trace', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to pass a parentSpan', () => { + let parentSpan: Span; + + startSpanManual({ name: 'detached' }, span => { + parentSpan = span; + }); + + startSpanManual({ name: 'GET users/[id]', parentSpan: parentSpan! }, span => { + expect(getActiveSpan()).toBe(span); + expect(spanToJSON(span).parent_span_id).toBe(parentSpan.spanContext().spanId); + + span.end(); + }); + + expect(getActiveSpan()).toBe(undefined); + }); + 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 36e5f56355c9..89e523f6c922 100644 --- a/packages/types/src/startSpanOptions.ts +++ b/packages/types/src/startSpanOptions.ts @@ -1,5 +1,5 @@ import type { Scope } from './scope'; -import type { SpanAttributes, SpanTimeInput } from './span'; +import type { Span, SpanAttributes, SpanTimeInput } from './span'; export interface StartSpanOptions { /** A manually specified start time for the created `Span` object. */ @@ -17,6 +17,12 @@ export interface StartSpanOptions { /** An op for the span. This is a categorization for spans. */ op?: string; + /** + * 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. + */ + parentSpan?: Span; + /** * If set to true, this span will be forced to be treated as a transaction in the Sentry UI, if possible and applicable. * Note that it is up to the SDK to decide how exactly the span will be sent, which may change in future SDK versions.