Rustify #256
Draft
Rustify #256
Conversation
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
This RFC outlines a phased migration plan to refactor xprofiler from a C++ Node.js native addon (using NAN) to a Rust-based addon using napi-rs. The migration will be incremental across 8 phases: - Phase 0: Project setup and infrastructure - Phase 1: Core utilities and configuration - Phase 2: Platform abstraction layer - Phase 3: Command system - Phase 4: Statistics collection (LogByPass) - Phase 5: Profilers (CPU, heap, GC) - Phase 6: V8 hooks and diagnostic report - Phase 7: Coredump support (optional) - Phase 8: Final integration and cleanup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Phase 0 - Project Setup: - Initialize Rust workspace in crates/xprofiler-rs/ - Configure Cargo.toml with napi-rs v3, serde, parking_lot, chrono, etc. - Add napi-rs build integration to package.json (build:rs scripts) - Create binding loader with XPROFILER_USE_RUST feature flag - Update .gitignore for Rust build artifacts Phase 1 - Core Utilities: - Implement configuration system (config.rs) with XprofilerConfig struct - Implement logger system (logger.rs) with file rotation - Implement common utilities (utils.rs) - uptime, timestamps, PID - Implement error types (error.rs) with thiserror - Implement environment data (env.rs) - per-thread statistics - Implement platform abstraction (platform/) - CPU usage, IPC paths The Rust binding can now be built with `npm run build:rs` and enabled with XPROFILER_USE_RUST=true environment variable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Phase 2 - Platform Abstraction and IPC: - Add tokio async runtime for IPC handling - Implement Unix domain socket server/client (ipc/unix.rs) - Implement Windows named pipe server/client (ipc/windows.rs) - Create IPC abstraction traits (IpcServer, IpcClient) Phase 3 - Command System: - Implement command parser with serde_json (commands/parser.rs) - Add command dispatcher for check_version, list_environments, get_config - Create commands listener thread (commands/listener.rs) - Wire up runCommandsListener to start IPC server Additional fixes: - Fix configure() to accept array format matching C++ API - Fix checkSocketPath() to accept boolean parameter - Add setHttpConfig() and HTTP tracking functions - Fix field naming with #[napi(js_name = "...")] for snake_case The Rust binding now successfully starts xprofiler and can receive commands via IPC from xprofctl. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ration - Add logbypass module with background thread for statistics collection - Implement CPU statistics sampling at 1-second intervals with time window averages (15s, 30s, 60s) - Implement log file writer with both xprofiler and alinode formats - Wire up runLogBypass function to start the statistics collection thread - Handle socket path too long gracefully (skip IPC server instead of failing) - Sync Rust crate version with package.json (3.1.0) All 356 logbypass tests pass with XPROFILER_USE_RUST=true. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Phase 5 Implementation: - Add profilers module with action state management - Implement GC profiler (pure Rust) with JSON output - Add CPU/heap profiler infrastructure (V8 interaction TBD) - Update command parser with all profiler command handlers - Fix IPC protocol to use separate response socket - Fix socket path length check (104 bytes on macOS, 108 on Linux) - Fix message reading (no newline in client messages) Working commands: - check_version - get_config / set_config - list_environments - start_gc_profiling / stop_gc_profiling - start_cpu_profiling / stop_cpu_profiling (infrastructure only) - start_heap_profiling / stop_heap_profiling (infrastructure only) - heapdump (infrastructure only) - generate_coredump (Linux only, infrastructure only) All 356 logbypass tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Phase 6 Implementation: - Add hooks module for V8 runtime event handling - Implement set_hooks() with configuration logging - Document napi-rs limitation for V8 isolate access - Provide guidance for heap management via Node.js options Note: Due to napi-rs limitations, actual V8 hook registration (SetFatalErrorHandler, AddNearHeapLimitCallback) is not possible. The configuration options are acknowledged and logged for compatibility. Users should use --max-old-space-size or NODE_OPTIONS for heap management. All 356 logbypass tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Phase 7 Implementation: - Add coredump module with platform-specific implementations - Linux: Creates info file with guidance for using gcore - macOS: Returns error with lldb guidance - Windows: Returns error with procdump guidance - Update generate_coredump command handler to use coredump module Note: Full coredump generation requires the google-coredumper library which is only available in the C++ binding. The Rust implementation provides helpful guidance for using platform-native tools. All 356 logbypass tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove broken client-server test that required two-socket protocol - Add simpler server lifecycle test (start/stop verification) - Add response socket path format test - Add socket path format test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The .only modifier was causing only the HTTP test suite to run, skipping all other tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Allow LogByPass and CommandsListener to be called multiple times (worker threads may call start again, return Ok instead of error) - Use /tmp on macOS for test directories to avoid socket path length limits - Match expected error message format for generate_coredump on non-Linux 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Change CommandOptions to serde_json::Value for flexible validation - Implement boolean type validation for set_config (matches C++ behavior) - Actually apply config updates in set_config command - Fix heap profiling action names to use "sampling" prefix (start_sampling_heap_profiling, stop_sampling_heap_profiling) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add binding matrix dimension: [cpp, rust] - Run C++ and Rust bindings in parallel - Conditionally setup Rust toolchain only for rust binding - Separate build and test steps for each binding - Code coverage only for C++ binding Total jobs: 3 OS × 4 Node versions × 2 bindings = 24 parallel jobs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a Rust implementation of xprofiler using napi-rs as an alternative to the existing C++ implementation. The migration is designed to be incremental with a feature flag system, allowing both implementations to coexist during the transition period.
Key Changes:
- Added Rust-based native binding using napi-rs (v3) for Node.js integration
- Implemented feature flag system via
XPROFILER_USE_RUSTenvironment variable - Added comprehensive RFC document outlining the migration plan from C++/NAN to Rust/napi-rs
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
xprofiler.js |
Updated to load native binding through new binding loader with debug logging |
lib/binding.js |
New binding loader that switches between C++ and Rust implementations |
package.json |
Added Rust build scripts, napi-rs CLI dependency, and CI tasks for Rust |
test/patch/http.test.js |
Removed .only from test suite |
test/fixtures/utils.js |
Fixed macOS socket path length issue by using /tmp |
crates/xprofiler-rs/* |
Complete Rust implementation of core modules (config, logger, platform, IPC, profilers, etc.) |
.github/workflows/nodejs.yml |
Updated CI to test both C++ and Rust bindings |
rfcs/napi-rs-refactor.md |
Comprehensive migration plan and architecture documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1351
to
+1352
| environment_variables: std::env::vars().collect(), | ||
| }; |
There was a problem hiding this comment.
The diagnostic report currently serializes all process environment variables into a JSON file via std::env::vars().collect(), which will include secrets such as API keys, database passwords, and other credentials in cleartext on disk. Any actor with read access to the log directory (e.g., other local users, sidecar processes, or log shipping tools) can recover these sensitive values from the report file. To mitigate this, avoid dumping the full environment and instead either omit this field entirely or explicitly filter/redact known-sensitive variables before serialization, and ensure report files are stored with appropriately restricted permissions in a non-world-readable location.
- Add Win32_System_IO feature for ReadFile/WriteFile - Fix Windows API imports for named pipe operations - Remove unused napi::bindgen_prelude import in error.rs - Conditionally import constants/utils only on Unix in platform/mod.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add napi config to package.json with binaryName: "xprofiler-rs" - Define target platforms for cross-compilation - Update binding loader to search for correct file names: - xprofiler-rs.linux-x64-gnu.node (Linux) - xprofiler-rs.darwin-arm64.node (macOS) - xprofiler-rs.win32-x64-msvc.node (Windows) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.