Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(instrumentation-fs): error hook #1963

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(instrumentation-fs): error hook #1963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions plugins/node/instrumentation-fs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,12 @@ 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. |
| `requireParentSpan` | `boolean` | Require parent to create fs span, default when unset is `false`. |
| 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; error?: NodeJS.ErrnoException } => void` | Hook called just before the span is ended. Useful for adding attributes. |
| `errorHook` | `(functionName: FMember \| FPMember, error: NodeJS.ErrnoException) => boolean` | Hook called when a filesystem function throws an error. If `false` is returned no error event will be added to the span and it won't end with an error status code. The error itself is always propagated. |
| `requireParentSpan` | `boolean` | Require parent to create fs span, default when unset is `false`. |

## Useful links

Expand Down
65 changes: 44 additions & 21 deletions plugins/node/instrumentation-fs/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import type {
CreateHook,
EndHook,
FsInstrumentationConfig,
ErrorHook,
} from './types';
import { promisify } from 'util';
import { indexFs } from './utils';
Expand Down Expand Up @@ -202,11 +203,13 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
instrumentation._runEndHook(functionName, { args: args, span });
return res;
} catch (error: any) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
if (instrumentation._runErrorHook(functionName, error)) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
}
instrumentation._runEndHook(functionName, { args: args, span, error });
throw error;
} finally {
Expand Down Expand Up @@ -251,7 +254,7 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
args[lastIdx] = api.context.bind(
activeContext,
function (this: unknown, error?: Error) {
if (error) {
if (error && instrumentation._runErrorHook(functionName, error)) {
span.recordException(error);
span.setStatus({
message: error.message,
Expand All @@ -277,11 +280,13 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
...args
);
} catch (error: any) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
if (instrumentation._runErrorHook(functionName, error)) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
}
instrumentation._runEndHook(functionName, {
args: args,
span,
Expand Down Expand Up @@ -351,11 +356,13 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
...args
);
} catch (error: any) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
if (instrumentation._runErrorHook(functionName, error)) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
}
instrumentation._runEndHook(functionName, {
args: args,
span,
Expand Down Expand Up @@ -425,11 +432,13 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
instrumentation._runEndHook(functionName, { args: args, span });
return res;
} catch (error: any) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
if (instrumentation._runErrorHook(functionName, error)) {
span.recordException(error);
span.setStatus({
message: error.message,
code: api.SpanStatusCode.ERROR,
});
}
instrumentation._runEndHook(functionName, { args: args, span, error });
throw error;
} finally {
Expand Down Expand Up @@ -464,6 +473,20 @@ export default class FsInstrumentation extends InstrumentationBase<FS> {
}
}

protected _runErrorHook(
...args: Parameters<ErrorHook>
): ReturnType<ErrorHook> {
const { errorHook } = this.getConfig() as FsInstrumentationConfig;
if (typeof errorHook === 'function') {
try {
return errorHook(...args);
} catch (e) {
this._diag.error('caught errorHook error', e);
}
}
return true;
}

protected _shouldTrace(context: api.Context): boolean {
if (isTracingSuppressed(context)) {
// Performance optimization. Avoid creating additional contexts and spans
Expand Down
11 changes: 10 additions & 1 deletion plugins/node/instrumentation-fs/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,20 @@ export type CreateHook = (
) => boolean;
export type EndHook = (
functionName: FMember | FPMember,
info: { args: ArrayLike<unknown>; span: api.Span; error?: Error }
info: {
args: ArrayLike<unknown>;
span: api.Span;
error?: NodeJS.ErrnoException;
}
) => void;
export type ErrorHook = (
functionName: FMember | FPMember,
error: NodeJS.ErrnoException
) => boolean;

export interface FsInstrumentationConfig extends InstrumentationConfig {
createHook?: CreateHook;
endHook?: EndHook;
errorHook?: ErrorHook;
requireParentSpan?: boolean;
}
161 changes: 161 additions & 0 deletions plugins/node/instrumentation-fs/test/error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/*
* 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 * as assert from 'assert';
import Instrumentation, { CreateHook, ErrorHook } from '../src';
import * as sinon from 'sinon';
import type * as FSType from 'fs';
import type { FsInstrumentationConfig } from '../src/types';

const createHook = sinon.spy<CreateHook>((functionName, { args }) => {
// `ts-node`, which we use via `ts-mocha` also patches module loading and creates
// a lot of unrelated spans. Filter those out.
const filename = args[0];
if (!/test\/fixtures/.test(filename as string)) {
return false;
}
return true;
});

let error: Error;
const errorHook = sinon.spy<ErrorHook>((functionName, e) => {
error = e;
if (functionName.includes('access') && e.code === 'ENOENT') {
return false;
}
return true;
});

const pluginConfig = {
createHook,
errorHook,
};

const provider = new BasicTracerProvider();
const memoryExporter = new InMemorySpanExporter();
provider.addSpanProcessor(new SimpleSpanProcessor(memoryExporter));

const assertFilteredOut = () => {
assert.strictEqual(memoryExporter.getFinishedSpans().length, 1);
const span = memoryExporter.getFinishedSpans()[0];
assert.strictEqual(span.events.length, 0);
};

const assertKept = () => {
assert.strictEqual(memoryExporter.getFinishedSpans().length, 1);
const span = memoryExporter.getFinishedSpans()[0];
assert.strictEqual(span.events.length, 1);
const event = span.events[0];
assert.strictEqual(event.name, 'exception');
assert.strictEqual(
(event.attributes || {})['exception.message'],
error.message
);
};

describe('fs instrumentation: error hook', () => {
let plugin: Instrumentation;
let fs: typeof FSType;

beforeEach(async () => {
plugin = new Instrumentation(pluginConfig);
plugin.setTracerProvider(provider);
plugin.setConfig(pluginConfig as FsInstrumentationConfig);
plugin.enable();
fs = require('fs');
createHook.resetHistory();
errorHook.resetHistory();
assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
});

afterEach(() => {
plugin.disable();
memoryExporter.reset();
});

describe('Syncronous API', () => {
it('should filter out errors according to hook', () => {
try {
fs.accessSync('./test/fixtures/readtest-404', fs.constants.R_OK);
} catch (e) {
assert.strictEqual(e, error);
}
assertFilteredOut();
});

it('should keep other error events', () => {
try {
fs.unlinkSync('./test/fixtures/readtest-404');
} catch (e) {
assert.strictEqual(e, error);
}
assertKept();
});
});

describe('Callback API', () => {
it('should filter out errors according to hook', done => {
fs.access('./test/fixtures/readtest-404', fs.constants.R_OK, e => {
try {
assert.strictEqual(e, error);
assertFilteredOut();
done();
} catch (e) {
done(e);
}
});
});

it('should keep other error events', done => {
fs.unlink('./test/fixtures/readtest-404', e => {
try {
assert.strictEqual(e, error);
assertKept();
done();
} catch (e) {
done(e);
}
});
});
});

describe('Promise API', () => {
it('should filter out errors according to hook', async () => {
try {
await fs.promises.access(
'./test/fixtures/readtest-404',
fs.constants.R_OK
);
} catch (e) {
assert.strictEqual(e, error);
}
assertFilteredOut();
});

it('should keep other error events', async () => {
try {
await fs.promises.unlink('./test/fixtures/readtest-404');
} catch (e) {
assert.strictEqual(e, error);
}
assertKept();
});
});
});
16 changes: 15 additions & 1 deletion plugins/node/instrumentation-fs/test/fsHooks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,14 @@ const endHookError = new Error('endHook failed');
const endHook = sinon.spy((_functionName: string) => {
throw endHookError;
});
const errorHookError = new Error('errorHook failed');
const errorHook = sinon.spy((_functionName: string) => {
throw errorHookError;
});
const pluginConfig = {
createHook,
endHook,
errorHook,
};

const provider = new BasicTracerProvider();
Expand All @@ -45,7 +50,8 @@ const assertNotHookError = (err?: Error | null) => {
assert.ok(
err &&
err.message !== createHookError.message &&
err.message !== endHookError.message,
err.message !== endHookError.message &&
err.message !== errorHookError.message,
'Hook error shadowed the error from the original call'
);
};
Expand All @@ -62,6 +68,9 @@ const assertSuccessfulCallHooks = (expectedFunctionName: string) => {
!(endHookCall.getCall(0).args as any)[1].error,
'Did not expect an error'
);

const errorHookCall = errorHook.withArgs(expectedFunctionName);
sinon.assert.notCalled(errorHookCall);
};

const assertFailingCallHooks = (expectedFunctionName: string) => {
Expand All @@ -73,6 +82,10 @@ const assertFailingCallHooks = (expectedFunctionName: string) => {
sinon.assert.called(endHookCall);
sinon.assert.threw(endHookCall, endHookError);
assert((endHookCall.getCall(0).args as any)[1].error, 'Expected an error');

const errorHookCall = errorHook.withArgs(expectedFunctionName);
sinon.assert.called(errorHookCall);
sinon.assert.threw(errorHookCall, errorHookError);
};

describe('fs instrumentation: hooks', () => {
Expand All @@ -87,6 +100,7 @@ describe('fs instrumentation: hooks', () => {
fs = require('fs');
createHook.resetHistory();
endHook.resetHistory();
errorHook.resetHistory();
assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
});

Expand Down
Loading
Loading