Repository navigation
Conversation
|
Review requested:
|
|
|
||
| #### Platform support | ||
|
|
||
| At `./configure` time, Node.js checks for a working `dtrace` tool and |
There was a problem hiding this comment.
We have removed dtrace support long time ago. Wouldn't it require us to include a new suite for testing it on that environment?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
This pull request has been marked as stale due to 90 days of inactivity. |
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.
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>
d49a2c5 to
22e8015
Compare
22e8015 to
4e1213d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62118 +/- ##
==========================================
+ Coverage 90.17% 90.43% +0.26%
==========================================
Files 771 791 +20
Lines 265496 276504 +11008
Branches 50483 53094 +2611
==========================================
+ Hits 239401 250046 +10645
+ Misses 17052 16858 -194
- Partials 9043 9600 +557
🚀 New features to boost your workflow:
|
5a684c8 to
6645936
Compare
Failed to resume CI
Full Auto Start CI output |
40f7683 to
591cf2f
Compare
|
@panva I think your comments are addressed now. PTAL. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Can we use a platform assert to ensure that probeSemaphore is not undefined on Linux, where the support is enabled by default?
This ensures that the test does run on Linux CI.
There was a problem hiding this comment.
sorry I think the GitHub UI is broken for my comment. I mean something like this:
// test/common/usdt.js
function skipIfUsdtIsDisabled() {
if (!process.config.variables.node_no_usdt && process.config.variables.node_use_dtrace) {
return;
}
common.skip("USDT is not enabled);
}and in this test:
skipIfUsdtIsDisabled();
assert.ok(
probeSemaphore instanceof Uint16Array,
`Expected probeSemaphore to be Uint16Array, got ${typeof probeSemaphore}`,
);
assert.ok(
probeSemaphore[0] === 0 || probeSemaphore[0] === 1,
`Expected semaphore to be 0 or 1, got ${probeSemaphore[0]}`,
);There was a problem hiding this comment.
Same, it would be helpful to add a test condition in test/common/index.js (or a new test/common/usdt.js as this accesses an internal binding) to ensure that the test is determined to be enabled when the USDT support is supposed to be enabled.
There was a problem hiding this comment.
+1 to the comments from @legendecas, but also, it sounds like JS subscribers are needed for the publishes to actually fire the probes? This appears to not be the case for the native-side publishes. This seems a bit unexpected to me. Is there some way we could read the semaphore or the bit the kernel modifies to detect liveness and use that to do the mode-swap on the channel somehow so it becomes active if a probe is attached?
Also, how exactly would the message object be consumed? It seems to be getting passed, but as just a void* which I'm not sure if there's any useful way to consume that externally? Is that actually useful?
Add a shared USDT publish probe for string-named channels. An attached tracer activates event production without JavaScript subscribers, including SQLite queries, permission events and opted-in FIPS indicators. Keep real subscriber and store lifecycles separate from tracer interest. Do not replay queued FIPS events to subscribers that arrived later. Read the semaphore view through the binding on each check so startup snapshot restoration cannot leave a stale copy. Use a native enabled check where the view is a constant-one placeholder. Keep unsupported builds inert. Signed-off-by: Bryan English <bryan@bryanenglish.com> Assisted-by: Pi using GLM-5.3
Commit the generated SystemTap provider header so Linux builds need only <sys/sdt.h>, not a build-time dtrace tool. Keep macOS opt-in through --with-dtrace and allow --without-dtrace to disable probes entirely. Add Linux CI coverage for enabled and disabled builds, with explicit capability assertions and a generated-header check. Exercise real probes with bpftrace, including snapshot restoration and tracer-only publishing from JavaScript and SQLite. Signed-off-by: Bryan English <bryan@bryanenglish.com> Assisted-by: Pi using GLM-5.3
|
This pull request has conflicts with its base branch, removing the |
Adds a
dc__publishUSDT probe for diagnostics channel publish events.Attaching a tracer counts as interest in every string-named channel, so publishers, including SQLite, permission and FIPS events, fire the probe without JS subscribers. JS delivery still requires a real subscriber.
Includes Linux CI coverage with and without USDT, plus a bpftrace snapshot-restore regression test. Both CI builds assert the expected USDT support.