Skip to content

ref(node): Streamline vendored fs instrumentation #21532

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

Draft
mydea wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

ref(node): Streamline vendored fs instrumentation #21532

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
1 change: 0 additions & 1 deletion .oxlintrc.base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@
},
{
"files": [
"**/integrations/fs/vendored/**/*.ts",
"**/integrations/tracing/knex/vendored/**/*.ts",
"**/integrations/tracing/mongo/vendored/**/*.ts",
"**/integrations/tracing/koa/vendored/**/*.ts",
Expand Down
39 changes: 39 additions & 0 deletions dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-errors-only.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import * as Sentry from '@sentry/node';
import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
transport: loggingTransport,
tracesSampleRate: 1,
integrations: [
// Only record error messages - file paths must NOT be recorded
Sentry.fsIntegration({
recordErrorMessagesAsSpanAttributes: true,
}),
],
});

import express from 'express';
import * as fs from 'fs';
import * as path from 'path';

const app = express();

app.get('/readFile', async (_, res) => {
await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file.txt'), 'utf-8');
res.send('done');
});

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
Loading
, but is not rate-limited.
Comment thread
mydea marked this conversation as resolved.
Dismissed

app.get('/readFile-error', async (_, res) => {
try {
await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file-that-doesnt-exist.txt'), 'utf-8');
} catch {
// noop
}
res.send('done');
});

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
Loading
, but is not rate-limited.
Comment thread
mydea marked this conversation as resolved.
Dismissed

Sentry.setupExpressErrorHandler(app);

startExpressServerAndSendPortToRunner(app);
39 changes: 39 additions & 0 deletions dev-packages/node-integration-tests/suites/fs-instrumentation/server-record-paths-only.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import * as Sentry from '@sentry/node';
import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
transport: loggingTransport,
tracesSampleRate: 1,
integrations: [
// Only record file paths - error messages must NOT be recorded
Sentry.fsIntegration({
recordFilePaths: true,
}),
],
});

import express from 'express';
import * as fs from 'fs';
import * as path from 'path';

const app = express();

app.get('/readFile', async (_, res) => {
await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file.txt'), 'utf-8');
res.send('done');
});

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
Loading
, but is not rate-limited.
Comment thread
mydea marked this conversation as resolved.
Dismissed

app.get('/readFile-error', async (_, res) => {
try {
await fs.promises.readFile(path.join(__dirname, 'fixtures', 'some-file-that-doesnt-exist.txt'), 'utf-8');
} catch {
// noop
}
res.send('done');
});

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
a file system access
Loading
, but is not rate-limited.
Comment thread
mydea marked this conversation as resolved.
Dismissed

Sentry.setupExpressErrorHandler(app);

startExpressServerAndSendPortToRunner(app);
80 changes: 80 additions & 0 deletions dev-packages/node-integration-tests/suites/fs-instrumentation/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,3 +271,83 @@ test('should create spans for fs operations that take target argument', async ()
expect(result).toEqual('done');
await runner.completed();
});

test('records file path but not error messages when only `recordFilePaths` is enabled', async () => {
const runner = createRunner(__dirname, 'server-record-paths-only.ts')
.expect({
transaction: {
transaction: 'GET /readFile-error',
spans: expect.arrayContaining([
expect.objectContaining({
description: 'fs.readFile',
op: 'file',
status: 'internal_error',
// `path_argument` is recorded, but `fs_error` is NOT, since `recordErrorMessagesAsSpanAttributes` is off
data: {
path_argument: expect.stringMatching('/fixtures/some-file-that-doesnt-exist.txt'),
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs',
},
}),
]),
},
})
.start();

const result = await runner.makeRequest('get', '/readFile-error');
expect(result).toEqual('done');
await runner.completed();
});

test('records error messages but not file paths when only `recordErrorMessagesAsSpanAttributes` is enabled', async () => {
const runner = createRunner(__dirname, 'server-record-errors-only.ts')
.expect({
transaction: {
transaction: 'GET /readFile-error',
spans: expect.arrayContaining([
expect.objectContaining({
description: 'fs.readFile',
op: 'file',
status: 'internal_error',
// `fs_error` is recorded, but `path_argument` is NOT, since `recordFilePaths` is off
data: {
fs_error: expect.stringMatching('ENOENT: no such file or directory,'),
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs',
},
}),
]),
},
})
.start();

const result = await runner.makeRequest('get', '/readFile-error');
expect(result).toEqual('done');
await runner.completed();
});

