Skip to content

diagnostics_channel: add USDT probes - #62118

Open
bengl wants to merge 2 commits into
nodejs:mainfrom
bengl:bengl/usdt-dc
Open

diagnostics_channel: add USDT probes#62118
bengl wants to merge 2 commits into
nodejs:mainfrom
bengl:bengl/usdt-dc

Conversation

@bengl

@bengl bengl commented Mar 5, 2026

Copy link
Copy Markdown
Member

Adds USDT probes that are fired for every diagnostics_channel publish.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Mar 5, 2026
Comment thread doc/api/diagnostics_channel.md Outdated

#### Platform support

At `./configure` time, Node.js checks for a working `dtrace` tool and

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have removed dtrace support long time ago. Wouldn't it require us to include a new suite for testing it on that environment?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what our build setup looks like now, but yes, everything that was required for the previous incarnation of probes in Node.js would likely be required again with this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we tested the dtrace stuff previously. I think when it was removed it had be broken for a while (or maybe that was one of the other non-tested removed features).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a possible path forward here where headers are pre-generated and added to git (much like we do for other cases of generated headers, in dependencies for example), and then a userland, self-contained test (at least on linux) checks for the placement and activation of the probe.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I've made some changes (many months later..)

Linux is now default-on with no dtrace build dependency. src/node_provider_linux.h is committed.
It can be regenerated via tools/usdt/generate_headers.py and a --check drift job runs in CI against it.
Default ./configure enables USDT whenever <sys/sdt.h> is present (systemtap-sdt-dev / systemtap-sdt-devel is the only build requirement on Linux).

macOS is opt-in via ./configure --with-dtrace (which at build-time calls dtrace -h -xnolibs).
--without-dtrace still disables everything.

New test-usdt CI job in test-linux.yml:

  • default: end-to-end bpftrace test as root (verifies the probe fires with the channel name) plus the committed-header drift check
  • --without-dtrace: pins the no-op tier

Bench (aarch64 VM, benchmark/diagnostics_channel/publish.js, subscribers=1, avg of 2): default ~282M ops/s vs --without-dtrace ~314M ops/s — ~10% on this microbench.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Aug 3, 2026
bengl added a commit to bengl/node that referenced this pull request Sep 9, 2026
Implements the approach from the nodejs#62118 review discussion to remove
the build-time 'dtrace' dependency on Linux:

* Commit the SystemTap-generated probe header
  (src/node_provider_linux.h, regenerate with
  tools/usdt/generate_headers.py).  USDT support is now on by
  default on Linux whenever <sys/sdt.h> is available
  (systemtap-sdt-dev on Debian/Ubuntu, systemtap-sdt-devel on
  Fedora/RHEL) and never needs a 'dtrace' tool at build time.  A
  committed-header drift check runs in CI.
* Make native DTrace opt-in on macOS via the new
  ./configure --with-dtrace; FreeBSD/illumos remain unsupported
  pending a 'dtrace -G' link step.  --without-dtrace still disables
  probes everywhere.  The always-on <sys/sdt.h> fallback tier is
  gone.
* Add a path-gated test-usdt job to the Linux CI workflow with a
  default leg that runs the end-to-end bpftrace probe test as
  root, and a --without-dtrace leg that pins the no-op tier.

The generated header is excluded from cpplint like
src/node_root_certs.h.
bengl added a commit to bengl/node that referenced this pull request Sep 9, 2026
Implements the approach from the nodejs#62118 review discussion to remove
the build-time 'dtrace' dependency on Linux:

* Commit the SystemTap-generated probe header
  (src/node_provider_linux.h, regenerate with
  tools/usdt/generate_headers.py).  USDT support is now on by
  default on Linux whenever <sys/sdt.h> is available
  (systemtap-sdt-dev on Debian/Ubuntu, systemtap-sdt-devel on
  Fedora/RHEL) and never needs a 'dtrace' tool at build time.  A
  committed-header drift check runs in CI.
