Use the correct user-space marker for SystemTap node_gc_stop probe #20152
Conversation
The process("node").mark("gc__stop") is actually
process("node").mark("gc__done"). Use the proper name so that a
developer can use SystemTap to determine the duration of garbage
collection.
nodejs-github-bot
added
the
c++
|
Not being familiar with SystemTap itself -- Is there some kind of reference that determines what the "proper" names are, or is this just convention? Is this a breaking change? Also, just a tiny thing: It would be great if you could alignt the commit message with our guidelines for that, but if you prefer not to or don't know how to do that, we can also do that when merging the PR. |
There was a problem hiding this comment.
Not being familiar with SystemTap itself -- Is there some kind of reference that determines what the "proper" names are, or is this just convention? Is this a breaking change?
The event name comes from Node.js; node.stp was simply using the wrong one.
That also says something about how much that file is used... it's never been 'not broken' in the five years since it was introduced.
BridgeAR
added
the
author ready
|
Can we test for this in any way? |
|
Not easily, I think. Requires that systemtap is installed on the CI machines and that |
Perhaps a name consistency check between |
|
The check between src/node_provider.d and src/node.stp wouldn't catch the case of both dtrace and systemtap probes being broken or missing if existing probe points are changed or new probe points are added to the code. For example this consistency test wouldn't catch probe systemtap/dtrace points not being in the code like commit d75fecf |
|
Fair enough, but it would have caught the issue being addressed by this PR where they are inconsistent. |
|
In the Diagnostics WG we're talking about adding tests for external profilers (like eBPF, DTrace and Linux perf). The requirements to test system probes are very similar, and we might be able to add some tests for this as well. I plan to start looking into that in the next couple of weeks. |
|
So the question right now is: shall we wait for a test or land this PR as it is? I guess it is fine to land it as is? |
|
Yep, it's good to go. |
|
I'm assuming there's no meaningful CI to run on this? |
|
@richardlau Don't think so, there are no tests or linters reaching this file. I was actually about to land this. |
|
Landed in 94e0e2c, thanks for the contribution! 🎉🎉🎉 |
The process("node").mark("gc__stop") is actually
process("node").mark("gc__done"). Use the proper name so that a
developer can use SystemTap to determine the duration of garbage
collection.
PR-URL: #20152
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
The process("node").mark("gc__stop") is actually
process("node").mark("gc__done"). Use the proper name so that a
developer can use SystemTap to determine the duration of garbage
collection.
PR-URL: #20152
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
The process("node").mark("gc__stop") is actually
process("node").mark("gc__done"). Use the proper name so that a
developer can use SystemTap to determine the duration of garbage
collection.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes