From 11632bc0a7db84231a780a4057d4023708373be6 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Tue, 27 Dec 2022 16:51:46 +0100 Subject: [PATCH] feat(instrumentation-fs): require parent span --- plugins/node/instrumentation-fs/README.md | 9 +- .../instrumentation-fs/src/instrumentation.ts | 62 ++++--- plugins/node/instrumentation-fs/src/types.ts | 1 + .../instrumentation-fs/test/parent.test.ts | 169 ++++++++++++++++++ 4 files changed, 215 insertions(+), 26 deletions(-) create mode 100644 plugins/node/instrumentation-fs/test/parent.test.ts diff --git a/plugins/node/instrumentation-fs/README.md b/plugins/node/instrumentation-fs/README.md index 29f545a15f..77718809da 100644 --- a/plugins/node/instrumentation-fs/README.md +++ b/plugins/node/instrumentation-fs/README.md @@ -40,10 +40,11 @@ registerInstrumentations({ 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. | +| 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. | +| `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..e10bf16bad 100644 --- a/plugins/node/instrumentation-fs/src/instrumentation.ts +++ b/plugins/node/instrumentation-fs/src/instrumentation.ts @@ -128,9 +128,9 @@ 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. + const activeContext = api.context.active(); + + if (!instrumentation._shouldTrace(activeContext)) { return original.apply(this, args); } if ( @@ -139,7 +139,7 @@ export default class FsInstrumentation extends InstrumentationBase { }) === false ) { return api.context.with( - suppressTracing(api.context.active()), + suppressTracing(activeContext), original, this, ...args @@ -153,7 +153,7 @@ export default class FsInstrumentation extends InstrumentationBase { try { // Suppress tracing for internal fs calls const res = api.context.with( - suppressTracing(api.trace.setSpan(api.context.active(), span)), + suppressTracing(api.trace.setSpan(activeContext, span)), original, this, ...args @@ -180,9 +180,9 @@ 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. + const activeContext = api.context.active(); + + if (!instrumentation._shouldTrace(activeContext)) { return original.apply(this, args); } if ( @@ -191,7 +191,7 @@ export default class FsInstrumentation extends InstrumentationBase { }) === false ) { return api.context.with( - suppressTracing(api.context.active()), + suppressTracing(activeContext), original, this, ...args @@ -207,7 +207,7 @@ export default class FsInstrumentation extends InstrumentationBase { // Return to the context active during the call in the callback args[lastIdx] = api.context.bind( - api.context.active(), + activeContext, function (this: unknown, error?: Error) { if (error) { span.recordException(error); @@ -229,7 +229,7 @@ export default class FsInstrumentation extends InstrumentationBase { try { // Suppress tracing for internal fs calls return api.context.with( - suppressTracing(api.trace.setSpan(api.context.active(), span)), + suppressTracing(api.trace.setSpan(activeContext, span)), original, this, ...args @@ -260,9 +260,9 @@ 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. + const activeContext = api.context.active(); + + if (!instrumentation._shouldTrace(activeContext)) { return original.apply(this, args); } if ( @@ -271,7 +271,7 @@ export default class FsInstrumentation extends InstrumentationBase { }) === false ) { return api.context.with( - suppressTracing(api.context.active()), + suppressTracing(activeContext), original, this, ...args @@ -287,7 +287,7 @@ export default class FsInstrumentation extends InstrumentationBase { // Return to the context active during the call in the callback args[lastIdx] = api.context.bind( - api.context.active(), + activeContext, function (this: unknown) { // `exists` never calls the callback with an error instrumentation._runEndHook(functionName, { @@ -302,7 +302,7 @@ export default class FsInstrumentation extends InstrumentationBase { try { // Suppress tracing for internal fs calls return api.context.with( - suppressTracing(api.trace.setSpan(api.context.active(), span)), + suppressTracing(api.trace.setSpan(activeContext, span)), original, this, ...args @@ -345,9 +345,9 @@ 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. + const activeContext = api.context.active(); + + if (!instrumentation._shouldTrace(activeContext)) { return original.apply(this, args); } if ( @@ -356,7 +356,7 @@ export default class FsInstrumentation extends InstrumentationBase { }) === false ) { return api.context.with( - suppressTracing(api.context.active()), + suppressTracing(activeContext), original, this, ...args @@ -370,7 +370,7 @@ export default class FsInstrumentation extends InstrumentationBase { try { // Suppress tracing for internal fs calls const res = await api.context.with( - suppressTracing(api.trace.setSpan(api.context.active(), span)), + suppressTracing(api.trace.setSpan(activeContext, span)), original, this, ...args @@ -415,4 +415,22 @@ export default class FsInstrumentation extends InstrumentationBase { } } } + + 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() as FsInstrumentationConfig; + if (requireParentSpan) { + const parentSpan = api.trace.getSpan(context); + if (parentSpan == null) { + 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..607149adc0 --- /dev/null +++ b/plugins/node/instrumentation-fs/test/parent.test.ts @@ -0,0 +1,169 @@ +/* + * 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 { + 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 ambientContext: api.Context; + let endRootSpan: () => void; + let expectedAmbientSpanCount: 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 generateTestsForVariants = ({ + expectFSSpan, + }: { + expectFSSpan: boolean; + }) => { + const prefix = expectFSSpan ? 'creates' : 'does not create'; + const expectedSpanCount = () => + (expectFSSpan ? 1 : 0) + expectedAmbientSpanCount; + + it(`${prefix} a span with the callback API`, async () => { + await new Promise(resolve => { + api.context.with(ambientContext, () => { + fs.access('./test/fixtures/readtest', fs.constants.R_OK, () => + resolve() + ); + }); + }).then(() => endRootSpan()); + + assert.deepEqual( + memoryExporter.getFinishedSpans().length, + expectedSpanCount() + ); + }); + + it(`${prefix} a span with the synchronous API`, () => { + api.context.with(ambientContext, () => { + fs.accessSync('./test/fixtures/readtest', fs.constants.R_OK); + endRootSpan(); + }); + + assert.deepEqual( + memoryExporter.getFinishedSpans().length, + expectedSpanCount() + ); + }); + + if (supportsPromises) { + it(`${prefix} a span with the promises API`, async () => { + await new Promise(resolve => { + api.context.with(ambientContext, () => { + fs.promises + .access('./test/fixtures/readtest', fs.constants.R_OK) + .finally(() => resolve(endRootSpan())); + }); + }); + + assert.deepEqual( + memoryExporter.getFinishedSpans().length, + expectedSpanCount() + ); + }); + } + }; + + const withRootSpan = (fn: () => void) => { + describe('with a root span', () => { + beforeEach(() => { + const rootSpan = tracer.startSpan('root span'); + + ambientContext = api.trace.setSpan(api.context.active(), rootSpan); + endRootSpan = () => rootSpan.end(); + expectedAmbientSpanCount = 1; + }); + + fn(); + }); + }; + + const withoutRootSpan = (fn: () => void) => { + describe('without a root span', () => { + beforeEach(() => { + ambientContext = api.context.active(); + endRootSpan = () => {}; + expectedAmbientSpanCount = 0; + }); + + fn(); + }); + }; + + describe('requireParentSpan enabled', () => { + beforeEach(() => { + initializePlugin({ requireParentSpan: true }); + }); + + withRootSpan(() => { + generateTestsForVariants({ expectFSSpan: true }); + }); + + withoutRootSpan(() => { + generateTestsForVariants({ expectFSSpan: false }); + }); + }); + + describe('requireParentSpan disabled', () => { + beforeEach(() => { + initializePlugin({ requireParentSpan: false }); + }); + + withRootSpan(() => { + generateTestsForVariants({ expectFSSpan: true }); + }); + + withoutRootSpan(() => { + generateTestsForVariants({ expectFSSpan: true }); + }); + }); +});