From 44780077ace7283fb46dd40312ed668c1fdc2412 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 11:56:34 +0200 Subject: [PATCH 1/4] fix(node): Ensure isolation scope is correctly cloned for non-recording spans --- .../server.ts | 23 +++++++++++ .../handle-error-no-tracesSampleRate/test.ts | 39 +++++++++++++++++++ .../server.ts | 5 ++- .../test.ts | 4 +- packages/node/src/integrations/http.ts | 3 +- .../node/src/integrations/tracing/express.ts | 7 +++- 6 files changed, 75 insertions(+), 6 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/server.ts create mode 100644 dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/test.ts rename dev-packages/node-integration-tests/suites/express/{handle-error => handle-error-tracesSampleRate-0}/server.ts (81%) rename dev-packages/node-integration-tests/suites/express/{handle-error => handle-error-tracesSampleRate-0}/test.ts (87%) diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/server.ts new file mode 100644 index 000000000000..c4db9e348617 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/server.ts @@ -0,0 +1,23 @@ +import { loggingTransport } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, + debug: true, + tracesSampleRate: 0, +}); + +import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import express from 'express'; + +const app = express(); + +app.get('/test/express/:id', req => { + throw new Error(`test_error with id ${req.params.id}`); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/test.ts new file mode 100644 index 000000000000..4f9001297356 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/test.ts @@ -0,0 +1,39 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('should capture and send Express controller error.', done => { + const runner = createRunner(__dirname, 'server.ts') + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + mechanism: { + type: 'middleware', + handled: false, + }, + type: 'Error', + value: 'test_error with id 123', + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + function: expect.any(String), + lineno: expect.any(Number), + colno: expect.any(Number), + }), + ]), + }, + }, + ], + }, + transaction: 'GET /test/express/:id', + }, + }) + .start(done); + + expect(() => runner.makeRequest('get', '/test/express/123')).rejects.toThrow(); +}); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts similarity index 81% rename from dev-packages/node-integration-tests/suites/express/handle-error/server.ts rename to dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts index 1f452fbecc97..d403a91f5522 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error/server.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts @@ -5,6 +5,7 @@ Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', transport: loggingTransport, + debug: true, }); import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; @@ -12,8 +13,8 @@ import express from 'express'; const app = express(); -app.get('/test/express', () => { - throw new Error('test_error'); +app.get('/test/express/:id', req => { + throw new Error(`test_error with id ${req.params.id}`); }); Sentry.setupExpressErrorHandler(app); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts similarity index 87% rename from dev-packages/node-integration-tests/suites/express/handle-error/test.ts rename to dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts index fca4d270da40..fecc57a5da0b 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts @@ -17,7 +17,7 @@ test('should capture and send Express controller error.', done => { handled: false, }, type: 'Error', - value: 'test_error', + value: 'test_error with id 123', stacktrace: { frames: expect.arrayContaining([ expect.objectContaining({ @@ -34,5 +34,5 @@ test('should capture and send Express controller error.', done => { }) .start(done); - expect(() => runner.makeRequest('get', '/test/express')).rejects.toThrow(); + expect(() => runner.makeRequest('get', '/test/express/123')).rejects.toThrow(); }); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 1c7c9bc8842e..8b4248760a27 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -93,7 +93,8 @@ const _httpIntegration = ((options: HttpOptions = {}) => { requestHook: (span, req) => { addOriginToSpan(span, 'auto.http.otel.http'); - if (getSpanKind(span) !== SpanKind.SERVER) { + if (span.isRecording() && getSpanKind(span) !== SpanKind.SERVER) { + // TODO: small comment why we check for the span kind at all here return; } diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index 57ea1cb75ac0..035d638c50c2 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -1,10 +1,11 @@ import type * as http from 'http'; import { registerInstrumentations } from '@opentelemetry/instrumentation'; import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express'; -import { defineIntegration } from '@sentry/core'; +import { defineIntegration, getDefaultIsolationScope } from '@sentry/core'; import { captureException, getClient, getIsolationScope } from '@sentry/core'; import type { IntegrationFn } from '@sentry/types'; +import { logger } from '@sentry/utils'; import type { NodeClient } from '../../sdk/client'; import { addOriginToSpan } from '../../utils/addOriginToSpan'; @@ -19,6 +20,10 @@ const _expressIntegration = (() => { addOriginToSpan(span, 'auto.http.otel.express'); }, spanNameHook(info, defaultName) { + if (getIsolationScope() === getDefaultIsolationScope()) { + logger.warn('Isolation scope is still default isolation scope - skipping setting transactionName'); + return defaultName; + } if (info.layerType === 'request_handler') { // type cast b/c Otel unfortunately types info.request as any :( const req = info.request as { method?: string }; From 28c30aee598af89a5485249143345d94fabb2320 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 14:11:29 +0200 Subject: [PATCH 2/4] distinguish by req instance --- .../handle-error-tracesSampleRate-0/server.ts | 1 - .../handle-error-tracesSampleRate-0/test.ts | 3 ++- .../server.ts | 2 -- .../test.ts | 3 +-- packages/node/src/integrations/http.ts | 16 +++++++++++++--- 5 files changed, 16 insertions(+), 9 deletions(-) rename dev-packages/node-integration-tests/suites/express/{handle-error-no-tracesSampleRate => handle-error-tracesSampleRate-unset}/server.ts (93%) rename dev-packages/node-integration-tests/suites/express/{handle-error-no-tracesSampleRate => handle-error-tracesSampleRate-unset}/test.ts (89%) diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts index d403a91f5522..38833d7a9bc7 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts @@ -5,7 +5,6 @@ Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', transport: loggingTransport, - debug: true, }); import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts index fecc57a5da0b..bf0d49ee62a4 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts @@ -4,7 +4,7 @@ afterAll(() => { cleanupChildProcesses(); }); -test('should capture and send Express controller error.', done => { +test('should capture and send Express controller error with txn name if tracesSampleRate is 0', done => { const runner = createRunner(__dirname, 'server.ts') .ignore('session', 'sessions') .expect({ @@ -30,6 +30,7 @@ test('should capture and send Express controller error.', done => { }, ], }, + transaction: 'GET /test/express/:id', }, }) .start(done); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/server.ts similarity index 93% rename from dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/server.ts rename to dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/server.ts index c4db9e348617..38833d7a9bc7 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/server.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/server.ts @@ -5,8 +5,6 @@ Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', transport: loggingTransport, - debug: true, - tracesSampleRate: 0, }); import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts similarity index 89% rename from dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/test.ts rename to dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts index 4f9001297356..17c158b7991f 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-no-tracesSampleRate/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts @@ -4,7 +4,7 @@ afterAll(() => { cleanupChildProcesses(); }); -test('should capture and send Express controller error.', done => { +test('should capture and send Express controller error if tracesSampleRate is not set.', done => { const runner = createRunner(__dirname, 'server.ts') .ignore('session', 'sessions') .expect({ @@ -30,7 +30,6 @@ test('should capture and send Express controller error.', done => { }, ], }, - transaction: 'GET /test/express/:id', }, }) .start(done); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 8b4248760a27..c5eefdc6fb2e 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,4 +1,4 @@ -import type { ServerResponse } from 'http'; +import type { ClientRequest, IncomingMessage, ServerResponse } from 'http'; import type { Span } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import { registerInstrumentations } from '@opentelemetry/instrumentation'; @@ -93,8 +93,9 @@ const _httpIntegration = ((options: HttpOptions = {}) => { requestHook: (span, req) => { addOriginToSpan(span, 'auto.http.otel.http'); - if (span.isRecording() && getSpanKind(span) !== SpanKind.SERVER) { - // TODO: small comment why we check for the span kind at all here + // both, incoming requests and "client" requests made within the app trigger the requestHook + // we only want to isolate and further annotate incoming requests (IncomingMessage) + if (_isClientRequest(req)) { return; } @@ -180,3 +181,12 @@ function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMe }, ); } + +/** + * Determines if @param req is a ClientRequest, meaning the request was created within the express app + * and it's an outgoing request. + * Checking for properties instead of using `instanceOf` to avoid importing the request classes. + */ +function _isClientRequest(req: ClientRequest | IncomingMessage): req is ClientRequest { + return 'outputData' in req && 'outputSize' in req && !('client' in req) && !('statusCode' in req); +} From cec5454d0ab67dbf10a73b58ab9a2791a45cc413 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 14:17:00 +0200 Subject: [PATCH 3/4] guard by DEBUG_BUILD --- packages/node/src/integrations/tracing/express.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index 035d638c50c2..8d430b939c41 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -6,6 +6,7 @@ import { captureException, getClient, getIsolationScope } from '@sentry/core'; import type { IntegrationFn } from '@sentry/types'; import { logger } from '@sentry/utils'; +import { DEBUG_BUILD } from '../../debug-build'; import type { NodeClient } from '../../sdk/client'; import { addOriginToSpan } from '../../utils/addOriginToSpan'; @@ -21,7 +22,8 @@ const _expressIntegration = (() => { }, spanNameHook(info, defaultName) { if (getIsolationScope() === getDefaultIsolationScope()) { - logger.warn('Isolation scope is still default isolation scope - skipping setting transactionName'); + DEBUG_BUILD && + logger.warn('Isolation scope is still default isolation scope - skipping setting transactionName'); return defaultName; } if (info.layerType === 'request_handler') { From 00ae2b3a2fafbcd51b504189c6a97e9149fbb4d3 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 14:53:59 +0200 Subject: [PATCH 4/4] fix tests --- .../suites/express/handle-error-tracesSampleRate-0/server.ts | 1 + .../suites/express/handle-error-tracesSampleRate-0/test.ts | 2 +- .../suites/express/handle-error-tracesSampleRate-unset/test.ts | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts index 38833d7a9bc7..3f52580dda1d 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts @@ -5,6 +5,7 @@ Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', transport: loggingTransport, + tracesSampleRate: 1, }); import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts index bf0d49ee62a4..f76edf06bedb 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts @@ -6,7 +6,7 @@ afterAll(() => { test('should capture and send Express controller error with txn name if tracesSampleRate is 0', done => { const runner = createRunner(__dirname, 'server.ts') - .ignore('session', 'sessions') + .ignore('session', 'sessions', 'transaction') .expect({ event: { exception: { diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts index 17c158b7991f..c48a3b1f9444 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-unset/test.ts @@ -6,7 +6,7 @@ afterAll(() => { test('should capture and send Express controller error if tracesSampleRate is not set.', done => { const runner = createRunner(__dirname, 'server.ts') - .ignore('session', 'sessions') + .ignore('session', 'sessions', 'transaction') .expect({ event: { exception: {