From 5754444a44a4f61a2ebc8cbd0a421989ac553353 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 15 Jun 2026 14:01:34 +0200 Subject: [PATCH] ref(node): Streamline vendored fs instrumentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: * Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs * Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check * Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation * Streamline the types to what we actually need * Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter * Add tests covering the previously-untested option gating While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes were gated on `recordErrorMessagesAsSpanAttributes` instead. File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the docs. Closes #20726 Co-Authored-By: Claude Opus 4.8 (1M context) --- .oxlintrc.base.json | 1 - .../server-record-errors-only.ts | 39 ++ .../server-record-paths-only.ts | 39 ++ .../suites/fs-instrumentation/test.ts | 80 ++++ packages/node/src/integrations/fs/index.ts | 109 +---- .../src/integrations/fs/vendored/constants.ts | 1 - .../fs/vendored/instrumentation.ts | 453 +++++++++--------- .../src/integrations/fs/vendored/types.ts | 31 +- .../src/integrations/fs/vendored/utils.ts | 10 +- 9 files changed, 399 insertions(+), 364 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-errors-only.ts create mode 100644 dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-paths-only.ts diff --git a/.oxlintrc.base.json b/.oxlintrc.base.json index e5c35bf49d5c..2ef218f3edd2 100644 --- a/.oxlintrc.base.json +++ b/.oxlintrc.base.json @@ -142,7 +142,6 @@ }, { "files": [ - "**/integrations/fs/vendored/**/*.ts", "**/integrations/tracing/knex/vendored/**/*.ts", "**/integrations/tracing/mongo/vendored/**/*.ts", "**/integrations/tracing/graphql/vendored/**/*.ts", diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-errors-only.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-errors-only.ts new file mode 100644 index 000000000000..4b4d87398148 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-errors-only.ts @@ -0,0 +1,39 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, + tracesSampleRate: 1, + integrations: [ + // Only record error messages - file paths must NOT be recorded + Sentry.fsIntegration({ + recordErrorMessagesAsSpanAttributes: true, + }), + ], +}); + +import express from 'express'; +import * as fs from 'fs'; +import * as path from 'path'; + +const app = express(); + +app.get('/readFile', async (_, res) => { + await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file.txt'), 'utf-8'); + res.send('done'); +}); + +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 + } + res.send('done'); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-paths-only.ts b/dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-paths-only.ts new file mode 100644 index 000000000000..385e53faae0d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-paths-only.ts @@ -0,0 +1,39 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, + tracesSampleRate: 1, + integrations: [ + // Only record file paths - error messages must NOT be recorded + Sentry.fsIntegration({ + recordFilePaths: true, + }), + ], +}); + +import express from 'express'; +import * as fs from 'fs'; +import * as path from 'path'; + +const app = express(); + +app.get('/readFile', async (_, res) => { + await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file.txt'), 'utf-8'); + res.send('done'); +}); + +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 + } + res.send('done'); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); 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 d64144754772..17ef25133cb4 100644 --- a/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts +++ b/dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts @@ -271,3 +271,83 @@ test('should create spans for fs operations that take target argument', async () expect(result).toEqual('done'); await runner.completed(); }); + +test('records file path but not error messages when only `recordFilePaths` is enabled', async () => { + const runner = createRunner(__dirname, 'server-record-paths-only.ts') + .expect({ + transaction: { + transaction: 'GET /readFile-error', + spans: expect.arrayContaining([ + expect.objectContaining({ + description: 'fs.readFile', + op: 'file', + status: 'internal_error', + // `path_argument` is recorded, but `fs_error` is NOT, since `recordErrorMessagesAsSpanAttributes` is off + data: { + path_argument: expect.stringMatching('/fixtures/some-file-that-doesnt-exist.txt'), + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs', + }, + }), + ]), + }, + }) + .start(); + + const result = await runner.makeRequest('get', '/readFile-error'); + expect(result).toEqual('done'); + await runner.completed(); +}); + +test('records error messages but not file paths when only `recordErrorMessagesAsSpanAttributes` is enabled', async () => { + const runner = createRunner(__dirname, 'server-record-errors-only.ts') + .expect({ + transaction: { + transaction: 'GET /readFile-error', + spans: expect.arrayContaining([ + expect.objectContaining({ + description: 'fs.readFile', + op: 'file', + status: 'internal_error', + // `fs_error` is recorded, but `path_argument` is NOT, since `recordFilePaths` is off + data: { + fs_error: expect.stringMatching('ENOENT: no such file or directory,'), + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs', + }, + }), + ]), + }, + }) + .start(); + + const result = await runner.makeRequest('get', '/readFile-error'); + expect(result).toEqual('done'); + await runner.completed(); +}); + +test('does not record file paths on successful operations when only `recordErrorMessagesAsSpanAttributes` is enabled', async () => { + const runner = createRunner(__dirname, 'server-record-errors-only.ts') + .expect({ + transaction: { + transaction: 'GET /readFile', + spans: expect.arrayContaining([ + expect.objectContaining({ + description: 'fs.readFile', + op: 'file', + status: 'ok', + // Neither `path_argument` nor `fs_error` are recorded + data: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs', + }, + }), + ]), + }, + }) + .start(); + + const result = await runner.makeRequest('get', '/readFile'); + expect(result).toEqual('done'); + await runner.completed(); +}); diff --git a/packages/node/src/integrations/fs/index.ts b/packages/node/src/integrations/fs/index.ts index 05dd8ebaadb3..444059161f83 100644 --- a/packages/node/src/integrations/fs/index.ts +++ b/packages/node/src/integrations/fs/index.ts @@ -1,6 +1,6 @@ -import { FsInstrumentation } from './vendored/instrumentation'; -import { defineIntegration, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; +import { defineIntegration } from '@sentry/core'; import { generateInstrumentOnce } from '@sentry/node-core'; +import { FsInstrumentation } from './vendored/instrumentation'; const INTEGRATION_NAME = 'FileSystem'; @@ -38,112 +38,11 @@ export const fsIntegration = defineIntegration( INTEGRATION_NAME, () => new FsInstrumentation({ - requireParentSpan: true, - endHook(functionName, { args, span, error }) { - span.updateName(`fs.${functionName}`); - - span.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs', - }); - - 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); - } - }, + recordFilePaths: options.recordFilePaths, + recordErrorMessagesAsSpanAttributes: options.recordErrorMessagesAsSpanAttributes, }), )(); }, }; }, ); - -const FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH = ['rename', 'renameSync']; -const FS_OPERATIONS_WITH_SRC_DEST = ['copyFile', 'cp', 'copyFileSync', 'cpSync']; -const FS_OPERATIONS_WITH_EXISTING_PATH_NEW_PATH = ['link', 'linkSync']; -const FS_OPERATIONS_WITH_PREFIX = ['mkdtemp', 'mkdtempSync']; -const FS_OPERATIONS_WITH_TARGET_PATH = ['symlink', 'symlinkSync']; -const FS_OPERATIONS_WITH_PATH_ARG = [ - 'access', - 'appendFile', - 'chmod', - 'chown', - 'exists', - 'mkdir', - 'lchown', - 'lstat', - 'lutimes', - 'open', - 'opendir', - 'readdir', - 'readFile', - 'readlink', - 'realpath', - 'realpath.native', - 'rm', - 'rmdir', - 'stat', - 'truncate', - 'unlink', - 'utimes', - 'writeFile', - 'accessSync', - 'appendFileSync', - 'chmodSync', - 'chownSync', - 'existsSync', - 'lchownSync', - 'lstatSync', - 'lutimesSync', - 'opendirSync', - 'mkdirSync', - 'openSync', - 'readdirSync', - 'readFileSync', - 'readlinkSync', - 'realpathSync', - 'realpathSync.native', - 'rmdirSync', - 'rmSync', - 'statSync', - 'truncateSync', - 'unlinkSync', - 'utimesSync', - 'writeFileSync', -]; diff --git a/packages/node/src/integrations/fs/vendored/constants.ts b/packages/node/src/integrations/fs/vendored/constants.ts index c454b761679d..14588b47ed19 100644 --- a/packages/node/src/integrations/fs/vendored/constants.ts +++ b/packages/node/src/integrations/fs/vendored/constants.ts @@ -17,7 +17,6 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-fs * - Upstream version: @opentelemetry/instrumentation-fs@0.37.0 */ -/* eslint-disable */ import type { FMember, FPMember } from './types'; diff --git a/packages/node/src/integrations/fs/vendored/instrumentation.ts b/packages/node/src/integrations/fs/vendored/instrumentation.ts index e48789f1a3f0..36712028a040 100644 --- a/packages/node/src/integrations/fs/vendored/instrumentation.ts +++ b/packages/node/src/integrations/fs/vendored/instrumentation.ts @@ -17,17 +17,27 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-fs * - Upstream version: @opentelemetry/instrumentation-fs@0.37.0 * - Minor TypeScript strictness adjustments for this repository's compiler settings + * - The OpenTelemetry tracer APIs were replaced with Sentry's `startSpan`/`startInactiveSpan`/`suppressTracing` + * and the configurable `createHook`/`endHook`/`requireParentSpan` options were removed in favor of inlined, + * Sentry-specific span attributes. */ -/* eslint-disable */ -import * as api from '@opentelemetry/api'; -import { isTracingSuppressed, suppressTracing } from '@opentelemetry/core'; +import { context } from '@opentelemetry/api'; import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation'; -import { SDK_VERSION } from '@sentry/core'; -import { CALLBACK_FUNCTIONS, PROMISE_FUNCTIONS, SYNC_FUNCTIONS } from './constants'; +import type { Span, SpanAttributes } from '@sentry/core'; +import { + SDK_VERSION, + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SPAN_STATUS_ERROR, + startInactiveSpan, + startSpan, + suppressTracing, +} from '@sentry/core'; import type * as fs from 'fs'; -import type { FMember, FPMember, CreateHook, EndHook, FsInstrumentationConfig } from './types'; import { promisify } from 'util'; +import { CALLBACK_FUNCTIONS, PROMISE_FUNCTIONS, SYNC_FUNCTIONS } from './constants'; +import type { FMember, FPMember, FsInstrumentationConfig, GenericFunction } from './types'; import { indexFs } from './utils'; type FS = typeof fs; @@ -35,23 +45,119 @@ type FSPromises = (typeof fs)['promises']; const PACKAGE_NAME = '@sentry/instrumentation-fs'; +const SPAN_ORIGIN = 'auto.file.fs'; +const SPAN_OP = 'file'; + +// The following lists categorize `fs` functions by the shape of their leading path arguments, so we can +// record meaningful span attributes for them. These are Sentry-specific additions (not part of upstream). +const FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH = ['rename', 'renameSync']; +const FS_OPERATIONS_WITH_SRC_DEST = ['copyFile', 'cp', 'copyFileSync', 'cpSync']; +const FS_OPERATIONS_WITH_EXISTING_PATH_NEW_PATH = ['link', 'linkSync']; +const FS_OPERATIONS_WITH_PREFIX = ['mkdtemp', 'mkdtempSync']; +const FS_OPERATIONS_WITH_TARGET_PATH = ['symlink', 'symlinkSync']; +const FS_OPERATIONS_WITH_PATH_ARG = [ + 'access', + 'appendFile', + 'chmod', + 'chown', + 'exists', + 'mkdir', + 'lchown', + 'lstat', + 'lutimes', + 'open', + 'opendir', + 'readdir', + 'readFile', + 'readlink', + 'realpath', + 'realpath.native', + 'rm', + 'rmdir', + 'stat', + 'truncate', + 'unlink', + 'utimes', + 'writeFile', + 'accessSync', + 'appendFileSync', + 'chmodSync', + 'chownSync', + 'existsSync', + 'lchownSync', + 'lstatSync', + 'lutimesSync', + 'opendirSync', + 'mkdirSync', + 'openSync', + 'readdirSync', + 'readFileSync', + 'readlinkSync', + 'realpathSync', + 'realpathSync.native', + 'rmdirSync', + 'rmSync', + 'statSync', + 'truncateSync', + 'unlinkSync', + 'utimesSync', + 'writeFileSync', +]; + +/** + * Builds the span attributes for a given `fs` operation, optionally including the file path arguments. + */ +function getSpanAttributes( + functionName: FMember | FPMember, + args: ArrayLike, + config: FsInstrumentationConfig, +): SpanAttributes { + const attributes: SpanAttributes = { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: SPAN_OP, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: SPAN_ORIGIN, + }; + + if (!config.recordFilePaths) { + return attributes; + } + + 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') { + if (FS_OPERATIONS_WITH_TARGET_PATH.includes(functionName)) { + attributes['target_argument'] = args[0]; + attributes['path_argument'] = args[1]; + } else if (FS_OPERATIONS_WITH_EXISTING_PATH_NEW_PATH.includes(functionName)) { + attributes['existing_path_argument'] = args[0]; + attributes['new_path_argument'] = args[1]; + } else if (FS_OPERATIONS_WITH_SRC_DEST.includes(functionName)) { + attributes['src_argument'] = args[0]; + attributes['dest_argument'] = args[1]; + } else if (FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH.includes(functionName)) { + attributes['old_path_argument'] = args[0]; + attributes['new_path_argument'] = args[1]; + } + } else if (typeof args[0] === 'string' && FS_OPERATIONS_WITH_PREFIX.includes(functionName)) { + attributes['prefix_argument'] = args[0]; + } + + return attributes; +} + /** * This is important for 2-level functions like `realpath.native` to retain the 2nd-level * when patching the 1st-level. */ -function patchedFunctionWithOriginalProperties ReturnType>( - patchedFunction: T, - original: T, -): T { +function patchedFunctionWithOriginalProperties(patchedFunction: T, original: T): T { return Object.assign(patchedFunction, original); } export class FsInstrumentation extends InstrumentationBase { - constructor(config: FsInstrumentationConfig = {}) { + public constructor(config: FsInstrumentationConfig = {}) { super(PACKAGE_NAME, SDK_VERSION, config); } - init(): (InstrumentationNodeModuleDefinition | InstrumentationNodeModuleDefinition)[] { + public init(): InstrumentationNodeModuleDefinition[] { return [ new InstrumentationNodeModuleDefinition( 'fs', @@ -63,7 +169,7 @@ export class FsInstrumentation extends InstrumentationBase ReturnType>(functionName: FMember, original: T): T { + protected _patchSyncFunction(functionName: FMember, original: T): T { + // oxlint-disable-next-line typescript/no-this-alias const instrumentation = this; - const patchedFunction = function (this: any, ...args: any[]) { - const activeContext = api.context.active(); - - if (!instrumentation._shouldTrace(activeContext)) { - return original.apply(this, args); - } - if ( - instrumentation._runCreateHook(functionName, { - args: args, - }) === false - ) { - return api.context.with(suppressTracing(activeContext), original, this, ...args); - } - - const span = instrumentation.tracer.startSpan(`fs ${functionName}`) as api.Span; + const patchedFunction = function (this: unknown, ...args: Parameters): ReturnType { + const config = instrumentation.getConfig(); + const attributes = getSpanAttributes(functionName, args, config); - try { - // Suppress tracing for internal fs calls - const res = api.context.with(suppressTracing(api.trace.setSpan(activeContext, span)), original, this, ...args); - instrumentation._runEndHook(functionName, { args: args, span }); - return res; - } catch (error: any) { - span.recordException(error); - span.setStatus({ - message: error.message, - code: api.SpanStatusCode.ERROR, - }); - instrumentation._runEndHook(functionName, { args: args, span, error }); - throw error; - } finally { - span.end(); - } + return startSpan({ name: `fs.${functionName}`, onlyIfParent: true, attributes }, span => { + try { + // Suppress tracing for internal fs calls + return suppressTracing(() => original.apply(this, args)) as ReturnType; + } catch (error) { + recordError(span, error, config); + throw error; + } + }); }; - return patchedFunctionWithOriginalProperties(patchedFunction as any, original); + return patchedFunctionWithOriginalProperties(patchedFunction as T, original); } - protected _patchCallbackFunction ReturnType>(functionName: FMember, original: T): T { + protected _patchCallbackFunction(functionName: FMember, original: T): T { + // oxlint-disable-next-line typescript/no-this-alias const instrumentation = this; - const patchedFunction = function (this: any, ...args: any[]) { - const activeContext = api.context.active(); - - if (!instrumentation._shouldTrace(activeContext)) { - return original.apply(this, args); - } - if ( - instrumentation._runCreateHook(functionName, { - args: args, - }) === false - ) { - return api.context.with(suppressTracing(activeContext), original, this, ...args); - } + const patchedFunction = function (this: unknown, ...args: Parameters): ReturnType { + const config = instrumentation.getConfig(); const lastIdx = args.length - 1; - const cb = args[lastIdx]; - if (typeof cb === 'function') { - const span = instrumentation.tracer.startSpan(`fs ${functionName}`) as api.Span; + const cb: unknown = args[lastIdx]; + if (typeof cb !== 'function') { + // TODO: what to do if we are pretty sure it's going to throw + return original.apply(this, args) as ReturnType; + } - // Return to the context active during the call in the callback - args[lastIdx] = api.context.bind(activeContext, function (this: unknown, error?: Error) { - if (error) { - span.recordException(error); - span.setStatus({ - message: error.message, - code: api.SpanStatusCode.ERROR, - }); - } - instrumentation._runEndHook(functionName, { - args: args, - span, - error, - }); - span.end(); - return cb.apply(this, arguments); - }); + const attributes = getSpanAttributes(functionName, args, config); + const span = startInactiveSpan({ name: `fs.${functionName}`, onlyIfParent: true, attributes }); - try { - // Suppress tracing for internal fs calls - return api.context.with(suppressTracing(api.trace.setSpan(activeContext, span)), original, this, ...args); - } catch (error: any) { - span.recordException(error); - span.setStatus({ - message: error.message, - code: api.SpanStatusCode.ERROR, - }); - instrumentation._runEndHook(functionName, { - args: args, - span, - error, - }); - span.end(); - throw error; + // Return to the context active during the call in the callback + args[lastIdx] = context.bind(context.active(), function (this: unknown, ...cbArgs: unknown[]) { + const error = cbArgs[0]; + if (error) { + recordError(span, error, config); } - } else { - // TODO: what to do if we are pretty sure it's going to throw - return original.apply(this, args); + span.end(); + return cb.apply(this, cbArgs); + }); + + try { + // Suppress tracing for internal fs calls + return suppressTracing(() => original.apply(this, args)) as ReturnType; + } catch (error) { + recordError(span, error, config); + span.end(); + throw error; } }; - return patchedFunctionWithOriginalProperties(patchedFunction as any, original); + return patchedFunctionWithOriginalProperties(patchedFunction as T, original); } - protected _patchExistsCallbackFunction ReturnType>( - functionName: 'exists', - original: T, - ): T { + protected _patchExistsCallbackFunction(functionName: 'exists', original: T): T { + // oxlint-disable-next-line typescript/no-this-alias const instrumentation = this; - const patchedFunction = function (this: any, ...args: any[]) { - const activeContext = api.context.active(); + const patchedFunction = function (this: unknown, ...args: Parameters): ReturnType { + const config = instrumentation.getConfig(); - if (!instrumentation._shouldTrace(activeContext)) { - return original.apply(this, args); - } - if ( - instrumentation._runCreateHook(functionName, { - args: args, - }) === false - ) { - return api.context.with(suppressTracing(activeContext), original, this, ...args); + const lastIdx = args.length - 1; + const cb: unknown = args[lastIdx]; + if (typeof cb !== 'function') { + return original.apply(this, args) as ReturnType; } - const lastIdx = args.length - 1; - const cb = args[lastIdx]; - if (typeof cb === 'function') { - const span = instrumentation.tracer.startSpan(`fs ${functionName}`) as api.Span; + const attributes = getSpanAttributes(functionName, args, config); + const span = startInactiveSpan({ name: `fs.${functionName}`, onlyIfParent: true, attributes }); - // Return to the context active during the call in the callback - args[lastIdx] = api.context.bind(activeContext, function (this: unknown) { - // `exists` never calls the callback with an error - instrumentation._runEndHook(functionName, { - args: args, - span, - }); - span.end(); - return cb.apply(this, arguments); - }); + // Return to the context active during the call in the callback + args[lastIdx] = context.bind(context.active(), function (this: unknown, ...cbArgs: unknown[]) { + // `exists` never calls the callback with an error + span.end(); + return cb.apply(this, cbArgs); + }); - try { - // Suppress tracing for internal fs calls - return api.context.with(suppressTracing(api.trace.setSpan(activeContext, span)), original, this, ...args); - } catch (error: any) { - span.recordException(error); - span.setStatus({ - message: error.message, - code: api.SpanStatusCode.ERROR, - }); - instrumentation._runEndHook(functionName, { - args: args, - span, - error, - }); - span.end(); - throw error; - } - } else { - return original.apply(this, args); + try { + // Suppress tracing for internal fs calls + return suppressTracing(() => original.apply(this, args)) as ReturnType; + } catch (error) { + recordError(span, error, config); + span.end(); + throw error; } }; - const functionWithOriginalProperties = patchedFunctionWithOriginalProperties(patchedFunction, original); + const functionWithOriginalProperties = patchedFunctionWithOriginalProperties(patchedFunction as T, original); // `exists` has a custom promisify function because of the inconsistent signature // replicating that on the patched function - const promisified = function (path: unknown) { - return new Promise(resolve => functionWithOriginalProperties(path, resolve)); + const promisified = function (path: unknown): Promise { + return new Promise(resolve => (functionWithOriginalProperties as GenericFunction)(path, resolve)); }; Object.defineProperty(promisified, 'name', { value: functionName }); Object.defineProperty(functionWithOriginalProperties, promisify.custom, { value: promisified, }); - return functionWithOriginalProperties as T; + return functionWithOriginalProperties; } - protected _patchPromiseFunction ReturnType>(functionName: FPMember, original: T): T { + protected _patchPromiseFunction(functionName: FPMember, original: T): T { + // oxlint-disable-next-line typescript/no-this-alias const instrumentation = this; - const patchedFunction = async function (this: any, ...args: any[]) { - const activeContext = api.context.active(); + const patchedFunction = async function (this: unknown, ...args: Parameters): Promise { + const config = instrumentation.getConfig(); + const attributes = getSpanAttributes(functionName, args, config); - if (!instrumentation._shouldTrace(activeContext)) { - return original.apply(this, args); - } - if ( - instrumentation._runCreateHook(functionName, { - args: args, - }) === false - ) { - return api.context.with(suppressTracing(activeContext), original, this, ...args); - } - - const span = instrumentation.tracer.startSpan(`fs ${functionName}`) as api.Span; - - try { - // Suppress tracing for internal fs calls - const res = await api.context.with( - suppressTracing(api.trace.setSpan(activeContext, span)), - original, - this, - ...args, - ); - instrumentation._runEndHook(functionName, { args: args, span }); - return res; - } catch (error: any) { - span.recordException(error); - span.setStatus({ - message: error.message, - code: api.SpanStatusCode.ERROR, - }); - instrumentation._runEndHook(functionName, { args: args, span, error }); - throw error; - } finally { - span.end(); - } + return startSpan({ name: `fs.${functionName}`, onlyIfParent: true, attributes }, async span => { + try { + // Suppress tracing for internal fs calls + return await suppressTracing(() => original.apply(this, args) as Promise); + } catch (error) { + recordError(span, error, config); + throw error; + } + }); }; - return patchedFunctionWithOriginalProperties(patchedFunction as any, original); + return patchedFunctionWithOriginalProperties(patchedFunction as unknown as T, original); } +} - protected _runCreateHook(...args: Parameters): ReturnType { - const { createHook } = this.getConfig(); - if (typeof createHook === 'function') { - try { - return createHook(...args); - } catch (e) { - this._diag.error('caught createHook error', e); - } - } - return true; - } - - protected _runEndHook(...args: Parameters): ReturnType { - const { endHook } = this.getConfig(); - if (typeof endHook === 'function') { - try { - endHook(...args); - } catch (e) { - this._diag.error('caught endHook error', e); - } - } - } - - protected _shouldTrace(context: api.Context): boolean { - if (isTracingSuppressed(context)) { - // Performance optimization. Avoid creating additional contexts and spans - // if we already know that the tracing is being suppressed. - return false; - } - - const { requireParentSpan } = this.getConfig(); - if (requireParentSpan) { - const parentSpan = api.trace.getSpan(context); - if (parentSpan == null) { - return false; - } - } - - return true; +/** + * Sets the error status on the span and, if configured, records the error message as a span attribute. + */ +function recordError(span: Span, error: unknown, config: FsInstrumentationConfig): void { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + if (config.recordErrorMessagesAsSpanAttributes && error instanceof Error) { + span.setAttribute('fs_error', error.message); } } diff --git a/packages/node/src/integrations/fs/vendored/types.ts b/packages/node/src/integrations/fs/vendored/types.ts index 38806d6a8435..ac0e3ce2e088 100644 --- a/packages/node/src/integrations/fs/vendored/types.ts +++ b/packages/node/src/integrations/fs/vendored/types.ts @@ -16,27 +16,29 @@ * NOTICE from the Sentry authors: * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-fs * - Upstream version: @opentelemetry/instrumentation-fs@0.37.0 + * - The `createHook`/`endHook`/`requireParentSpan` config options were removed in favor of Sentry-specific options. */ -/* eslint-disable */ +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; import type * as fs from 'fs'; -import type * as api from '@opentelemetry/api'; -import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +// oxlint-disable-next-line typescript/no-explicit-any +export type GenericFunction = (...args: any[]) => unknown; export type FunctionPropertyNames = Exclude< { + // oxlint-disable-next-line typescript/no-unsafe-function-type [K in keyof T]: T[K] extends Function ? K : never; }[keyof T], undefined >; -export type FunctionProperties = Pick>; export type FunctionPropertyNamesTwoLevels = Exclude< { [K in keyof T]: { [L in keyof T[K]]: L extends string - ? T[K][L] extends Function + ? // oxlint-disable-next-line typescript/no-unsafe-function-type + T[K][L] extends Function ? K extends string ? L extends string ? `${K}.${L}` @@ -49,20 +51,19 @@ export type FunctionPropertyNamesTwoLevels = Exclude< undefined >; -export type Member = FunctionPropertyNames | FunctionPropertyNamesTwoLevels; export type FMember = FunctionPropertyNames | FunctionPropertyNamesTwoLevels; export type FPMember = | FunctionPropertyNames<(typeof fs)['promises']> | FunctionPropertyNamesTwoLevels<(typeof fs)['promises']>; -export type CreateHook = (functionName: FMember | FPMember, info: { args: ArrayLike }) => boolean; -export type EndHook = ( - functionName: FMember | FPMember, - info: { args: ArrayLike; span: api.Span; error?: Error }, -) => void; - export interface FsInstrumentationConfig extends InstrumentationConfig { - createHook?: CreateHook; - endHook?: EndHook; - requireParentSpan?: boolean; + /** + * Setting this option to `true` will include any filepath arguments from your `fs` API calls as span attributes. + */ + recordFilePaths?: boolean; + + /** + * Setting this option to `true` will include the error messages of failed `fs` API calls as a span attribute. + */ + recordErrorMessagesAsSpanAttributes?: boolean; } diff --git a/packages/node/src/integrations/fs/vendored/utils.ts b/packages/node/src/integrations/fs/vendored/utils.ts index 3a76d0c17493..e9ecf59bd1ba 100644 --- a/packages/node/src/integrations/fs/vendored/utils.ts +++ b/packages/node/src/integrations/fs/vendored/utils.ts @@ -17,10 +17,10 @@ * - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-fs * - Upstream version: @opentelemetry/instrumentation-fs@0.37.0 */ -/* eslint-disable */ -import type { FunctionPropertyNames, FMember } from './types'; import type * as fs from 'fs'; +import type { FMember, FunctionPropertyNames, GenericFunction } from './types'; + type FS = typeof fs; export function splitTwoLevels( @@ -38,18 +38,18 @@ export function splitTwoLevels( export function indexFs( fs: FSObject, member: FMember, -): { objectToPatch: any; functionNameToPatch: string } { +): { objectToPatch: Record; functionNameToPatch: string } { if (!member) throw new Error(JSON.stringify({ member })); const splitResult = splitTwoLevels(member); const [functionName1, functionName2] = splitResult; if (functionName2) { return { - objectToPatch: fs[functionName1], + objectToPatch: fs[functionName1] as Record, functionNameToPatch: functionName2, }; } else { return { - objectToPatch: fs, + objectToPatch: fs as unknown as Record, functionNameToPatch: functionName1, }; }