feat(node): Add `fsInstrumentation` by lforst · Pull Request #13291 · getsentry/sentry-javascript

lforst · 2024-08-09T08:34:20Z

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lforst · 2024-08-09T08:34:20Z

Resolves#13057

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lforst · 2024-08-09T08:34:20Z

Resolves#13057

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lforst · 2024-08-09T08:34:20Z

Resolves#13057

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lforst · 2024-08-09T08:34:20Z

Resolves#13057

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lforst · 2024-08-09T08:34:20Z

Resolves#13057

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lforst · 2024-08-09T08:34:20Z

Resolves#13057

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

lforst · 2024-08-09T08:34:20Z

Resolves#13057

This PR adds an integration to instrument fs API. We are using OTEL's @opentelemetry/instrumentation-fs instrumentation under the hood.

The integration creates spans with naming patterns of fs.readFile, fs.unlink, and so on.

Users can configure via option whether they want to include path arguments as attributes, and whether to include error messages as an attribute when an fs call fails.

I have found that the integration can slow down applications massively when there are a lot of fs calls, for example when running a dev server - so I put a disclaimer in the JS doc, and we should also put a similar disclaimer in the docs when we write them.

+app.get('/readFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.readFile(path.join(__dirname, 'some-file.txt'), 'utf-8', () => {
+ resolve();
+ });
+ });
+ await fs.promises.readFile(path.join(__dirname, 'some-file-promises.txt'), 'utf-8');
+ await util.promisify(fs.readFile)(path.join(__dirname, 'some-file-promisify.txt'), 'utf-8');
+ res.send('done');
+});


+app.get('/mkdtemp', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.mkdtemp(path.join(os.tmpdir(), 'foo-'), () => {
+ resolve();
+ });
+ });
+ await fs.promises.mkdtemp(path.join(os.tmpdir(), 'foo-'));
+ await util.promisify(fs.mkdtemp)(path.join(os.tmpdir(), 'foo-'));
+
+ res.send('done');
+});


mydea · 2024-08-09T10:53:40Z

@@ -0,0 +1,11 @@
+some-file.txt.copy


Can we just make this: some-file.txt.* ?

and maybe move them out of the root of the package, into e.g. fixtures/ or something like this?

mydea

nice!

+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');
+});


+app.get('/copyFile', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.copy'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.copyFile(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.copy'),
+ );
+ await util.promisify(fs.copyFile)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.copy'),
+ );
+ res.send('done');
+});


+app.get('/link', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.link(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.link'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.link(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.link'),
+ );
+ await util.promisify(fs.link)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.link')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.link')),
+ ]);
+
+ res.send('done');
+});


+app.get('/symlink', async (_, res) => {
+ await new Promise<void>(resolve => {
+ fs.symlink(
+ path.join(__dirname, 'fixtures', 'some-file.txt'),
+ path.join(__dirname, 'fixtures', 'some-file.txt.symlink'),
+ () => {
+ resolve();
+ },
+ );
+ });
+ await fs.promises.symlink(
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink'),
+ );
+ await util.promisify(fs.symlink)(
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt'),
+ path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink'),
+ );
+
+ await Promise.all([
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promises.txt.symlink')),
+ fs.promises.unlink(path.join(__dirname, 'fixtures', 'some-file-promisify.txt.symlink')),
+ ]);
+
+ res.send('done');
+});


## Summary This streamlines the vendored `fs` instrumentation, following the same approach as the dataloader (#21475) and generic-pool (#21523) refactors: - Use `Sentry.startSpan`/`startInactiveSpan`/`suppressTracing` instead of the OTel tracer APIs - Use the built-in `onlyIfParent` option instead of `requireParentSpan` + a hand-rolled parent check - Remove the unused `createHook`/`endHook`/`requireParentSpan` config and inline the span name, `op`/`origin` and file-path/error attributes directly into the instrumentation - Streamline the types to what we actually need - Remove the `/* eslint-disable */` and make the vendored files pass the (type-aware) linter - Add tests covering the previously-untested option gating `context.bind` from `@opentelemetry/api` is kept only for the callback-style functions, where the user's async callback must be restored to the parent context (so it isn't parented to the fs span or caught by tracing suppression) — there is no Sentry equivalent for this. ## Root cause (drive-by bug fix) While inlining the attribute logic, this also fixes a pre-existing bug: the documented `recordFilePaths` option was never read — file-path span attributes (`path_argument`, `src_argument`, etc.) were gated on `recordErrorMessagesAsSpanAttributes` instead (dates back to the original integration in #13291). File paths are now gated on `recordFilePaths` and error messages on `recordErrorMessagesAsSpanAttributes`, matching the documented options. New integration tests cover each gate in isolation. Closes#20726 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Luca Forstner added 2 commits August 9, 2024 08:30

