Conversation
Enable profiling Node.js applications with JavaScript function names resolved via V8's perf-map files. This works by leveraging blazesym's built-in /tmp/perf-<pid>.map support (already enabled by default) and adding several integration features: - Register anonymous executable mappings (JIT code regions) as FP-only zones in the eBPF LPM trie so mixed native/JIT stacks unwind correctly through JIT frames via frame pointers while using DWARF for native frames - Auto-detect Node.js when spawning via -- node <script> and inject NODE_OPTIONS with --perf-prof and --interpreted-frames-native-stack - Format V8 perf-map symbols (LazyCompile:*func file.js:10:5) into clean display names (func (file.js:10)) for readable flamegraphs - Warn when profiling an existing Node.js process (--pid) that lacks a perf-map file, with instructions on how to enable JIT symbols - Add Node.js E2E test fixture and two test cases in run_e2e.sh
📝 WalkthroughWalkthroughAdds Node.js perf-map detection and warnings when attaching to running Node processes, injects NODE_OPTIONS when spawning Node, adds V8-aware perf-map symbol formatting, treats anonymous/other mappings as FP-only DWARF entries, and introduces Node.js end-to-end tests and fixture. Changes
Sequence DiagramsequenceDiagram
participant CLI as User/CLI
participant PB as profile-bee
participant Spawn as Spawn Module
participant Node as Node.js Process
participant Prof as Profiler/eBPF
participant Fmt as Symbol Formatter
CLI->>PB: start with --pid (attach) or spawn command
PB->>PB: detect target program (is_nodejs_program?)
alt Spawning new Node.js
PB->>Spawn: spawn(program, args, extra_env)
Spawn->>Spawn: build_runtime_env() -> extra_env (NODE_OPTIONS)
Spawn->>Node: exec with NODE_OPTIONS
Node->>Node: JIT may produce /tmp/perf-<pid>.map
else Attaching to existing process
PB->>PB: warn_nodejs_without_perf_map(pid)
PB->>PB: check /tmp/perf-<pid>.map (emit warning if missing)
end
Prof->>Prof: collect stacks via eBPF
Prof->>Fmt: resolve user symbols (perf-map aware)
Fmt->>Fmt: if V8 symbol -> format_v8_symbol()/shorten source
Fmt->>CLI: return formatted stacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/run_e2e.sh (1)
585-587: Add one Node E2E with--dwarf true.Lines 585-587 only register non-DWARF Node cases, so the FP-only anonymous-mapping flow added in
profile-bee/src/dwarf_unwind.rsLines 616-633 never runs in CI.run_node_profiler()already forwards extra profiler args, so a small--dwarf truevariant would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run_e2e.sh` around lines 585 - 587, Add a Node E2E invocation that runs one of the existing Node tests with DWARF enabled so the FP-only anonymous-mapping path in profile-bee/src/dwarf_unwind.rs is exercised; specifically, register an additional run_test entry next to the existing run_test calls that triggers either test_nodejs_samples_collected or test_nodejs_callstack with the DWARF flag forwarded (use the same mechanism as run_node_profiler which already accepts extra profiler args, passing --dwarf true).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@profile-bee/src/trace_handler.rs`:
- Around line 104-120: The current format_short_source splits on ':'
left-to-right so file paths containing ':' (e.g., node:internal/... or
file://...) are mangled; change the split to be from the right (use rsplitn with
the same count) and adjust how you pick file_path and line/column from the
resulting parts so the filepath is the last/remaining segment before the
trailing line/column suffixes; update references in the function
format_short_source (parts, file_path, line, basename) accordingly so basename
extraction uses the full remaining path.
In `@tests/run_e2e.sh`:
- Around line 452-479: The run_node_profiler helper can falsely pass if an old
collapse file exists; before invoking the profiler in run_node_profiler, delete
any existing collapse file at $collapse_file (or otherwise ensure it will be
freshly created) so the later existence check verifies a new run; update the
run_node_profiler function to remove/unlink "$collapse_file" (or clear it)
before running the profiler and then proceed with the existing profiler
invocation and post-check.
---
Nitpick comments:
In `@tests/run_e2e.sh`:
- Around line 585-587: Add a Node E2E invocation that runs one of the existing
Node tests with DWARF enabled so the FP-only anonymous-mapping path in
profile-bee/src/dwarf_unwind.rs is exercised; specifically, register an
additional run_test entry next to the existing run_test calls that triggers
either test_nodejs_samples_collected or test_nodejs_callstack with the DWARF
flag forwarded (use the same mechanism as run_node_profiler which already
accepts extra profiler args, passing --dwarf true).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fd07d69-c71d-4556-8007-6a4f8fae2708
📒 Files selected for processing (6)
profile-bee/bin/profile-bee.rsprofile-bee/src/dwarf_unwind.rsprofile-bee/src/spawn.rsprofile-bee/src/trace_handler.rstests/fixtures/src/node_callstack.jstests/run_e2e.sh
…dd DWARF Node.js test - Fix format_short_source to split from the right (rsplitn) so paths like node:internal/modules/cjs/loader.js:42:5 are not mangled by treating the colon after 'node' as a line-number separator - Remove any existing collapse file before profiler invocation in run_node_profiler so the post-run existence check catches actual failures instead of passing on stale output - Add test_nodejs_dwarf_callstack E2E test that runs with --dwarf true to exercise the FP-only anonymous mapping code path
There was a problem hiding this comment.
🧹 Nitpick comments (1)
profile-bee/src/trace_handler.rs (1)
506-510: Minor:Script:entries produce a leading space in output.The test expects
" (main.js:1)"with a leading space. This occurs becauseScript:(with trailing space) leaves an empty function name after prefix stripping.This is cosmetic and won't break flamegraphs, but you may want to trim the result for cleaner display.
💅 Optional trim for cleaner output
if let Some(src) = source_loc { let short_source = format_short_source(src); - Some(format!("{} ({})", display_name, short_source)) + let formatted = format!("{} ({})", display_name, short_source); + Some(formatted.trim_start().to_string()) } else { Some(display_name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/trace_handler.rs` around lines 506 - 510, The output of format_v8_symbol (exercised by test_v8_script) leaves a leading space when the V8 symbol starts with "Script: " because stripping the prefix yields an empty function name; update format_v8_symbol to trim whitespace after removing prefixes (or specifically handle the "Script:" prefix to avoid inserting an extra leading space) so the returned Option<String> contains a clean "(main.js:1)" without a leading space.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@profile-bee/src/trace_handler.rs`:
- Around line 506-510: The output of format_v8_symbol (exercised by
test_v8_script) leaves a leading space when the V8 symbol starts with "Script: "
because stripping the prefix yields an empty function name; update
format_v8_symbol to trim whitespace after removing prefixes (or specifically
handle the "Script:" prefix to avoid inserting an extra leading space) so the
returned Option<String> contains a clean "(main.js:1)" without a leading space.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cbc7520-6d88-40d0-a901-6831a355cadc
📒 Files selected for processing (3)
profile-bee/bin/profile-bee.rsprofile-bee/src/trace_handler.rstests/run_e2e.sh
--perf-prof writes a binary jitdump (/tmp/jit-<pid>.dump) intended for `perf inject`, not the text-format perf-map that blazesym reads. Switch to --perf-basic-prof which writes /tmp/perf-<pid>.map with JIT symbol addresses in the format blazesym expects. Also add a warmup phase to the JS test fixture so V8 JIT-compiles the functions before the profiling window, and broaden E2E assertions to accept V8 internal frames (Builtins_JSEntry, v8::) as proof of working JIT stack walking even when perf-map timing varies across CI envs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/run_e2e.sh (1)
486-517: Consider returning a distinct exit code for skipped tests.When
nodeor the fixture is missing, the test returns0(success) with a "SKIP" message to stderr, butrun_testwill count it as PASSED. This can mask missing dependencies in CI summaries.One option is to introduce a skip exit code (e.g.,
return 77per autotools convention) and handle it inrun_test:# In run_test, after capturing exit_code: if [[ "$exit_code" -eq 77 ]]; then echo -e "${YELLOW}SKIP${NC}" SKIPPED=$((SKIPPED + 1)) return fiThis is a nice-to-have for better CI observability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/run_e2e.sh` around lines 486 - 517, Change skipped-test returns to a distinct skip exit code and make the test runner treat that code as SKIP: in the test_nodejs_callstack function replace the early "return 0" paths when node or the fixture is missing with "return 77" (or another agreed-upon skip code) so skips are not counted as passes; then update run_test to check the captured exit_code and, if it equals 77, print the SKIP status, increment the SKIPPED counter, and return early instead of treating it as a PASS. Ensure you reference and update the functions run_test and test_nodejs_callstack only, leaving run_node_profiler and the normal pass/fail handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@profile-bee/src/spawn.rs`:
- Around line 183-191: The docstring for the function build_runtime_env
incorrectly states that we inject the Node flag `--perf-prof` while the code
actually uses `--perf-basic-prof`; update the doc comment to mention
`--perf-basic-prof` (and optionally note that this produces /tmp/perf-<pid>.map
for JIT symbol resolution) and ensure the description of
`--interpreted-frames-native-stack` remains accurate so the documentation
matches the implementation in build_runtime_env.
---
Nitpick comments:
In `@tests/run_e2e.sh`:
- Around line 486-517: Change skipped-test returns to a distinct skip exit code
and make the test runner treat that code as SKIP: in the test_nodejs_callstack
function replace the early "return 0" paths when node or the fixture is missing
with "return 77" (or another agreed-upon skip code) so skips are not counted as
passes; then update run_test to check the captured exit_code and, if it equals
77, print the SKIP status, increment the SKIPPED counter, and return early
instead of treating it as a PASS. Ensure you reference and update the functions
run_test and test_nodejs_callstack only, leaving run_node_profiler and the
normal pass/fail handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e86eb23b-a0da-40cb-a04b-68017c71be2b
📒 Files selected for processing (4)
profile-bee/bin/profile-bee.rsprofile-bee/src/spawn.rstests/fixtures/src/node_callstack.jstests/run_e2e.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/fixtures/src/node_callstack.js
- profile-bee/bin/profile-bee.rs
Comment on lines
+183
to
+191
| /// Build extra environment variables for runtime-specific profiling support. | ||
| /// | ||
| /// For Node.js processes, injects `NODE_OPTIONS` with `--perf-prof` (writes | ||
| /// `/tmp/perf-<pid>.map` for JIT symbol resolution) and | ||
| /// `--interpreted-frames-native-stack` (enables frame pointers in interpreted | ||
| /// frames for reliable stack unwinding). | ||
| /// | ||
| /// Merges with any existing `NODE_OPTIONS` from the parent environment. | ||
| fn build_runtime_env(program: &str) -> Vec<(&'static str, String)> { |
There was a problem hiding this comment.
Fix docstring: mentions --perf-prof but code uses --perf-basic-prof.
The inline comment on lines 195-196 correctly explains the flag difference, but the docstring is inconsistent.
📝 Suggested fix
/// Build extra environment variables for runtime-specific profiling support.
///
-/// For Node.js processes, injects `NODE_OPTIONS` with `--perf-prof` (writes
+/// For Node.js processes, injects `NODE_OPTIONS` with `--perf-basic-prof` (writes
/// `/tmp/perf-<pid>.map` for JIT symbol resolution) and🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@profile-bee/src/spawn.rs` around lines 183 - 191, The docstring for the
function build_runtime_env incorrectly states that we inject the Node flag
`--perf-prof` while the code actually uses `--perf-basic-prof`; update the doc
comment to mention `--perf-basic-prof` (and optionally note that this produces
/tmp/perf-<pid>.map for JIT symbol resolution) and ensure the description of
`--interpreted-frames-native-stack` remains accurate so the documentation
matches the implementation in build_runtime_env.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enable profiling Node.js applications with JavaScript function names resolved via V8's perf-map files. This works by leveraging blazesym's built-in /tmp/perf-.map support (already enabled by default) and adding several integration features:
Summary by CodeRabbit
New Features
Bug Fixes
Tests