fix(node-experimental): Ensure span.finish() works as expected (#8947)

For `Sentry.startSpan()` API, we need to ensure that when the Sentry
Span is finished, we also finish the OTEL Span.

Also adding some tests for these APIs.

Author: mydea

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

@@ -24,7 +24,7 @@ export function startActiveSpan<T>(context: TransactionContext, callback: (span:
     return callback(undefined);
   }
 
-  const name = context.description || context.op || '<unknown>';
+  const name = context.name || context.description || context.op || '<unknown>';
 
   return tracer.startActiveSpan(name, (span: OtelSpan): T => {
     const otelSpanId = span.spanContext().spanId;
@@ -82,18 +82,35 @@ export function startSpan(context: TransactionContext): Span | undefined {
     return undefined;
   }
 
-  const name = context.description || context.op || '<unknown>';
+  const name = context.name || context.description || context.op || '<unknown>';
   const otelSpan = tracer.startSpan(name);
 
   const otelSpanId = otelSpan.spanContext().spanId;
 
   const sentrySpan = _INTERNAL_getSentrySpan(otelSpanId);
 
-  if (sentrySpan && isTransaction(sentrySpan) && context.metadata) {
+  if (!sentrySpan) {
+    return undefined;
+  }
+
+  if (isTransaction(sentrySpan) && context.metadata) {
     sentrySpan.setMetadata(context.metadata);
   }
 
-  return sentrySpan;
+  // Monkey-patch `finish()` to finish the OTEL span instead
+  // This will also in turn finish the Sentry Span, so no need to call this ourselves
+  const wrappedSentrySpan = new Proxy(sentrySpan, {
+    get(target, prop, receiver) {
+      if (prop === 'finish') {
+        return () => {
+          otelSpan.end();
+        };
+      }
+      return Reflect.get(target, prop, receiver);
+    },
+  });
+
+  return wrappedSentrySpan;
 }
 
 /**

packages/node-experimental/test/helpers/mockSdkInit.ts

@@ -0,0 +1,13 @@
+import { init } from '../../src/sdk/init';
+import type { NodeExperimentalClientOptions } from '../../src/types';
+
+// eslint-disable-next-line no-var
+declare var global: any;
+
+const PUBLIC_DSN = 'https://username@domain/123';
+
+export function mockSdkInit(options?: Partial<NodeExperimentalClientOptions>) {
+  global.__SENTRY__ = {};
+
+  init({ dsn: PUBLIC_DSN, defaultIntegrations: false, ...options });
+}

packages/node-experimental/test/sdk.test.ts renamed to packages/node-experimental/test/sdk/init.test.ts

@@ -1,8 +1,8 @@
 import type { Integration } from '@sentry/types';
 
-import * as auto from '../src/integrations/getAutoPerformanceIntegrations';
-import { init } from '../src/sdk/init';
-import * as sdk from '../src/sdk/init';
+import * as auto from '../../src/integrations/getAutoPerformanceIntegrations';
+import * as sdk from '../../src/sdk/init';
+import { init } from '../../src/sdk/init';
 
 // eslint-disable-next-line no-var
 declare var global: any;

packages/node-experimental/test/sdk/trace.test.ts

@@ -0,0 +1,173 @@
+import { Span, Transaction } from '@sentry/core';
+
+import * as Sentry from '../../src';
+import { mockSdkInit } from '../helpers/mockSdkInit';
+
+describe('trace', () => {
+  beforeEach(() => {
+    mockSdkInit({ enableTracing: true });
+  });
+
+  describe('startActiveSpan', () => {
+    it('works with a sync callback', () => {
+      const spans: Span[] = [];
+
+      expect(Sentry.getActiveSpan()).toEqual(undefined);
+
+      Sentry.startActiveSpan({ name: 'outer' }, outerSpan => {
+        expect(outerSpan).toBeDefined();
+        spans.push(outerSpan!);
+
+        expect(outerSpan?.name).toEqual('outer');
+        expect(outerSpan).toBeInstanceOf(Transaction);
+        expect(Sentry.getActiveSpan()).toEqual(outerSpan);
+
+        Sentry.startActiveSpan({ name: 'inner' }, innerSpan => {
+          expect(innerSpan).toBeDefined();
+          spans.push(innerSpan!);
+
+          expect(innerSpan?.description).toEqual('inner');
+          expect(innerSpan).toBeInstanceOf(Span);
+          expect(innerSpan).not.toBeInstanceOf(Transaction);
+          expect(Sentry.getActiveSpan()).toEqual(innerSpan);
+        });
+      });
+
+      expect(Sentry.getActiveSpan()).toEqual(undefined);
+      expect(spans).toHaveLength(2);
+      const [outerSpan, innerSpan] = spans;
+
+      expect((outerSpan as Transaction).name).toEqual('outer');
+      expect(innerSpan.description).toEqual('inner');
+
+      expect(outerSpan.endTimestamp).toEqual(expect.any(Number));
+      expect(innerSpan.endTimestamp).toEqual(expect.any(Number));
+    });
+
+    it('works with an async callback', async () => {
+      const spans: Span[] = [];
+
+      expect(Sentry.getActiveSpan()).toEqual(undefined);
+
+      await Sentry.startActiveSpan({ name: 'outer' }, async outerSpan => {
+        expect(outerSpan).toBeDefined();
+        spans.push(outerSpan!);
+
+        await new Promise(resolve => setTimeout(resolve, 10));
+
+        expect(outerSpan?.name).toEqual('outer');
+        expect(outerSpan).toBeInstanceOf(Transaction);
+        expect(Sentry.getActiveSpan()).toEqual(outerSpan);
+
+        await Sentry.startActiveSpan({ name: 'inner' }, async innerSpan => {
+          expect(innerSpan).toBeDefined();
+          spans.push(innerSpan!);
+
+          await new Promise(resolve => setTimeout(resolve, 10));
+
+          expect(innerSpan?.description).toEqual('inner');
+          expect(innerSpan).toBeInstanceOf(Span);
+          expect(innerSpan).not.toBeInstanceOf(Transaction);
+          expect(Sentry.getActiveSpan()).toEqual(innerSpan);
+        });
+      });
+
+      expect(Sentry.getActiveSpan()).toEqual(undefined);
+      expect(spans).toHaveLength(2);
+      const [outerSpan, innerSpan] = spans;
+
+      expect((outerSpan as Transaction).name).toEqual('outer');
+      expect(innerSpan.description).toEqual('inner');
+
+      expect(outerSpan.endTimestamp).toEqual(expect.any(Number));
+      expect(innerSpan.endTimestamp).toEqual(expect.any(Number));
+    });
+
+    it('works with multiple parallel calls', () => {
+      const spans1: Span[] = [];
+      const spans2: Span[] = [];
+
+      expect(Sentry.getActiveSpan()).toEqual(undefined);
+
+      Sentry.startActiveSpan({ name: 'outer' }, outerSpan => {
+        expect(outerSpan).toBeDefined();
+        spans1.push(outerSpan!);
+
+        expect(outerSpan?.name).toEqual('outer');
+        expect(outerSpan).toBeInstanceOf(Transaction);
+        expect(Sentry.getActiveSpan()).toEqual(outerSpan);
+
+        Sentry.startActiveSpan({ name: 'inner' }, innerSpan => {
+          expect(innerSpan).toBeDefined();
+          spans1.push(innerSpan!);
+
+          expect(innerSpan?.description).toEqual('inner');
+          expect(innerSpan).toBeInstanceOf(Span);
+          expect(innerSpan).not.toBeInstanceOf(Transaction);
+          expect(Sentry.getActiveSpan()).toEqual(innerSpan);
+        });
+      });
+
+      Sentry.startActiveSpan({ name: 'outer2' }, outerSpan => {
+        expect(outerSpan).toBeDefined();
+        spans2.push(outerSpan!);
+
+        expect(outerSpan?.name).toEqual('outer2');
+        expect(outerSpan).toBeInstanceOf(Transaction);
+        expect(Sentry.getActiveSpan()).toEqual(outerSpan);
+
+        Sentry.startActiveSpan({ name: 'inner2' }, innerSpan => {
+          expect(innerSpan).toBeDefined();
+          spans2.push(innerSpan!);
+
+          expect(innerSpan?.description).toEqual('inner2');
+          expect(innerSpan).toBeInstanceOf(Span);
+          expect(innerSpan).not.toBeInstanceOf(Transaction);
+          expect(Sentry.getActiveSpan()).toEqual(innerSpan);
+        });
+      });
+
+      expect(Sentry.getActiveSpan()).toEqual(undefined);
+      expect(spans1).toHaveLength(2);
+      expect(spans2).toHaveLength(2);
+    });
+  });
+
+  describe('startSpan', () => {
+    it('works at the root', () => {
+      const span = Sentry.startSpan({ name: 'test' });
+
+      expect(span).toBeDefined();
+      expect(span).toBeInstanceOf(Transaction);
+      expect(span?.name).toEqual('test');
+      expect(span?.endTimestamp).toBeUndefined();
+      expect(Sentry.getActiveSpan()).toBeUndefined();
+
+      span?.finish();
+
+      expect(span?.endTimestamp).toEqual(expect.any(Number));
+      expect(Sentry.getActiveSpan()).toBeUndefined();
+    });
+
+    it('works as a child span', () => {
+      Sentry.startActiveSpan({ name: 'outer' }, outerSpan => {
+        expect(outerSpan).toBeDefined();
+        expect(Sentry.getActiveSpan()).toEqual(outerSpan);
+
+        const innerSpan = Sentry.startSpan({ name: 'test' });
+
+        expect(innerSpan).toBeDefined();
+        expect(innerSpan).toBeInstanceOf(Span);
+        expect(innerSpan).not.toBeInstanceOf(Transaction);
+        expect(innerSpan?.description).toEqual('test');
+        expect(innerSpan?.endTimestamp).toBeUndefined();
+        expect(Sentry.getActiveSpan()).toEqual(outerSpan);
+
+        innerSpan?.finish();
+
+        expect(innerSpan?.endTimestamp).toEqual(expect.any(Number));
+        expect(Sentry.getActiveSpan()).toEqual(outerSpan);
+      });
+    });
+  });
+});