feat(node): Add fsInstrumentation
93a2031

Add integration tests
e59b33b

github-advanced-securityAI found potential problems Aug 9, 2024
View reviewed changes

fix exports
7190436

lforst marked this pull request as ready for review August 9, 2024 09:46

lforst requested review from AbhiPrasad, mydea and s1gr1d August 9, 2024 09:47

mydea reviewed Aug 9, 2024
View reviewed changes

mydea approved these changes Aug 9, 2024
View reviewed changes

move to fixtures and clean gitignore
04d76e9

github-advanced-securityAI found potential problems Aug 9, 2024
View reviewed changes

lforst merged commit 2a1d8ee into developAug 9, 2024

lforst deleted the lforst-fs-instrumentation branch August 9, 2024 11:32

This was referenced Sep 4, 2024
[Snyk] Upgrade @sentry/node from 8.22.0 to 8.26.0 Aam-Digital/account-backend#42
Closed
[Snyk] Upgrade @sentry/node from 8.22.0 to 8.26.0 Aam-Digital/replication-backend#158
Closed

q1blue mentioned this pull request Sep 7, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.26.0 https-quantumblockchainai-atlassian-net/ossinsight#846
Open

FengShangWuQi mentioned this pull request Sep 8, 2024
[Snyk] Upgrade @sentry/gatsby from 7.20.0 to 8.26.0 FengShangWuQi/fengshangwuqi.github.io#1919
Closed

bagulm123 mentioned this pull request Sep 8, 2024
[Snyk] Upgrade: , , chart.js, classnames, cytoscape, typescript bagulm123/komiser#18
Open

KeeghanM mentioned this pull request Sep 12, 2024
[Snyk] Upgrade: , , , , , , next, posthog-js, posthog-node, react-chessboard, stripe KeeghanM/chess-training-app#94
Merged

This was referenced Sep 13, 2024
[Snyk] Upgrade: , , chart.js, classnames, cytoscape, next, typescript bagulm123/komiser#19
Open
[Snyk] Upgrade: , , , chart.js, classnames, cytoscape, next, typescript bagulm123/komiser#20
Open

onsails mentioned this pull request Sep 18, 2024
[Snyk] Upgrade: , , , next haqq-network/islamic-website#37
Open

mohamed-helmy22020 mentioned this pull request Sep 18, 2024
[Snyk] Upgrade @sentry/nextjs from 8.26.0 to 8.27.0 mohamed-helmy22020/banking#4
Open

gokulk16 mentioned this pull request Sep 23, 2024
[Snyk] Upgrade: , gokulk16/type-to-calculate-web#40
Closed

q1blue mentioned this pull request Sep 24, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.27.0 https-quantumblockchainai-atlassian-net/ossinsight#893
Open

This was referenced Sep 24, 2024
[Snyk] Upgrade @sentry/browser from 8.25.0 to 8.27.0 gokulk16/type-to-calculate-web#42
Closed
[Snyk] Upgrade @sentry/browser from 8.25.0 to 8.28.0 gokulk16/type-to-calculate-web#44
Closed

FengShangWuQi mentioned this pull request Sep 27, 2024
[Snyk] Upgrade @sentry/gatsby from 7.20.0 to 8.28.0 FengShangWuQi/fengshangwuqi.github.io#1923
Closed

q1blue mentioned this pull request Sep 27, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.28.0 https-quantumblockchainai-atlassian-net/ossinsight#899
Open

q1blue mentioned this pull request Sep 30, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.29.0 https-quantumblockchainai-atlassian-net/ossinsight#904
Open

bagulm123 mentioned this pull request Oct 1, 2024
[Snyk] Upgrade @sentry/react from 8.22.0 to 8.29.0 bagulm123/komiser#23
Open

q1blue mentioned this pull request Oct 1, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.30.0 https-quantumblockchainai-atlassian-net/ossinsight#909
Open

KeeghanM mentioned this pull request Oct 3, 2024
[Snyk] Upgrade @sentry/react from 8.26.0 to 8.30.0 KeeghanM/chess-training-app#99
Merged

mydea mentioned this pull request Jun 15, 2026
ref(node): Streamline vendored fs instrumentation #21532
Merged