diff --git a/plugins/node/instrumentation-fs/README.md b/plugins/node/instrumentation-fs/README.md index 29f545a15f..4918c17c6e 100644 --- a/plugins/node/instrumentation-fs/README.md +++ b/plugins/node/instrumentation-fs/README.md @@ -42,8 +42,9 @@ You can set the following: | Options | Type | Description | | ------- | ---- | ----------- | -| `createHook` | `(functionName: FMember | FPMember, info: { args: ArrayLike }) => boolean` | Hook called before creating the span. If `false` is returned this and all the sibling calls will not be traced. | -| `endHook` | `( functionName: FMember | FPMember, info: { args: ArrayLike; span: api.Span } ) => void` | Function called just before the span is ended. Useful for adding attributes. | +| `createHook` | `(functionName: FMember \| FPMember, info: { args: ArrayLike }) => boolean` | Hook called before creating the span. If `false` is returned this and all the sibling calls will not be traced. | +| `endHook` | `( functionName: FMember \| FPMember, info: { args: ArrayLike; span: api.Span } ) => void` | Function called just before the span is ended. Useful for adding attributes. | +| `requireParentSpan` | `boolean` | Require parent to create fs span, default when unset is `false`. | ## Useful links diff --git a/plugins/node/instrumentation-fs/src/instrumentation.ts b/plugins/node/instrumentation-fs/src/instrumentation.ts index 6bd778da83..503e0cc238 100644 --- a/plugins/node/instrumentation-fs/src/instrumentation.ts +++ b/plugins/node/instrumentation-fs/src/instrumentation.ts @@ -128,9 +128,7 @@ export default class FsInstrumentation extends InstrumentationBase { ): T { const instrumentation = this; return function (this: any, ...args: any[]) { - if (isTracingSuppressed(api.context.active())) { - // Performance optimization. Avoid creating additional contexts and spans - // if we already know that the tracing is being suppressed. + if (!instrumentation._shouldTrace()) { return original.apply(this, args); } if ( @@ -180,9 +178,7 @@ export default class FsInstrumentation extends InstrumentationBase { ): T { const instrumentation = this; return function (this: any, ...args: any[]) { - if (isTracingSuppressed(api.context.active())) { - // Performance optimization. Avoid creating additional contexts and spans - // if we already know that the tracing is being suppressed. + if (!instrumentation._shouldTrace()) { return original.apply(this, args); } if ( @@ -260,9 +256,7 @@ export default class FsInstrumentation extends InstrumentationBase { >(functionName: FMember, original: T): T { const instrumentation = this; const patchedFunction = function (this: any, ...args: any[]) { - if (isTracingSuppressed(api.context.active())) { - // Performance optimization. Avoid creating additional contexts and spans - // if we already know that the tracing is being suppressed. + if (!instrumentation._shouldTrace()) { return original.apply(this, args); } if ( @@ -345,9 +339,7 @@ export default class FsInstrumentation extends InstrumentationBase { ): T { const instrumentation = this; return async function (this: any, ...args: any[]) { - if (isTracingSuppressed(api.context.active())) { - // Performance optimization. Avoid creating additional contexts and spans - // if we already know that the tracing is being suppressed. + if (!instrumentation._shouldTrace()) { return original.apply(this, args); } if ( @@ -415,4 +407,23 @@ export default class FsInstrumentation extends InstrumentationBase { } } } + + protected _shouldTrace(): boolean { + const activeContext = api.context.active(); + if (isTracingSuppressed(activeContext)) { + // Performance optimization. Avoid creating additional contexts and spans + // if we already know that the tracing is being suppressed. + return false; + } + + const { requireParentSpan } = this.getConfig() as FsInstrumentationConfig; + if (requireParentSpan) { + const parentSpan = api.trace.getSpan(activeContext); + if (parentSpan === undefined) { + return false; + } + } + + return true; + } } diff --git a/plugins/node/instrumentation-fs/src/types.ts b/plugins/node/instrumentation-fs/src/types.ts index d531cae43c..04b1611db2 100644 --- a/plugins/node/instrumentation-fs/src/types.ts +++ b/plugins/node/instrumentation-fs/src/types.ts @@ -38,4 +38,5 @@ export type EndHook = ( export interface FsInstrumentationConfig extends InstrumentationConfig { createHook?: CreateHook; endHook?: EndHook; + requireParentSpan?: boolean; } diff --git a/plugins/node/instrumentation-fs/test/parent.test.ts b/plugins/node/instrumentation-fs/test/parent.test.ts new file mode 100644 index 0000000000..82c579221c --- /dev/null +++ b/plugins/node/instrumentation-fs/test/parent.test.ts @@ -0,0 +1,137 @@ +import { + BasicTracerProvider, + InMemorySpanExporter, + SimpleSpanProcessor, +} from '@opentelemetry/sdk-trace-base'; +import Instrumentation from '../src'; +import * as assert from 'assert'; +import type * as FSType from 'fs'; +import type { FsInstrumentationConfig } from '../src/types'; +import * as api from '@opentelemetry/api'; +import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; + +const supportsPromises = parseInt(process.versions.node.split('.')[0], 10) > 8; + +const provider = new BasicTracerProvider(); +const memoryExporter = new InMemorySpanExporter(); +provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter)); + +const tracer = provider.getTracer('default'); + +describe('fs instrumentation: requireParentSpan', () => { + let plugin: Instrumentation; + let fs: typeof FSType; + let context: api.Context; + let endRootSpan: () => void; + let additionalSpans: number; + + const initializePlugin = (pluginConfig: FsInstrumentationConfig) => { + plugin = new Instrumentation(); + plugin.setTracerProvider(provider); + plugin.setConfig(pluginConfig); + plugin.enable(); + fs = require('fs'); + assert.strictEqual(memoryExporter.getFinishedSpans().length, 0); + } + + beforeEach(() => { + const contextManager = new AsyncHooksContextManager(); + api.context.setGlobalContextManager(contextManager.enable()); + }) + + afterEach(() => { + plugin.disable(); + memoryExporter.reset(); + }); + + const createSpanTests = (shouldCreateSpan: boolean) => { + const prefix = shouldCreateSpan ? "creates" : "does not create" + const expectedSpans = () => (shouldCreateSpan ? 1 : 0) + additionalSpans + + it(`${prefix} a span with the callback API`, async () => { + await new Promise((resolve) => { + api.context.with(context, () => { + fs.access('./test/fixtures/readtest', fs.constants.R_OK, () => resolve()) + }) + }).then(() => endRootSpan()) + + assert.deepEqual(memoryExporter.getFinishedSpans().length, expectedSpans()); + }) + + it(`${prefix} a span with the synchronous API`, () => { + api.context.with(context, () => { + fs.accessSync('./test/fixtures/readtest', fs.constants.R_OK) + endRootSpan() + }) + + assert.deepEqual(memoryExporter.getFinishedSpans().length, expectedSpans()); + }) + + if (supportsPromises) { + it(`${prefix} a span with the promises API`, async () => { + await new Promise((resolve) => { + api.context.with(context, () => { + fs.promises.access('./test/fixtures/readtest', fs.constants.R_OK) + .finally(() => resolve(endRootSpan())) + }) + }) + + assert.deepEqual(memoryExporter.getFinishedSpans().length, expectedSpans()); + }) + } + } + + const withRootSpan = (fn: () => void) => { + describe('with a root span', () => { + beforeEach(() => { + const rootSpan = tracer.startSpan("root span") + + context = api.trace.setSpan(api.context.active(), rootSpan) + endRootSpan = () => rootSpan.end() + additionalSpans = 1 + }) + + fn() + }) + } + + const withoutRootSpan = (fn: () => void) => { + describe('without a root span', () => { + beforeEach(() => { + context = api.context.active() + endRootSpan = () => {} + additionalSpans = 0 + }) + + fn() + }) + } + + describe('requireParentSpan enabled', () => { + beforeEach(() => { + initializePlugin({requireParentSpan: true}) + }) + + withRootSpan(() => { + createSpanTests(true) + }) + + withoutRootSpan(() => { + createSpanTests(false) + }) + }) + + describe('requireParentSpan disabled', () => { + beforeEach(() => { + initializePlugin({requireParentSpan: false}) + }) + + withRootSpan(() => { + createSpanTests(true) + }) + + withoutRootSpan(() => { + createSpanTests(true) + }) + }) +})