Add Node/V8 CPU and heap profile adapters - #299
Conversation
Register two new built-in adapters for Node.js/V8 profiling: - node-cpu-prof: captures V8 CPU profiles via Node's stable --cpu-prof flags, preserving the native .cpuprofile as a sample_profile artifact. - node-heap-prof: captures V8 sampling heap profiles via Node's stable --heap-prof flags, preserving the native .heapprofile as a memory_profile artifact. Both adapters inject the profiling flags after argv[0] and before the user script, binding the output directory and file name explicitly so Flameox owns the artifact path. Closes #297 and #298.
Add V8CpuProfExtractor and V8HeapProfExtractor that parse the native V8 CPU profile (.cpuprofile) and V8 sampling heap profile (.heapprofile) formats and publish bounded frame measurements for hotspot and memory analysis. - V8CpuProfExtractor: parses nodes/samples, publishes hit-count frame measurements keyed by function name, URL, line, and column. - V8HeapProfExtractor: parses the sampling heap profile head tree, publishes self-size frame measurements in bytes, explicitly distinguishing sampled allocation bytes from retained heap or RSS. Both extractors preserve JavaScript locations and script URLs without applying source maps, keeping generated locations distinguishable. Export the new extractors from the adapters package.
Add unit tests for the node-cpu-prof and node-heap-prof capture invocations (flag injection, workload argument preservation, rejection of non-Node workloads) and integration tests for the V8CpuProfExtractor and V8HeapProfExtractor (frame measurement publication, malformed profile rejection).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8c24a9466
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for node in nodes_by_id.values(): | ||
| self._aggregate_node( |
There was a problem hiding this comment.
In a normal .cpuprofile, nodes is already a flat list containing every node, but this loop calls _aggregate_node for each entry while _aggregate_node also recursively visits every child. Consequently, a node is counted once for every ancestor plus its own top-level iteration (for example, a root → main → helper tree counts helper.hitCount three times), corrupting self_value, inclusive_value, and sample_count for nearly every nontrivial profile. Start traversal only from the root, or aggregate flat nodes without recursively adding their self counts.
AGENTS.md reference: AGENTS.md:L5-L13
Useful? React with 👍 / 👎.
| "V8CpuProfExtractionResult", | ||
| "V8CpuProfExtractor", | ||
| "V8HeapProfExtractionResult", | ||
| "V8HeapProfExtractor", |
There was a problem hiding this comment.
These exports make the extractors callable only from Python; the reviewed src/flameox/cli.py extract command group and src/flameox/mcp/server.py tool registrations contain no CPU- or heap-V8 extraction entry point. Thus an agent following the documented capture → extract workflow can capture these artifacts but cannot produce the newly implemented normalized evidence through either supported transport, unlike the existing Memray and other extractors. Add corresponding CLI commands and MCP tools over the same extraction behavior.
AGENTS.md reference: AGENTS.md:L40-L45
Useful? React with 👍 / 👎.
| values["self"] += self_size | ||
| values["inclusive"] += self_size | ||
| values["samples"] += 1 |
There was a problem hiding this comment.
For a heap profile with nested allocation frames, this assigns each frame's inclusive_value only its own selfSize; child totals are never propagated to callers. Since no stack edges are published, caller frames with zero direct allocations appear to have zero inclusive cost and hotspot analysis, which sorts by inclusive value, loses the profile's call-tree evidence. Return each subtree total and add it to the parent's inclusive aggregate rather than copying the self value.
AGENTS.md reference: AGENTS.md:L5-L13
Useful? React with 👍 / 👎.
| for node in nodes: | ||
| if not isinstance(node, dict): | ||
| continue | ||
| nodes_by_id[int(node["id"])] = node |
There was a problem hiding this comment.
When an imported profile has a non-dictionary node filtered through here but a remaining node lacks id or has a non-integer ID, node["id"]/int(...) raises KeyError, TypeError, or ValueError directly. This bypasses the extractor's ARTIFACT_PARSE_FAILED contract and causes callers to receive an untyped internal failure for malformed or unsupported artifacts; validate required node fields and translate conversion failures before aggregation.
Useful? React with 👍 / 👎.
| aggregates=aggregates, | ||
| artifact_id=registration.artifact_id, | ||
| ) | ||
| total_sampled_bytes = sum(int(sample.get("size", 0)) for sample in samples) |
There was a problem hiding this comment.
If the top-level samples value is a list but any element is not an object, which is a plausible malformed imported artifact, sample.get(...) raises AttributeError rather than the promised ARTIFACT_PARSE_FAILED domain error. Numeric strings, null entries, or invalid size values can similarly leak conversion exceptions; validate every sample and convert these failures into the bounded parse error before calculating totals.
Useful? React with 👍 / 👎.
| remediation=( | ||
| "Install Node.js 20.16+ or 22.4+ which expose stable --cpu-prof flags.", | ||
| ), | ||
| version_args=("--version",), |
There was a problem hiding this comment.
On a host with Node older than the stated 20.16+/22.4+ stable-support floor, capability discovery and active probing still report these adapters as available because they only resolve node and run node --version; no code interprets or constrains the returned version. Planning can therefore authorize a producer version the adapter describes as unsupported without any compatibility limitation. Parse the probed Node version and mark older releases incompatible, or truthfully declare and qualify the broader supported range.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| "source_state_id": None, | ||
| "artifact_id": artifact_id, | ||
| "inlined": False, | ||
| "symbolization": "complete", |
There was a problem hiding this comment.
Valid V8 profiles routinely contain synthetic or unresolved frames with an empty URL, and this extractor also substitutes (anonymous) when the function name is absent, yet every resulting frame is labeled symbolization="complete". Hotspot coverage counts that field directly, so these profiles can report 100% complete symbolization despite lacking source identity and without source-map resolution; classify empty or fallback locations as partial/unresolved and reserve complete for frames with the required symbol information.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| registration = registrations[0] | ||
| artifact = ArtifactStore(self.workspace).get(registration.artifact_id) | ||
| try: | ||
| payload = json.loads(artifact.payload_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
Both V8 extractors call read_text() and json.loads() in the application process even though imported artifacts may be as large as the configured 4 GiB limit. A realistically large CPU or heap profile can therefore allocate several times its file size, block the MCP/CLI control process, or terminate it with an out-of-memory error before any bounded row publication occurs. Use a bounded streaming parser in an isolated worker, with explicit node/sample limits, rather than materializing the complete profile in the control process.
AGENTS.md reference: AGENTS.md:L27-L29
Useful? React with 👍 / 👎.
| ErrorCode.INVALID_CAPTURE_PLAN, | ||
| "A declared Node.js workload command is required for V8 profiling.", | ||
| ) | ||
| node_executable = workload_argv[0] |
There was a problem hiding this comment.
When a workload names a custom Node executable such as /opt/node18/bin/node while capability discovery resolves a different PATH node, this assignment runs the workload's executable but the capture plan records the capability executable's version as adapter_version, which is later registered as the artifact's producer version. The result is internally inconsistent provenance and can qualify or compare a Node 18 artifact as though Node 24 produced it. Probe and bind the declared workload executable itself, or reject a mismatch instead of ignoring the supplied resolved adapter executable.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
| if url.startswith("node:") or not Path(url).is_absolute(): | ||
| return url |
There was a problem hiding this comment.
Node profiles normally encode local scripts as file:///... URLs, but Path(url).is_absolute() is false for such values, so this branch returns the absolute file URL unchanged and never relativizes it to workspace.project_root. Profiles of the same source captured under different checkout roots consequently produce different frame IDs and cannot be aligned in cross-run comparisons; parse and decode file: URLs to filesystem paths before applying project-relative normalization.
AGENTS.md reference: AGENTS.md:L48-L51
Useful? React with 👍 / 👎.
1 participant
Problem
GitHub issues #297 and #298 requested typed evidence paths for Node.js/V8 profiles:
.cpuprofile) whenperfsampling is unavailable for Node workloads..heapprofile) as typed allocation evidence for Node workloads.Both issues are labeled
enhancement/triageand had no existing adapter support in the codebase.Approach
Add two new built-in adapters consistent with Flameox's existing profiling contracts:
node-cpu-prof(#297)--cpu-prof,--cpu-prof-dir, and--cpu-prof-nameflags (Node 20.16+ / 22.4+)..cpuprofileas asample_profileartifact.V8CpuProfExtractorpublishescpu.hit_countframe measurements keyed by function name, script URL, line, and column.perfis denied for a Node workload, agents can now choosenode-cpu-profas a concrete typed next action.node-heap-prof(#298)--heap-prof,--heap-prof-dir, and--heap-prof-nameflags..heapprofileas amemory_profileartifact.V8HeapProfExtractorpublishesmemory.self_sizeframe measurements in bytes, explicitly distinguishing sampled allocation bytes from retained heap or process RSS in the limitation metadata.Implementation details
argv[0]and before the user script, binding the output directory and file name explicitly so Flameox owns the artifact path.INVALID_CAPTURE_PLANerrors.Issues closed
Commands run
All new tests pass (12 tests: 8 unit capture invocation + 4 integration extraction). Linting and strict-mode type checking pass with no issues.
Compatibility / safety
json); no new runtime dependencies.Continue this on Linzumi