From 8eccfe90009c48c659b27687470029c6e4016838 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Feb 2024 12:41:43 +0100 Subject: [PATCH 1/8] WIP: fix isolation span otel handling --- .../opentelemetry/src/asyncContextStrategy.ts | 3 +- packages/opentelemetry/src/constants.ts | 2 + packages/opentelemetry/src/contextManager.ts | 39 ++++- packages/opentelemetry/src/custom/client.ts | 4 +- packages/opentelemetry/src/custom/hub.ts | 5 +- .../test/integration/scope.test.ts | 155 +++++++++++++++++- .../test/integration/transactions.test.ts | 61 +++---- 7 files changed, 225 insertions(+), 44 deletions(-) diff --git a/packages/opentelemetry/src/asyncContextStrategy.ts b/packages/opentelemetry/src/asyncContextStrategy.ts index c4cc48c1cfb5..a548fc20116c 100644 --- a/packages/opentelemetry/src/asyncContextStrategy.ts +++ b/packages/opentelemetry/src/asyncContextStrategy.ts @@ -1,6 +1,7 @@ import * as api from '@opentelemetry/api'; import type { Hub, RunWithAsyncContextOptions } from '@sentry/core'; import { setAsyncContextStrategy } from '@sentry/core'; +import { SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY } from './constants'; import { getHubFromContext } from './utils/contextData'; @@ -30,7 +31,7 @@ export function setOpenTelemetryContextAsyncContextStrategy(): void { const ctx = api.context.active(); // We depend on the otelContextManager to handle the context/hub - return api.context.with(ctx, () => { + return api.context.with(ctx.setValue(SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY, true), () => { return callback(); }); } diff --git a/packages/opentelemetry/src/constants.ts b/packages/opentelemetry/src/constants.ts index 36c8a36f886c..d38f8a91f121 100644 --- a/packages/opentelemetry/src/constants.ts +++ b/packages/opentelemetry/src/constants.ts @@ -8,3 +8,5 @@ export const SENTRY_PROPAGATION_CONTEXT_CONTEXT_KEY = createContextKey('SENTRY_P /** Context Key to hold a Hub. */ export const SENTRY_HUB_CONTEXT_KEY = createContextKey('sentry_hub'); + +export const SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY = createContextKey('sentry_fork_isolation_scope'); diff --git a/packages/opentelemetry/src/contextManager.ts b/packages/opentelemetry/src/contextManager.ts index d904a4847a06..14c682da9fb4 100644 --- a/packages/opentelemetry/src/contextManager.ts +++ b/packages/opentelemetry/src/contextManager.ts @@ -1,14 +1,28 @@ import type { Context, ContextManager } from '@opentelemetry/api'; -import type { Carrier, Hub } from '@sentry/core'; -import { getCurrentHub, getHubFromCarrier } from '@sentry/core'; -import { ensureHubOnCarrier } from '@sentry/core'; +import type { Hub } from '@sentry/core'; +import { getCurrentHub } from '@sentry/core'; +import { SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY } from './constants'; +import { OpenTelemetryHub } from './custom/hub'; import { setHubOnContext } from './utils/contextData'; -function createNewHub(parent: Hub | undefined): Hub { - const carrier: Carrier = {}; - ensureHubOnCarrier(carrier, parent); - return getHubFromCarrier(carrier); +function createNewHub(parent: Hub | undefined, shouldForkIsolationScope: boolean): Hub { + if (parent) { + // eslint-disable-next-line deprecation/deprecation + const client = parent.getClient(); + // eslint-disable-next-line deprecation/deprecation + const scope = parent.getScope(); + // eslint-disable-next-line deprecation/deprecation + const isolationScope = parent.getIsolationScope(); + + return new OpenTelemetryHub( + client, + scope.clone(), + shouldForkIsolationScope ? isolationScope.clone() : isolationScope, + ); + } + + return new OpenTelemetryHub(); } // Typescript complains if we do not use `...args: any[]` for the mixin, with: @@ -46,11 +60,18 @@ export function wrapContextManagerClass, ...args: A ): ReturnType { + const shouldForkIsolationScope = context.getValue(SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY) === true; + // eslint-disable-next-line deprecation/deprecation const existingHub = getCurrentHub(); - const newHub = createNewHub(existingHub); + const newHub = createNewHub(existingHub, shouldForkIsolationScope); - return super.with(setHubOnContext(context, newHub), fn, thisArg, ...args); + return super.with( + setHubOnContext(context.deleteValue(SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY), newHub), + fn, + thisArg, + ...args, + ); } } diff --git a/packages/opentelemetry/src/custom/client.ts b/packages/opentelemetry/src/custom/client.ts index 5bf39208a79f..62088f8295f1 100644 --- a/packages/opentelemetry/src/custom/client.ts +++ b/packages/opentelemetry/src/custom/client.ts @@ -71,7 +71,7 @@ export function wrapClientClass< event: Event, hint: EventHint, scope?: Scope, - _isolationScope?: Scope, + isolationScope?: Scope, ): PromiseLike { let actualScope = scope; @@ -81,7 +81,7 @@ export function wrapClientClass< delete hint.captureContext; } - return super._prepareEvent(event, hint, actualScope); + return super._prepareEvent(event, hint, actualScope, isolationScope); } } diff --git a/packages/opentelemetry/src/custom/hub.ts b/packages/opentelemetry/src/custom/hub.ts index 6f1554eda7d2..db455cf01172 100644 --- a/packages/opentelemetry/src/custom/hub.ts +++ b/packages/opentelemetry/src/custom/hub.ts @@ -10,9 +10,10 @@ import { OpenTelemetryScope } from './scope'; * Exported only for testing */ export class OpenTelemetryHub extends Hub { - public constructor(client?: Client, scope: Scope = new OpenTelemetryScope()) { - super(client, scope); + public constructor(client?: Client, scope: Scope = new OpenTelemetryScope(), isolationScope?: Scope) { + super(client, scope, isolationScope); } + } /** diff --git a/packages/opentelemetry/test/integration/scope.test.ts b/packages/opentelemetry/test/integration/scope.test.ts index 791f93d75c54..6fdb242af2d8 100644 --- a/packages/opentelemetry/test/integration/scope.test.ts +++ b/packages/opentelemetry/test/integration/scope.test.ts @@ -1,4 +1,13 @@ -import { captureException, getClient, getCurrentHub, getCurrentScope, setTag, withScope } from '@sentry/core'; +import { + captureException, + getClient, + getCurrentHub, + getCurrentScope, + getIsolationScope, + setTag, + withIsolationScope, + withScope, +} from '@sentry/core'; import { OpenTelemetryHub } from '../../src/custom/hub'; import { OpenTelemetryScope } from '../../src/custom/scope'; @@ -111,6 +120,7 @@ describe('Integration | Scope', () => { tag1: 'val1', tag2: 'val2', tag3: 'val3', + tag4: 'val4', }, timestamp: expect.any(Number), transaction: 'outer', @@ -124,7 +134,7 @@ describe('Integration | Scope', () => { } }); - it('isolates parallel root scopes', async () => { + it('isolates parallel scopes', async () => { const beforeSend = jest.fn(() => null); const beforeSendTransaction = jest.fn(() => null); @@ -147,13 +157,19 @@ describe('Integration | Scope', () => { rootScope.setTag('tag1', 'val1'); + const initialIsolationScope = getIsolationScope(); + withScope(scope1 => { scope1.setTag('tag2', 'val2a'); + expect(getIsolationScope()).toBe(initialIsolationScope); + withScope(scope2 => { scope2.setTag('tag3', 'val3a'); startSpan({ name: 'outer' }, span => { + expect(getIsolationScope()).toBe(initialIsolationScope); + spanId1 = span.spanContext().spanId; traceId1 = span.spanContext().traceId; @@ -167,10 +183,141 @@ describe('Integration | Scope', () => { withScope(scope1 => { scope1.setTag('tag2', 'val2b'); + expect(getIsolationScope()).toBe(initialIsolationScope); + withScope(scope2 => { scope2.setTag('tag3', 'val3b'); startSpan({ name: 'outer' }, span => { + expect(getIsolationScope()).toBe(initialIsolationScope); + + spanId2 = span.spanContext().spanId; + traceId2 = span.spanContext().traceId; + + setTag('tag4', 'val4b'); + + captureException(error2); + }); + }); + }); + + await client.flush(); + + expect(beforeSend).toHaveBeenCalledTimes(2); + expect(beforeSend).toHaveBeenCalledWith( + expect.objectContaining({ + contexts: expect.objectContaining({ + trace: spanId1 + ? { + span_id: spanId1, + trace_id: traceId1, + } + : expect.any(Object), + }), + tags: { + tag1: 'val1', + tag2: 'val2a', + tag3: 'val3a', + tag4: 'val4a', + }, + }), + { + event_id: expect.any(String), + originalException: error1, + syntheticException: expect.any(Error), + }, + ); + + expect(beforeSend).toHaveBeenCalledWith( + expect.objectContaining({ + contexts: expect.objectContaining({ + trace: spanId2 + ? { + span_id: spanId2, + trace_id: traceId2, + parent_span_id: undefined, + } + : expect.any(Object), + }), + tags: { + tag1: 'val1', + tag2: 'val2b', + tag3: 'val3b', + tag4: 'val4b', + }, + }), + { + event_id: expect.any(String), + originalException: error2, + syntheticException: expect.any(Error), + }, + ); + + if (enableTracing) { + expect(beforeSendTransaction).toHaveBeenCalledTimes(2); + } + }); + + it('isolates parallel isolation scopes', async () => { + const beforeSend = jest.fn(() => null); + const beforeSendTransaction = jest.fn(() => null); + + mockSdkInit({ enableTracing, beforeSend, beforeSendTransaction }); + + // eslint-disable-next-line deprecation/deprecation + const hub = getCurrentHub(); + const client = getClient() as TestClientInterface; + const rootScope = getCurrentScope(); + + expect(hub).toBeInstanceOf(OpenTelemetryHub); + expect(rootScope).toBeInstanceOf(OpenTelemetryScope); + + const error1 = new Error('test error 1'); + const error2 = new Error('test error 2'); + let spanId1: string | undefined; + let spanId2: string | undefined; + let traceId1: string | undefined; + let traceId2: string | undefined; + + rootScope.setTag('tag1', 'val1'); + + const initialIsolationScope = getIsolationScope(); + initialIsolationScope.setTag('isolationTag1', 'val1'); + + withIsolationScope(scope1 => { + scope1.setTag('tag2', 'val2a'); + + expect(getIsolationScope()).not.toBe(initialIsolationScope); + getIsolationScope().setTag('isolationTag2', 'val2'); + + withScope(scope2 => { + scope2.setTag('tag3', 'val3a'); + + startSpan({ name: 'outer' }, span => { + expect(getIsolationScope()).not.toBe(initialIsolationScope); + + spanId1 = span.spanContext().spanId; + traceId1 = span.spanContext().traceId; + + setTag('tag4', 'val4a'); + + captureException(error1); + }); + }); + }); + + withIsolationScope(scope1 => { + scope1.setTag('tag2', 'val2b'); + + expect(getIsolationScope()).not.toBe(initialIsolationScope); + getIsolationScope().setTag('isolationTag2', 'val2b'); + + withScope(scope2 => { + scope2.setTag('tag3', 'val3b'); + + startSpan({ name: 'outer' }, span => { + expect(getIsolationScope()).not.toBe(initialIsolationScope); + spanId2 = span.spanContext().spanId; traceId2 = span.spanContext().traceId; @@ -199,6 +346,8 @@ describe('Integration | Scope', () => { tag2: 'val2a', tag3: 'val3a', tag4: 'val4a', + isolationTag1: 'val1', + isolationTag2: 'val2', }, }), { @@ -224,6 +373,8 @@ describe('Integration | Scope', () => { tag2: 'val2b', tag3: 'val3b', tag4: 'val4b', + isolationTag1: 'val1', + isolationTag2: 'val2b', }, }), { diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index d131f91ba0dc..bd2cd9b3642e 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -1,6 +1,6 @@ import { TraceFlags, context, trace } from '@opentelemetry/api'; import type { SpanProcessor } from '@opentelemetry/sdk-trace-base'; -import { addBreadcrumb, getClient, setTag } from '@sentry/core'; +import { addBreadcrumb, getClient, getIsolationScope, setTag, withIsolationScope } from '@sentry/core'; import type { PropagationContext, TransactionEvent } from '@sentry/types'; import { logger } from '@sentry/utils'; @@ -123,6 +123,7 @@ describe('Integration | Transactions', () => { start_timestamp: expect.any(Number), tags: { 'outer.tag': 'test value', + 'test.tag': 'test value', }, timestamp: expect.any(Number), transaction: 'test name', @@ -173,7 +174,7 @@ describe('Integration | Transactions', () => { ]); }); - it('correctly creates concurrent transaction & spans', async () => { + it('correctly creates concurrent transaction & spans xxx', async () => { const beforeSendTransaction = jest.fn(() => null); mockSdkInit({ enableTracing: true, beforeSendTransaction }); @@ -182,44 +183,48 @@ describe('Integration | Transactions', () => { addBreadcrumb({ message: 'test breadcrumb 1', timestamp: 123456 }); - startSpan({ op: 'test op', name: 'test name', source: 'task', origin: 'auto.test' }, span => { - addBreadcrumb({ message: 'test breadcrumb 2', timestamp: 123456 }); + withIsolationScope(() => { + startSpan({ op: 'test op', name: 'test name', source: 'task', origin: 'auto.test' }, span => { + addBreadcrumb({ message: 'test breadcrumb 2', timestamp: 123456 }); - span.setAttributes({ - 'test.outer': 'test value', - }); + span.setAttributes({ + 'test.outer': 'test value', + }); - const subSpan = startInactiveSpan({ name: 'inner span 1' }); - subSpan.end(); + const subSpan = startInactiveSpan({ name: 'inner span 1' }); + subSpan.end(); - setTag('test.tag', 'test value'); + setTag('test.tag', 'test value'); - startSpan({ name: 'inner span 2' }, innerSpan => { - addBreadcrumb({ message: 'test breadcrumb 3', timestamp: 123456 }); + startSpan({ name: 'inner span 2' }, innerSpan => { + addBreadcrumb({ message: 'test breadcrumb 3', timestamp: 123456 }); - innerSpan.setAttributes({ - 'test.inner': 'test value', + innerSpan.setAttributes({ + 'test.inner': 'test value', + }); }); }); }); - startSpan({ op: 'test op b', name: 'test name b' }, span => { - addBreadcrumb({ message: 'test breadcrumb 2b', timestamp: 123456 }); + withIsolationScope(() => { + startSpan({ op: 'test op b', name: 'test name b' }, span => { + addBreadcrumb({ message: 'test breadcrumb 2b', timestamp: 123456 }); - span.setAttributes({ - 'test.outer': 'test value b', - }); + span.setAttributes({ + 'test.outer': 'test value b', + }); - const subSpan = startInactiveSpan({ name: 'inner span 1b' }); - subSpan.end(); + const subSpan = startInactiveSpan({ name: 'inner span 1b' }); + subSpan.end(); - setTag('test.tag', 'test value b'); + setTag('test.tag', 'test value b'); - startSpan({ name: 'inner span 2b' }, innerSpan => { - addBreadcrumb({ message: 'test breadcrumb 3b', timestamp: 123456 }); + startSpan({ name: 'inner span 2b' }, innerSpan => { + addBreadcrumb({ message: 'test breadcrumb 3b', timestamp: 123456 }); - innerSpan.setAttributes({ - 'test.inner': 'test value b', + innerSpan.setAttributes({ + 'test.inner': 'test value b', + }); }); }); }); @@ -262,7 +267,7 @@ describe('Integration | Transactions', () => { }), ], start_timestamp: expect.any(Number), - tags: {}, + tags: { 'test.tag': 'test value' }, timestamp: expect.any(Number), transaction: 'test name', transaction_info: { source: 'task' }, @@ -308,7 +313,7 @@ describe('Integration | Transactions', () => { }), ], start_timestamp: expect.any(Number), - tags: {}, + tags: { 'test.tag': 'test value b' }, timestamp: expect.any(Number), transaction: 'test name b', transaction_info: { source: 'custom' }, From dad76f87ad65baf83bde8ae0931eceb44980a6d5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Feb 2024 12:51:28 +0100 Subject: [PATCH 2/8] further fix isolation scopes --- packages/core/src/tracing/index.ts | 1 + packages/core/src/tracing/trace.ts | 3 ++- packages/opentelemetry/src/index.ts | 2 +- packages/opentelemetry/src/spanExporter.ts | 27 +++++++++++++++----- packages/opentelemetry/src/spanProcessor.ts | 7 +++-- packages/opentelemetry/src/utils/spanData.ts | 27 ++++++++++++++++---- 6 files changed, 51 insertions(+), 16 deletions(-) diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts index c45389da53c6..58afa0041e68 100644 --- a/packages/core/src/tracing/index.ts +++ b/packages/core/src/tracing/index.ts @@ -22,6 +22,7 @@ export { startActiveSpan, startSpanManual, continueTrace, + setCapturedScopesOnSpan, } from './trace'; export { getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; export { setMeasurement } from './measurement'; diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 832180ef3c72..b9563d612296 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -399,7 +399,8 @@ type SpanWithScopes = Span & { [ISOLATION_SCOPE_ON_START_SPAN_FIELD]?: Scope; }; -function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void { +/** Store the scope & isolation scope for a span, which can the be used when it is finished. */ +export function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void { if (span) { addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, isolationScope); addNonEnumerableProperty(span, SCOPE_ON_START_SPAN_FIELD, scope); diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index 602dcdda6234..974d645277f9 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -12,7 +12,7 @@ export { getSpanParent, getSpanScope, setSpanMetadata, - getSpanFinishScope, + getSpanFinishScopes, } from './utils/spanData'; export { getPropagationContextFromContext, setPropagationContextOnContext, setHubOnContext } from './utils/contextData'; diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index 97a7a77c26dc..e08cc6842592 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -3,7 +3,14 @@ import type { ExportResult } from '@opentelemetry/core'; import { ExportResultCode } from '@opentelemetry/core'; import type { ReadableSpan, SpanExporter } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, flush, getCurrentHub, getCurrentScope } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, + flush, + getCurrentHub, + getCurrentScope, + getIsolationScope, + setCapturedScopesOnSpan, +} from '@sentry/core'; import type { Scope, Span as SentrySpan, SpanOrigin, TransactionSource } from '@sentry/types'; import { logger } from '@sentry/utils'; @@ -17,7 +24,7 @@ import type { SpanNode } from './utils/groupSpansWithParents'; import { groupSpansWithParents } from './utils/groupSpansWithParents'; import { mapStatus } from './utils/mapStatus'; import { parseSpanDescription } from './utils/parseSpanDescription'; -import { getSpanFinishScope, getSpanHub, getSpanMetadata, getSpanScope } from './utils/spanData'; +import { getSpanFinishScopes, getSpanHub, getSpanMetadata, getSpanScope } from './utils/spanData'; type SpanNodeCompleted = SpanNode & { span: ReadableSpan }; @@ -109,8 +116,10 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] { // Now finish the transaction, which will send it together with all the spans // We make sure to use the finish scope - const scope = getScopeForTransactionFinish(span); - transaction.finishWithScope(convertOtelTimeToSeconds(span.endTime), scope); + const { scope, isolationScope } = getScopesForTransactionFinish(span); + setCapturedScopesOnSpan(transaction, scope, isolationScope); + + transaction.end(span.endTime); }); return Array.from(remaining) @@ -118,15 +127,19 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] { .filter((span): span is ReadableSpan => !!span); } -function getScopeForTransactionFinish(span: ReadableSpan): Scope { +function getScopesForTransactionFinish(span: ReadableSpan): { scope: Scope; isolationScope: Scope } { // The finish scope should normally always be there (and it is already a clone), // but for the sake of type safety we fall back to a clone of the current scope - const scope = getSpanFinishScope(span) || getCurrentScope().clone(); + const scopes = getSpanFinishScopes(span); + const scope = scopes?.scope || getCurrentScope().clone(); + const isolationScope = scopes?.isolationScope || getIsolationScope(); + scope.setContext('otel', { attributes: removeSentryAttributes(span.attributes), resource: span.resource.attributes, }); - return scope; + + return { scope, isolationScope }; } function getCompletedRootNodes(nodes: SpanNode[]): SpanNodeCompleted[] { diff --git a/packages/opentelemetry/src/spanProcessor.ts b/packages/opentelemetry/src/spanProcessor.ts index 7cb452a03d98..7ee770b046fa 100644 --- a/packages/opentelemetry/src/spanProcessor.ts +++ b/packages/opentelemetry/src/spanProcessor.ts @@ -10,7 +10,7 @@ import { DEBUG_BUILD } from './debug-build'; import { SentrySpanExporter } from './spanExporter'; import { maybeCaptureExceptionForTimedEvent } from './utils/captureExceptionForTimedEvent'; import { getHubFromContext } from './utils/contextData'; -import { getSpanHub, setSpanFinishScope, setSpanHub, setSpanParent, setSpanScope } from './utils/spanData'; +import { getSpanHub, setSpanFinishScopes, setSpanHub, setSpanParent, setSpanScope } from './utils/spanData'; function onSpanStart(span: Span, parentContext: Context, _ScopeClass: typeof OpenTelemetryScope): void { // This is a reliable way to get the parent span - because this is exactly how the parent is identified in the OTEL SDK @@ -34,13 +34,16 @@ function onSpanStart(span: Span, parentContext: Context, _ScopeClass: typeof Ope if (actualHub) { // eslint-disable-next-line deprecation/deprecation const scope = actualHub.getScope(); + // eslint-disable-next-line deprecation/deprecation + const isolationScope = actualHub.getIsolationScope(); setSpanScope(span, scope); setSpanHub(span, actualHub); // Use this scope for finishing the span const finishScope = (scope as OpenTelemetryScope).clone(); + // this is needed for breadcrumbs, for now, as they are stored on the span currently finishScope.activeSpan = span; - setSpanFinishScope(span, finishScope); + setSpanFinishScopes(span, { scope: finishScope, isolationScope }); } } diff --git a/packages/opentelemetry/src/utils/spanData.ts b/packages/opentelemetry/src/utils/spanData.ts index 18d9661a6488..2dc1e637acdf 100644 --- a/packages/opentelemetry/src/utils/spanData.ts +++ b/packages/opentelemetry/src/utils/spanData.ts @@ -7,7 +7,13 @@ import type { AbstractSpan } from '../types'; // This way we can enhance the data that an OTEL Span natively gives us // and since we are using weakmaps, we do not need to clean up after ourselves const SpanScope = new WeakMap(); -const SpanFinishScope = new WeakMap(); +const SpanFinishScopes = new WeakMap< + AbstractSpan, + { + scope: Scope; + isolationScope: Scope; + } +>(); const SpanHub = new WeakMap(); const SpanParent = new WeakMap(); const SpanMetadata = new WeakMap>(); @@ -53,11 +59,22 @@ export function getSpanMetadata(span: AbstractSpan): Partial Date: Thu, 8 Feb 2024 13:10:35 +0100 Subject: [PATCH 3/8] fix it in node-experimental --- packages/node-experimental/src/sdk/client.ts | 6 +- packages/node-experimental/src/sdk/scope.ts | 3 - .../src/sdk/spanProcessor.ts | 16 +- packages/node-experimental/src/sdk/types.ts | 2 - .../test/integration/transactions.test.ts | 157 +++++++++++++++++- packages/opentelemetry/src/custom/hub.ts | 1 - .../test/integration/transactions.test.ts | 2 +- 7 files changed, 159 insertions(+), 28 deletions(-) diff --git a/packages/node-experimental/src/sdk/client.ts b/packages/node-experimental/src/sdk/client.ts index 15d5720e4ac9..5e4ccafaa5d3 100644 --- a/packages/node-experimental/src/sdk/client.ts +++ b/packages/node-experimental/src/sdk/client.ts @@ -5,7 +5,7 @@ import { trace } from '@opentelemetry/api'; import type { BasicTracerProvider } from '@opentelemetry/sdk-trace-base'; import { applySdkMetadata } from '@sentry/core'; import type { CaptureContext, Event, EventHint } from '@sentry/types'; -import { Scope, getIsolationScope } from './scope'; +import { Scope } from './scope'; /** A client for using Sentry with Node & OpenTelemetry. */ export class NodeExperimentalClient extends NodeClient { @@ -54,7 +54,7 @@ export class NodeExperimentalClient extends NodeClient { event: Event, hint: EventHint, scope?: Scope, - _isolationScope?: Scope, + isolationScope?: Scope, ): PromiseLike { let actualScope = scope; @@ -64,8 +64,6 @@ export class NodeExperimentalClient extends NodeClient { delete hint.captureContext; } - const isolationScope = _isolationScope || (scope && scope.isolationScope) || getIsolationScope(); - return super._prepareEvent(event, hint, actualScope, isolationScope); } } diff --git a/packages/node-experimental/src/sdk/scope.ts b/packages/node-experimental/src/sdk/scope.ts index dfe19e63fdbe..2f8091586fbf 100644 --- a/packages/node-experimental/src/sdk/scope.ts +++ b/packages/node-experimental/src/sdk/scope.ts @@ -72,9 +72,6 @@ export function isInitialized(): boolean { /** A fork of the classic scope with some otel specific stuff. */ export class Scope extends OpenTelemetryScope implements ScopeInterface { - // Overwrite this if you want to use a specific isolation scope here - public isolationScope: Scope | undefined; - protected _client: Client | undefined; protected _lastEventId: string | undefined; diff --git a/packages/node-experimental/src/sdk/spanProcessor.ts b/packages/node-experimental/src/sdk/spanProcessor.ts index a85085077e94..f298d049f94b 100644 --- a/packages/node-experimental/src/sdk/spanProcessor.ts +++ b/packages/node-experimental/src/sdk/spanProcessor.ts @@ -1,13 +1,11 @@ -import type { Context } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import type { Span } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import { SentrySpanProcessor, getClient, getSpanFinishScope } from '@sentry/opentelemetry'; +import { SentrySpanProcessor, getClient } from '@sentry/opentelemetry'; import type { Http } from '../integrations/http'; import type { NodeFetch } from '../integrations/node-fetch'; import type { NodeExperimentalClient } from '../types'; -import { getIsolationScope } from './api'; import { Scope } from './scope'; /** @@ -18,18 +16,6 @@ export class NodeExperimentalSentrySpanProcessor extends SentrySpanProcessor { super({ scopeClass: Scope }); } - /** @inheritDoc */ - public onStart(span: Span, parentContext: Context): void { - super.onStart(span, parentContext); - - // We need to make sure that we use the correct isolation scope when finishing the span - // so we store it on the span finish scope for later use - const scope = getSpanFinishScope(span) as Scope | undefined; - if (scope) { - scope.isolationScope = getIsolationScope(); - } - } - /** @inheritDoc */ protected _shouldSendSpanToSentry(span: Span): boolean { const client = getClient(); diff --git a/packages/node-experimental/src/sdk/types.ts b/packages/node-experimental/src/sdk/types.ts index 57ad321a5470..37fd80bfab1a 100644 --- a/packages/node-experimental/src/sdk/types.ts +++ b/packages/node-experimental/src/sdk/types.ts @@ -28,8 +28,6 @@ export interface ScopeData { } export interface Scope extends BaseScope { - // @ts-expect-error typeof this is what we want here - isolationScope: typeof this | undefined; // @ts-expect-error typeof this is what we want here clone(scope?: Scope): typeof this; /** diff --git a/packages/node-experimental/test/integration/transactions.test.ts b/packages/node-experimental/test/integration/transactions.test.ts index d379070c4ee1..7de51c7811e6 100644 --- a/packages/node-experimental/test/integration/transactions.test.ts +++ b/packages/node-experimental/test/integration/transactions.test.ts @@ -99,7 +99,7 @@ describe('Integration | Transactions', () => { environment: 'production', event_id: expect.any(String), platform: 'node', - sdkProcessingMetadata: { + sdkProcessingMetadata: expect.objectContaining({ dynamicSamplingContext: expect.objectContaining({ environment: 'production', public_key: expect.any(String), @@ -112,7 +112,7 @@ describe('Integration | Transactions', () => { source: 'task', spanMetadata: expect.any(Object), requestPath: 'test-path', - }, + }), server_name: expect.any(String), // spans are circular (they have a reference to the transaction), which leads to jest choking on this // instead we compare them in detail below @@ -329,6 +329,159 @@ describe('Integration | Transactions', () => { ); }); + it('correctly creates concurrent transaction & spans when using native OTEL tracer', async () => { + const beforeSendTransaction = jest.fn(() => null); + + mockSdkInit({ enableTracing: true, beforeSendTransaction }); + + const client = Sentry.getClient(); + + Sentry.addBreadcrumb({ message: 'test breadcrumb 1', timestamp: 123456 }); + + Sentry.withIsolationScope(() => { + client.tracer.startActiveSpan('test name', span => { + Sentry.addBreadcrumb({ message: 'test breadcrumb 2', timestamp: 123456 }); + + span.setAttributes({ + 'test.outer': 'test value', + }); + + const subSpan = Sentry.startInactiveSpan({ name: 'inner span 1' }); + subSpan.end(); + + Sentry.setTag('test.tag', 'test value'); + + client.tracer.startActiveSpan('inner span 2', innerSpan => { + Sentry.addBreadcrumb({ message: 'test breadcrumb 3', timestamp: 123456 }); + + innerSpan.setAttributes({ + 'test.inner': 'test value', + }); + + innerSpan.end(); + }); + + span.end(); + }); + }); + + Sentry.withIsolationScope(() => { + client.tracer.startActiveSpan('test name b', span => { + Sentry.addBreadcrumb({ message: 'test breadcrumb 2b', timestamp: 123456 }); + + span.setAttributes({ + 'test.outer': 'test value b', + }); + + const subSpan = Sentry.startInactiveSpan({ name: 'inner span 1b' }); + subSpan.end(); + + Sentry.setTag('test.tag', 'test value b'); + + client.tracer.startActiveSpan('inner span 2b', innerSpan => { + Sentry.addBreadcrumb({ message: 'test breadcrumb 3b', timestamp: 123456 }); + + innerSpan.setAttributes({ + 'test.inner': 'test value b', + }); + + innerSpan.end(); + }); + + span.end(); + }); + }); + + await client.flush(); + + expect(beforeSendTransaction).toHaveBeenCalledTimes(2); + expect(beforeSendTransaction).toHaveBeenCalledWith( + expect.objectContaining({ + breadcrumbs: [ + { message: 'test breadcrumb 1', timestamp: 123456 }, + { message: 'test breadcrumb 2', timestamp: 123456 }, + { message: 'test breadcrumb 3', timestamp: 123456 }, + ], + contexts: expect.objectContaining({ + otel: expect.objectContaining({ + attributes: { + 'test.outer': 'test value', + }, + }), + trace: { + data: { + 'otel.kind': 'INTERNAL', + 'sentry.origin': 'manual', + }, + span_id: expect.any(String), + status: 'ok', + trace_id: expect.any(String), + origin: 'manual', + }, + }), + spans: [ + expect.objectContaining({ + description: 'inner span 1', + }), + expect.objectContaining({ + description: 'inner span 2', + }), + ], + start_timestamp: expect.any(Number), + tags: { 'test.tag': 'test value' }, + timestamp: expect.any(Number), + transaction: 'test name', + type: 'transaction', + }), + { + event_id: expect.any(String), + }, + ); + + expect(beforeSendTransaction).toHaveBeenCalledWith( + expect.objectContaining({ + breadcrumbs: [ + { message: 'test breadcrumb 1', timestamp: 123456 }, + { message: 'test breadcrumb 2b', timestamp: 123456 }, + { message: 'test breadcrumb 3b', timestamp: 123456 }, + ], + contexts: expect.objectContaining({ + otel: expect.objectContaining({ + attributes: { + 'test.outer': 'test value b', + }, + }), + trace: { + data: { + 'otel.kind': 'INTERNAL', + 'sentry.origin': 'manual', + }, + span_id: expect.any(String), + status: 'ok', + trace_id: expect.any(String), + origin: 'manual', + }, + }), + spans: [ + expect.objectContaining({ + description: 'inner span 1b', + }), + expect.objectContaining({ + description: 'inner span 2b', + }), + ], + start_timestamp: expect.any(Number), + tags: { 'test.tag': 'test value b' }, + timestamp: expect.any(Number), + transaction: 'test name b', + type: 'transaction', + }), + { + event_id: expect.any(String), + }, + ); + }); + it('correctly creates transaction & spans with a trace header data', async () => { const beforeSendTransaction = jest.fn(() => null); diff --git a/packages/opentelemetry/src/custom/hub.ts b/packages/opentelemetry/src/custom/hub.ts index db455cf01172..74e4c3c7b90e 100644 --- a/packages/opentelemetry/src/custom/hub.ts +++ b/packages/opentelemetry/src/custom/hub.ts @@ -13,7 +13,6 @@ export class OpenTelemetryHub extends Hub { public constructor(client?: Client, scope: Scope = new OpenTelemetryScope(), isolationScope?: Scope) { super(client, scope, isolationScope); } - } /** diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index bd2cd9b3642e..f228d545861b 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -1,6 +1,6 @@ import { TraceFlags, context, trace } from '@opentelemetry/api'; import type { SpanProcessor } from '@opentelemetry/sdk-trace-base'; -import { addBreadcrumb, getClient, getIsolationScope, setTag, withIsolationScope } from '@sentry/core'; +import { addBreadcrumb, getClient, setTag, withIsolationScope } from '@sentry/core'; import type { PropagationContext, TransactionEvent } from '@sentry/types'; import { logger } from '@sentry/utils'; From f0b98fb6a5bb05848b65a3e2004bc0105930ef36 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Feb 2024 13:15:32 +0100 Subject: [PATCH 4/8] remove unneeded transaction --- .../opentelemetry/src/custom/transaction.ts | 31 +--- packages/opentelemetry/src/spanExporter.ts | 9 +- .../test/custom/transaction.test.ts | 155 +----------------- 3 files changed, 12 insertions(+), 183 deletions(-) diff --git a/packages/opentelemetry/src/custom/transaction.ts b/packages/opentelemetry/src/custom/transaction.ts index fe5001f8b050..ceb950bc610d 100644 --- a/packages/opentelemetry/src/custom/transaction.ts +++ b/packages/opentelemetry/src/custom/transaction.ts @@ -1,7 +1,6 @@ import type { Hub } from '@sentry/core'; import { Transaction } from '@sentry/core'; -import type { ClientOptions, Hub as HubInterface, Scope, TransactionContext } from '@sentry/types'; -import { uuid4 } from '@sentry/utils'; +import type { ClientOptions, Hub as HubInterface, TransactionContext } from '@sentry/types'; /** * This is a fork of core's tracing/hubextensions.ts _startTransaction, @@ -13,7 +12,7 @@ export function startTransaction(hub: HubInterface, transactionContext: Transact const options: Partial = (client && client.getOptions()) || {}; // eslint-disable-next-line deprecation/deprecation - const transaction = new OpenTelemetryTransaction(transactionContext, hub as Hub); + const transaction = new Transaction(transactionContext, hub as Hub); // Since we do not do sampling here, we assume that this is _always_ sampled // Any sampling decision happens in OpenTelemetry's sampler transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number)); @@ -23,29 +22,3 @@ export function startTransaction(hub: HubInterface, transactionContext: Transact } return transaction; } - -/** - * This is a fork of the base Transaction with OTEL specific stuff added. - */ -export class OpenTelemetryTransaction extends Transaction { - /** - * Finish the transaction, but apply the given scope instead of the current one. - */ - public finishWithScope(endTimestamp?: number, scope?: Scope): string | undefined { - const event = this._finishTransaction(endTimestamp); - - if (!event) { - return undefined; - } - - // eslint-disable-next-line deprecation/deprecation - const client = this._hub.getClient(); - - if (!client) { - return undefined; - } - - const eventId = uuid4(); - return client.captureEvent(event, { event_id: eventId }, scope); - } -} diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index e08cc6842592..efded249036d 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -3,6 +3,8 @@ import type { ExportResult } from '@opentelemetry/core'; import { ExportResultCode } from '@opentelemetry/core'; import type { ReadableSpan, SpanExporter } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; +import type { + Transaction} from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, flush, @@ -13,9 +15,8 @@ import { } from '@sentry/core'; import type { Scope, Span as SentrySpan, SpanOrigin, TransactionSource } from '@sentry/types'; import { logger } from '@sentry/utils'; - -import type { OpenTelemetryTransaction } from './custom/transaction'; import { startTransaction } from './custom/transaction'; + import { DEBUG_BUILD } from './debug-build'; import { InternalSentrySemanticAttributes } from './semanticAttributes'; import { convertOtelTimeToSeconds } from './utils/convertOtelTimeToSeconds'; @@ -161,7 +162,7 @@ function parseSpan(span: ReadableSpan): { op?: string; origin?: SpanOrigin; sour return { origin, op, source }; } -function createTransactionForOtelSpan(span: ReadableSpan): OpenTelemetryTransaction { +function createTransactionForOtelSpan(span: ReadableSpan): Transaction { const scope = getSpanScope(span); // eslint-disable-next-line deprecation/deprecation const hub = getSpanHub(span) || getCurrentHub(); @@ -196,7 +197,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): OpenTelemetryTransact origin, tags, sampled: true, - }) as OpenTelemetryTransaction; + }); return transaction; } diff --git a/packages/opentelemetry/test/custom/transaction.test.ts b/packages/opentelemetry/test/custom/transaction.test.ts index ff76e2653d90..ae07939981cc 100644 --- a/packages/opentelemetry/test/custom/transaction.test.ts +++ b/packages/opentelemetry/test/custom/transaction.test.ts @@ -1,158 +1,13 @@ -import { getCurrentHub, setCurrentClient, spanToJSON } from '@sentry/core'; -import { OpenTelemetryScope } from '../../src/custom/scope'; -import { OpenTelemetryTransaction, startTransaction } from '../../src/custom/transaction'; +import { Transaction, getCurrentHub, setCurrentClient, spanToJSON } from '@sentry/core'; +import { startTransaction } from '../../src/custom/transaction'; import { TestClient, getDefaultTestClientOptions } from '../helpers/TestClient'; -describe('NodeExperimentalTransaction', () => { - afterEach(() => { - jest.resetAllMocks(); - }); - - it('works with finishWithScope without arguments', () => { - const client = new TestClient(getDefaultTestClientOptions()); - - const mockSend = jest.spyOn(client, 'captureEvent').mockImplementation(() => 'mocked'); - - // eslint-disable-next-line deprecation/deprecation - const hub = getCurrentHub(); - setCurrentClient(client); - client.init(); - - // eslint-disable-next-line deprecation/deprecation - const transaction = new OpenTelemetryTransaction({ name: 'test', sampled: true }, hub); - - const res = transaction.finishWithScope(); - - expect(mockSend).toBeCalledTimes(1); - expect(mockSend).toBeCalledWith( - expect.objectContaining({ - contexts: { - trace: { - data: { - 'sentry.origin': 'manual', - }, - span_id: expect.any(String), - trace_id: expect.any(String), - origin: 'manual', - }, - }, - spans: [], - start_timestamp: expect.any(Number), - tags: {}, - timestamp: expect.any(Number), - transaction: 'test', - type: 'transaction', - sdkProcessingMetadata: { - source: 'custom', - spanMetadata: {}, - dynamicSamplingContext: { - environment: 'production', - trace_id: expect.any(String), - transaction: 'test', - sampled: 'true', - }, - }, - transaction_info: { source: 'custom' }, - }), - { event_id: expect.any(String) }, - undefined, - ); - expect(res).toBe('mocked'); - }); - - it('works with finishWithScope with endTime', () => { - const client = new TestClient(getDefaultTestClientOptions()); - - const mockSend = jest.spyOn(client, 'captureEvent').mockImplementation(() => 'mocked'); - - // eslint-disable-next-line deprecation/deprecation - const hub = getCurrentHub(); - setCurrentClient(client); - client.init(); - - // eslint-disable-next-line deprecation/deprecation - const transaction = new OpenTelemetryTransaction({ name: 'test', startTimestamp: 123456, sampled: true }, hub); - - const res = transaction.finishWithScope(1234567); - - expect(mockSend).toBeCalledTimes(1); - expect(mockSend).toBeCalledWith( - expect.objectContaining({ - start_timestamp: 123456, - timestamp: 1234567, - }), - { event_id: expect.any(String) }, - undefined, - ); - expect(res).toBe('mocked'); - }); - - it('works with finishWithScope with endTime & scope', () => { - const client = new TestClient(getDefaultTestClientOptions()); - - const mockSend = jest.spyOn(client, 'captureEvent').mockImplementation(() => 'mocked'); - - // eslint-disable-next-line deprecation/deprecation - const hub = getCurrentHub(); - setCurrentClient(client); - client.init(); - - // eslint-disable-next-line deprecation/deprecation - const transaction = new OpenTelemetryTransaction({ name: 'test', startTimestamp: 123456, sampled: true }, hub); - - const scope = new OpenTelemetryScope(); - scope.setTags({ - tag1: 'yes', - tag2: 'no', - }); - scope.setContext('os', { name: 'Custom OS' }); - - const res = transaction.finishWithScope(1234567, scope); - - expect(mockSend).toBeCalledTimes(1); - expect(mockSend).toBeCalledWith( - expect.objectContaining({ - contexts: { - trace: { - data: { - 'sentry.origin': 'manual', - }, - span_id: expect.any(String), - trace_id: expect.any(String), - origin: 'manual', - }, - }, - spans: [], - start_timestamp: 123456, - tags: {}, - timestamp: 1234567, - transaction: 'test', - type: 'transaction', - sdkProcessingMetadata: { - source: 'custom', - spanMetadata: {}, - dynamicSamplingContext: { - environment: 'production', - trace_id: expect.any(String), - transaction: 'test', - sampled: 'true', - }, - }, - transaction_info: { source: 'custom' }, - }), - { event_id: expect.any(String) }, - scope, - ); - expect(res).toBe('mocked'); - }); -}); - describe('startTranscation', () => { afterEach(() => { jest.resetAllMocks(); }); - it('creates a NodeExperimentalTransaction', () => { + it('creates a Transaction', () => { const client = new TestClient(getDefaultTestClientOptions()); // eslint-disable-next-line deprecation/deprecation const hub = getCurrentHub(); @@ -161,7 +16,7 @@ describe('startTranscation', () => { const transaction = startTransaction(hub, { name: 'test' }); - expect(transaction).toBeInstanceOf(OpenTelemetryTransaction); + expect(transaction).toBeInstanceOf(Transaction); expect(transaction['_sampled']).toBe(undefined); // eslint-disable-next-line deprecation/deprecation expect(transaction.spanRecorder).toBeDefined(); @@ -197,7 +52,7 @@ describe('startTranscation', () => { traceId: 'trace1', }); - expect(transaction).toBeInstanceOf(OpenTelemetryTransaction); + expect(transaction).toBeInstanceOf(Transaction); // eslint-disable-next-line deprecation/deprecation expect(transaction.metadata).toEqual({ source: 'custom', From 822ec12933cb5fbf209df8b151fcf15e7fd7dd64 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Feb 2024 13:33:13 +0100 Subject: [PATCH 5/8] fix linting --- packages/opentelemetry/src/spanExporter.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index efded249036d..b6b6a89f753c 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -3,8 +3,7 @@ import type { ExportResult } from '@opentelemetry/core'; import { ExportResultCode } from '@opentelemetry/core'; import type { ReadableSpan, SpanExporter } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import type { - Transaction} from '@sentry/core'; +import type { Transaction } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, flush, From 38677d927097a3bc56d8538f0c44ba01bdc58e32 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Feb 2024 15:25:52 +0100 Subject: [PATCH 6/8] get rid of `setSpanScope` --- .../test/integration/scope.test.ts | 8 +++-- .../opentelemetry/src/asyncContextStrategy.ts | 4 +++ packages/opentelemetry/src/index.ts | 3 +- packages/opentelemetry/src/propagator.ts | 4 +-- packages/opentelemetry/src/spanExporter.ts | 6 ++-- packages/opentelemetry/src/spanProcessor.ts | 8 +++-- packages/opentelemetry/src/utils/spanData.ts | 31 +++++++------------ .../test/integration/scope.test.ts | 8 +++-- 8 files changed, 39 insertions(+), 33 deletions(-) diff --git a/packages/node-experimental/test/integration/scope.test.ts b/packages/node-experimental/test/integration/scope.test.ts index 6dfcad34ff81..163bed572f24 100644 --- a/packages/node-experimental/test/integration/scope.test.ts +++ b/packages/node-experimental/test/integration/scope.test.ts @@ -1,5 +1,5 @@ import { getCurrentScope, setGlobalScope } from '@sentry/core'; -import { getClient, getSpanScope } from '@sentry/opentelemetry'; +import { getClient, getSpanScopes } from '@sentry/opentelemetry'; import * as Sentry from '../../src/'; import type { NodeExperimentalClient } from '../../src/types'; @@ -41,7 +41,11 @@ describe('Integration | Scope', () => { scope2.setTag('tag3', 'val3'); Sentry.startSpan({ name: 'outer' }, span => { - expect(getSpanScope(span)).toBe(enableTracing ? scope2 : undefined); + // TODO: This is "incorrect" until we stop cloning the current scope for setSpanScopes + // Once we change this, the scopes _should_ be the same again + if (enableTracing) { + expect(getSpanScopes(span)?.scope).not.toBe(scope2); + } spanId = span.spanContext().spanId; traceId = span.spanContext().traceId; diff --git a/packages/opentelemetry/src/asyncContextStrategy.ts b/packages/opentelemetry/src/asyncContextStrategy.ts index a548fc20116c..f7523a557766 100644 --- a/packages/opentelemetry/src/asyncContextStrategy.ts +++ b/packages/opentelemetry/src/asyncContextStrategy.ts @@ -31,6 +31,10 @@ export function setOpenTelemetryContextAsyncContextStrategy(): void { const ctx = api.context.active(); // We depend on the otelContextManager to handle the context/hub + // We set the `SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY` context value, which is picked up by + // the OTEL context manager, which uses the presence of this key to determine if it should + // fork the isolation scope, or not + // as by default, we don't want to fork this, unless triggered explicitly by `runWithAsyncContext` return api.context.with(ctx.setValue(SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY, true), () => { return callback(); }); diff --git a/packages/opentelemetry/src/index.ts b/packages/opentelemetry/src/index.ts index 974d645277f9..f0b43ab8a386 100644 --- a/packages/opentelemetry/src/index.ts +++ b/packages/opentelemetry/src/index.ts @@ -10,9 +10,8 @@ export { getSpanHub, getSpanMetadata, getSpanParent, - getSpanScope, setSpanMetadata, - getSpanFinishScopes, + getSpanScopes, } from './utils/spanData'; export { getPropagationContextFromContext, setPropagationContextOnContext, setHubOnContext } from './utils/contextData'; diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts index 5ab31e3f4804..3091a4d7d399 100644 --- a/packages/opentelemetry/src/propagator.ts +++ b/packages/opentelemetry/src/propagator.ts @@ -7,7 +7,7 @@ import { SENTRY_BAGGAGE_KEY_PREFIX, generateSentryTraceHeader, propagationContex import { SENTRY_BAGGAGE_HEADER, SENTRY_TRACE_HEADER } from './constants'; import { getPropagationContextFromContext, setPropagationContextOnContext } from './utils/contextData'; -import { getSpanScope } from './utils/spanData'; +import { getSpanScopes } from './utils/spanData'; /** * Injects and extracts `sentry-trace` and `baggage` headers from carriers. @@ -91,7 +91,7 @@ function getDsc( // Else, we try to generate a new one const client = getClient(); const activeSpan = trace.getSpan(context); - const scope = activeSpan ? getSpanScope(activeSpan) : undefined; + const { scope } = (activeSpan && getSpanScopes(activeSpan)) || {}; if (client) { return getDynamicSamplingContextFromClient(traceId || propagationContext.traceId, client, scope); diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index b6b6a89f753c..6c081ae4948a 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -24,7 +24,7 @@ import type { SpanNode } from './utils/groupSpansWithParents'; import { groupSpansWithParents } from './utils/groupSpansWithParents'; import { mapStatus } from './utils/mapStatus'; import { parseSpanDescription } from './utils/parseSpanDescription'; -import { getSpanFinishScopes, getSpanHub, getSpanMetadata, getSpanScope } from './utils/spanData'; +import { getSpanHub, getSpanMetadata, getSpanScopes } from './utils/spanData'; type SpanNodeCompleted = SpanNode & { span: ReadableSpan }; @@ -130,7 +130,7 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] { function getScopesForTransactionFinish(span: ReadableSpan): { scope: Scope; isolationScope: Scope } { // The finish scope should normally always be there (and it is already a clone), // but for the sake of type safety we fall back to a clone of the current scope - const scopes = getSpanFinishScopes(span); + const scopes = getSpanScopes(span); const scope = scopes?.scope || getCurrentScope().clone(); const isolationScope = scopes?.isolationScope || getIsolationScope(); @@ -162,7 +162,7 @@ function parseSpan(span: ReadableSpan): { op?: string; origin?: SpanOrigin; sour } function createTransactionForOtelSpan(span: ReadableSpan): Transaction { - const scope = getSpanScope(span); + const { scope } = getSpanScopes(span) || {}; // eslint-disable-next-line deprecation/deprecation const hub = getSpanHub(span) || getCurrentHub(); const spanContext = span.spanContext(); diff --git a/packages/opentelemetry/src/spanProcessor.ts b/packages/opentelemetry/src/spanProcessor.ts index 7ee770b046fa..83aac51f9391 100644 --- a/packages/opentelemetry/src/spanProcessor.ts +++ b/packages/opentelemetry/src/spanProcessor.ts @@ -10,7 +10,7 @@ import { DEBUG_BUILD } from './debug-build'; import { SentrySpanExporter } from './spanExporter'; import { maybeCaptureExceptionForTimedEvent } from './utils/captureExceptionForTimedEvent'; import { getHubFromContext } from './utils/contextData'; -import { getSpanHub, setSpanFinishScopes, setSpanHub, setSpanParent, setSpanScope } from './utils/spanData'; +import { getSpanHub, setSpanHub, setSpanParent, setSpanScopes } from './utils/spanData'; function onSpanStart(span: Span, parentContext: Context, _ScopeClass: typeof OpenTelemetryScope): void { // This is a reliable way to get the parent span - because this is exactly how the parent is identified in the OTEL SDK @@ -36,14 +36,16 @@ function onSpanStart(span: Span, parentContext: Context, _ScopeClass: typeof Ope const scope = actualHub.getScope(); // eslint-disable-next-line deprecation/deprecation const isolationScope = actualHub.getIsolationScope(); - setSpanScope(span, scope); setSpanHub(span, actualHub); // Use this scope for finishing the span + // TODO: For now we need to clone this, as we need to store the `activeSpan` on it + // Once we can get rid of this (when we move breadcrumbs to the isolation scope), + // we can stop cloning this here const finishScope = (scope as OpenTelemetryScope).clone(); // this is needed for breadcrumbs, for now, as they are stored on the span currently finishScope.activeSpan = span; - setSpanFinishScopes(span, { scope: finishScope, isolationScope }); + setSpanScopes(span, { scope: finishScope, isolationScope }); } } diff --git a/packages/opentelemetry/src/utils/spanData.ts b/packages/opentelemetry/src/utils/spanData.ts index 2dc1e637acdf..174573bd19f9 100644 --- a/packages/opentelemetry/src/utils/spanData.ts +++ b/packages/opentelemetry/src/utils/spanData.ts @@ -3,11 +3,10 @@ import type { Hub, Scope, TransactionMetadata } from '@sentry/types'; import type { AbstractSpan } from '../types'; -// We store the parent span, scope & metadata in separate weakmaps, so we can access them for a given span +// We store the parent span, scopes & metadata in separate weakmaps, so we can access them for a given span // This way we can enhance the data that an OTEL Span natively gives us // and since we are using weakmaps, we do not need to clean up after ourselves -const SpanScope = new WeakMap(); -const SpanFinishScopes = new WeakMap< +const SpanScopes = new WeakMap< AbstractSpan, { scope: Scope; @@ -18,16 +17,6 @@ const SpanHub = new WeakMap(); const SpanParent = new WeakMap(); const SpanMetadata = new WeakMap>(); -/** Set the Sentry scope on an OTEL span. */ -export function setSpanScope(span: AbstractSpan, scope: Scope): void { - SpanScope.set(span, scope); -} - -/** Get the Sentry scope of an OTEL span. */ -export function getSpanScope(span: AbstractSpan): Scope | undefined { - return SpanScope.get(span); -} - /** Set the Sentry hub on an OTEL span. */ export function setSpanHub(span: AbstractSpan, hub: Hub): void { SpanHub.set(span, hub); @@ -58,23 +47,27 @@ export function getSpanMetadata(span: AbstractSpan): Partial { scope2.setTag('tag3', 'val3'); startSpan({ name: 'outer' }, span => { - expect(getSpanScope(span)).toBe(enableTracing ? scope2 : undefined); + // TODO: This is "incorrect" until we stop cloning the current scope for setSpanScopes + // Once we change this, the scopes _should_ be the same again + if (enableTracing) { + expect(getSpanScopes(span)?.scope).not.toBe(scope2); + } spanId = span.spanContext().spanId; traceId = span.spanContext().traceId; From 09e9f6fd826cc52487feaed5ab0038ec0f95258f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 8 Feb 2024 15:51:15 +0100 Subject: [PATCH 7/8] get rid of custom dsc code --- packages/opentelemetry/src/spanExporter.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index 6c081ae4948a..e424a4e39cfc 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -162,7 +162,6 @@ function parseSpan(span: ReadableSpan): { op?: string; origin?: SpanOrigin; sour } function createTransactionForOtelSpan(span: ReadableSpan): Transaction { - const { scope } = getSpanScopes(span) || {}; // eslint-disable-next-line deprecation/deprecation const hub = getSpanHub(span) || getCurrentHub(); const spanContext = span.spanContext(); @@ -171,7 +170,6 @@ function createTransactionForOtelSpan(span: ReadableSpan): Transaction { const parentSpanId = span.parentSpanId; const parentSampled = span.attributes[InternalSentrySemanticAttributes.PARENT_SAMPLED] as boolean | undefined; - const dynamicSamplingContext = scope ? scope.getPropagationContext().dsc : undefined; const { op, description, tags, data, origin, source } = getSpanData(span); const metadata = getSpanMetadata(span); @@ -187,7 +185,6 @@ function createTransactionForOtelSpan(span: ReadableSpan): Transaction { status: mapStatus(span), startTimestamp: convertOtelTimeToSeconds(span.startTime), metadata: { - dynamicSamplingContext, source, sampleRate: span.attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] as number | undefined, ...metadata, From 30abc45e48985df9900839610d7e189caade7422 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 9 Feb 2024 10:11:30 +0000 Subject: [PATCH 8/8] Pass the captured scopes to transaction without extending existing API surface --- packages/core/src/tracing/index.ts | 1 - packages/core/src/tracing/trace.ts | 2 +- packages/opentelemetry/src/spanExporter.ts | 55 ++++++++++------------ 3 files changed, 27 insertions(+), 31 deletions(-) diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts index 58afa0041e68..c45389da53c6 100644 --- a/packages/core/src/tracing/index.ts +++ b/packages/core/src/tracing/index.ts @@ -22,7 +22,6 @@ export { startActiveSpan, startSpanManual, continueTrace, - setCapturedScopesOnSpan, } from './trace'; export { getDynamicSamplingContextFromClient, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; export { setMeasurement } from './measurement'; diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index b9563d612296..a1327d0f737c 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -400,7 +400,7 @@ type SpanWithScopes = Span & { }; /** Store the scope & isolation scope for a span, which can the be used when it is finished. */ -export function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void { +function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void { if (span) { addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, isolationScope); addNonEnumerableProperty(span, SCOPE_ON_START_SPAN_FIELD, scope); diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index e424a4e39cfc..1f6792932dcd 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -4,16 +4,9 @@ import { ExportResultCode } from '@opentelemetry/core'; import type { ReadableSpan, SpanExporter } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import type { Transaction } from '@sentry/core'; -import { - SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, - flush, - getCurrentHub, - getCurrentScope, - getIsolationScope, - setCapturedScopesOnSpan, -} from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, flush, getCurrentHub } from '@sentry/core'; import type { Scope, Span as SentrySpan, SpanOrigin, TransactionSource } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import { addNonEnumerableProperty, logger } from '@sentry/utils'; import { startTransaction } from './custom/transaction'; import { DEBUG_BUILD } from './debug-build'; @@ -114,11 +107,6 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] { createAndFinishSpanForOtelSpan(child, transaction, remaining); }); - // Now finish the transaction, which will send it together with all the spans - // We make sure to use the finish scope - const { scope, isolationScope } = getScopesForTransactionFinish(span); - setCapturedScopesOnSpan(transaction, scope, isolationScope); - transaction.end(span.endTime); }); @@ -127,21 +115,6 @@ function maybeSend(spans: ReadableSpan[]): ReadableSpan[] { .filter((span): span is ReadableSpan => !!span); } -function getScopesForTransactionFinish(span: ReadableSpan): { scope: Scope; isolationScope: Scope } { - // The finish scope should normally always be there (and it is already a clone), - // but for the sake of type safety we fall back to a clone of the current scope - const scopes = getSpanScopes(span); - const scope = scopes?.scope || getCurrentScope().clone(); - const isolationScope = scopes?.isolationScope || getIsolationScope(); - - scope.setContext('otel', { - attributes: removeSentryAttributes(span.attributes), - resource: span.resource.attributes, - }); - - return { scope, isolationScope }; -} - function getCompletedRootNodes(nodes: SpanNode[]): SpanNodeCompleted[] { return nodes.filter((node): node is SpanNodeCompleted => !!node.span && !node.parentNode); } @@ -173,6 +146,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): Transaction { const { op, description, tags, data, origin, source } = getSpanData(span); const metadata = getSpanMetadata(span); + const capturedSpanScopes = getSpanScopes(span); const transaction = startTransaction(hub, { spanId, @@ -195,6 +169,18 @@ function createTransactionForOtelSpan(span: ReadableSpan): Transaction { sampled: true, }); + // We currently don't want to write this to the scope because it would mutate it. + // In the future we will likely have some sort of transaction payload factory where we can pass this context in directly + // eslint-disable-next-line deprecation/deprecation + transaction.setContext('otel', { + attributes: removeSentryAttributes(span.attributes), + resource: span.resource.attributes, + }); + + if (capturedSpanScopes) { + setCapturedScopesOnTransaction(transaction, capturedSpanScopes.scope, capturedSpanScopes.isolationScope); + } + return transaction; } @@ -321,3 +307,14 @@ function getData(span: ReadableSpan): Record { return data; } + +const SCOPE_ON_START_SPAN_FIELD = '_sentryScope'; +const ISOLATION_SCOPE_ON_START_SPAN_FIELD = '_sentryIsolationScope'; + +/** Sets the scope and isolation scope to be used for when the transaction is finished. */ +function setCapturedScopesOnTransaction(span: Transaction, scope: Scope, isolationScope: Scope): void { + if (span) { + addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, isolationScope); + addNonEnumerableProperty(span, SCOPE_ON_START_SPAN_FIELD, scope); + } +}