diff --git a/plugins/node/instrumentation-fs/src/constants.ts b/plugins/node/instrumentation-fs/src/constants.ts index 4b2a8ac0fc..a8588aae8f 100644 --- a/plugins/node/instrumentation-fs/src/constants.ts +++ b/plugins/node/instrumentation-fs/src/constants.ts @@ -67,6 +67,7 @@ export const CALLBACK_FUNCTIONS: FMember[] = [ 'readFile', 'readlink', 'realpath', + 'realpath.native', 'rename', 'rm', // added in v14 'rmdir', @@ -111,6 +112,7 @@ export const SYNC_FUNCTIONS: FMember[] = [ 'readFileSync', 'readlinkSync', 'realpathSync', + 'realpathSync.native', 'renameSync', 'rmdirSync', 'rmSync', // added in v14 diff --git a/plugins/node/instrumentation-fs/src/instrumentation.ts b/plugins/node/instrumentation-fs/src/instrumentation.ts index 49af222b61..73059be47c 100644 --- a/plugins/node/instrumentation-fs/src/instrumentation.ts +++ b/plugins/node/instrumentation-fs/src/instrumentation.ts @@ -36,9 +36,20 @@ import type { FsInstrumentationConfig, } from './types'; import { promisify } from 'util'; +import { indexFs } from './utils'; type FS = typeof fs; +/** + * This is important for 2-level functions like `realpath.native` to retain the 2nd-level + * when patching the 1st-level. + */ +function patchedFunctionWithOriginalProperties< + T extends (...args: any[]) => ReturnType +>(patchedFunction: T, original: T): T { + return Object.assign(patchedFunction, original); +} + export default class FsInstrumentation extends InstrumentationBase { constructor(config?: FsInstrumentationConfig) { super('@opentelemetry/instrumentation-fs', VERSION, config); @@ -52,32 +63,35 @@ export default class FsInstrumentation extends InstrumentationBase { (fs: FS) => { this._diag.debug('Applying patch for fs'); for (const fName of SYNC_FUNCTIONS) { - if (isWrapped(fs[fName])) { - this._unwrap(fs, fName); + const { objectToPatch, functionNameToPatch } = indexFs(fs, fName); + + if (isWrapped(objectToPatch[functionNameToPatch])) { + this._unwrap(objectToPatch, functionNameToPatch); } this._wrap( - fs, - fName, + objectToPatch, + functionNameToPatch, this._patchSyncFunction.bind(this, fName) ); } for (const fName of CALLBACK_FUNCTIONS) { - if (isWrapped(fs[fName])) { - this._unwrap(fs, fName); + const { objectToPatch, functionNameToPatch } = indexFs(fs, fName); + if (isWrapped(objectToPatch[functionNameToPatch])) { + this._unwrap(objectToPatch, functionNameToPatch); } if (fName === 'exists') { // handling separately because of the inconsistent cb style: // `exists` doesn't have error as the first argument, but the result this._wrap( - fs, - fName, + objectToPatch, + functionNameToPatch, this._patchExistsCallbackFunction.bind(this, fName) ); continue; } this._wrap( - fs, - fName, + objectToPatch, + functionNameToPatch, this._patchCallbackFunction.bind(this, fName) ); } @@ -97,13 +111,15 @@ export default class FsInstrumentation extends InstrumentationBase { if (fs === undefined) return; this._diag.debug('Removing patch for fs'); for (const fName of SYNC_FUNCTIONS) { - if (isWrapped(fs[fName])) { - this._unwrap(fs, fName); + const { objectToPatch, functionNameToPatch } = indexFs(fs, fName); + if (isWrapped(objectToPatch[functionNameToPatch])) { + this._unwrap(objectToPatch, functionNameToPatch); } } for (const fName of CALLBACK_FUNCTIONS) { - if (isWrapped(fs[fName])) { - this._unwrap(fs, fName); + const { objectToPatch, functionNameToPatch } = indexFs(fs, fName); + if (isWrapped(objectToPatch[functionNameToPatch])) { + this._unwrap(objectToPatch, functionNameToPatch); } } for (const fName of PROMISE_FUNCTIONS) { @@ -121,7 +137,7 @@ export default class FsInstrumentation extends InstrumentationBase { original: T ): T { const instrumentation = this; - return function (this: any, ...args: any[]) { + const patchedFunction = function (this: any, ...args: any[]) { const activeContext = api.context.active(); if (!instrumentation._shouldTrace(activeContext)) { @@ -166,6 +182,7 @@ export default class FsInstrumentation extends InstrumentationBase { span.end(); } }; + return patchedFunctionWithOriginalProperties(patchedFunction, original); } protected _patchCallbackFunction ReturnType>( @@ -173,7 +190,7 @@ export default class FsInstrumentation extends InstrumentationBase { original: T ): T { const instrumentation = this; - return function (this: any, ...args: any[]) { + const patchedFunction = function (this: any, ...args: any[]) { const activeContext = api.context.active(); if (!instrumentation._shouldTrace(activeContext)) { @@ -247,11 +264,12 @@ export default class FsInstrumentation extends InstrumentationBase { return original.apply(this, args); } }; + return patchedFunctionWithOriginalProperties(patchedFunction, original); } protected _patchExistsCallbackFunction< T extends (...args: any[]) => ReturnType - >(functionName: FMember, original: T): T { + >(functionName: 'exists', original: T): T { const instrumentation = this; const patchedFunction = function (this: any, ...args: any[]) { const activeContext = api.context.active(); @@ -319,18 +337,22 @@ export default class FsInstrumentation extends InstrumentationBase { return original.apply(this, args); } }; + const functionWithOriginalProperties = + patchedFunctionWithOriginalProperties(patchedFunction, 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 => patchedFunction(path, resolve)); + return new Promise(resolve => + functionWithOriginalProperties(path, resolve) + ); }; Object.defineProperty(promisified, 'name', { value: functionName }); - Object.defineProperty(patchedFunction, promisify.custom, { + Object.defineProperty(functionWithOriginalProperties, promisify.custom, { value: promisified, }); - return patchedFunction; + return functionWithOriginalProperties; } protected _patchPromiseFunction ReturnType>( @@ -338,7 +360,7 @@ export default class FsInstrumentation extends InstrumentationBase { original: T ): T { const instrumentation = this; - return async function (this: any, ...args: any[]) { + const patchedFunction = async function (this: any, ...args: any[]) { const activeContext = api.context.active(); if (!instrumentation._shouldTrace(activeContext)) { @@ -383,6 +405,7 @@ export default class FsInstrumentation extends InstrumentationBase { span.end(); } }; + return patchedFunctionWithOriginalProperties(patchedFunction, original); } protected _runCreateHook( diff --git a/plugins/node/instrumentation-fs/src/types.ts b/plugins/node/instrumentation-fs/src/types.ts index 0b4e3caeef..fe0a4132c5 100644 --- a/plugins/node/instrumentation-fs/src/types.ts +++ b/plugins/node/instrumentation-fs/src/types.ts @@ -18,13 +18,40 @@ import type * as fs from 'fs'; import type * as api from '@opentelemetry/api'; import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; -export type FunctionPropertyNames = { - [K in keyof T]: T[K] extends Function ? K : never; -}[keyof T]; +export type FunctionPropertyNames = Exclude< + { + [K in keyof T]: T[K] extends Function ? K : never; + }[keyof T], + undefined +>; export type FunctionProperties = Pick>; -export type FMember = FunctionPropertyNames; -export type FPMember = FunctionPropertyNames<(typeof fs)['promises']>; +export type FunctionPropertyNamesTwoLevels = Exclude< + { + [K in keyof T]: { + [L in keyof T[K]]: L extends string + ? T[K][L] extends Function + ? K extends string + ? L extends string + ? `${K}.${L}` + : never + : never + : never + : never; + }[keyof T[K]]; + }[keyof T], + 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, diff --git a/plugins/node/instrumentation-fs/src/utils.ts b/plugins/node/instrumentation-fs/src/utils.ts new file mode 100644 index 0000000000..3a8c37149f --- /dev/null +++ b/plugins/node/instrumentation-fs/src/utils.ts @@ -0,0 +1,53 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import type { FunctionPropertyNames, FMember } from './types'; +import type * as fs from 'fs'; +type FS = typeof fs; + +export function splitTwoLevels( + functionName: FMember +): + | [FunctionPropertyNames & string] + | [FunctionPropertyNames & string, string] { + const memberParts = functionName.split('.'); + if (memberParts.length > 1) { + if (memberParts.length !== 2) + throw Error(`Invalid member function name ${functionName}`); + return memberParts as [FunctionPropertyNames & string, string]; + } else { + return [functionName as FunctionPropertyNames & string]; + } +} + +export function indexFs( + fs: FSObject, + member: FMember +): { objectToPatch: any; functionNameToPatch: string } { + if (!member) throw new Error(JSON.stringify({ member })); + const splitResult = splitTwoLevels(member); + const [functionName1, functionName2] = splitResult; + if (functionName2) { + return { + objectToPatch: fs[functionName1], + functionNameToPatch: functionName2, + }; + } else { + return { + objectToPatch: fs, + functionNameToPatch: functionName1, + }; + } +} diff --git a/plugins/node/instrumentation-fs/test/definitions.ts b/plugins/node/instrumentation-fs/test/definitions.ts index 551f104986..ed4a0673bc 100644 --- a/plugins/node/instrumentation-fs/test/definitions.ts +++ b/plugins/node/instrumentation-fs/test/definitions.ts @@ -17,16 +17,21 @@ import { FMember, FPMember } from '../src/types'; import * as fs from 'fs'; -export type FsFunction = FPMember & FMember; +export type FsFunction = FMember; export type Opts = { sync?: boolean; callback?: boolean; promise?: boolean; }; -export type Result = { error?: RegExp; result?: any; resultAsError?: any }; +export type Result = { + error?: RegExp; + result?: any; + resultAsError?: any; + hasPromiseVersion?: boolean; +}; export type TestCase = [FsFunction, any[], Result, any[], Opts?]; -export type TestCreator = ( - name: FsFunction, +export type TestCreator = ( + name: Member, args: any[], result: Result, spans: any[] @@ -77,6 +82,13 @@ const tests: TestCase[] = [ { resultAsError: true }, [{ name: 'fs %NAME' }], ], + ['realpath', ['/./'], { result: '/' }, [{ name: 'fs %NAME' }]], + [ + 'realpath.native', + ['/./'], + { result: '/', hasPromiseVersion: false }, + [{ name: 'fs %NAME' }], + ], ]; export default tests; diff --git a/plugins/node/instrumentation-fs/test/index.test.ts b/plugins/node/instrumentation-fs/test/index.test.ts index 774e97f221..c29fb0b40a 100644 --- a/plugins/node/instrumentation-fs/test/index.test.ts +++ b/plugins/node/instrumentation-fs/test/index.test.ts @@ -28,6 +28,12 @@ import * as sinon from 'sinon'; import type * as FSType from 'fs'; import tests, { TestCase, TestCreator } from './definitions'; import type { FMember, FPMember, CreateHook, EndHook } from '../src/types'; +import { + CALLBACK_FUNCTIONS, + PROMISE_FUNCTIONS, + SYNC_FUNCTIONS, +} from '../src/constants'; +import { indexFs, splitTwoLevels } from '../src/utils'; const TEST_ATTRIBUTE = 'test.attr'; const TEST_VALUE = 'test.attr.value'; @@ -36,7 +42,7 @@ const createHook = sinon.spy( (fnName: FMember | FPMember, { args, span }) => { // `ts-node`, which we use via `ts-mocha` also patches module loading and creates // a lot of unrelated spans. Filter those out. - if (['readFileSync', 'existsSync'].includes(fnName)) { + if (['readFileSync', 'existsSync'].includes(fnName as string)) { const filename = args[0]; if (!/test\/fixtures/.test(filename)) { return false; @@ -85,24 +91,41 @@ describe('fs instrumentation', () => { context.disable(); }); - const syncTest: TestCreator = ( + const syncTest: TestCreator = ( name: FMember, args, { error, result, resultAsError = null }, spans ) => { - const syncName: FMember = `${name}Sync` as FMember; + const [functionNamePart1, functionNamePart2] = + splitTwoLevels(name); + const syncName = `${functionNamePart1}Sync${ + functionNamePart2 ? `.${functionNamePart2}` : '' + }` as FMember; const rootSpanName = `${syncName} test span`; it(`${syncName} ${error ? 'error' : 'success'}`, () => { + const { objectToPatch, functionNameToPatch } = indexFs(fs, syncName); const rootSpan = tracer.startSpan(rootSpanName); assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); context.with(trace.setSpan(context.active(), rootSpan), () => { if (error) { - assert.throws(() => Reflect.apply(fs[syncName], fs, args), error); + assert.throws( + () => + Reflect.apply( + objectToPatch[functionNameToPatch], + objectToPatch, + args + ), + error + ); } else { assert.deepEqual( - Reflect.apply(fs[syncName], fs, args), + Reflect.apply( + objectToPatch[functionNameToPatch], + objectToPatch, + args + ), result ?? resultAsError ); } @@ -126,21 +149,32 @@ describe('fs instrumentation', () => { ]); }); }; + const makeRootSpanName = (name: FMember): string => { + let rsn: string; + if (Array.isArray(name)) { + rsn = `${name[0]}.${name[1]}`; + } else { + rsn = `${name}`; + } + rsn = `${rsn} test span`; + return rsn; + }; - const callbackTest: TestCreator = ( + const callbackTest: TestCreator = ( name: FMember, args, { error, result, resultAsError = null }, spans ) => { - const rootSpanName = `${name} test span`; + const rootSpanName = makeRootSpanName(name); it(`${name} ${error ? 'error' : 'success'}`, done => { + const { objectToPatch, functionNameToPatch } = indexFs(fs, name); const rootSpan = tracer.startSpan(rootSpanName); assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); context.with(trace.setSpan(context.active(), rootSpan), () => { - (fs[name] as Function)( + (objectToPatch[functionNameToPatch] as Function)( ...args, (actualError: any | undefined, actualResult: any) => { assert.strictEqual(trace.getSpan(context.active()), rootSpan); @@ -197,13 +231,14 @@ describe('fs instrumentation', () => { }); }; - const promiseTest: TestCreator = ( + const promiseTest: TestCreator = ( name: FPMember, args, - { error, result, resultAsError = null }, + { error, result, resultAsError = null, hasPromiseVersion = true }, spans ) => { - const rootSpanName = `${name} test span`; + if (!hasPromiseVersion) return; + const rootSpanName = makeRootSpanName(name); it(`promises.${name} ${error ? 'error' : 'success'}`, async () => { const rootSpan = tracer.startSpan(rootSpanName); @@ -258,6 +293,43 @@ describe('fs instrumentation', () => { }); }; + describe('Synchronous API native', () => { + beforeEach(() => { + plugin.enable(); + }); + it('should not remove fs functions', () => { + const isNode14 = process.versions.node.startsWith('14'); + const node14MissingFunctionNames = new Set([ + 'cpSync', + 'cp', + 'promises.cp', + ]); + for (const fname of [...SYNC_FUNCTIONS, ...CALLBACK_FUNCTIONS]) { + // some function were added after node 14 + if (node14MissingFunctionNames.has(fname) && isNode14) continue; + + const { objectToPatch, functionNameToPatch } = indexFs(fs, fname); + assert.strictEqual( + typeof objectToPatch[functionNameToPatch], + 'function', + `fs.${fname} is not a function` + ); + } + for (const fname of PROMISE_FUNCTIONS) { + if (node14MissingFunctionNames.has(fname) && isNode14) continue; + const { objectToPatch, functionNameToPatch } = indexFs( + fs.promises, + fname + ); + assert.strictEqual( + typeof objectToPatch[functionNameToPatch], + 'function', + `fs.promises.${fname} is not a function` + ); + } + }); + }); + describe('Syncronous API', () => { const selection: TestCase[] = tests.filter( ([, , , , options = {}]) => options.sync !== false