Visitar URL original
fs: add `diagnostics_channel` tracing by timfish · Pull Request #66575 · nodejs/node · GitHub
Skip to content

fs: add diagnostics_channel tracing - #66575

Open
timfish wants to merge 1 commit into
nodejs:mainfrom
timfish:fs-diagnostics-channel
Open

timfish wants to merge 1 commit into
nodejs:mainfrom
timfish:fs-diagnostics-channel

Conversation

@timfish

@timfish timfish commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

This adds a diagnostics_channel TracingChannel for each node:fs operation. It is a different approach to #65370, based on the review feedback there.

Each operation publishes to tracing:fs.<operation>:*, for example tracing:fs.stat:start. The sync, callback and promise forms of an operation share a channel, so a subscriber to fs.readFile sees fs.readFile(), fs.readFileSync(), fsPromises.readFile() and filehandle.readFile(). The context has api ('sync', 'callback' or 'promise'), args (a copy of the arguments as passed), and the leading arguments by name, such as path, fd and dest. The docs have the full list.

The events are published from JS with traceSync(), traceCallback() and tracePromise(), so all events of one call share one context object, error is always followed by end or asyncEnd, asyncStart has result, and bindStore() works. Buffers and the other arguments are passed by reference in args. The readFile and writeFile one-round-trip paths, fsPromises.readFile(), readFileSync() with an encoding, existsSync() and the FileHandle methods all publish.

Each traced function starts with one line, for example if (shouldTraceFs('stat')) return traceFsCallback('stat', stat, this, arguments);. When the channel has subscribers, the helper calls the same function again inside the trace, and a flag makes that inner call skip the check. The whole body then runs inside the start scope. This matters for bindStore(): the FSReqCallback or binding promise must be created inside the scope, because a request created before it does not carry the store to the callback. Nested fs calls also run inside the parent's store. Because the check is in the function body, captured references and ESM named imports still publish. When nothing subscribes, the rest of the body is the same as on main.

Calls with invalid arguments publish start, error and end. Calls that a VFS handler serves also publish. fsPromises.glob(), fs.watch(), fs.watchFile() and dir.read() do not publish. An operation that uses other fs operations publishes those too, inside its own events. For example, fs.readFileSync() without an encoding publishes fs.open, fs.read and fs.close inside fs.readFile.

With plain compare.js, the async and stream benchmarks in benchmark/fs do not change, but some short sync benchmarks are slower. With 15 runs and no subscribers, fstatSync was about 5% slower, statSync about 3%, and failing readFileSync calls 5% to 9%. Most of this is the cost of TracingChannel.hasSubscribers before V8 optimizes it. Many of these benchmarks do 10,000 calls in a new process, so most checks run while the getter is still in the interpreter or in Sparkplug. There, the getter calls BoundedChannel.hasSubscribers and Channel.hasSubscribers for all five channels, which costs 60 to 100 ns per check. Once it is optimised, it costs about 3 ns. Next to a 300 to 900 ns sync call, the difference shows.

Once hasSubscribers has been optimized, calling it becomes insignificant apart from sync calls that throw. These are 1-3% slower. When an exception passes through a function that runs as Sparkplug code, V8 finds the bytecode offset with a linear walk from the start of the function (Code::GetBytecodeOffsetForBaselinePC, from Isolate::Throw and LookupExceptionHandlerInTable). So any code before the binding call makes each throw slower. A single if (x.active !== 0) return; costs about 50 ns per failing openSync() there. I did not find a way around this.

I did not change the benchmarks to warm the hasSubscribers getters. A follow-up could make TracingChannel.hasSubscribers cheaper before optimisation.

The new tests cover the event order, the shared context, bindStore() across callbacks, promises and nested calls, and the FileHandle methods. A coverage test fails if a public fs function does not publish, or publishes more than once.

Fixes: #65330
Refs: #65370
CC: @nodejs/diagnostics


This PR was partly AI generated but everything was reviewed by a water based human

Each node:fs operation publishes to its own TracingChannel, named
fs.<operation>. The sync, callback, and promise forms of an operation
share a channel, and the event context carries the form in `api`, the
caller's arguments in `args`, and the leading arguments by name.

The events are published from JavaScript with the TracingChannel
helpers, so all events of a call share one context object, stores bound
with bindStore() reach the callback and nested operations, and the
error event is always followed by end or asyncEnd.

Each traced function starts with a check that calls the function again
inside the trace when the channel has subscribers. The whole body then
runs inside the start scope, so requests capture the bound stores, and
captured function references still publish.

Assisted-by: claude:opus-5.5
Signed-off-by: Tim Fish <tim@timfish.uk>
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Oct 7, 2026
@timfish
timfish force-pushed the fs-diagnostics-channel branch from f73bebb to f7a0dc8 Compare October 7, 2026 18:21
@codecov

codecov Bot commented Oct 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.33795% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.44%. Comparing base (3a44541) to head (f7a0dc8).
⚠️ Report is 50 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/promises.js 94.66% 4 Missing ⚠️
lib/fs.js 98.18% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66575      +/-   ##
==========================================
+ Coverage   90.41%   90.44%   +0.03%     
==========================================
  Files         791      791              
  Lines      276382   276833     +451     
  Branches    53076    53335     +259     
==========================================
+ Hits       249877   250392     +515     
+ Misses      16901    16848      -53     
+ Partials     9604     9593      -11     
Files with missing lines Coverage Δ
lib/internal/fs/dir.js 94.25% <100.00%> (+0.41%) ⬆️
lib/internal/fs/utils.js 96.67% <100.00%> (+0.38%) ⬆️
lib/fs.js 97.41% <98.18%> (+0.09%) ⬆️
lib/internal/fs/promises.js 91.11% <94.66%> (+0.09%) ⬆️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

diagnostics_channel: add a channel for filesystem operations

2 participants