From f533ed89e4674bb3acb3f0249b4c6ef4e0bf742a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 27 Feb 2024 08:43:57 +0000 Subject: [PATCH] feat(core): Allow to pass `forceTransaction` to `startSpan()` APIs (backport) This will ensure a span is sent as a transaction to Sentry. This only implements this option for the core implementation, not yet for OTEL - that is a follow up here: https://github.com/getsentry/sentry-javascript/pull/10807 fixes... --- packages/core/src/tracing/trace.ts | 129 +++++--- packages/core/src/tracing/transaction.ts | 4 +- packages/core/test/lib/tracing/trace.test.ts | 315 ++++++++++++++++++- packages/types/src/startSpanOptions.ts | 7 + 4 files changed, 403 insertions(+), 52 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 832180ef3c72..44171e934c7d 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -1,6 +1,7 @@ import type { Scope, Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types'; import { addNonEnumerableProperty, dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils'; +import { getDynamicSamplingContextFromSpan } from '.'; import { DEBUG_BUILD } from '../debug-build'; import { getCurrentScope, withScope } from '../exports'; @@ -8,9 +9,10 @@ import type { Hub } from '../hub'; import { runWithAsyncContext } from '../hub'; import { getIsolationScope } from '../hub'; import { getCurrentHub } from '../hub'; +import type { Scope as ScopeClass } from '../scope'; import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; -import { spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils'; +import { spanIsSampled, spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils'; /** * Wraps a function with a transaction/span and finishes the span after the function is done. @@ -40,8 +42,13 @@ export function trace( // eslint-disable-next-line deprecation/deprecation const parentSpan = scope.getSpan(); - const ctx = normalizeContext(context); - const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx); + const spanContext = normalizeContext(context); + const activeSpan = createChildSpanOrTransaction(hub, { + parentSpan, + spanContext, + forceTransaction: false, + scope, + }); // eslint-disable-next-line deprecation/deprecation scope.setSpan(activeSpan); @@ -73,7 +80,7 @@ export function trace( * and the `span` returned from the callback will be undefined. */ export function startSpan(context: StartSpanOptions, callback: (span: Span | undefined) => T): T { - const ctx = normalizeContext(context); + const spanContext = normalizeContext(context); return runWithAsyncContext(() => { return withScope(context.scope, scope => { @@ -83,10 +90,14 @@ export function startSpan(context: StartSpanOptions, callback: (span: Span | const parentSpan = scope.getSpan(); const shouldSkipSpan = context.onlyIfParent && !parentSpan; - const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx); - - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(activeSpan); + const activeSpan = shouldSkipSpan + ? undefined + : createChildSpanOrTransaction(hub, { + parentSpan, + spanContext, + forceTransaction: context.forceTransaction, + scope, + }); return handleCallbackErrors( () => callback(activeSpan), @@ -125,7 +136,7 @@ export function startSpanManual( context: StartSpanOptions, callback: (span: Span | undefined, finish: () => void) => T, ): T { - const ctx = normalizeContext(context); + const spanContext = normalizeContext(context); return runWithAsyncContext(() => { return withScope(context.scope, scope => { @@ -135,10 +146,14 @@ export function startSpanManual( const parentSpan = scope.getSpan(); const shouldSkipSpan = context.onlyIfParent && !parentSpan; - const activeSpan = shouldSkipSpan ? undefined : createChildSpanOrTransaction(hub, parentSpan, ctx); - - // eslint-disable-next-line deprecation/deprecation - scope.setSpan(activeSpan); + const activeSpan = shouldSkipSpan + ? undefined + : createChildSpanOrTransaction(hub, { + parentSpan, + spanContext, + forceTransaction: context.forceTransaction, + scope, + }); function finishAndSetSpan(): void { activeSpan && activeSpan.end(); @@ -175,7 +190,7 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined { return undefined; } - const ctx = normalizeContext(context); + const spanContext = normalizeContext(context); // eslint-disable-next-line deprecation/deprecation const hub = getCurrentHub(); const parentSpan = context.scope @@ -189,37 +204,19 @@ export function startInactiveSpan(context: StartSpanOptions): Span | undefined { return undefined; } - const isolationScope = getIsolationScope(); - const scope = getCurrentScope(); - - let span: Span | undefined; - - if (parentSpan) { - // eslint-disable-next-line deprecation/deprecation - span = parentSpan.startChild(ctx); - } else { - const { traceId, dsc, parentSpanId, sampled } = { - ...isolationScope.getPropagationContext(), - ...scope.getPropagationContext(), - }; - - // eslint-disable-next-line deprecation/deprecation - span = hub.startTransaction({ - traceId, - parentSpanId, - parentSampled: sampled, - ...ctx, - metadata: { - dynamicSamplingContext: dsc, - // eslint-disable-next-line deprecation/deprecation - ...ctx.metadata, - }, - }); - } + const scope = context.scope || getCurrentScope(); - setCapturedScopesOnSpan(span, scope, isolationScope); + // Even though we don't actually want to make this span active on the current scope, + // we need to make it active on a temporary scope that we use for event processing + // as otherwise, it won't pick the correct span for the event when processing it + const temporaryScope = (scope as ScopeClass).clone(); - return span; + return createChildSpanOrTransaction(hub, { + parentSpan, + spanContext, + forceTransaction: context.forceTransaction, + scope: temporaryScope, + }); } /** @@ -334,20 +331,46 @@ export const continueTrace: ContinueTrace = ( function createChildSpanOrTransaction( hub: Hub, - parentSpan: Span | undefined, - ctx: TransactionContext, + { + parentSpan, + spanContext, + forceTransaction, + scope, + }: { + parentSpan: Span | undefined; + spanContext: TransactionContext; + forceTransaction?: boolean; + scope: Scope; + }, ): Span | undefined { if (!hasTracingEnabled()) { return undefined; } const isolationScope = getIsolationScope(); - const scope = getCurrentScope(); let span: Span | undefined; - if (parentSpan) { + if (parentSpan && !forceTransaction) { + // eslint-disable-next-line deprecation/deprecation + span = parentSpan.startChild(spanContext); + } else if (parentSpan) { + // If we forced a transaction but have a parent span, make sure to continue from the parent span, not the scope + const dsc = getDynamicSamplingContextFromSpan(parentSpan); + const { traceId, spanId: parentSpanId } = parentSpan.spanContext(); + const sampled = spanIsSampled(parentSpan); + // eslint-disable-next-line deprecation/deprecation - span = parentSpan.startChild(ctx); + span = hub.startTransaction({ + traceId, + parentSpanId, + parentSampled: sampled, + ...spanContext, + metadata: { + dynamicSamplingContext: dsc, + // eslint-disable-next-line deprecation/deprecation + ...spanContext.metadata, + }, + }); } else { const { traceId, dsc, parentSpanId, sampled } = { ...isolationScope.getPropagationContext(), @@ -359,15 +382,21 @@ function createChildSpanOrTransaction( traceId, parentSpanId, parentSampled: sampled, - ...ctx, + ...spanContext, metadata: { dynamicSamplingContext: dsc, // eslint-disable-next-line deprecation/deprecation - ...ctx.metadata, + ...spanContext.metadata, }, }); } + // We always set this as active span on the scope + // In the case of this being an inactive span, we ensure to pass a detached scope in here in the first place + // But by having this here, we can ensure that the lookup through `getCapturedScopesOnSpan` results in the correct scope & span combo + // eslint-disable-next-line deprecation/deprecation + scope.setSpan(span); + setCapturedScopesOnSpan(span, scope, isolationScope); return span; diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index a399137d1301..8f82c9b03d95 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -330,7 +330,9 @@ export class Transaction extends SpanClass implements TransactionInterface { ...metadata, capturedSpanScope, capturedSpanIsolationScope, - dynamicSamplingContext: getDynamicSamplingContextFromSpan(this), + ...dropUndefinedKeys({ + dynamicSamplingContext: getDynamicSamplingContextFromSpan(this), + }), }, _metrics_summary: getMetricSummaryJsonForSpan(this), ...(source && { diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index bb5302bb54db..8dbc440d1b76 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -1,4 +1,4 @@ -import type { Span as SpanType } from '@sentry/types'; +import type { Event, Span as SpanType } from '@sentry/types'; import { Hub, SEMANTIC_ATTRIBUTE_SENTRY_OP, @@ -323,6 +323,110 @@ describe('startSpan', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to force a transaction with forceTransaction=true', async () => { + const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + + const transactionEvents: Event[] = []; + + client.addEventProcessor(event => { + if (event.type === 'transaction') { + transactionEvents.push(event); + } + return event; + }); + + startSpan({ name: 'outer transaction' }, () => { + startSpan({ name: 'inner span' }, () => { + startSpan({ name: 'inner transaction', forceTransaction: true }, () => { + startSpan({ name: 'inner span 2' }, () => { + // all good + }); + }); + }); + }); + + await client.flush(); + + const normalizedTransactionEvents = transactionEvents.map(event => { + return { + ...event, + spans: event.spans?.map(span => ({ name: spanToJSON(span).description, id: span.spanContext().spanId })), + sdkProcessingMetadata: { + dynamicSamplingContext: event.sdkProcessingMetadata?.dynamicSamplingContext, + }, + }; + }); + + expect(normalizedTransactionEvents).toHaveLength(2); + + const outerTransaction = normalizedTransactionEvents.find(event => event.transaction === 'outer transaction'); + const innerTransaction = normalizedTransactionEvents.find(event => event.transaction === 'inner transaction'); + + const outerTraceId = outerTransaction?.contexts?.trace?.trace_id; + // The inner transaction should be a child of the last span of the outer transaction + const innerParentSpanId = outerTransaction?.spans?.[0].id; + const innerSpanId = innerTransaction?.contexts?.trace?.span_id; + + expect(outerTraceId).toBeDefined(); + expect(innerParentSpanId).toBeDefined(); + expect(innerSpanId).toBeDefined(); + // inner span ID should _not_ be the parent span ID, but the id of the new span + expect(innerSpanId).not.toEqual(innerParentSpanId); + + expect(outerTransaction?.contexts).toEqual({ + trace: { + data: { + 'sentry.sample_rate': 1, + 'sentry.origin': 'manual', + }, + span_id: expect.any(String), + trace_id: expect.any(String), + origin: 'manual', + }, + }); + expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]); + expect(outerTransaction?.tags).toEqual({ + transaction: 'outer transaction', + }); + expect(outerTransaction?.sdkProcessingMetadata).toEqual({ + dynamicSamplingContext: { + environment: 'production', + trace_id: outerTraceId, + sample_rate: '1', + transaction: 'outer transaction', + sampled: 'true', + }, + }); + + expect(innerTransaction?.contexts).toEqual({ + trace: { + data: { + 'sentry.origin': 'manual', + }, + parent_span_id: innerParentSpanId, + span_id: expect.any(String), + trace_id: outerTraceId, + origin: 'manual', + }, + }); + expect(innerTransaction?.spans).toEqual([{ name: 'inner span 2', id: expect.any(String) }]); + expect(innerTransaction?.tags).toEqual({ + transaction: 'inner transaction', + }); + expect(innerTransaction?.sdkProcessingMetadata).toEqual({ + dynamicSamplingContext: { + environment: 'production', + trace_id: outerTraceId, + sample_rate: '1', + transaction: 'outer transaction', + sampled: 'true', + }, + }); + }); + it("picks up the trace id off the parent scope's propagation context", () => { expect.assertions(1); withScope(scope => { @@ -486,6 +590,114 @@ describe('startSpanManual', () => { expect(getActiveSpan()).toBe(undefined); }); + it('allows to force a transaction with forceTransaction=true', async () => { + const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + + const transactionEvents: Event[] = []; + + client.addEventProcessor(event => { + if (event.type === 'transaction') { + transactionEvents.push(event); + } + return event; + }); + + startSpanManual({ name: 'outer transaction' }, span => { + startSpanManual({ name: 'inner span' }, span => { + startSpanManual({ name: 'inner transaction', forceTransaction: true }, span => { + startSpanManual({ name: 'inner span 2' }, span => { + // all good + span?.end(); + }); + span?.end(); + }); + span?.end(); + }); + span?.end(); + }); + + await client.flush(); + + const normalizedTransactionEvents = transactionEvents.map(event => { + return { + ...event, + spans: event.spans?.map(span => ({ name: spanToJSON(span).description, id: span.spanContext().spanId })), + sdkProcessingMetadata: { + dynamicSamplingContext: event.sdkProcessingMetadata?.dynamicSamplingContext, + }, + }; + }); + + expect(normalizedTransactionEvents).toHaveLength(2); + + const outerTransaction = normalizedTransactionEvents.find(event => event.transaction === 'outer transaction'); + const innerTransaction = normalizedTransactionEvents.find(event => event.transaction === 'inner transaction'); + + const outerTraceId = outerTransaction?.contexts?.trace?.trace_id; + // The inner transaction should be a child of the last span of the outer transaction + const innerParentSpanId = outerTransaction?.spans?.[0].id; + const innerSpanId = innerTransaction?.contexts?.trace?.span_id; + + expect(outerTraceId).toBeDefined(); + expect(innerParentSpanId).toBeDefined(); + expect(innerSpanId).toBeDefined(); + // inner span ID should _not_ be the parent span ID, but the id of the new span + expect(innerSpanId).not.toEqual(innerParentSpanId); + + expect(outerTransaction?.contexts).toEqual({ + trace: { + data: { + 'sentry.sample_rate': 1, + 'sentry.origin': 'manual', + }, + span_id: expect.any(String), + trace_id: expect.any(String), + origin: 'manual', + }, + }); + expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]); + expect(outerTransaction?.tags).toEqual({ + transaction: 'outer transaction', + }); + expect(outerTransaction?.sdkProcessingMetadata).toEqual({ + dynamicSamplingContext: { + environment: 'production', + trace_id: outerTraceId, + sample_rate: '1', + transaction: 'outer transaction', + sampled: 'true', + }, + }); + + expect(innerTransaction?.contexts).toEqual({ + trace: { + data: { + 'sentry.origin': 'manual', + }, + parent_span_id: innerParentSpanId, + span_id: expect.any(String), + trace_id: outerTraceId, + origin: 'manual', + }, + }); + expect(innerTransaction?.spans).toEqual([{ name: 'inner span 2', id: expect.any(String) }]); + expect(innerTransaction?.tags).toEqual({ + transaction: 'inner transaction', + }); + expect(innerTransaction?.sdkProcessingMetadata).toEqual({ + dynamicSamplingContext: { + environment: 'production', + trace_id: outerTraceId, + sample_rate: '1', + transaction: 'outer transaction', + sampled: 'true', + }, + }); + }); + it('allows to pass a `startTime`', () => { const start = startSpanManual({ name: 'outer', startTime: [1234, 0] }, span => { span?.end(); @@ -586,6 +798,107 @@ describe('startInactiveSpan', () => { expect(getActiveSpan()).toBeUndefined(); }); + it('allows to force a transaction with forceTransaction=true', async () => { + const options = getDefaultTestClientOptions({ tracesSampleRate: 1.0 }); + client = new TestClient(options); + setCurrentClient(client); + client.init(); + + const transactionEvents: Event[] = []; + + client.addEventProcessor(event => { + if (event.type === 'transaction') { + transactionEvents.push(event); + } + return event; + }); + + startSpan({ name: 'outer transaction' }, () => { + startSpan({ name: 'inner span' }, () => { + const innerTransaction = startInactiveSpan({ name: 'inner transaction', forceTransaction: true }); + innerTransaction?.end(); + }); + }); + + await client.flush(); + + const normalizedTransactionEvents = transactionEvents.map(event => { + return { + ...event, + spans: event.spans?.map(span => ({ name: spanToJSON(span).description, id: span.spanContext().spanId })), + sdkProcessingMetadata: { + dynamicSamplingContext: event.sdkProcessingMetadata?.dynamicSamplingContext, + }, + }; + }); + + expect(normalizedTransactionEvents).toHaveLength(2); + + const outerTransaction = normalizedTransactionEvents.find(event => event.transaction === 'outer transaction'); + const innerTransaction = normalizedTransactionEvents.find(event => event.transaction === 'inner transaction'); + + const outerTraceId = outerTransaction?.contexts?.trace?.trace_id; + // The inner transaction should be a child of the last span of the outer transaction + const innerParentSpanId = outerTransaction?.spans?.[0].id; + const innerSpanId = innerTransaction?.contexts?.trace?.span_id; + + expect(outerTraceId).toBeDefined(); + expect(innerParentSpanId).toBeDefined(); + expect(innerSpanId).toBeDefined(); + // inner span ID should _not_ be the parent span ID, but the id of the new span + expect(innerSpanId).not.toEqual(innerParentSpanId); + + expect(outerTransaction?.contexts).toEqual({ + trace: { + data: { + 'sentry.sample_rate': 1, + 'sentry.origin': 'manual', + }, + span_id: expect.any(String), + trace_id: expect.any(String), + origin: 'manual', + }, + }); + expect(outerTransaction?.spans).toEqual([{ name: 'inner span', id: expect.any(String) }]); + expect(outerTransaction?.tags).toEqual({ + transaction: 'outer transaction', + }); + expect(outerTransaction?.sdkProcessingMetadata).toEqual({ + dynamicSamplingContext: { + environment: 'production', + trace_id: outerTraceId, + sample_rate: '1', + transaction: 'outer transaction', + sampled: 'true', + }, + }); + + expect(innerTransaction?.contexts).toEqual({ + trace: { + data: { + 'sentry.origin': 'manual', + }, + parent_span_id: innerParentSpanId, + span_id: expect.any(String), + trace_id: outerTraceId, + origin: 'manual', + }, + }); + expect(innerTransaction?.spans).toEqual([]); + expect(innerTransaction?.tags).toEqual({ + transaction: 'inner transaction', + }); + expect(innerTransaction?.sdkProcessingMetadata).toEqual({ + dynamicSamplingContext: { + environment: 'production', + trace_id: outerTraceId, + sample_rate: '1', + transaction: 'outer transaction', + sampled: 'true', + }, + }); + }); + it('allows to pass a `startTime`', () => { const span = startInactiveSpan({ name: 'outer', startTime: [1234, 0] }); expect(spanToJSON(span!).start_timestamp).toEqual(1234); diff --git a/packages/types/src/startSpanOptions.ts b/packages/types/src/startSpanOptions.ts index 57ff96b3169f..f095119c1ac5 100644 --- a/packages/types/src/startSpanOptions.ts +++ b/packages/types/src/startSpanOptions.ts @@ -20,6 +20,13 @@ export interface StartSpanOptions extends TransactionContext { /** An op for the span. This is a categorization for spans. */ op?: string; + /** + * 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. + * It is not guaranteed that a span started with this flag set to `true` will be sent as a transaction. + */ + forceTransaction?: boolean; + /** * The origin of the span - if it comes from auto instrumentation or manual instrumentation. *