From 2a00b10a2a639801bd16eeccef19ab43e0ceae68 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 19 Mar 2024 17:37:19 +0100 Subject: [PATCH 1/6] test: Add tests to demonstrate wrong scope data loss --- .../withScope/throwError/subject.js | 7 ++++ .../public-api/withScope/throwError/test.ts | 19 +++++++++ .../express/handle-error-withScope/server.ts | 26 ++++++++++++ .../express/handle-error-withScope/test.ts | 42 +++++++++++++++++++ 4 files changed, 94 insertions(+) create mode 100644 dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts create mode 100644 dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts create mode 100644 dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts diff --git a/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/subject.js b/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/subject.js new file mode 100644 index 000000000000..67cc16af1d40 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/subject.js @@ -0,0 +1,7 @@ +Sentry.setTag('global', 'tag'); +setTimeout(() => { + Sentry.withScope(scope => { + scope.setTag('local', 'tag'); + throw new Error('test error'); + }); +}, 10); diff --git a/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts b/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts new file mode 100644 index 000000000000..dc6094129ef8 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts @@ -0,0 +1,19 @@ +import { expect } from '@playwright/test'; +import type { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('scope is applied to thrown error', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + const ex = eventData.exception?.values ? eventData.exception.values[0] : undefined; + + expect(eventData.tags).toMatchObject({ + global: 'tag', + local: 'tag', // this tag is missing :( + }); + expect(ex?.value).toBe('test error'); +}); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts new file mode 100644 index 000000000000..cf0e18edc9fc --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts @@ -0,0 +1,26 @@ +import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node-experimental'; +import express from 'express'; + +const app = express(); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, +}); + +app.use(Sentry.Handlers.requestHandler()); + +Sentry.setTag('global', 'tag'); + +app.get('/test/express', () => { + Sentry.withScope(scope => { + scope.setTag('local', 'tag'); + throw new Error('test_error'); + }) +}); + +app.use(Sentry.Handlers.errorHandler()); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts new file mode 100644 index 000000000000..c76cb25fdba3 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts @@ -0,0 +1,42 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +afterAll(() => { + cleanupChildProcesses(); +}); + +test('applies withScope scope to thrown 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', + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + function: expect.any(String), + lineno: expect.any(Number), + colno: expect.any(Number), + }), + ]), + }, + }, + ], + }, + tags: { + 'global': 'tag', + 'local': 'tag', // This tag is missing :( + } + }, + }) + .start(done); + + expect(() => runner.makeRequest('get', '/test/express')).rejects.toThrow(); +}); From 99bca742a008561a11e720722604e7c5f9966ebf Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 19 Mar 2024 17:40:58 +0100 Subject: [PATCH 2/6] formatting --- .../suites/express/handle-error-withScope/server.ts | 2 +- .../suites/express/handle-error-withScope/test.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts index cf0e18edc9fc..e4e2e70970d4 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts @@ -18,7 +18,7 @@ app.get('/test/express', () => { Sentry.withScope(scope => { scope.setTag('local', 'tag'); throw new Error('test_error'); - }) + }); }); app.use(Sentry.Handlers.errorHandler()); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts index c76cb25fdba3..c97b070d4f41 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts @@ -31,9 +31,9 @@ test('applies withScope scope to thrown error', done => { ], }, tags: { - 'global': 'tag', - 'local': 'tag', // This tag is missing :( - } + global: 'tag', + local: 'tag', // This tag is missing :( + }, }, }) .start(done); From bfb8f4c54d79be924e7945bbb45e7594072226ee Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 21 Mar 2024 11:30:00 +0100 Subject: [PATCH 3/6] cleanup --- .../public-api/withScope/throwError/test.ts | 34 +++++++++++++------ .../express/handle-error-withScope/server.ts | 2 +- .../express/handle-error-withScope/test.ts | 13 +++++-- 3 files changed, 36 insertions(+), 13 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts b/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts index dc6094129ef8..cb21bebb8241 100644 --- a/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts +++ b/dev-packages/browser-integration-tests/suites/public-api/withScope/throwError/test.ts @@ -4,16 +4,30 @@ import type { Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; -sentryTest('scope is applied to thrown error', async ({ getLocalTestPath, page }) => { - const url = await getLocalTestPath({ testDir: __dirname }); +/** + * Why does this test exist? + * + * We recently discovered that errors caught by global handlers will potentially loose scope data from the active scope + * where the error was thrown in. The simple example in this test (see subject.ts) demonstrates this behavior (in a + * browser environment but the same behavior applies to the server; see the test there). + * + * This test nevertheless covers the behavior so that we're aware. + */ +sentryTest( + 'withScope scope is NOT applied to thrown error caught by global handler', + async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); - const eventData = await getFirstSentryEnvelopeRequest(page, url); + const eventData = await getFirstSentryEnvelopeRequest(page, url); - const ex = eventData.exception?.values ? eventData.exception.values[0] : undefined; + const ex = eventData.exception?.values ? eventData.exception.values[0] : undefined; - expect(eventData.tags).toMatchObject({ - global: 'tag', - local: 'tag', // this tag is missing :( - }); - expect(ex?.value).toBe('test error'); -}); + // This tag is missing :( + expect(eventData.tags?.local).toBeUndefined(); + + expect(eventData.tags).toMatchObject({ + global: 'tag', + }); + expect(ex?.value).toBe('test error'); + }, +); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts index e4e2e70970d4..b5866bb1ab41 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts @@ -14,7 +14,7 @@ app.use(Sentry.Handlers.requestHandler()); Sentry.setTag('global', 'tag'); -app.get('/test/express', () => { +app.get('/test/withScope', () => { Sentry.withScope(scope => { scope.setTag('local', 'tag'); throw new Error('test_error'); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts index c97b070d4f41..9c12e4494142 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts @@ -4,6 +4,15 @@ afterAll(() => { cleanupChildProcesses(); }); +/** + * Why does this test exist? + * + * We recently discovered that errors caught by global handlers will potentially loose scope data from the active scope + * where the error was originally thrown in. The simple example in this test (see subject.ts) demonstrates this behavior + * (in a Node environment but the same behavior applies to the browser; see the test there). + * + * This test nevertheless covers the behavior so that we're aware. + */ test('applies withScope scope to thrown error', done => { const runner = createRunner(__dirname, 'server.ts') .ignore('session', 'sessions') @@ -32,11 +41,11 @@ test('applies withScope scope to thrown error', done => { }, tags: { global: 'tag', - local: 'tag', // This tag is missing :( + local: undefined, // This tag is missing :( }, }, }) .start(done); - expect(() => runner.makeRequest('get', '/test/express')).rejects.toThrow(); + expect(() => runner.makeRequest('get', '/test/withScope')).rejects.toThrow(); }); From 7c91b7b6a4ed3ddbe112d5a244f63b4d58b50a11 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 21 Mar 2024 11:37:19 +0100 Subject: [PATCH 4/6] add test demonstrating correct isolationScope behavior --- .../server.ts | 5 +++ .../test.ts | 39 ++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) rename dev-packages/node-integration-tests/suites/express/{handle-error-withScope => handle-error-scope-data-loss}/server.ts (81%) rename dev-packages/node-integration-tests/suites/express/{handle-error-withScope => handle-error-scope-data-loss}/test.ts (57%) diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/server.ts similarity index 81% rename from dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts rename to dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/server.ts index b5866bb1ab41..ad45cd5d6713 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/server.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/server.ts @@ -21,6 +21,11 @@ app.get('/test/withScope', () => { }); }); +app.get('/test/isolationScope', () => { + Sentry.getIsolationScope().setTag('isolation-scope', 'tag'); + throw new Error('isolation_test_error'); +}); + app.use(Sentry.Handlers.errorHandler()); startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts similarity index 57% rename from dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts rename to dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts index 9c12e4494142..a75708e40230 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-withScope/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts @@ -13,7 +13,7 @@ afterAll(() => { * * This test nevertheless covers the behavior so that we're aware. */ -test('applies withScope scope to thrown error', done => { +test('withScope scope is NOT applied to thrown error caught by global handler', done => { const runner = createRunner(__dirname, 'server.ts') .ignore('session', 'sessions') .expect({ @@ -49,3 +49,40 @@ test('applies withScope scope to thrown error', done => { expect(() => runner.makeRequest('get', '/test/withScope')).rejects.toThrow(); }); + +test('isolation scope is applied to thrown error caught by global handler', done => { + const runner = createRunner(__dirname, 'server.ts') + .ignore('session', 'sessions') + .expect({ + event: { + exception: { + values: [ + { + mechanism: { + type: 'middleware', + handled: false, + }, + type: 'Error', + value: 'isolation_test_error', + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + function: expect.any(String), + lineno: expect.any(Number), + colno: expect.any(Number), + }), + ]), + }, + }, + ], + }, + tags: { + global: 'tag', + 'isolation-scope': 'tag', + }, + }, + }) + .start(done); + + expect(() => runner.makeRequest('get', '/test/isolationScope')).rejects.toThrow(); +}); From 6673bc064918a035fa9642f69224dbb1dd158b05 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 21 Mar 2024 11:38:29 +0100 Subject: [PATCH 5/6] cleanup --- .../suites/express/handle-error-scope-data-loss/test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts index a75708e40230..29728ebe5a89 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts @@ -50,6 +50,9 @@ test('withScope scope is NOT applied to thrown error caught by global handler', expect(() => runner.makeRequest('get', '/test/withScope')).rejects.toThrow(); }); +/** + * This test shows that the isolation scope set tags are applied correctly to the error. + */ test('isolation scope is applied to thrown error caught by global handler', done => { const runner = createRunner(__dirname, 'server.ts') .ignore('session', 'sessions') From ff073158d2b7cbbb71fcd392e73a1c8d48852664 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 21 Mar 2024 12:34:01 +0100 Subject: [PATCH 6/6] fix node test --- .../suites/express/handle-error-scope-data-loss/test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts index 29728ebe5a89..f1bb9a1229b1 100644 --- a/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts +++ b/dev-packages/node-integration-tests/suites/express/handle-error-scope-data-loss/test.ts @@ -39,10 +39,8 @@ test('withScope scope is NOT applied to thrown error caught by global handler', }, ], }, - tags: { - global: 'tag', - local: undefined, // This tag is missing :( - }, + // 'local' tag is not applied to the event + tags: expect.not.objectContaining({ local: expect.anything() }), }, }) .start(done);