feat(opentelemetry): Fix & align isolation scope usage in node-experimental (#10570)

Co-authored-by: Luca Forstner <[email protected]>

Author: mydea

packages/core/src/tracing/trace.ts

@@ -399,6 +399,7 @@ type SpanWithScopes = Span & {
   [ISOLATION_SCOPE_ON_START_SPAN_FIELD]?: Scope;
 };
 
+/** Store the scope & isolation scope for a span, which can the be used when it is finished. */
 function setCapturedScopesOnSpan(span: Span | undefined, scope: Scope, isolationScope: Scope): void {
   if (span) {
     addNonEnumerableProperty(span, ISOLATION_SCOPE_ON_START_SPAN_FIELD, isolationScope);

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<Event | null> {
     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);
   }
 }

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;

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<NodeExperimentalClient>();

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;
   /**

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;

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<NodeExperimentalClient>();
+
+    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);
 

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,11 @@ export function setOpenTelemetryContextAsyncContextStrategy(): void {
     const ctx = api.context.active();
 
     // We depend on the otelContextManager to handle the context/hub
-    return api.context.with(ctx, () => {
+    // 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();
     });
   }

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');

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<ContextManagerInstance extends ContextMa
       thisArg?: ThisParameterType<F>,
       ...args: A
     ): ReturnType<F> {
+      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,
+      );
     }
   }