test('does not record file paths on successful operations when only `recordErrorMessagesAsSpanAttributes` is enabled', async () => {
const runner = createRunner(__dirname, 'server-record-errors-only.ts')
.expect({
transaction: {
transaction: 'GET /readFile',
spans: expect.arrayContaining([
expect.objectContaining({
description: 'fs.readFile',
op: 'file',
status: 'ok',
// Neither `path_argument` nor `fs_error` are recorded
data: {
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs',
},
}),
]),
},
})
.start();

const result = await runner.makeRequest('get', '/readFile');
expect(result).toEqual('done');
await runner.completed();
});
109 changes: 4 additions & 105 deletions packages/node/src/integrations/fs/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { FsInstrumentation } from './vendored/instrumentation';
import { defineIntegration, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
import { defineIntegration } from '@sentry/core';
import { generateInstrumentOnce } from '@sentry/node-core';
import { FsInstrumentation } from './vendored/instrumentation';

const INTEGRATION_NAME = 'FileSystem';

Expand Down Expand Up @@ -38,112 +38,11 @@ export const fsIntegration = defineIntegration(
INTEGRATION_NAME,
() =>
new FsInstrumentation({
requireParentSpan: true,
endHook(functionName, { args, span, error }) {
span.updateName(`fs.${functionName}`);

span.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'file',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.file.fs',
});

if (options.recordErrorMessagesAsSpanAttributes) {
if (typeof args[0] === 'string' && FS_OPERATIONS_WITH_PATH_ARG.includes(functionName)) {
span.setAttribute('path_argument', args[0]);
} else if (
typeof args[0] === 'string' &&
typeof args[1] === 'string' &&
FS_OPERATIONS_WITH_TARGET_PATH.includes(functionName)
) {
span.setAttribute('target_argument', args[0]);
span.setAttribute('path_argument', args[1]);
} else if (typeof args[0] === 'string' && FS_OPERATIONS_WITH_PREFIX.includes(functionName)) {
span.setAttribute('prefix_argument', args[0]);
} else if (
typeof args[0] === 'string' &&
typeof args[1] === 'string' &&
FS_OPERATIONS_WITH_EXISTING_PATH_NEW_PATH.includes(functionName)
) {
span.setAttribute('existing_path_argument', args[0]);
span.setAttribute('new_path_argument', args[1]);
} else if (
typeof args[0] === 'string' &&
typeof args[1] === 'string' &&
FS_OPERATIONS_WITH_SRC_DEST.includes(functionName)
) {
span.setAttribute('src_argument', args[0]);
span.setAttribute('dest_argument', args[1]);
} else if (
typeof args[0] === 'string' &&
typeof args[1] === 'string' &&
FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH.includes(functionName)
) {
span.setAttribute('old_path_argument', args[0]);
span.setAttribute('new_path_argument', args[1]);
}
}

if (error && options.recordErrorMessagesAsSpanAttributes) {
span.setAttribute('fs_error', error.message);
}
},
recordFilePaths: options.recordFilePaths,
recordErrorMessagesAsSpanAttributes: options.recordErrorMessagesAsSpanAttributes,
}),
)();
},
};
},
);

const FS_OPERATIONS_WITH_OLD_PATH_NEW_PATH = ['rename', 'renameSync'];
const FS_OPERATIONS_WITH_SRC_DEST = ['copyFile', 'cp', 'copyFileSync', 'cpSync'];
const FS_OPERATIONS_WITH_EXISTING_PATH_NEW_PATH = ['link', 'linkSync'];
const FS_OPERATIONS_WITH_PREFIX = ['mkdtemp', 'mkdtempSync'];
const FS_OPERATIONS_WITH_TARGET_PATH = ['symlink', 'symlinkSync'];
const FS_OPERATIONS_WITH_PATH_ARG = [
'access',
'appendFile',
'chmod',
'chown',
'exists',
'mkdir',
'lchown',
'lstat',
'lutimes',
'open',
'opendir',
'readdir',
'readFile',
'readlink',
'realpath',
'realpath.native',
'rm',
'rmdir',
'stat',
'truncate',
'unlink',
'utimes',
'writeFile',
'accessSync',
'appendFileSync',
'chmodSync',
'chownSync',
'existsSync',
'lchownSync',
'lstatSync',
'lutimesSync',
'opendirSync',
'mkdirSync',
'openSync',
'readdirSync',
'readFileSync',
'readlinkSync',
'realpathSync',
'realpathSync.native',
'rmdirSync',
'rmSync',
'statSync',
'truncateSync',
'unlinkSync',
'utimesSync',
'writeFileSync',
];
1 change: 0 additions & 1 deletion packages/node/src/integrations/fs/vendored/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* - Vendored from: https://github.com/open-telemetry/opentelemetry-js-contrib/tree/15ef7506553f631ea4181391e0c5725a56f0d082/packages/instrumentation-fs
* - Upstream version: @opentelemetry/instrumentation-fs@0.37.0
*/
/* eslint-disable */

import type { FMember, FPMember } from './types';

Expand Down
Loading
Loading