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

mydea · 2024-02-08T12:16:58Z

This PR aligns - and actually fixes - isolation scope usage in both @sentry/opentelemetry and @sentry/node-experimental.

Since we've added a lot of this in the main SDK now, we can get rid of a bunch of our special handling in the otel SDKs.
Also, I noticed that in opentelemetry (when not using node-experimental) the isolation scope was not properly used before - this is fixed now.

This allows to remove a bunch of stuff, like the custom transaction fork (scopes can be handled by setCapturedScopesOnSpan() natively).

mydea · 2024-02-08T12:17:17Z

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 {


we need this in otel!

h: We should just vendor this in the otel package. This function should not be exposed to the user.

not sure, this is subject to break if we do that as that is not typed anywhere properly and relies purely on casting stuff around etc 😬 we need this to be the exact same implementation in node-experimental as here, as we need this to be picked up by getCaputredScopesOnSpan().

mydea · 2024-02-08T12:17:35Z

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;


we don't need this anymore 🚀

mydea · 2024-02-08T12:17:50Z

packages/node-experimental/src/sdk/spanProcessor.ts

-    // so we store it on the span finish scope for later use
-    const scope = getSpanFinishScope(span) as Scope | undefined;
-    if (scope) {
-      scope.isolationScope = getIsolationScope();


this is simply handled in the base span processor now.

mydea · 2024-02-08T12:18:04Z

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({


this has some more fields now, but we don't care about that.

mydea · 2024-02-08T12:18:14Z

packages/node-experimental/test/integration/transactions.test.ts

@@ -329,6 +329,159 @@ describe('Integration | Transactions', () => {
    );
  });

+  it('correctly creates concurrent transaction & spans when using native OTEL tracer', async () => {


added a test to actually check that it works with native otel tracing too.

mydea · 2024-02-08T12:18:37Z

packages/opentelemetry/src/asyncContextStrategy.ts

@@ -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), () => {


we use this attribute to tell the otel context manager if the isolation scope should be forked or not.

Can we explain this a bit further in comments? I still don't fully get it.

mydea · 2024-02-08T12:19:15Z

packages/opentelemetry/src/contextManager.ts

-  const carrier: Carrier = {};
-  ensureHubOnCarrier(carrier, parent);
-  return getHubFromCarrier(carrier);
+function createNewHub(parent: Hub | undefined, shouldForkIsolationScope: boolean): Hub {


ensureHubOnCarrier used to always fork the isolation scope, which is not what we want here. Now, we only fork this if withIsolationScope is used.

mydea · 2024-02-08T12:19:30Z

packages/opentelemetry/src/custom/client.ts

@@ -81,7 +81,7 @@ export function wrapClientClass<
        delete hint.captureContext;
      }

-      return super._prepareEvent(event, hint, actualScope);
+      return super._prepareEvent(event, hint, actualScope, isolationScope);


this was not correctly passed through here before, oops.

mydea · 2024-02-08T12:19:49Z

packages/opentelemetry/src/custom/hub.ts

@@ -10,8 +10,8 @@ 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) {


we accept this in the upstream implementation too.

mydea · 2024-02-08T12:20:17Z

packages/opentelemetry/src/custom/transaction.ts

-/**
- * This is a fork of the base Transaction with OTEL specific stuff added.
- */
-export class OpenTelemetryTransaction extends Transaction {


not needed anymore - we just leverage this in the core implementation through getCapturedScopesOnSpan!

mydea · 2024-02-08T12:21:10Z

packages/opentelemetry/test/integration/scope.test.ts

@@ -111,6 +120,7 @@ describe('Integration | Scope', () => {
              tag1: 'val1',
              tag2: 'val2',
              tag3: 'val3',
+              tag4: 'val4',


this aligns the behavior with the core one, as Sentry.setTag() sets on the isolation scope which will be applied to the transaction too.

github-actions · 2024-02-08T12:22:40Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	78.16 KB (-0.22% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	69.37 KB (-0.29% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped)	73.31 KB (-0.25% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	63.01 KB (-0.26% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	33.35 KB (-0.5% 🔽)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped)	33.22 KB (-0.51% 🔽)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	31.2 KB (-0.54% 🔽)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped)	31.21 KB (-0.53% 🔽)
@sentry/browser - Webpack (gzipped)	22.48 KB (-0.69% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	76.19 KB (-0.23% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	67.71 KB (-0.28% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	33.47 KB (-0.56% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped)	24.56 KB (-0.7% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	213.68 KB (-0.19% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	101.48 KB (-0.39% 🔽)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	73.86 KB (-0.54% 🔽)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	36.64 KB (-0.44% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	69.76 KB (-0.24% 🔽)
@sentry/react - Webpack (gzipped)	22.52 KB (-0.66% 🔽)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	87.33 KB (-0.26% 🔽)
@sentry/nextjs Client - Webpack (gzipped)	51.52 KB (-0.41% 🔽)
@sentry-internal/feedback - Webpack (gzipped)	17.2 KB (-0.1% 🔽)

lforst · 2024-02-08T12:25:29Z

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 {


h: We should just vendor this in the otel package. This function should not be exposed to the user.

lforst · 2024-02-08T12:28:36Z

packages/opentelemetry/src/utils/spanData.ts

+const SpanFinishScopes = new WeakMap<
+  AbstractSpan,
+  {
+    scope: Scope;
+    isolationScope: Scope;
+  }
+>();


h: Do we really need this if the span has the scopes attached to it already? Also, how is this different from SpanScope and setSpanScope?

the tricky thing here is that we use this here to "cache" the scopes on the OTEL spans, then we use setCapturedScopes() to set them on the sentry span when we create it in the span exporter.

Once we have aligned our span types, we can probably use setCapturedScopes here as well - but we'll still need to pick them up in the span exporter via getCapturedScopes and set them again on the sentry span before we send it.

we can probably get rid of getSpanScope actually, I'll check it out!

lforst · 2024-02-08T12:29:34Z

packages/opentelemetry/src/asyncContextStrategy.ts

@@ -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), () => {


Can we explain this a bit further in comments? I still don't fully get it.

…al-isolation-scope

…I surface

lforst · 2024-02-09T10:30:23Z

I took the liberty of vendoring the code to pass the scope and isolation scope for the transaction into the otel package. This turns it into an implementation detail instead of an API. I do respect your opinion though so I implemented it in a way so that it can be revisited any time in the future.

Also, I will merge this PR because I need it for #10572 :)

mydea requested review from lforst, Lms24 and AbhiPrasad February 8, 2024 12:16

mydea self-assigned this Feb 8, 2024

mydea commented Feb 8, 2024

View reviewed changes

Lms24 approved these changes Feb 8, 2024

View reviewed changes

lforst reviewed Feb 8, 2024

View reviewed changes

mydea added 7 commits February 8, 2024 15:47

WIP: fix isolation span otel handling

8eccfe9

further fix isolation scopes

dad76f8

fix it in node-experimental

13f9f23

remove unneeded transaction

f0b98fb

fix linting

822ec12

get rid of setSpanScope

38677d9

get rid of custom dsc code

09e9f6f

mydea force-pushed the fn/node-experimental-isolation-scope branch from c9f8c44 to 09e9f6f Compare February 8, 2024 14:57

Luca Forstner added 2 commits February 9, 2024 09:29

Merge remote-tracking branch 'origin/develop' into fn/node-experiment…

9a6bc55

…al-isolation-scope

Pass the captured scopes to transaction without extending existing AP…

30abc45

…I surface

lforst merged commit 5a2b676 into develop Feb 9, 2024

lforst deleted the fn/node-experimental-isolation-scope branch February 9, 2024 10:30

Uh oh!

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

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

Uh oh!

Conversation

mydea commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lforst commented Feb 9, 2024

Uh oh!

Uh oh!

github-actions bot commented Feb 8, 2024 •

edited

Loading