From 7f7a23b68f865858a7ca922bcedd7a4101117632 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 5 Sep 2023 14:23:44 +0200 Subject: [PATCH] fix(node-experimental): Ensure `span.finish()` works as expected 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. --- packages/node-experimental/src/sdk/trace.ts | 25 ++- .../test/helpers/mockSdkInit.ts | 13 ++ .../test/{sdk.test.ts => sdk/init.test.ts} | 6 +- .../node-experimental/test/sdk/trace.test.ts | 173 ++++++++++++++++++ 4 files changed, 210 insertions(+), 7 deletions(-) create mode 100644 packages/node-experimental/test/helpers/mockSdkInit.ts rename packages/node-experimental/test/{sdk.test.ts => sdk/init.test.ts} (96%) create mode 100644 packages/node-experimental/test/sdk/trace.test.ts diff --git a/packages/node-experimental/src/sdk/trace.ts b/packages/node-experimental/src/sdk/trace.ts index 8585c0fa8597..a086b8edd2c2 100644 --- a/packages/node-experimental/src/sdk/trace.ts +++ b/packages/node-experimental/src/sdk/trace.ts @@ -24,7 +24,7 @@ export function startActiveSpan(context: TransactionContext, callback: (span: return callback(undefined); } - const name = context.description || context.op || ''; + const name = context.name || context.description || context.op || ''; 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 || ''; + const name = context.name || context.description || context.op || ''; 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; } /** diff --git a/packages/node-experimental/test/helpers/mockSdkInit.ts b/packages/node-experimental/test/helpers/mockSdkInit.ts new file mode 100644 index 000000000000..f7bfb68f6bf6 --- /dev/null +++ b/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) { + global.__SENTRY__ = {}; + + init({ dsn: PUBLIC_DSN, defaultIntegrations: false, ...options }); +} diff --git a/packages/node-experimental/test/sdk.test.ts b/packages/node-experimental/test/sdk/init.test.ts similarity index 96% rename from packages/node-experimental/test/sdk.test.ts rename to packages/node-experimental/test/sdk/init.test.ts index ea230368b1d8..a9c2a11885a8 100644 --- a/packages/node-experimental/test/sdk.test.ts +++ b/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; diff --git a/packages/node-experimental/test/sdk/trace.test.ts b/packages/node-experimental/test/sdk/trace.test.ts new file mode 100644 index 000000000000..87aae6c66689 --- /dev/null +++ b/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); + }); + }); + }); +});