From 32f618da4b892ba591bde17a658ebdef9b9bb740 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 25 Sep 2024 21:01:19 +0200 Subject: [PATCH 1/4] feat(node): Capture `fs` breadcrumbs --- .../suites/fs-instrumentation/server.ts | 4 +- .../suites/fs-instrumentation/test.ts | 15 ++ packages/node/src/integrations/fs.ts | 184 ++++++++++-------- 3 files changed, 123 insertions(+), 80 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts index 1320d7c3b4e2..3d8df1d786c1 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts @@ -26,8 +26,8 @@ const app = express(); app.get('/readFile-error', async (_, res) => { try { await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file-that-doesnt-exist.txt'), 'utf-8'); - } catch { - // noop + } catch (e) { + Sentry.captureException(e); } res.send('done'); }); diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts index 05860fa1ce26..53f184360288 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts @@ -7,6 +7,21 @@ afterAll(() => { test('should create spans for fs operations that take target argument', done => { const runner = createRunner(__dirname, 'server.ts') + .expect({ + event: { + breadcrumbs: expect.arrayContaining([ + expect.objectContaining({ + timestamp: expect.any(Number), + message: 'fs.readFile', + level: 'error', + data: { + path_argument: expect.stringContaining('some-file-that-doesnt-exist.txt'), + fs_error: expect.stringContaining('ENOENT: no such file or directory'), + }, + }), + ]), + }, + }) .expect({ transaction: { transaction: 'GET /readFile-error', diff --git a/packages/node/src/integrations/fs.ts b/packages/node/src/integrations/fs.ts index 2288096dad43..2f35b68e10d8 100644 --- a/packages/node/src/integrations/fs.ts +++ b/packages/node/src/integrations/fs.ts @@ -1,9 +1,37 @@ import { FsInstrumentation } from '@opentelemetry/instrumentation-fs'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, defineIntegration } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + addBreadcrumb, + defineIntegration, +} from '@sentry/core'; +import type { SpanAttributes } from '@sentry/types'; import { generateInstrumentOnce } from '../otel/instrument'; const INTEGRATION_NAME = 'FileSystem'; +interface Options { + /** + * Whether to capture breadcrumbs for `fs` operations. + * + * Defaults to `true`. + */ + breadcrumbs?: boolean; + /** + * Setting this option to `true` will include any filepath arguments from your `fs` API calls as span attributes. + * + * Defaults to `false`. + */ + recordFilePaths?: boolean; + + /** + * Setting this option to `true` will include the error messages of failed `fs` API calls as a span attribute. + * + * Defaults to `false`. + */ + recordErrorMessagesAsSpanAttributes?: boolean; +} + /** * This integration will create spans for `fs` API operations, like reading and writing files. * @@ -13,86 +41,38 @@ const INTEGRATION_NAME = 'FileSystem'; * * @param options Configuration for this integration. */ -export const fsIntegration = defineIntegration( - ( - options: { - /** - * Setting this option to `true` will include any filepath arguments from your `fs` API calls as span attributes. - * - * Defaults to `false`. - */ - recordFilePaths?: boolean; +export const fsIntegration = defineIntegration((options: Options = {}) => { + return { + name: INTEGRATION_NAME, + setupOnce() { + generateInstrumentOnce( + INTEGRATION_NAME, + () => + new FsInstrumentation({ + requireParentSpan: true, + endHook(functionName, { args, span, error }) { + span.updateName(`fs.${functionName}`); - /** - * Setting this option to `true` will include the error messages of failed `fs` API calls as a span attribute. - * - * Defaults to `false`. - */ - recordErrorMessagesAsSpanAttributes?: boolean; - } = {}, - ) => { - return { - name: INTEGRATION_NAME, - setupOnce() { - generateInstrumentOnce( - INTEGRATION_NAME, - () => - new FsInstrumentation({ - requireParentSpan: true, - endHook(functionName, { args, span, error }) { - span.updateName(`fs.${functionName}`); + const additionalAttributes = { + ...(options.recordFilePaths && getFilePathAttributes(functionName, args)), + ...(error && options.recordErrorMessagesAsSpanAttributes && { fs_error: error.message }), + }; - span.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs', - }); + span.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs', + ...additionalAttributes, + }); - if (options.recordErrorMessagesAsSpanAttributes) { - if (typeof args[0] === 'string' && FS_OPERATIONS_WITH_PATH_ARG.includes(functionName)) { - span.setAttribute('path_argument', args[0]); - } else if ( - typeof args[0] === 'string' && - typeof args[1] === 'string' && - FS_OPERATIONS_WITH_TARGET_PATH.includes(functionName) - ) { - span.setAttribute('target_argument', args[0]); - span.setAttribute('path_argument', args[1]); - } else if (typeof args[0] === 'string' && FS_OPERATIONS_WITH_PREFIX.includes(functionName)) { - span.setAttribute('prefix_argument', args[0]); - } else if ( - typeof args[0] === 'string' && - typeof args[1] === 'string' && - FS_OPERATIONS_WITH_EXISTING_PATH_NEW_PATH.includes(functionName) - ) { - span.setAttribute('existing_path_argument', args[0]); - span.setAttribute('new_path_argument', args[1]); - } else if ( - typeof args[0] === 'string' && - typeof args[1] === 'string' && - FS_OPERATIONS_WITH_SRC_DEST.includes(functionName) - ) { - span.setAttribute('src_argument', args[0]); - span.setAttribute('dest_argument', args[1]); - } else if ( - typeof args[0] === 'string' && - typeof args[1] === 'string' && - FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH.includes(functionName) - ) { - span.setAttribute('old_path_argument', args[0]); - span.setAttribute('new_path_argument', args[1]); - } - } - - if (error && options.recordErrorMessagesAsSpanAttributes) { - span.setAttribute('fs_error', error.message); - } - }, - }), - )(); - }, - }; - }, -); + if (options.breadcrumbs !== false) { + captureBreadcrumb(functionName, additionalAttributes, !!error); + } + }, + }), + )(); + }, + }; +}); const FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH = ['rename', 'renameSync']; const FS_OPERATIONS_WITH_SRC_DEST = ['copyFile', 'cp', 'copyFileSync', 'cpSync']; @@ -147,3 +127,51 @@ const FS_OPERATIONS_WITH_PATH_ARG = [ 'utimesSync', 'writeFileSync', ]; + +function getFilePathAttributes(functionName: string, args: ArrayLike): SpanAttributes { + const attributes: SpanAttributes = {}; + + if (typeof args[0] === 'string' && FS_OPERATIONS_WITH_PATH_ARG.includes(functionName)) { + attributes['path_argument'] = args[0]; + } else if ( + typeof args[0] === 'string' && + typeof args[1] === 'string' && + FS_OPERATIONS_WITH_TARGET_PATH.includes(functionName) + ) { + attributes['target_argument'] = args[0]; + attributes['path_argument'] = args[1]; + } else if (typeof args[0] === 'string' && FS_OPERATIONS_WITH_PREFIX.includes(functionName)) { + attributes['prefix_argument'] = args[0]; + } else if ( + typeof args[0] === 'string' && + typeof args[1] === 'string' && + FS_OPERATIONS_WITH_EXISTING_PATH_NEW_PATH.includes(functionName) + ) { + attributes['existing_path_argument'] = args[0]; + attributes['new_path_argument'] = args[1]; + } else if ( + typeof args[0] === 'string' && + typeof args[1] === 'string' && + FS_OPERATIONS_WITH_SRC_DEST.includes(functionName) + ) { + attributes['src_argument'] = args[0]; + attributes['dest_argument'] = args[1]; + } else if ( + typeof args[0] === 'string' && + typeof args[1] === 'string' && + FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH.includes(functionName) + ) { + attributes['old_path_argument'] = args[0]; + attributes['new_path_argument'] = args[1]; + } + + return attributes; +} + +function captureBreadcrumb(functionName: string, attributes: SpanAttributes | undefined, error: boolean): void { + addBreadcrumb({ + message: `fs.${functionName}`, + level: error ? 'error' : 'info', + data: attributes, + }); +} From 8d1668ce8cdffd00662be0392246ab144709d86b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 26 Sep 2024 10:57:42 +0200 Subject: [PATCH 2/4] Ensure event is always after the transaction --- .../suites/fs-instrumentation/server.ts | 5 +++- .../suites/fs-instrumentation/test.ts | 30 +++++++++---------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts index 3d8df1d786c1..deb234e19184 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts @@ -27,7 +27,10 @@ app.get('/readFile-error', async (_, res) => { try { await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file-that-doesnt-exist.txt'), 'utf-8'); } catch (e) { - Sentry.captureException(e); + // delay so the event is always after the transaction + setTimeout(() => { + Sentry.captureException(e); + }, 1000); } res.send('done'); }); diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts index 53f184360288..5d9216d5930e 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts @@ -7,21 +7,6 @@ afterAll(() => { test('should create spans for fs operations that take target argument', done => { const runner = createRunner(__dirname, 'server.ts') - .expect({ - event: { - breadcrumbs: expect.arrayContaining([ - expect.objectContaining({ - timestamp: expect.any(Number), - message: 'fs.readFile', - level: 'error', - data: { - path_argument: expect.stringContaining('some-file-that-doesnt-exist.txt'), - fs_error: expect.stringContaining('ENOENT: no such file or directory'), - }, - }), - ]), - }, - }) .expect({ transaction: { transaction: 'GET /readFile-error', @@ -40,6 +25,21 @@ test('should create spans for fs operations that take target argument', done => ]), }, }) + .expect({ + event: { + breadcrumbs: expect.arrayContaining([ + expect.objectContaining({ + timestamp: expect.any(Number), + message: 'fs.readFile', + level: 'error', + data: { + path_argument: expect.stringContaining('some-file-that-doesnt-exist.txt'), + fs_error: expect.stringContaining('ENOENT: no such file or directory'), + }, + }), + ]), + }, + }) .start(done); expect(runner.makeRequest('get', '/readFile-error')).resolves.toBe('done'); From 0dabdd42a087e455db1cf494dc28b59809cc6011 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 26 Sep 2024 18:11:50 +0200 Subject: [PATCH 3/4] remove delay --- .../suites/fs-instrumentation/server.ts | 5 +--- .../suites/fs-instrumentation/test.ts | 30 +++++++++---------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts index deb234e19184..3d8df1d786c1 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/server.ts @@ -27,10 +27,7 @@ app.get('/readFile-error', async (_, res) => { try { await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file-that-doesnt-exist.txt'), 'utf-8'); } catch (e) { - // delay so the event is always after the transaction - setTimeout(() => { - Sentry.captureException(e); - }, 1000); + Sentry.captureException(e); } res.send('done'); }); diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts index 5d9216d5930e..53f184360288 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts @@ -7,6 +7,21 @@ afterAll(() => { test('should create spans for fs operations that take target argument', done => { const runner = createRunner(__dirname, 'server.ts') + .expect({ + event: { + breadcrumbs: expect.arrayContaining([ + expect.objectContaining({ + timestamp: expect.any(Number), + message: 'fs.readFile', + level: 'error', + data: { + path_argument: expect.stringContaining('some-file-that-doesnt-exist.txt'), + fs_error: expect.stringContaining('ENOENT: no such file or directory'), + }, + }), + ]), + }, + }) .expect({ transaction: { transaction: 'GET /readFile-error', @@ -25,21 +40,6 @@ test('should create spans for fs operations that take target argument', done => ]), }, }) - .expect({ - event: { - breadcrumbs: expect.arrayContaining([ - expect.objectContaining({ - timestamp: expect.any(Number), - message: 'fs.readFile', - level: 'error', - data: { - path_argument: expect.stringContaining('some-file-that-doesnt-exist.txt'), - fs_error: expect.stringContaining('ENOENT: no such file or directory'), - }, - }), - ]), - }, - }) .start(done); expect(runner.makeRequest('get', '/readFile-error')).resolves.toBe('done'); From 8777702e3ed17943d3ec1ff85701ed80b528c9a6 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 14 Oct 2024 14:58:48 +0100 Subject: [PATCH 4/4] Test breadcrumbs seperatly --- .../suites/fs-instrumentation/test.ts | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts index 53f184360288..dcac63a10245 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts @@ -7,21 +7,7 @@ afterAll(() => { test('should create spans for fs operations that take target argument', done => { const runner = createRunner(__dirname, 'server.ts') - .expect({ - event: { - breadcrumbs: expect.arrayContaining([ - expect.objectContaining({ - timestamp: expect.any(Number), - message: 'fs.readFile', - level: 'error', - data: { - path_argument: expect.stringContaining('some-file-that-doesnt-exist.txt'), - fs_error: expect.stringContaining('ENOENT: no such file or directory'), - }, - }), - ]), - }, - }) + .ignore('event') .expect({ transaction: { transaction: 'GET /readFile-error', @@ -45,6 +31,29 @@ test('should create spans for fs operations that take target argument', done => expect(runner.makeRequest('get', '/readFile-error')).resolves.toBe('done'); }); +test('should create breadcrumbs for fs operations', done => { + const runner = createRunner(__dirname, 'server.ts') + .ignore('transaction') + .expect({ + event: { + breadcrumbs: expect.arrayContaining([ + expect.objectContaining({ + timestamp: expect.any(Number), + message: 'fs.readFile', + level: 'error', + data: { + path_argument: expect.stringContaining('some-file-that-doesnt-exist.txt'), + fs_error: expect.stringContaining('ENOENT: no such file or directory'), + }, + }), + ]), + }, + }) + .start(done); + + expect(runner.makeRequest('get', '/readFile-error')).resolves.toBe('done'); +}); + test('should create spans for fs operations that take one path', done => { const runner = createRunner(__dirname, 'server.ts') .expect({