* Make native DTrace opt-in on macOS via the new
  ./configure --with-dtrace; FreeBSD/illumos remain unsupported
  pending a 'dtrace -G' link step.  --without-dtrace still disables
  probes everywhere.  The always-on <sys/sdt.h> fallback tier is
  gone.
* Add a path-gated test-usdt job to the Linux CI workflow with a
  default leg that runs the end-to-end bpftrace probe test as
  root, and a --without-dtrace leg that pins the no-op tier.

The generated header is excluded from cpplint like
src/node_root_certs.h.

Signed-off-by: Bryan English <bryan@bryanenglish.com>
@github-actions github-actions Bot removed the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Sep 10, 2026
@bengl
bengl marked this pull request as ready for review September 10, 2026 20:56
Comment thread src/node_usdt.h
#define NODE_DC_PUBLISH_PROBE(name, msg) NODE_DC_PUBLISH((name), (msg))

// Real semaphore — JS can check it without crossing into C++.
inline unsigned short* NodeDCPublishSemaphore() { // NOLINT(runtime/int)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't in any namespace (e.g. namespace node { ... }) ... is that intentional/necessary? If either, that should be documented here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional, since we're doing all this wrapping and handling of non-namespaced stuff in here. I'll add a comment to clarify.

NodeDCPublishSemaphore(),
sizeof(unsigned short), // NOLINT(runtime/int)
[](void*, size_t, void*) {}, // no-op deleter — memory is static
nullptr);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might run into issues when v8 sandbox is enabled. @codebytere

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, arbitrary process memory can't be wrapped/exposed to JS with sandbox enabled. We'll just fall back to non-semaphore mode then (like I'm doing on macOS). Fix incoming.

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 48.48485% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.17%. Comparing base (4bf4c00) to head (6645936).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/node_diagnostics_channel.cc 0.00% 12 Missing and 5 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #62118   +/-   ##
=======================================
  Coverage   90.17%   90.17%           
=======================================
  Files         771      771           
  Lines      265496   265479   -17     
  Branches    50483    50464   -19     
=======================================
+ Hits       239401   239404    +3     
+ Misses      17052    17018   -34     
- Partials     9043     9057   +14     
Files with missing lines Coverage Δ
lib/diagnostics_channel.js 97.40% <100.00%> (+0.06%) ⬆️
src/node_diagnostics_channel.h 57.14% <ø> (ø)
src/node_diagnostics_channel.cc 79.09% <0.00%> (-4.57%) ⬇️

... and 42 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.

Fire a `dc__publish` USDT probe for every diagnostics_channel
publish, passing the channel name, so tracers such as bpftrace,
perf, or SystemTap can observe publish traffic with near-zero cost
when nothing is attached. Probes are enabled by default on Linux;
--without-dtrace disables them.

The probe semaphore is exposed to JS as a Uint16Array over the
native semaphore so the publish hot path can gate on one indexed
load instead of a binding call. The array is resolved lazily rather
than captured at module load: this module is baked into the startup
snapshot, and a view captured while building the snapshot is
detached when the snapshot is deserialized. Builds without USDT
support keep a branch-only path that never calls the binding.

Signed-off-by: Bryan English <bryan@bryanenglish.com>
Assisted-by: Pi using GLM-5.3
Remove the build-time dtrace dependency on Linux, following the
approach discussed in the PR review: generate the SystemTap
provider header once with tools/usdt/generate_headers.py and commit
it as src/node_provider_linux.h, so Linux builds need only
<sys/sdt.h> and never a dtrace tool. --with-dtrace becomes the
opt-in switch for macOS; --without-dtrace disables probes entirely.

Add a path-filtered test-usdt job to the Linux CI workflow that
installs bpftrace and systemtap-sdt-dev, verifies the committed
header stays in sync with the generator, builds with and without
dtrace, and runs the USDT tests, including a root-only bpftrace
end-to-end test that attaches to the dc__publish probe and asserts
the channel name argument.

Signed-off-by: Bryan English <bryan@bryanenglish.com>
Assisted-by: Pi using GLM-5.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants