Skip to content

Commit

Permalink
feat(instrumentation-fs): require parent span (open-telemetry#1335)
Browse files Browse the repository at this point in the history
Co-authored-by: Amir Blum <[email protected]>
  • Loading branch information
unflxw and blumamir authored Jan 16, 2023
1 parent 6e33568 commit 79b2d3f
Show file tree
Hide file tree
Showing 4 changed files with 215 additions and 26 deletions.
9 changes: 5 additions & 4 deletions plugins/node/instrumentation-fs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,11 @@ registerInstrumentations({

You can set the following:

| Options | Type | Description |
| ------- | ---- | ----------- |
| `createHook` | `(functionName: FMember | FPMember, info: { args: ArrayLike<unknown> }) => 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<unknown>; 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<unknown> }) => 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<unknown>; 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

Expand Down
62 changes: 40 additions & 22 deletions plugins/node/instrumentation-fs/src/instrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,9 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
): T {
const instrumentation = this;
return <any>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 (
Expand All @@ -139,7 +139,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
}) === false
) {
return api.context.with(
suppressTracing(api.context.active()),
suppressTracing(activeContext),
original,
this,
...args
Expand All @@ -153,7 +153,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
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
Expand All @@ -180,9 +180,9 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
): T {
const instrumentation = this;
return <any>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 (
Expand All @@ -191,7 +191,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
}) === false
) {
return api.context.with(
suppressTracing(api.context.active()),
suppressTracing(activeContext),
original,
this,
...args
Expand All @@ -207,7 +207,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {

// 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);
Expand All @@ -229,7 +229,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
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
Expand Down Expand Up @@ -260,9 +260,9 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
>(functionName: FMember, original: T): T {
const instrumentation = this;
const patchedFunction = <any>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 (
Expand All @@ -271,7 +271,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
}) === false
) {
return api.context.with(
suppressTracing(api.context.active()),
suppressTracing(activeContext),
original,
this,
...args
Expand All @@ -287,7 +287,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {

// 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, {
Expand All @@ -302,7 +302,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
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
Expand Down Expand Up @@ -345,9 +345,9 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
): T {
const instrumentation = this;
return <any>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 (
Expand All @@ -356,7 +356,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
}) === false
) {
return api.context.with(
suppressTracing(api.context.active()),
suppressTracing(activeContext),
original,
this,
...args
Expand All @@ -370,7 +370,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
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
Expand Down Expand Up @@ -415,4 +415,22 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
}
}
}

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;
}
}
1 change: 1 addition & 0 deletions plugins/node/instrumentation-fs/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,5 @@ export type EndHook = (
export interface FsInstrumentationConfig extends InstrumentationConfig {
createHook?: CreateHook;
endHook?: EndHook;
requireParentSpan?: boolean;
}
169 changes: 169 additions & 0 deletions plugins/node/instrumentation-fs/test/parent.test.ts
Original file line number Diff line number Diff line change
@@ -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<void>(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<void>(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 });
});
});
});

0 comments on commit 79b2d3f

Please sign in to comment.