From 368bfaad8f8669cb8200c47192fddb0c5606edc5 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 20 Jun 2024 11:12:15 -0400 Subject: [PATCH] Revert "feat(core): Add `parentSpan` option to `startSpan*` APIs (#12567)" This reverts commit 001930922c632df16597311913fe76096c161fca. --- 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, 139 insertions(+), 287 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index cdb2efd7221c..e34c2c1a62d3 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -1,5 +1,3 @@ -/* 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'; @@ -34,47 +32,40 @@ 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(options: StartSpanOptions, callback: (span: Span) => T): T { +export function startSpan(context: StartSpanOptions, callback: (span: Span) => T): T { const acs = getAcs(); if (acs.startSpan) { - return acs.startSpan(options, callback); + return acs.startSpan(context, callback); } - 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(), - ); - }); + 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(), + ); }); } @@ -88,50 +79,43 @@ export function startSpan(options: 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(options: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T { +export function startSpanManual(context: StartSpanOptions, callback: (span: Span, finish: () => void) => T): T { const acs = getAcs(); if (acs.startSpanManual) { - return acs.startSpanManual(options, callback); + return acs.startSpanManual(context, callback); } - 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' }); - } - }, - ); - }); + 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' }); + } + }, + ); }); } @@ -144,39 +128,28 @@ export function startSpanManual(options: 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(options: StartSpanOptions): Span { +export function startInactiveSpan(context: StartSpanOptions): Span { const acs = getAcs(); if (acs.startInactiveSpan) { - return acs.startInactiveSpan(options); + return acs.startInactiveSpan(context); } - const spanArguments = parseSentrySpanArguments(options); - const { forceTransaction, parentSpan: customParentSpan } = options; + const spanContext = normalizeContext(context); - // 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 scope = context.scope || getCurrentScope(); + const parentSpan = getParentSpan(scope); - return wrapper(() => { - const scope = getCurrentScope(); - const parentSpan = getParentSpan(scope); + const shouldSkipSpan = context.onlyIfParent && !parentSpan; - const shouldSkipSpan = options.onlyIfParent && !parentSpan; - - if (shouldSkipSpan) { - return new SentryNonRecordingSpan(); - } + if (shouldSkipSpan) { + return new SentryNonRecordingSpan(); + } - return createChildOrRootSpan({ - parentSpan, - spanArguments, - forceTransaction, - scope, - }); + return createChildOrRootSpan({ + parentSpan, + spanContext, + forceTransaction: context.forceTransaction, + scope, }); } @@ -266,12 +239,12 @@ export function startNewTrace(callback: () => T): T { function createChildOrRootSpan({ parentSpan, - spanArguments, + spanContext, forceTransaction, scope, }: { parentSpan: SentrySpan | undefined; - spanArguments: SentrySpanArguments; + spanContext: SentrySpanArguments; forceTransaction?: boolean; scope: Scope; }): Span { @@ -283,7 +256,7 @@ function createChildOrRootSpan({ let span: Span; if (parentSpan && !forceTransaction) { - span = _startChildSpan(parentSpan, scope, spanArguments); + span = _startChildSpan(parentSpan, scope, spanContext); 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 @@ -295,7 +268,7 @@ function createChildOrRootSpan({ { traceId, parentSpanId, - ...spanArguments, + ...spanContext, }, scope, parentSampled, @@ -317,7 +290,7 @@ function createChildOrRootSpan({ { traceId, parentSpanId, - ...spanArguments, + ...spanContext, }, scope, parentSampled, @@ -339,17 +312,19 @@ 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 parseSentrySpanArguments(options: StartSpanOptions): SentrySpanArguments { - const exp = options.experimental || {}; +function normalizeContext(context: StartSpanOptions): SentrySpanArguments { + const exp = context.experimental || {}; const initialCtx: SentrySpanArguments = { isStandalone: exp.standalone, - ...options, + ...context, }; - if (options.startTime) { + if (context.startTime) { const ctx: SentrySpanArguments & { startTime?: SpanTimeInput } = { ...initialCtx }; - ctx.startTimestamp = spanTimeInputToSeconds(options.startTime); + ctx.startTimestamp = spanTimeInputToSeconds(context.startTime); delete ctx.startTime; return ctx; } @@ -444,11 +419,3 @@ 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 33b8e0572835..47c709ced1dd 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -271,17 +271,6 @@ 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); @@ -664,13 +653,13 @@ describe('startSpanManual', () => { const parentSpan = new SentrySpan({ spanId: 'parent-span-id', sampled: true }); _setSpanForScope(manualScope, parentSpan); - startSpanManual({ name: 'GET users/[id]', scope: manualScope }, span => { + startSpanManual({ name: 'GET users/[id]', scope: manualScope }, (span, finish) => { expect(getCurrentScope()).not.toBe(initialScope); expect(getCurrentScope()).toBe(manualScope); expect(getActiveSpan()).toBe(span); expect(spanToJSON(span).parent_span_id).toBe('parent-span-id'); - span.end(); + finish(); // Is still the active span expect(getActiveSpan()).toBe(span); @@ -680,19 +669,6 @@ 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); @@ -1001,19 +977,6 @@ 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 5ea5381a2db3..b7d60ab117ad 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, Span as SentrySpan } from '@sentry/types'; +import type { Client, Scope } from '@sentry/types'; import { continueTraceAsRemoteSpan, makeTraceState } from './propagator'; import type { OpenTelemetryClient, OpenTelemetrySpanContext } from './types'; @@ -32,32 +32,27 @@ import { getSamplingDecision } from './utils/getSamplingDecision'; export function startSpan(options: OpenTelemetrySpanContext, callback: (span: Span) => T): T { const tracer = getTracer(); - const { name, parentSpan: customParentSpan } = options; + const { name } = options; - // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` - const wrapper = getActiveSpanWrapper(customParentSpan); + const activeCtx = getContext(options.scope, options.forceTransaction); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); + const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; - return wrapper(() => { - const activeCtx = getContext(options.scope, options.forceTransaction); - const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); - const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + const spanContext = getSpanContext(options); - const spanOptions = getSpanOptions(options); - - return tracer.startActiveSpan(name, spanOptions, ctx, span => { - _applySentryAttributesToSpan(span, options); + return tracer.startActiveSpan(name, spanContext, 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(), + ); }); } @@ -77,31 +72,26 @@ export function startSpanManual( ): T { const tracer = getTracer(); - const { name, parentSpan: customParentSpan } = options; - - // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` - const wrapper = getActiveSpanWrapper(customParentSpan); + const { name } = options; - return wrapper(() => { - const activeCtx = getContext(options.scope, options.forceTransaction); - const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); - const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + const activeCtx = getContext(options.scope, options.forceTransaction); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); + const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; - const spanOptions = getSpanOptions(options); + const spanContext = getSpanContext(options); - return tracer.startActiveSpan(name, spanOptions, ctx, span => { - _applySentryAttributesToSpan(span, options); + return tracer.startActiveSpan(name, spanContext, 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 }); - } - }, - ); - }); + 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 }); + } + }, + ); }); } @@ -117,24 +107,19 @@ export function startSpanManual( export function startInactiveSpan(options: OpenTelemetrySpanContext): Span { const tracer = getTracer(); - const { name, parentSpan: customParentSpan } = options; + const { name } = options; - // If `options.parentSpan` is defined, we want to wrap the callback in `withActiveSpan` - const wrapper = getActiveSpanWrapper(customParentSpan); + const activeCtx = getContext(options.scope, options.forceTransaction); + const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); + const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; - return wrapper(() => { - const activeCtx = getContext(options.scope, options.forceTransaction); - const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx); - const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx; + const spanContext = getSpanContext(options); - const spanOptions = getSpanOptions(options); + const span = tracer.startSpan(name, spanContext, ctx); - const span = tracer.startSpan(name, spanOptions, ctx); + _applySentryAttributesToSpan(span, options); - _applySentryAttributesToSpan(span, options); - - return span; - }); + return span; } /** @@ -164,7 +149,7 @@ function _applySentryAttributesToSpan(span: Span, options: OpenTelemetrySpanCont } } -function getSpanOptions(options: OpenTelemetrySpanContext): SpanOptions { +function getSpanContext(options: OpenTelemetrySpanContext): SpanOptions { const { startTime, attributes, kind } = options; // OTEL expects timestamps in ms, not seconds @@ -203,7 +188,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi sampled: propagationContext.sampled, }); - const spanOptions: SpanContext = { + const spanContext: SpanContext = { traceId: propagationContext.traceId, spanId: propagationContext.parentSpanId || propagationContext.spanId, isRemote: true, @@ -212,7 +197,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi }; // Add remote parent span context, - return trace.setSpanContext(ctx, spanOptions); + return trace.setSpanContext(ctx, spanContext); } // if we have no scope or client, we just return the context as-is @@ -245,7 +230,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi sampled, }); - const spanOptions: SpanContext = { + const spanContext: SpanContext = { traceId, spanId, isRemote: true, @@ -253,7 +238,7 @@ function getContext(scope: Scope | undefined, forceTransaction: boolean | undefi traceState, }; - const ctxWithSpanContext = trace.setSpanContext(ctxWithoutSpan, spanOptions); + const ctxWithSpanContext = trace.setSpanContext(ctxWithoutSpan, spanContext); return ctxWithSpanContext; } @@ -285,13 +270,3 @@ 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 6fd4ada4dc46..2da8c7a8b511 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -310,21 +310,6 @@ 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[] = []; @@ -564,21 +549,6 @@ 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[] = []; @@ -843,23 +813,6 @@ 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 89e523f6c922..36e5f56355c9 100644 --- a/packages/types/src/startSpanOptions.ts +++ b/packages/types/src/startSpanOptions.ts @@ -1,5 +1,5 @@ import type { Scope } from './scope'; -import type { Span, SpanAttributes, SpanTimeInput } from './span'; +import type { SpanAttributes, SpanTimeInput } from './span'; export interface StartSpanOptions { /** A manually specified start time for the created `Span` object. */ @@ -17,12 +17,6 @@ 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.