agents: add runtime control for asset collection#377
agents: add runtime control for asset collection#377santigimeno wants to merge 1 commit intonode-v24.x-nsolid-v6.xfrom
Conversation
WalkthroughAdded an optional proto field Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant JS as JS TestClient / App
participant IPC as Test Child (agent wrapper)
participant Agent as Agent (gRPC / ZMQ)
participant Profiler as Profiler subsystem
JS->>IPC: send config / enableAssets / disableAssets / trace
IPC->>Agent: translate -> reconfigure / control message
Agent->>Agent: update atomic assets_enabled_ / cont_cpu_profile_enabled_
alt assetsEnabled == false
Agent-->>IPC: respond with EAssetsDisabled (mapped -> 500/422)
IPC-->>JS: surface error to caller
else assetsEnabled == true
Agent->>Profiler: request start (profile/sampling/snapshot/trace)
Profiler-->>Agent: emit asset-data events / completion
Agent-->>IPC: forward events
IPC-->>JS: deliver events/results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents/grpc/src/grpc_agent.cc (1)
1710-1760: Propagate assetsEnabled via reconfigure().
reconfigure()ignores the new proto field, so gRPC-driven config updates can’t flip the assets gate and the feature regresses. Please write the field into the outbound JSON before callingUpdateConfig().if (body.has_contcpuprofile()) { out["contCpuProfile"] = body.contcpuprofile(); } + + if (body.has_assetsenabled()) { + out["assetsEnabled"] = body.assetsenabled(); + }
🧹 Nitpick comments (4)
test/parallel/test-nsolid-heap-sampling-stream.js (2)
191-224: Consider refactoring nested callbacks to improve readability.The test uses four levels of nested
setImmediate()callbacks, making the flow difficult to follow. Consider refactoring to async/await or helper functions to flatten the structure.Example refactor using helper functions:
function runHeapSamplingStreamAssetsToggleTests() { // Disable assets through config update nsolid.start({ command: 'localhost:9001', data: 'localhost:9002', assetsEnabled: false, }); setImmediate(() => { testDisabled(() => { testEnabled(() => { testDisabled(() => { testFinalEnabled(); }); }); }); }); } function testDisabled(callback) { assert.throws( () => nsolid.heapSamplingStream(0, 1000), { message: 'Heap sampling could not be started' } ); callback(); } function testEnabled(callback) { nsolid.enableAssets(); setImmediate(() => { let profile = ''; const stream = nsolid.heapSamplingStream(0, 1200); stream.on('data', (chunk) => { profile += chunk; }); stream.on('end', common.mustCall(() => { assert(JSON.parse(profile)); testProfileSchema(JSON.parse(profile)); callback(); })); }); } function testFinalEnabled() { nsolid.enableAssets(); setImmediate(() => { const stream = nsolid.heapSamplingStream(0, 1200); stream.resume(); stream.on('end', common.mustCall(() => { assert.ok(true); })); }); }
193-199: Consider adding memory allocations for consistency.The initial test (lines 128-131) performs allocations to ensure there's data to sample. The subsequent heap sampling calls don't do this, which might lead to less deterministic results. While the tests may still pass, adding allocations would make the behavior more consistent.
Add allocations before each heap sampling operation:
nsolid.enableAssets(); setImmediate(() => { + // Do some allocations to ensure sampling data + const arr = []; + for (let i = 0; i < 1000; i++) { + arr.push(new Array(1000)); + } + let profile = ''; const enabledStream = nsolid.heapSamplingStream(0, 1200);Apply the same pattern before line 216.
Also applies to: 216-221
test/parallel/test-nsolid-profile.js (1)
120-178: Reduce reliance on fixed sleeps; gate on observed config stateThe toggle flow is correct, but hardcoded 100ms delays can flake under slow CI or propagation lag. After each start()/enable/disable step, prefer waiting until nsolid.config.assetsEnabled reflects the expected value before proceeding (or assert it directly), instead of fixed timeouts.
Example pattern:
function waitForAssetsState(expected, deadline = Date.now() + 2000) { return new Promise((resolve, reject) => { (function poll() { if (nsolid.config.assetsEnabled === expected) return resolve(); if (Date.now() > deadline) return reject(new Error('assetsEnabled not applied')); setImmediate(poll); })(); }); } // Usage: nsolid.start({ /* … */ assetsEnabled: false }); await waitForAssetsState(false); // then assert throws, etc.test/parallel/test-nsolid-heap-profile.js (1)
133-200: Consider using async/await instead of nested setTimeout calls.The nested setTimeout pattern (lines 141, 167, 173, 193) makes the test flow harder to follow and maintain. Consider refactoring to use async/await with timer promises:
-function runHeapProfileAssetsToggleTests() { +async function runHeapProfileAssetsToggleTests() { // Disable assets via config update nsolid.start({ command: 'localhost:9001', data: 'localhost:9002', assetsEnabled: false, }); - setTimeout(() => { + await setTimeout(100); assert.throws( () => { nsolid.heapProfile(); }, { message: 'Heap profile could not be started' } ); // ... rest of assertions - }, 100); }This would improve readability and make the test more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(4 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(2 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(2 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🔇 Additional comments (32)
test/parallel/test-nsolid-heap-sampling-stream.js (1)
179-226: Verify synchronization guarantees for config changes.The test uses
setImmediate()to sequence operations after callingnsolid.start(),enableAssets(), anddisableAssets(). However, there's no explicit guarantee that config changes have fully propagated before the next assertion executes. This could lead to flaky test failures if the timing assumptions don't hold in all environments.Consider one of the following approaches:
- If nsolid provides synchronous config updates or completion callbacks, use those instead of
setImmediate().- Add explicit verification that the config state has changed before proceeding (e.g., check a status flag).
- Document the timing assumptions if
setImmediate()is sufficient based on the nsolid implementation.Run the following script to check if other nsolid tests use similar patterns:
agents/zmq/src/zmq_agent.h (1)
731-731: LGTM! Atomic flag added for thread-safe asset control.The
assets_enabled_flag is appropriately declared asstd::atomic<bool>for thread-safe access and is correctly placed in the profiling section alongside related state variables.agents/grpc/src/grpc_errors.h (1)
12-13: LGTM! New error type correctly defined.The
EAssetsDisablederror entry is properly added with:
- Sequential error code (1008)
- Appropriate HTTP status (500) for internal/configuration errors
- Clear error message that aligns with test expectations
agents/zmq/src/zmq_agent.cc (2)
1282-1287: LGTM! Configuration reading follows established pattern.The
assetsEnabledconfiguration is correctly read from the config and stored in the atomic flag, following the same pattern used for other configuration fields.
2115-2117: LGTM! Asset gating correctly implemented.The early return when
assets_enabled_is false properly prevents profiling operations when assets are disabled. TheUV_EINVALerror code maps to "Invalid arguments" (code 422) increate_command_error, which is appropriate for this validation failure.test/agents/test-grpc-tracing.mjs (3)
5-5: LGTM! Import added for async delays in test flow.The
delayimport fromnode:timers/promisesis appropriately added to support the new phase-based test flow that requires waiting for spans to flush between enable/disable cycles.
205-247: LGTM! Comprehensive phase-based test for HTTP tracing.The test correctly implements a multi-phase flow to verify:
- Initial span collection works
- Disabling traces prevents span collection (verified with delay and assertion)
- Re-enabling traces restores span collection
The phase state machine and early return on 'done' prevent race conditions and ensure proper test sequencing.
273-315: LGTM! Comprehensive phase-based test for custom tracing.The custom tracing test mirrors the HTTP tracing test structure, providing consistent coverage for the disable/enable functionality across different tracing types.
lib/nsolid.js (8)
133-136: LGTM! Public API surface extended with asset and trace controls.The four new functions (
enableAssets,disableAssets,enableTraces,disableTraces) are appropriately exported and provide runtime control over asset collection and tracing.
195-206: LGTM! Getters provide appropriate defaults.The new getters correctly:
- Default
assetsEnabledtotrue(assets enabled by default)- Default
tracingEnabledtofalse(tracing disabled by default)- Use nullish coalescing (
??) to handle undefined config values
413-415: LGTM! Assets guard correctly prevents heap profiling when disabled.The early check throws
ERR_NSOLID_HEAP_PROFILE_STARTwhen assets are disabled, preventing the operation before any profiling setup occurs.
497-498: LGTM! Assets guard correctly prevents heap sampling when disabled.The early check throws
ERR_NSOLID_HEAP_SAMPLING_STARTwhen assets are disabled, consistent with the heap profiling guard.
604-633: LGTM! Enable/disable functions correctly check state before updating.All four functions follow a consistent pattern:
- Check current state to avoid unnecessary updates
- Call
updateConfigwith the new valueThis prevents redundant configuration updates and ensures idempotent behavior.
761-765: LGTM! Config update handling uses normalization helper.The
assetsEnabledconfiguration is correctly normalized usingoptionToBoolbefore storing, which handles various input types (boolean, number, string) consistently.
947-956: LGTM! Config initialization with proper defaults.The
assetsEnabledfield is correctly initialized with:
- Priority order: environment variable > package.json > default (true)
- Proper normalization using
optionToBool- Sensible default of
truewhen neither source provides a value
1106-1116: LGTM! Flexible normalization helper for boolean configuration.The
optionToBoolfunction appropriately handles:
undefined/null→undefined(allows detecting absence of value)- Boolean → direct return
- Number → falsy check (0 → false, non-zero → true)
- String → delegates to
envToBoolfor "0"/"false" handling- Other types →
undefined(graceful handling)agents/grpc/proto/reconfigure.proto (1)
20-20: LGTM! Protocol buffer field correctly defined.The
assetsEnabledfield is properly added with:
- Sequential field number (13)
- Optional modifier for backward compatibility
- Appropriate boolean type
- Consistent camelCase naming
test/common/nsolid-zmq-agent/client.js (1)
141-148: LGTM! IPC message handlers correctly wire up runtime controls.The four new message handlers properly:
- Call the corresponding
nsolidfunctions to enable/disable assets and traces- Follow the fire-and-forget pattern (no response sent)
- Are consistent with the existing message handler structure
agents/grpc/src/grpc_agent.h (1)
352-352: LGTM! Atomic flag added for thread-safe asset control.The
assets_enabled_flag is appropriately declared asstd::atomic<bool>for thread-safe access and is correctly placed in the profiling section alongside related profiling state.test/parallel/test-nsolid-config-tracing.js (1)
24-29: LGTM: runtime tracing helpers keep config in syncThe toggles and corresponding assertions look correct and consistent with earlier start() checks.
test/agents/test-zmq-profile.mjs (1)
120-177: LGTM: ZMQ profile respects assetsEnabled and event flowGood coverage of disabled → 422 path, enabled happy-path, and final config verification. Resolve guard avoids double-resolve.
test/parallel/test-nsolid-heap-profile-stream.js (1)
68-70: LGTM: deterministic heap profile stream togglingNice sequencing around ‘end’ to avoid races; assertions match the API’s disabled vs enabled behavior.
Also applies to: 72-126
test/agents/test-zmq-heap-profile.mjs (1)
342-408: LGTM: ZMQ heap-profile honors assetsEnabled and validates payloadCovers error path, success path, and final config sync. Bootstrap opts prevent stray events. Looks good.
test/agents/test-zmq-snapshot.mjs (1)
235-296: LGTM: ZMQ snapshot gating via assetsEnabledError and success paths validated with proper event sequencing and final config check. All good.
test/parallel/test-nsolid-profile.js (1)
107-111: LGTM: assertion formattingNo behavior change; readability improved.
test/agents/test-zmq-heap-sampling.mjs (1)
135-201: LGTM! Comprehensive test for assetsEnabled toggling.The test properly validates:
- Disabling assets via config results in error code 422
- Re-enabling assets allows profiling to proceed
- Final config state is verified
The use of the
resolvedflag (lines 170-171) prevents duplicate event processing, which is a good defensive pattern.test/agents/test-grpc-heap-sampling.mjs (1)
248-378: LGTM! Comprehensive coverage of asset control methods.The three new tests thoroughly validate asset gating across different control surfaces:
- Environment variable (NSOLID_ASSETS_ENABLED)
- Configuration API (child.config())
- Helper methods (enableAssets/disableAssets)
Each test properly verifies:
- Error 500 with message "Assets collection disabled(1008)" when disabled
- Successful profiling when enabled
- Final config state consistency
The pattern is consistent and maintainable.
test/agents/test-grpc-snapshot.mjs (1)
246-367: LGTM! Consistent test coverage for snapshot operations.The tests mirror the heap sampling pattern and properly validate snapshot behavior with asset controls. The consistency across test files makes the test suite easier to understand and maintain.
test/agents/test-grpc-heap-profile.mjs (1)
259-389: LGTM! Complete test coverage for heap profiling with asset controls.The tests maintain consistency with the other asset control tests and properly validate heap profiling behavior across all three control surfaces.
test/common/nsolid-zmq-agent/index.js (1)
187-233: LGTM! Clean helper methods for runtime control.The four new methods (
enableTraces,disableTraces,enableAssets,disableAssets) follow the established async pattern in the TestClient class. The implementation is straightforward and consistent.test/common/nsolid-grpc-agent/index.js (1)
346-401: LGTM! Excellent refactoring with the #sendAndWait helper.The introduction of the
#sendAndWaithelper method (lines 389-401) is a good abstraction that:
- Reduces code duplication
- Provides a consistent pattern for message handling
- Makes the new helper methods (
enableAssets,disableAssets,enableTraces,disableTraces) more concise and maintainableThe refactored
config()method now uses this helper, demonstrating its utility.src/nsolid/nsolid_api.cc (1)
1161-1186: Verify continuous profiler is disabled when assetsEnabled is false.The logic correctly gates
contCpuProfileandcontCpuProfileIntervalreads behind theassetsEnabledcheck. However, ensure thatupdate_continuous_profilerproperly handles the case when both flags are false (which happens whenassetsEnabledis false).Run the following to verify the continuous profiler handling:
santigimeno
force-pushed
the
santi/assets_enabled
branch
2 times, most recently
from
b3ccdf4 to
74e0d6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/parallel/test-nsolid-heap-sampling-stream.js (1)
179-187: Also assert the error code for gating failuresWhen assets are disabled, heapSamplingStream throws ERR_NSOLID_HEAP_SAMPLING_START synchronously. Assert the code property too for stronger checks.
assert.throws( () => { nsolid.heapSamplingStream(0, 1000); }, { - message: 'Heap sampling could not be started' + message: 'Heap sampling could not be started', + code: 'ERR_NSOLID_HEAP_SAMPLING_START' } );Apply the same change to Lines 204‑211.
Also applies to: 204-211
🧹 Nitpick comments (10)
test/parallel/test-nsolid-heap-sampling-stream.js (1)
173-177: Avoid starting ZMQ in this unit testNo need to pass command/data to nsolid.start() here. It can introduce unwanted ZMQ side-effects. Set only assetsEnabled.
- nsolid.start({ - command: 'localhost:9001', - data: 'localhost:9002', - assetsEnabled: false, - }); + nsolid.start({ assetsEnabled: false });lib/nsolid.js (3)
413-416: Validate args before gating; remove unused parameter
- Move the assetsEnabled check after validateNumber/validateBoolean to preserve argument error semantics consistently.
- The options parameter in heapProfileStream is unused; drop it to avoid confusion.
-function heapProfileStream(threadId, duration, trackAllocations, options) { - if (getConfig('assetsEnabled') === false) - throw new ERR_NSOLID_HEAP_PROFILE_START(); +function heapProfileStream(threadId, duration, trackAllocations) { + // Validate arguments first. validateNumber(threadId, 'threadId'); validateNumber(duration, 'duration'); validateBoolean(trackAllocations, 'trackAllocations'); + // Gate after validation. + if (getConfig('assetsEnabled') === false) + throw new ERR_NSOLID_HEAP_PROFILE_START();
947-957: Initialize assetsEnabled from env/pkg with sane defaultSolid defaulting logic. Be mindful that empty-string env values may not normalize to boolean (see optionToBool suggestion below).
Consider how NSOLID_ASSETS_ENABLED="" should behave. If it should be treated as “unset”, ensure optionToBool handles empty strings accordingly (see proposed change on Lines 1106‑1116).
1106-1116: optionToBool: handle empty strings explicitly and return strict booleansIf a string is empty, current envToBool returns a falsy value (empty string), not a strict boolean. When stored in config, later strict comparisons (=== false) can misbehave.
function optionToBool(value) { if (value === undefined || value === null) return undefined; if (typeof value === 'boolean') return value; if (typeof value === 'number') return value !== 0; - if (typeof value === 'string') - return envToBool(value); + if (typeof value === 'string') { + if (value === '') + return undefined; // treat empty string as “unset” + return value !== '0' && value.toLowerCase() !== 'false'; + } return undefined; }test/parallel/test-nsolid-profile.js (1)
120-182: Reduce flakiness: replace fixed 100ms delays with platform-aware timeoutsUse common.platformTimeout(100) for the nested setTimeout calls to better tolerate slow CI hosts.
Apply this diff within runAssetsToggleTests():
- }, 100); + }, common.platformTimeout(100)); ... - }, 100); + }, common.platformTimeout(100)); ... - }, 100); + }, common.platformTimeout(100)); ... - }, 100); + }, common.platformTimeout(100));test/agents/test-zmq-heap-profile.mjs (1)
342-409: Confirm error semantics across protocolsHere ZMQ returns 422 "Invalid arguments" when assets are disabled, while gRPC tests assert 500 "Assets collection disabled(1008)". Is this divergence intentional? If not, consider aligning error codes/messages for consistency across agents.
test/parallel/test-nsolid-heap-sampling.js (1)
177-248: Harden timeouts against CI varianceUse common.platformTimeout(100) instead of raw 100ms in nested setTimeout calls to reduce flakiness.
Suggested changes within runHeapSamplingAssetsToggleTests():
- }, 100); + }, common.platformTimeout(100)); - }, 100); + }, common.platformTimeout(100)); - }, 100); + }, common.platformTimeout(100)); - }, 100); + }, common.platformTimeout(100));test/parallel/test-nsolid-heap-profile.js (1)
133-205: Use platform-aware timeouts to avoid flakesReplace fixed 100ms in nested setTimeout calls with common.platformTimeout(100).
Apply within runHeapProfileAssetsToggleTests():
- }, 100); + }, common.platformTimeout(100)); - }, 100); + }, common.platformTimeout(100)); - }, 100); + }, common.platformTimeout(100)); - }, 100); + }, common.platformTimeout(100));test/agents/test-grpc-snapshot.mjs (1)
282-324: Reconfigure propagation timingAfter child.config({ assetsEnabled: false/true }), the test immediately calls heapSnapshot(). If reconfigure is eventually consistent, add a small wait or poll child.config() until it reflects the new value before proceeding to avoid rare races.
test/agents/test-grpc-continuous-profile.mjs (1)
275-432: Consider removing or documenting the commented-out code.This ~160-line commented-out section contains three test cases. If these tests are obsolete, remove them to improve code clarity. If they're deferred for future work, consider opening an issue to track them and adding a brief comment explaining why they're commented out.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(4 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(2 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(2 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- test/common/nsolid-zmq-agent/client.js
- agents/grpc/src/grpc_agent.cc
- test/agents/test-zmq-snapshot.mjs
- agents/grpc/src/grpc_agent.h
- test/parallel/test-nsolid-snapshot.js
- test/agents/test-zmq-tracing.mjs
- test/agents/test-grpc-profile.mjs
- agents/zmq/src/zmq_agent.h
- agents/zmq/src/zmq_agent.cc
- src/nsolid/nsolid_api.cc
- test/parallel/test-nsolid-config-tracing.js
- test/agents/test-grpc-heap-profile.mjs
- test/common/nsolid-grpc-agent/index.js
- agents/grpc/src/grpc_errors.h
- test/agents/test-zmq-heap-sampling.mjs
- test/parallel/test-nsolid-heap-profile-stream.js
- agents/grpc/src/proto/reconfigure.pb.h
🔇 Additional comments (21)
lib/nsolid.js (7)
133-137: Public toggles for assets/traces exposedGood addition. Clear, minimal API with enable/disable helpers.
195-207: Config getters for assetsEnabled/tracingEnabledGood defaults and exposure via nsolid.config and getters.
497-499: Gating for heapSamplingStreamLooks correct; early throw prevents stream creation when assets disabled.
761-765: assetsEnabled in updateConfigNormalization via optionToBool and conditional assignment is correct.
604-618: enableAssets/disableAssets helpersSimple, idempotent toggles are good.
620-634: enableTraces/disableTraces helpersConsistent with assets toggles.
637-638: Doc nitMinor doc change acknowledged. No issues.
agents/grpc/proto/reconfigure.proto (1)
20-20: Add assetsEnabled to ReconfigureBodyLooks good. Field number 13 follows existing numbering without conflict.
test/agents/test-zmq-profile.mjs (1)
120-177: ZMQ profile test for assetsEnabled toggleGood coverage: disables via config, asserts 422, re-enables and validates profile flow; also verifies config state.
test/common/nsolid-grpc-agent/client.js (2)
64-69: Guard otel.register against duplicate registrationGood defensive handling; ignores expected already-registered error.
124-137: Support runtime toggle messages for assets/tracesHandlers invoke nsolid toggles and send acks. Looks correct.
test/parallel/test-nsolid-profile.js (1)
107-112: LGTM on assertion formatting and flowMulti-line strictEqual improves readability, and invoking runAssetsToggleTests() extends coverage.
test/parallel/test-nsolid-heap-sampling.js (1)
164-168: ** LGTM on assertion formatting**Message split across lines is fine and consistent with other tests.
test/parallel/test-nsolid-heap-profile.js (1)
124-124: LGTMRunning runHeapProfileAssetsToggleTests() extends coverage for assets toggling.
test/agents/test-grpc-snapshot.mjs (2)
246-280: LGTM: env-based gating coverageGood validation of NSOLID_ASSETS_ENABLED=0 yielding 500 "Assets collection disabled(1008)".
325-368: LGTM: helper-based togglingdisableAssets()/enableAssets() paths look correct; asserting currentConfig.assetsEnabled ensures state consistency.
test/agents/test-grpc-heap-sampling.mjs (1)
248-378: LGTM! Comprehensive test coverage for assets gating.The three new test cases thoroughly validate the assetsEnabled feature across different configuration methods (environment variable, config API, and helper methods). The tests correctly verify that heap sampling returns a 500 error when assets are disabled and succeeds when re-enabled.
test/agents/test-grpc-tracing.mjs (1)
5-5: LGTM! Well-structured phase-based testing for tracing toggle.The phase-based state machine ('initial' → 'disabled' → 'reenabled' → 'done') effectively validates that tracing can be toggled at runtime. The tests correctly verify that no spans are emitted when tracing is disabled and that spans resume after re-enabling.
Also applies to: 205-247, 273-315
test/agents/test-grpc-continuous-profile.mjs (1)
4-4: LGTM! Thorough validation of continuous profiling with assets gating.The new and modified tests effectively verify that continuous CPU profiling respects the assetsEnabled flag across different configuration methods. The use of profile counters and timing delays provides clear validation of the gating behavior.
Also applies to: 100-224
test/common/nsolid-zmq-agent/index.js (1)
187-233: LGTM! Helper methods follow established patterns.The four new methods (enableTraces, disableTraces, enableAssets, disableAssets) correctly follow the existing IPC messaging pattern used throughout the TestClient class. The implementation is consistent with other helper methods.
agents/grpc/src/proto/reconfigure.pb.cc (1)
1-1082: Auto-generated protobuf code.This file contains auto-generated code from the protocol buffer compiler. The changes mechanically add the assetsEnabled field (field 13) to ReconfigureBody with appropriate initialization, serialization, and accessor logic. No manual review required.
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
74e0d6b to
de122d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/agents/test-zmq-heap-sampling.mjs (1)
135-201: LGTM! Completes consistent coverage across asset types.The test maintains the established pattern for heap sampling, with appropriate sampling-specific validations (lines 189-191). The consistent implementation across all three asset types (snapshot, heap profile, heap sampling) demonstrates thorough test coverage for the
assetsEnabledfeature.Note: The three tests share a nearly identical structure. While this duplication is acceptable and even beneficial for test clarity and isolation, consider extracting a reusable test helper if additional asset types are introduced in the future. For example:
async function testAssetsEnabledToggle(playground, startAssetFn, validateAssetFn) { // Common toggle logic }However, this refactor is entirely optional given the current scope.
test/parallel/test-nsolid-snapshot.js (3)
102-104: Confirm mustSucceed() without handler; otherwise pass a noop for safetyIf common.mustSucceed() requires a function, this could throw. Passing a noop keeps semantics identical and avoids coupling to helper internals.
- nsolid.snapshot(common.mustSucceed()); + nsolid.snapshot(common.mustSucceed(() => {}));
60-66: Re-invoking nsolid.start: verify idempotency and side-effectsStarting twice in the same process may reinitialize state/transport. If not guaranteed safe, prefer a single start then drive behavior via enableAssets()/disableAssets() and disableSnapshots flags, or reconfigure if supported.
68-76: Strengthen assertions: check the specific error code when assets are disabledThis PR introduces EAssetsDisabled. Asserting the code (in addition to message) avoids conflating “assets disabled” with other failure causes.
Example tweaks:
- assert.throws( + assert.throws( () => { nsolid.snapshot(); }, { - message: 'Heap snapshot could not be generated' + message: 'Heap snapshot could not be generated', + code: 'EAssetsDisabled' } );- assert.notStrictEqual(err.code, 0); - assert.strictEqual(err.message, 'Heap snapshot could not be generated'); + assert.strictEqual(err.code, 'EAssetsDisabled'); + assert.strictEqual(err.message, 'Heap snapshot could not be generated');Apply similarly to the later assert. If codes can vary by transport, consider allowing either the new code or falling back to the message match.
Also applies to: 78-81, 90-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(4 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(2 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(2 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- agents/zmq/src/zmq_agent.h
- test/agents/test-zmq-tracing.mjs
- test/agents/test-zmq-profile.mjs
- agents/grpc/src/grpc_agent.h
- agents/grpc/src/grpc_agent.cc
- test/agents/test-grpc-heap-profile.mjs
- test/parallel/test-nsolid-heap-profile-stream.js
- test/common/nsolid-grpc-agent/client.js
- agents/grpc/proto/reconfigure.proto
- test/common/nsolid-zmq-agent/client.js
- test/parallel/test-nsolid-profile.js
- agents/zmq/src/zmq_agent.cc
- src/nsolid/nsolid_api.cc
- test/parallel/test-nsolid-heap-sampling-stream.js
- agents/grpc/src/grpc_errors.h
- test/common/nsolid-zmq-agent/index.js
- test/agents/test-grpc-snapshot.mjs
- test/agents/test-grpc-profile.mjs
🔇 Additional comments (36)
test/parallel/test-nsolid-heap-profile.js (2)
124-124: LGTM!The placement and invocation of the new test function is appropriate, ensuring it runs after the existing test flow completes.
133-204: Verify enableAssets/disableAssets return values and consider timing robustness.The test provides good coverage of asset toggle scenarios. However, consider these improvements:
Missing verification: The calls to
enableAssets()anddisableAssets()(lines 166, 172, 193) don't verify success. If these operations can fail or return status, add assertions to confirm the state changes actually occurred.Timing robustness: The test relies on multiple hardcoded 100ms timeouts (lines 141, 167, 173, 194). In slow CI environments or under load, these may be insufficient and cause flakiness. Consider whether these delays can be replaced with event-driven synchronization or if they need to be more generous.
Callback nesting: The deeply nested structure (5 levels deep starting at line 169) reduces readability. While it follows the existing pattern in this file, consider whether this could be flattened in a future refactor.
To verify the reliability of the timing assumptions, you could run this test repeatedly under load:
test/agents/test-zmq-snapshot.mjs (1)
235-296: LGTM! Well-structured test for runtime asset toggling.The test correctly validates the
assetsEnabledruntime control:
- Properly disables assets and verifies the expected 422 error when attempting a snapshot
- Re-enables assets and confirms the snapshot succeeds
- Verifies the final configuration state
The defensive
resolvedflag (lines 242, 269-270) appropriately prevents duplicate event processing after test completion.test/agents/test-zmq-heap-profile.mjs (1)
342-408: LGTM! Consistent implementation for heap profiling.The test follows the same proven pattern as the snapshot test and includes appropriate heap-profile-specific validations:
- Lines 396-398 correctly verify that
trackAllocations: falseresults in empty trace data- Runtime toggling flow is properly implemented with correct error handling
- Final state verification confirms
assetsEnabledpersists after operation completionagents/grpc/src/proto/reconfigure.pb.cc (6)
49-50: LGTM! Generated field initialization is correct.The new
assetsenabled_field is properly initialized tofalsein the constexpr constructor, following the same pattern as other boolean fields likecontcpuprofile_.
107-133: LGTM! Has-bit layout correctly updated.The has-bit index offset (line 107) and field offsets (lines 120, 133) are properly updated to accommodate the new
assetsenabled_field. The hasbit index of 11 for the new field is consistent with it being the 12th optional field (0-indexed).
243-245: LGTM! Memory copy ranges correctly extended.The copy constructor (lines 243-245) and
SharedCtor(lines 263-265) memory ranges are properly updated to include the newassetsenabled_field as the last member in the contiguous memory block.Also applies to: 263-265
465-468: LGTM! Clear and serialization logic properly updated.The
Clearoperation (lines 465-468) now uses the correct has-bits mask0x00000f00U(covering bits 8-11), and the serialization code (lines 581-586) correctly emits the new field with tag 13 when present.Also applies to: 581-586
624-624: LGTM! Size calculation and merge logic correctly updated.Line 624: The has-bits mask in
ByteSizeLongis correctly updated from0x000005e0Uto0x00000de0U, adding bit 11 for the new field.Lines 702-714: The
MergeImplfunction properly handles merging the newassetsenabled_field when has-bit0x00000800Uis set.Also applies to: 702-714
739-740: LGTM! Swap logic correctly updated.The
InternalSwaprange (lines 739-740) now correctly usesassetsenabled_as the end marker since it's the last field in the contiguous memory block, replacing the previous end markercontcpuprofile_.agents/grpc/src/proto/reconfigure.pb.h (4)
233-233: LGTM! Field number and parse table correctly defined.Line 233: The new field number
kAssetsEnabledFieldNumber = 13is correctly defined in the enum.Line 408: The
TcParseTabletemplate parameter is properly updated from<4, 12, ...>to<4, 13, ...>to reflect the increased field count.Also applies to: 408-408
392-401: LGTM! Public accessor declarations are complete.Lines 392-401 declare the standard protobuf accessors for the
assetsEnabledfield:
- Public:
has_assetsenabled(),clear_assetsenabled(),assetsenabled(),set_assetsenabled()- Private:
_internal_assetsenabled(),_internal_set_assetsenabled()All follow the standard protobuf API patterns.
442-442: LGTM! Field added to internal struct.Line 442: The
assetsenabled_field is correctly added as the last boolean member in theImpl_struct, maintaining consistency with the implementation file.
1175-1201: LGTM! Accessor implementations are complete and correct.Lines 1175-1201 provide the inline implementations for all
assetsEnabledaccessors. The implementation:
- Uses the correct has-bit mask
0x00000800U(bit 11)- Includes proper TSan annotations for thread safety
- Follows the exact pattern of other boolean fields
The generated code is mechanically correct and consistent.
test/parallel/test-nsolid-config-tracing.js (1)
23-29: LGTM!The test properly validates that the new runtime tracing toggle helpers (
disableTraces()andenableTraces()) correctly update theconfig.tracingEnabledstate.test/agents/test-grpc-tracing.mjs (2)
205-247: Well-structured phase-based testing approach.The introduction of a phase state machine (
'initial'→'disabled'→'reenabled'→'done') properly orchestrates the multi-step tracing toggle test. The 200ms delays ensure spans have time to emit (or not emit when disabled), and the phase guards prevent race conditions after test completion.
273-315: LGTM!The custom tracing test follows the same solid phase-based pattern as the HTTP tracing test, ensuring consistent validation of the trace enable/disable functionality.
test/parallel/test-nsolid-heap-sampling.js (2)
164-169: LGTM!The multi-line assertion formatting improves readability, and the subsequent call to
runHeapSamplingAssetsToggleTests()properly integrates the new test flow.
177-248: Comprehensive asset toggle testing.The test function thoroughly validates heap sampling behavior when assets are toggled:
- Disables assets and verifies errors are thrown
- Re-enables assets and confirms sampling works
- Disables again and re-validates error conditions
The timing and sequencing properly ensure each operation completes before the next begins.
test/agents/test-grpc-heap-sampling.mjs (3)
248-285: LGTM!This test properly validates that heap sampling respects the
NSOLID_ASSETS_ENABLEDenvironment variable, returning the expected error when assets are disabled.
287-331: LGTM!The test correctly validates toggling
assetsEnabledviansolid.start()configuration, verifying both the disabled error state and the successful profile data collection when re-enabled.
333-378: LGTM!This test properly exercises the runtime helper methods (
disableAssets()andenableAssets()), confirming they gate heap sampling operations correctly and update the configuration state.test/agents/test-grpc-continuous-profile.mjs (4)
100-135: LGTM!The test properly validates that continuous profiling is suppressed when
NSOLID_ASSETS_ENABLED='0', waiting an appropriate duration to confirm no profiles are emitted.
137-180: LGTM!This test correctly validates that continuous profiling stops/resumes when
assetsEnabledis toggled via configuration, with appropriate delays to observe profile emission patterns.
182-224: LGTM!The test properly exercises the runtime helpers (
disableAssets()/enableAssets()) with continuous profiling, confirming that profile emission is gated correctly and configuration state is updated.
275-432: Verify the intent of the extensive commented code.There are 157 lines of commented-out test code. Please confirm whether:
- This code is intentionally disabled for future use
- These tests are superseded by the active tests
- The commented code should be removed
If this is intentional technical debt or work-in-progress, consider adding a comment explaining the rationale.
test/common/nsolid-grpc-agent/index.js (2)
346-387: Well-designed API extensions for runtime testing.The new TestClient methods provide a clean interface for testing asset and tracing toggles:
- Consistent early returns when
#childis null- Unified
#sendAndWaithelper eliminates code duplication- The
config()method's result mapping properly extracts the payload
389-401: LGTM!The private
#sendAndWaithelper properly centralizes the request/response pattern with:
- Configurable message type expectations
- Optional result mapping
- Proper listener cleanup after response
lib/nsolid.js (7)
133-136: LGTM!The new runtime toggle functions (
enableAssets,disableAssets,enableTraces,disableTraces) are properly exported on the public API surface.
195-206: LGTM!The new getters expose the current asset and tracing states with appropriate defaults (
assetsEnabled: true,tracingEnabled: false), using nullish coalescing to handle missing config values.
413-416: Proper runtime gating for asset collection.Both
heapProfileStreamandheapSamplingStreamcorrectly checkassetsEnabledand throw the appropriate error when disabled, preventing expensive profiling operations at runtime.Also applies to: 496-499
604-633: Well-implemented toggle functions.All four toggle functions (
enableAssets,disableAssets,enableTraces,disableTraces) follow a consistent pattern:
- Check current state to avoid unnecessary updates
- Call
updateConfigwith the new stateThis ensures idempotent behavior and proper config propagation.
761-765: LGTM!The
updateConfigfunction properly handles theassetsEnabledoption usingoptionToBoolfor normalization, ensuring only valid boolean values are assigned.
947-956: Proper configuration initialization for assetsEnabled.The initialization correctly prioritizes environment variables over package.json settings, with a sensible default of
truewhen neither is specified. This follows the established configuration precedence pattern.
1106-1116: Robust boolean normalization helper.The
optionToBoolfunction properly handles various input types:
- Returns
undefinedfornull/undefined(preserving absence)- Passes through booleans unchanged
- Converts numbers to boolean (0 = false, non-zero = true)
- Uses
envToBoolfor string conversionThis provides consistent boolean normalization across configuration sources.
test/parallel/test-nsolid-snapshot.js (1)
82-107: Good fix: toggles are now chained to snapshot completion, avoiding racesSequencing enable/disable after the respective snapshot callbacks resolves the prior “in‑flight snapshot” race. This should make the test deterministic.
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
de122d2 to
22ac97c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
test/parallel/test-nsolid-profile.js (1)
120-182: Consider checking for asset-disabled-specific error codes or messages.The test comprehensively exercises the asset toggle functionality, but all error assertions verify the generic message "CPU profile could not be started" without distinguishing whether the failure is specifically due to disabled assets versus other causes. Since the PR introduces an
EAssetsDisablederror, consider adding more specific assertions to improve test precision and debugging.For example, check for a specific error code when assets are disabled:
assert.throws( () => { nsolid.profile(); }, { + code: 'ERR_NSOLID_ASSETS_DISABLED', // or whatever the actual code is message: 'CPU profile could not be started' } );This would make test failures easier to diagnose and catch regressions where the wrong error type is returned.
test/parallel/test-nsolid-heap-profile-stream.js (2)
94-96: Minor: Inconsistent stream consumption pattern.Lines 94-96 consume the stream by listening to 'data' events, while line 117 uses
.resume()to consume the stream without listening to data. This inconsistency makes the test harder to follow.Consider using a consistent pattern. If you don't need the profile data in the final test, use
.resume()consistently:- let profile = ''; const enabledStream = nsolid.heapProfileStream(0, 1200, true); - enabledStream.on('data', (chunk) => { - profile += chunk; - }); + enabledStream.resume(); enabledStream.on('end', common.mustCall(() => { - assert(JSON.parse(profile));Also applies to: 117-117
119-119: Minor: Weak assertion.
assert.ok(true)is a tautology that doesn't validate any meaningful outcome. The test should either verify that the stream completed successfully (whichcommon.mustCallon the 'end' handler already does) or be removed entirely.Remove the redundant assertion:
finalStream.on('end', common.mustCall(() => { - assert.ok(true); }));Alternatively, if you want to verify stream completion, the presence of
common.mustCallon the 'end' event handler is already sufficient validation.test/parallel/test-nsolid-heap-sampling-stream.js (3)
192-200: Consider adding allocation activity before heap sampling.The original test (lines 128-131) creates allocations to ensure heap sampling has data to sample. The re-enabled heap sampling tests might produce minimal or empty profiles without similar allocation activity.
If you want to ensure non-empty profiles in these tests, apply this diff:
nsolid.enableAssets(); setImmediate(() => { + const arr = []; + for (let i = 0; i < 1000; i++) { + arr.push(new Array(1000)); + } let profile = '';
221-221: Remove the meaningless assertion.The
assert.ok(true)assertion doesn't validate anything meaningful. Thecommon.mustCallwrapper already ensures the callback is invoked.Apply this diff to remove the redundant assertion:
- finalStream.on('end', common.mustCall(() => { - assert.ok(true); - })); + finalStream.on('end', common.mustCall());
180-188: Consider extracting the repeated error assertion pattern.The error assertion pattern is duplicated twice. Extracting it into a helper function would reduce duplication and improve maintainability.
Consider refactoring like this:
function assertHeapSamplingDisabled() { assert.throws( () => { nsolid.heapSamplingStream(0, 1000); }, { code: 'ERR_NSOLID_HEAP_SAMPLING_START', message: 'Heap sampling could not be started' } ); }Then replace both assertion blocks with calls to
assertHeapSamplingDisabled().Also applies to: 205-213
test/parallel/test-nsolid-heap-profile.js (2)
130-132: Tight 100 ms waits may flake; prefer longer or state-driven waitsCI can be slower; 100 ms can race with config propagation. Consider increasing timeouts (or polling for state) to reduce flakes.
Apply this minimal diff to relax timing:
- }, common.platformTimeout(100)); -}, common.platformTimeout(100)); + }, common.platformTimeout(250)); +}, common.platformTimeout(250));Optional: replace fixed sleeps with a small poll for the desired outcome to avoid magic delays:
async function waitForHeapProfileStartFailure() { // Poll until calling heapProfile() throws synchronously as expected. for (let i = 0; i < 25; i++) { try { nsolid.heapProfile(); } catch (e) { assert.strictEqual(e.message, 'Heap profile could not be started'); return; } await new Promise((r) => setTimeout(r, common.platformTimeout(20))); } assert.fail('heapProfile() did not start failing within the expected time'); }Please verify on slow builders whether 100 ms is sufficient or adopt one of the above.
133-204: Solid toggle tests; add callback end-error checks and (optionally) “disable while active” caseGreat coverage of disabled/enabled transitions. Two small gaps:
- Also assert the callback form of heapProfileEnd() fails while disabled (mirrors start path).
- Optionally, validate that disabling while an active profile is running still allows ending successfully (important runtime contract per PR).
Apply these small insertions to cover the callback end-error while disabled:
@@ assert.throws( () => { nsolid.heapProfileEnd(); }, { message: 'Heap profile could not be stopped' } ); + // Callback variant should also surface the disabled state + nsolid.heapProfileEnd(common.mustCall((err) => { + assert.notStrictEqual(err.code, 0); + assert.strictEqual(err.message, 'Heap profile could not be stopped'); + })); @@ assert.throws( () => { nsolid.heapProfileEnd(); }, { message: 'Heap profile could not be stopped' } ); + // Callback variant should also surface the disabled state + nsolid.heapProfileEnd(common.mustCall((err) => { + assert.notStrictEqual(err.code, 0); + assert.strictEqual(err.message, 'Heap profile could not be stopped'); + }));Optional addition to assert “disable while active” still allows end to succeed:
// Start a profile, disable assets while active, ensure end still succeeds. nsolid.enableAssets(); setTimeout(() => { nsolid.heapProfile(common.mustSucceed(() => { nsolid.disableAssets(); nsolid.heapProfileEnd(common.mustSucceed(() => { // Re-enable for any subsequent tests nsolid.enableAssets(); })); })); }, common.platformTimeout(100));If the implementation surfaces a specific disabled error code (e.g., EAssetsDisabled), consider asserting it in these disabled paths to strengthen the check. Based on PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(4 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(2 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(6 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- agents/grpc/proto/reconfigure.proto
- test/agents/test-grpc-profile.mjs
- test/agents/test-zmq-profile.mjs
- agents/grpc/src/grpc_agent.cc
- test/parallel/test-nsolid-config-tracing.js
- test/common/nsolid-zmq-agent/client.js
- src/nsolid/nsolid_api.cc
- test/agents/test-zmq-tracing.mjs
- test/agents/test-zmq-heap-profile.mjs
- test/common/nsolid-zmq-agent/index.js
- test/agents/test-grpc-snapshot.mjs
- test/agents/test-zmq-heap-sampling.mjs
- test/agents/test-grpc-heap-sampling.mjs
- test/common/nsolid-grpc-agent/client.js
- test/agents/test-zmq-snapshot.mjs
- agents/grpc/src/grpc_agent.h
- lib/nsolid.js
- agents/zmq/src/zmq_agent.h
- agents/zmq/src/zmq_agent.cc
🔇 Additional comments (24)
test/parallel/test-nsolid-profile.js (2)
107-111: LGTM!The assertion formatting improves readability, and the integration of asset toggle testing at this point ensures all baseline profiling tests complete before exercising the new asset control surface.
117-118: LGTM!Wrapping timeouts with
common.platformTimeout()is a best practice that accounts for varying platform speeds and CI environments, reducing test flakiness.test/parallel/test-nsolid-snapshot.js (3)
56-57: LGTM! Proper test integration.The new test function is correctly integrated into the existing test flow, ensuring it runs after the previous assertions complete.
60-76: LGTM! Clear test setup and validation.The test correctly configures nsolid with assets disabled but snapshots enabled, then validates that snapshot operations fail with the appropriate error message when assets are disabled.
78-108: LGTM! Race condition properly addressed.The test correctly chains callbacks to ensure snapshots complete before toggling asset states, preventing the "Snapshot already in progress" race condition flagged in the previous review. The sequence properly:
- Waits for the error callback when assets are disabled (line 78)
- Enables assets only after the error is confirmed (line 83)
- Waits for the successful snapshot to complete before disabling assets (lines 86-88)
- Verifies the expected error after disabling (lines 90-97)
- Waits for the final snapshot to complete (line 103)
The use of
common.mustSucceed()ensures all expected callbacks are invoked, providing deterministic test behavior.test/parallel/test-nsolid-heap-profile-stream.js (1)
72-126: Major: Timing/synchronization concerns with nested setImmediate.The test relies on nested
setImmediatecallbacks to ensure config changes (nsolid.start(),enableAssets(),disableAssets()) take effect before attempting operations. If these configuration updates are truly asynchronous or involve internal state propagation, a singlesetImmediatetick might be insufficient, leading to race conditions and flaky test behavior.Verify whether the configuration methods are synchronous and whether
setImmediateprovides adequate synchronization:test/parallel/test-nsolid-heap-sampling.js (4)
164-167: LGTM: Formatting improvement for readability.The multi-line formatting of the error message assertion improves readability without changing the test logic.
174-175: Good: Consistent use of platform timeouts.Using
common.platformTimeout()ensures the test behaves correctly across different platform speeds and CI environments.
168-168: Good integration point for the new test.Calling
runHeapSamplingAssetsToggleTests()after the existing error path tests is a logical placement that extends the test coverage naturally.
177-248: Well-structured asset toggle test with timing considerations.The test comprehensively covers the asset enable/disable scenarios for heap sampling:
- Tests error paths when assets are disabled (both sync and async)
- Properly sequences operations to avoid mid-sampling disruption (line 216 only disables after sampling completes)
- Verifies successful sampling after re-enabling assets
However, the test relies heavily on
setTimeoutdelays (100ms) for configuration changes to propagate. Whilecommon.platformTimeout()helps, this could still be a source of flakiness on heavily loaded systems.Additionally, the test checks
err.code !== 0but doesn't verify the specific error code for the "assets disabled" scenario. According to the PR summary, anEAssetsDisablederror was added. Consider verifying the specific error code to make the test more precise and easier to debug if the wrong error is returned.Run the following script to check if there's a specific error code constant for assets disabled that should be verified:
test/parallel/test-nsolid-heap-sampling-stream.js (1)
138-138: LGTM!The integration point for the asset toggle tests is well-placed after validating the initial profile.
test/agents/test-grpc-tracing.mjs (2)
205-247: LGTM! Well-structured phase-based testing.The multi-phase flow (initial → disabled → reenabled → done) effectively validates that tracing can be toggled at runtime and that spans are properly suppressed when tracing is disabled.
273-315: LGTM! Consistent test coverage.The custom tracing test mirrors the http tracing test structure, ensuring comprehensive coverage of the enableTraces/disableTraces API across different span types.
test/agents/test-grpc-heap-profile.mjs (3)
259-296: LGTM! Proper error handling verification.The test correctly validates that heap profiling returns a 500 error with code 1008 when assets collection is disabled via the environment variable.
298-342: LGTM! Comprehensive config toggle validation.The test properly verifies that assetsEnabled can be toggled dynamically via nsolid.start() configuration and that the state changes affect profiling behavior as expected.
344-389: LGTM! Helper API coverage is complete.The test validates the enableAssets/disableAssets convenience methods and confirms the final state with a config check, ensuring the API surface is working correctly.
agents/grpc/src/grpc_errors.h (1)
12-13: LGTM! Properly defined error type.The new EAssetsDisabled error is correctly added to the GRPC_ERRORS macro with appropriate HTTP status code (500), message, and runtime code (1008) that aligns with the test expectations.
test/agents/test-grpc-continuous-profile.mjs (3)
121-156: LGTM! Proper verification of disabled continuous profiling.The test correctly validates that continuous CPU profiles are not emitted when assets collection is disabled via environment variable, using an appropriate wait period to confirm no emissions.
158-201: LGTM! Dynamic toggle behavior is well-tested.The test properly validates that continuous profiling can be stopped and restarted by toggling assetsEnabled via configuration, with appropriate tolerance for profiles in flight during state transitions.
203-245: LGTM! Helper methods are properly validated.The test confirms that the disableAssets/enableAssets convenience methods correctly control continuous profiling behavior and verifies the final configuration state.
test/common/nsolid-grpc-agent/index.js (1)
346-401: LGTM! Well-structured test client API.The new methods provide a clean interface for controlling assets and tracing at runtime. The centralized
#sendAndWaithelper promotes consistency and reduces duplication, with appropriate null checks for the child process.agents/grpc/src/proto/reconfigure.pb.cc (1)
1-1082: Auto-generated protobuf code.This file is automatically generated by the protocol buffer compiler from the proto definition. The changes correctly add support for the new
assetsEnabledfield (field 13) throughout the serialization, deserialization, copy, merge, and swap operations. The field is properly initialized tofalseand integrated into the has-bits mechanism.agents/grpc/src/proto/reconfigure.pb.h (1)
233-233: Auto-generated protobuf header.This file is automatically generated by the protocol buffer compiler. The changes correctly add the
assetsEnabledfield (field 13) with:
- Public field number enum constant
kAssetsEnabledFieldNumber = 13- Public accessors:
has_assetsenabled(),clear_assetsenabled(),assetsenabled(),set_assetsenabled()- Private internal helpers:
_internal_assetsenabled(),_internal_set_assetsenabled()- Private member variable
assetsenabled_in theImpl_structThe inline implementations follow standard protobuf patterns for optional boolean fields with proper has-bit management.
Also applies to: 392-401, 442-442, 1175-1201
test/parallel/test-nsolid-heap-profile.js (1)
124-124: Good placement for asset-toggle coverageRunning the toggle suite after the error-path chain is complete is sensible and avoids overlapping async state. LGTM.
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
22ac97c to
8a2bb66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
agents/grpc/src/grpc_errors.h (1)
12-13: New EAssetsDisabled enum looks correct; consider status code semantics.Runtime code 1008 fits sequence; macro continuation is correct. Optionally consider HTTP-ish code 403 (forbidden) or 409 (conflict) instead of 500 to better signal operator-controlled disablement; align with clients/tests before changing.
Please confirm clients/tests map 1008 → “Assets collection disabled” and no other components expect 500 here.
agents/grpc/src/grpc_agent.cc (1)
1194-1199: Type‑safety when reading JSON; store atomically.Guard against non-boolean values and prepare for atomic storage (see race comment below).
Apply:
- { - auto it = config_.find("assetsEnabled"); - if (it != config_.end()) { - assets_enabled_ = *it; - } - } + { + auto it = config_.find("assetsEnabled"); + if (it != config_.end()) { + if (it->is_boolean()) { + assets_enabled_.store(*it, std::memory_order_relaxed); + } else { + Debug("Invalid assetsEnabled type; expected boolean\n"); + } + } + }Note: assets_enabled_ change assumes std::atomic (see next comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(2 hunks)agents/zmq/src/zmq_agent.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- agents/zmq/src/zmq_agent.h
- agents/grpc/src/grpc_agent.h
- agents/zmq/src/zmq_agent.cc
- agents/grpc/proto/reconfigure.proto
🧰 Additional context used
🧬 Code graph analysis (3)
agents/grpc/src/grpc_agent.cc (1)
agents/zmq/src/zmq_agent.cc (10)
config(430-476)config(430-430)config(1263-1378)config(1263-1263)config(2760-2795)config(2760-2760)config(2888-2905)config(2888-2888)config(2983-3000)config(2983-2983)
agents/grpc/src/proto/reconfigure.pb.h (2)
agents/grpc/src/proto/command.pb.h (5)
internal(44-47)ReconfigureBody(452-582)ReconfigureBody(837-843)agents/grpc/src/proto/reconfigure.pb.cc (4)
ReconfigureBody(53-53)ReconfigureBody(267-270)_impl_(59-60)_impl_(85-86)
agents/grpc/src/proto/reconfigure.pb.cc (1)
agents/grpc/src/proto/reconfigure.pb.h (16)
ReconfigureBody(94-94)ReconfigureBody(95-96)ReconfigureBody(681-693)ReconfigureBody(694-698)ReconfigureBody(699-721)ReconfigureBody(722-726)ReconfigureBody(727-749)ReconfigureBody(750-754)ReconfigureBody(755-777)ReconfigureBody(778-782)ReconfigureBody(783-805)ReconfigureBody(806-810)ReconfigureBody(811-834)ReconfigureBody(836-841)ReconfigureBody(875-887)ReconfigureBody(890-903)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: build-tarball
- GitHub Check: coverage-windows
- GitHub Check: test-macOS
🔇 Additional comments (5)
agents/grpc/src/grpc_agent.cc (3)
395-398: Good: propagate assetsEnabled in ReconfigureEvent.Only sets when present; matches ZMQ agent behavior.
Ensure reconfigure.proto field 13 is optional and absent when not set to avoid default false flips.
447-448: Defaulting assets to enabled is fine; confirm env/config precedence.assets_enabled_ defaults true. Please confirm any NSOLID_ASSETS_ENABLED env or nsolid.start config is applied before first asset command to avoid a short window with the wrong default.
1755-1757: Good: include assetsEnabled in reconfigure output.Keeps parity with ZMQ agent.
agents/grpc/src/proto/reconfigure.pb.h (1)
233-234: Generated field assetsEnabled = 13 is wired correctly.Enum, accessors, has-bits (0x00000800U), and parse table size updates look consistent with protoc 6.32.0.
Please confirm these files were re-generated from the updated reconfigure.proto (no manual edits) and that build scripts pin protoc/runtime (6.32.0) to avoid cross-version CI drift.
Also applies to: 392-402, 408-446, 1175-1202
agents/grpc/src/proto/reconfigure.pb.cc (1)
49-51: Proto gencode changes align with assetsEnabled field; no manual edits suggested.Init/clear/serialize/merge paths and descriptor/table deltas reflect field 13; has-bits masks updated (incl. 0x00000de0 popcount).
Ensure descriptor size/hash changes are committed wherever cached (if any), and that both pb.h/pb.cc are produced by the same protoc (6.32.0).
Also applies to: 107-121, 152-173, 338-429, 465-469, 581-587, 624-625, 702-715, 738-744
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
ba2865a to
d4fdf79
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/nsolid.js (1)
392-416: Gating heapProfileStream when assets are disabled is correct; doc/param mismatch.The early throw with ERR_NSOLID_HEAP_PROFILE_START is consistent with the intended synchronous-state checks. However, the function signature now includes an
optionsargument that is neither documented nor used.Consider one of:
- Document
optionsin the JSDoc (even if currently unused), or- Drop the unused parameter to avoid confusion.
Example doc tweak:
/** * Starts a heap profile in a specific JS thread and returns a readable stream * of the profile data. * @param {number} threadId - The ID of the thread for which to start the heap profile. * @param {number} duration - The duration in milliseconds for which to run the heap profile. * @param {boolean} trackAllocations - Whether to track allocations during the heap profile. + * @param {object} [options] - Reserved for future use.Based on learnings.
agents/grpc/src/grpc_agent.cc (3)
569-575: Off-thread JSON read in stop(): snapshot contCpuProfile to atomic.stop(false-path) reads config_["contCpuProfile"] outside the agent loop. Replace with the atomic snapshot as above.
- if (profile_stopped) { - auto it = config_.find("contCpuProfile"); - if (it == config_.end() || it->get<bool>() == false) { - profile_on_exit_ = profile_stopped; - } - } + if (profile_stopped) { + if (!cont_cpu_profile_enabled_.load(std::memory_order_acquire)) { + profile_on_exit_ = true; + } + }
1487-1604: Continuous profiler ignores assetsEnabled at runtime.got_continuous_profile() streams profiles even when assetsEnabled is false. PR objective says continuous profiler must respect assetsEnabled. Add a fast gate and close any open streams.
void GrpcAgent::got_continuous_profile( const ProfileCollector::ProfileQStor& stor) { + if (!assets_enabled_.load(std::memory_order_acquire)) { + // Best-effort: close any existing stream for this thread and drop the data + auto it = cont_profile_stor_map_.find(std::visit([](auto& opt){ return opt.thread_id; }, stor.options)); + if (it != cont_profile_stor_map_.end()) { + it->second.stream->WritesDone(true); + cont_profile_stor_map_.erase(it); + } + return; + }
2222-2235: Be consistent with JSON type checks for disableSnapshots.Here disableSnapshots is read without is_boolean() check, unlike redactSnapshots. Align for safety.
- auto it = config_.find("disableSnapshots"); - if (it != config_.end()) { - disable_snapshots = *it; - } + auto it = config_.find("disableSnapshots"); + if (it != config_.end()) { + if (it->is_boolean()) { + disable_snapshots = *it; + } else { + Debug("Invalid disableSnapshots type; expected boolean\n"); + } + }
♻️ Duplicate comments (3)
test/parallel/test-nsolid-heap-sampling-stream.js (1)
193-200: About missing error handlers on streams.Noting prior discussion: adding
enabledStream.on('error', ...)was suggested earlier and intentionally declined. Acknowledging that decision here to avoid duplication.test/agents/test-zmq-tracing.mjs (1)
431-433: Good: immediate failure on spans while disabled.The explicit assertion to fail if spans arrive in the 'disabled' phase addresses the earlier review concern.
agents/grpc/src/grpc_agent.cc (1)
2080-2097: Fix data races: make assetsEnabled and contCpuProfile checks atomic and stop reading JSON off-thread.do_start_prof_init() is called from *from_js paths (non‑agent threads). It reads a non‑atomic assets_enabled and config_["contCpuProfile"] without synchronization. This is undefined behavior and can misgate profiling or crash under contention. Use atomics and snapshot contCpuProfile in config().
Apply these targeted changes:
@@ - if (!assets_enabled_) { + // Off-thread read; use atomic load with acquire semantics + if (!assets_enabled_.load(std::memory_order_acquire)) { return ErrorType::EAssetsDisabled; } @@ - if (type == ProfileType::kCpu) { - auto it = config_.find("contCpuProfile"); - if (it != config_.end() && it->get<bool>() == true) { - // Continuous CPU profiling is enabled, don't allow manual CPU profiles - return ErrorType::EInProgressError; - } - } + if (type == ProfileType::kCpu && + cont_cpu_profile_enabled_.load(std::memory_order_acquire)) { + // Continuous CPU profiling is enabled, don't allow manual CPU profiles + return ErrorType::EInProgressError; + }And in config() (see lines 1178–1183 and where contCpuProfile is handled) snapshot booleans into atomics:
@@ - { - auto it = config_.find("assetsEnabled"); - if (it != config_.end()) { - assets_enabled_ = *it; - } - } + { + auto it = config_.find("assetsEnabled"); + if (it != config_.end()) { + if (it->is_boolean()) { + assets_enabled_.store(*it, std::memory_order_release); + } else { + Debug("Invalid assetsEnabled type; expected boolean\n"); + } + } + } + { + auto it = config_.find("contCpuProfile"); + if (it != config_.end()) { + if (it->is_boolean()) { + cont_cpu_profile_enabled_.store(*it, std::memory_order_release); + } else { + Debug("Invalid contCpuProfile type; expected boolean\n"); + cont_cpu_profile_enabled_.store(false, std::memory_order_release); + } + } else { + cont_cpu_profile_enabled_.store(false, std::memory_order_release); + } + }Header additions (agents/grpc/src/grpc_agent.h):
+#include <atomic> @@ - bool assets_enabled_; + std::atomic<bool> assets_enabled_{true}; + std::atomic<bool> cont_cpu_profile_enabled_{false};
🧹 Nitpick comments (18)
lib/nsolid.js (1)
636-641: Minor JSDoc grammar nit.“Resumes the process metrics collection when exiting. works only if called...” — sentence casing is off. Optional fix:
- * It resumes the process metrics collection when exiting. - * works only if called from the + * Resumes the process metrics collection when exiting. + * Works only if called from thetest/parallel/test-nsolid-heap-sampling-stream.js (1)
171-228: Solid coverage for assets toggling; consider simplifying control flow.The sequence thoroughly verifies disabled→enabled→disabled→enabled behavior and schema checks. The nested setImmediate chain is a bit hard to follow; consider extracting small helpers or flattening to improve readability.
If binding.updateConfig propagation is asynchronous, a single setImmediate might race on slow machines. If flakiness appears, gate with a small retry/assert loop on
nsolid.config.assetsEnabledbefore invoking heapSamplingStream.test/parallel/test-nsolid-snapshot.js (1)
68-108: Prefer platformTimeout over setImmediate to avoid propagation flakesReconfig (enable/disable) may take longer than a tick. Use common.platformTimeout(100) like other tests to stabilize.
- setImmediate(() => { + setTimeout(() => { assert.throws( () => { nsolid.snapshot(); }, { message: 'Heap snapshot could not be generated' } ); nsolid.snapshot(common.mustCall((err) => { assert.notStrictEqual(err.code, 0); assert.strictEqual(err.message, 'Heap snapshot could not be generated'); // Only enable assets after the error callback completes nsolid.enableAssets(); - setImmediate(() => { + setTimeout(() => { // Wait for snapshot to complete before disabling assets nsolid.snapshot(common.mustSucceed(() => { // Only disable assets after snapshot completes nsolid.disableAssets(); - setImmediate(() => { + setTimeout(() => { assert.throws( () => { nsolid.snapshot(); }, { message: 'Heap snapshot could not be generated' } ); // Only enable assets after error is confirmed nsolid.enableAssets(); - setImmediate(() => { + setTimeout(() => { // Wait for final snapshot to complete nsolid.snapshot(common.mustSucceed()); - }); - }); + }, common.platformTimeout(100)); + }, common.platformTimeout(100)); })); - }); + }, common.platformTimeout(100)); })); - }); + }, common.platformTimeout(100));test/parallel/test-nsolid-heap-profile.js (1)
141-163: Reduce duplication with a small helper for “assets disabled” assertions (optional)The repeated assert.throws for start/stop can be centralized to improve readability.
+function assertHeapProfileDisabled() { + assert.throws(() => nsolid.heapProfile(), { message: 'Heap profile could not be started' }); + assert.throws(() => nsolid.heapProfileEnd(), { message: 'Heap profile could not be stopped' }); +} ... - assert.throws( - () => { - nsolid.heapProfile(); - }, - { message: 'Heap profile could not be started' } - ); + assertHeapProfileDisabled(); ... - assert.throws( - () => { - nsolid.heapProfile(); - }, - { message: 'Heap profile could not be started' } - ); - - assert.throws( - () => { - nsolid.heapProfileEnd(); - }, - { message: 'Heap profile could not be stopped' } - ); + assertHeapProfileDisabled();Also applies to: 173-199
test/parallel/test-nsolid-heap-sampling.js (1)
185-207: Optional: extract a helper to assert “sampling disabled”Refactor repeated throws to a helper for clarity.
+function assertHeapSamplingDisabled() { + assert.throws(() => nsolid.heapSampling(), { message: 'Heap sampling could not be started' }); + assert.throws(() => nsolid.heapSamplingEnd(), { message: 'Heap sampling could not be stopped' }); +} ... - assert.throws( - () => { - nsolid.heapSampling(); - }, - { message: 'Heap sampling could not be started' } - ); + assertHeapSamplingDisabled(); ... - assert.throws( - () => { - nsolid.heapSampling(); - }, - { message: 'Heap sampling could not be started' } - ); - - assert.throws( - () => { - nsolid.heapSamplingEnd(); - }, - { message: 'Heap sampling could not be stopped' } - ); + assertHeapSamplingDisabled();Also applies to: 217-243
test/parallel/test-nsolid-profile.js (1)
152-177: Optional: also assert profileEnd() error after second disableFor parity with other suites, add a stop assertion when assets are disabled.
setTimeout(() => { assert.throws( () => { nsolid.profile(); }, { message: 'CPU profile could not be started' } ); + assert.throws( + () => { + nsolid.profileEnd(); + }, + { + message: 'CPU profile could not be stopped' + } + ); + // Only re-enable after error is confirmed nsolid.enableAssets();test/agents/test-grpc-profile.mjs (1)
322-364: Add a test toggling assets to false during an active profile (recommended)To guarantee in‑flight profiles aren’t aborted by reconfigure, start a profile, then disable assets, assert the ongoing one completes and subsequent starts return 500.
+tests.push({ + name: 'should not abort an in-flight profile when assets are disabled mid-run', + test: async (getEnv) => { + return new Promise((resolve) => { + const grpcServer = new GRPCServer(); + grpcServer.start(mustSucceed(async (port) => { + const env = getEnv(port); + const child = new TestClient([], { stdio: ['inherit','inherit','inherit','ipc'], env }); + const agentId = await child.id(); + const opts = { duration: 200, threadId: 0 }; + + const first = grpcServer.cpuProfile(agentId, opts); // start + await child.disableAssets(); // toggle off mid-run + + // in-flight should still resolve with data + const { data, requestId } = await first; + checkProfileData(data.msg, data.metadata, requestId, agentId, opts, true); + + // new starts should be rejected + const disabled = await grpcServer.cpuProfile(agentId, opts); + checkProfileError(disabled.data.msg, disabled.data.metadata, disabled.requestId, + agentId, 500, 'Assets collection disabled(1008)'); + + await child.shutdown(0); + grpcServer.close(); + resolve(); + })); + }); + }, +});test/agents/test-grpc-snapshot.mjs (1)
246-280: Harden toggle tests against races and brittle string matches.
- Verify assetsEnabled state right after toggles before issuing the RPC to avoid timing races.
- Match the error message more defensively (keep code strict; loosen message equality).
-const { data, requestId } = await grpcServer.heapSnapshot(agentId, options); +// ensure disabled took effect before attempting +const cfg = await child.config(); +assert.strictEqual(cfg.assetsEnabled, false); +const { data, requestId } = await grpcServer.heapSnapshot(agentId, options);-checkSnapshotError(disabledResult.data.msg, - disabledResult.data.metadata, - disabledResult.requestId, - agentId, - 500, - 'Assets collection disabled(1008)'); +checkSnapshotError( + disabledResult.data.msg, + disabledResult.data.metadata, + disabledResult.requestId, + agentId, + 500, + disabledResult.data.msg.common.error.message // rely on server; assert code below +); +assert.ok( + disabledResult.data.msg.common.error.message.includes('(1008)'), + 'Expected EAssetsDisabled (1008)' +);If you prefer to keep equality, at least assert the numeric code and use includes for the suffix to reduce brittleness. Please confirm that
child.disableAssets()andchild.enableAssets()return after the setting is applied; if not, the addedchild.config()check is needed. [Based on learnings]Also applies to: 282-323, 325-367
test/agents/test-grpc-heap-sampling.mjs (1)
248-285: Stabilize assets gating assertions (avoid races, reduce brittleness).
- After disabling via config/helpers, fetch
child.config()and assertassetsEnabled === falsebefore callingheapSampling.- Keep strict on HTTP status/code, but use
.includes('(1008)')for the message to avoid brittle equality.-const disabledResult = await grpcServer.heapSampling(agentId, profileOpts); +const cfg = await child.config(); +assert.strictEqual(cfg.assetsEnabled, false); +const disabledResult = await grpcServer.heapSampling(agentId, profileOpts);-checkProfileError(disabledResult.data.msg, - disabledResult.data.metadata, - disabledResult.requestId, - agentId, - 500, - 'Assets collection disabled(1008)'); +checkProfileError( + disabledResult.data.msg, + disabledResult.data.metadata, + disabledResult.requestId, + agentId, + 500, + disabledResult.data.msg.common.error.message +); +assert.ok( + disabledResult.data.msg.common.error.message.includes('(1008)'), + 'Expected EAssetsDisabled (1008)' +);Also applies to: 287-331, 333-378
test/agents/test-grpc-continuous-profile.mjs (4)
121-156: Consider a slightly longer quiet window to reduce CI flakes.Twice the interval plus margin (e.g., 350–500ms) is safer than 300ms under load.
- await setTimeout(300); + await setTimeout(400); // ~4×100ms intervals incl. jitter
158-201: Verify toggle applied and use adaptive assertions.
- Confirm
assetsEnabled === falseafterchild.config({ assetsEnabled: false })before measuring emissions.- Use a time window of ≥3 intervals to observe cessation/restart.
- await child.config({ assetsEnabled: false }); + const disabledCfg = await child.config({ assetsEnabled: false }); + assert.strictEqual(disabledCfg.assetsEnabled, false); @@ - await setTimeout(250); + await setTimeout(350); @@ - await child.config({ assetsEnabled: true }); + const enabledCfg = await child.config({ assetsEnabled: true }); + assert.strictEqual(enabledCfg.assetsEnabled, true); - await setTimeout(250); + await setTimeout(350);
203-245: Do the same verification for enableAssets()/disableAssets().Add config checks after each toggle and modestly increase windows.
- await child.disableAssets(); + await child.disableAssets(); + const cfg1 = await child.config(); + assert.strictEqual(cfg1.assetsEnabled, false); @@ - await setTimeout(250); + await setTimeout(350); @@ - await child.enableAssets(); + await child.enableAssets(); + const cfg2 = await child.config(); + assert.strictEqual(cfg2.assetsEnabled, true); - await setTimeout(250); + await setTimeout(350);
429-449: Precondition check: ensure continuous profiling is enabled.Before attempting the manual CPU profile conflict, assert the runtime config has continuous profiling enabled to avoid environment coupling.
- // Wait for continuous profiling to start - await setTimeout(400); + // Wait for continuous profiling to start and verify it's enabled + await setTimeout(400); + const cfg = await child.config(); + assert.strictEqual(cfg.contCpuProfile, true);test/agents/test-grpc-heap-profile.mjs (1)
259-390: Make assets gating tests race‑resistant and less brittle on messages.
- Assert
assetsEnabledpost‑toggle before issuingheapProfile.- Keep strict on status/error code; relax message match to
.includes('(1008)').-const { data, requestId } = await grpcServer.heapProfile(agentId, options); +const cfg = await child.config(); +assert.strictEqual(cfg.assetsEnabled, false); +const { data, requestId } = await grpcServer.heapProfile(agentId, options);-checkProfileError(disabledResult.data.msg, - disabledResult.data.metadata, - disabledResult.requestId, - agentId, - 500, - 'Assets collection disabled(1008)'); +checkProfileError( + disabledResult.data.msg, + disabledResult.data.metadata, + disabledResult.requestId, + agentId, + 500, + disabledResult.data.msg.common.error.message +); +assert.ok( + disabledResult.data.msg.common.error.message.includes('(1008)'), + 'Expected EAssetsDisabled (1008)' +);agents/grpc/src/grpc_agent.cc (1)
72-73: Nit: GRPC_MAX_SIZE comment is wrong.4L10241024 is 4 MiB, not 4 GB. Fix the comment or the constant.
test/common/nsolid-grpc-agent/index.js (3)
357-379: Helper symmetry is good; consider returning ack for assertions.enableAssets/disableAssets/enableTraces/disableTraces fire-and-wait but discard result. Returning a boolean ack would help tests assert success.
389-401: Add correlation and timeout to #sendAndWait to avoid cross-talk and hangs.Multiple in-flight calls of the same type can race, and missing responses can hang forever. Include a requestId and a timeout.
Example change:
-#sendAndWait({ type, payload = {}, expect = type, map }) { - return new Promise((resolve) => { - const handler = (msg) => { - if (msg.type !== expect) return; - this.#child.off('message', handler); - resolve(map ? map(msg) : undefined); - }; - this.#child.on('message', handler); - this.#child.send({ type, ...payload }); - }); -} +#sendAndWait({ type, payload = {}, expect = type, map, timeoutMs = 5000 }) { + return new Promise((resolve, reject) => { + const requestId = randomUUID(); + const handler = (msg) => { + if (msg.type !== expect || msg.requestId !== requestId) return; + clearTimeout(timer); + this.#child.off('message', handler); + resolve(map ? map(msg) : undefined); + }; + const timer = setTimeout(() => { + this.#child.off('message', handler); + reject(new Error(`Timeout waiting for ${expect}`)); + }, timeoutMs); + this.#child.on('message', handler); + this.#child.send({ type, requestId, ...payload }); + }); +}Note: requires client.js to echo requestId. Do you want a follow-up patch for client.js?
381-387: Duplicate trace APIs; consider consolidating.trace() and tracing() overlap. Prefer one name and deprecate the other to reduce confusion.
Also applies to: 593-603
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(2 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(6 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- test/common/nsolid-zmq-agent/index.js
- test/parallel/test-nsolid-heap-profile-stream.js
- test/common/nsolid-zmq-agent/client.js
- test/agents/test-zmq-snapshot.mjs
- test/agents/test-zmq-profile.mjs
- src/nsolid/nsolid_api.cc
- test/common/nsolid-grpc-agent/client.js
- agents/grpc/proto/reconfigure.proto
- agents/grpc/src/grpc_agent.h
- test/agents/test-zmq-heap-profile.mjs
- test/agents/test-grpc-tracing.mjs
- agents/zmq/src/zmq_agent.cc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T10:18:23.123Z
Learnt from: santigimeno
PR: nodesource/nsolid#377
File: test/parallel/test-nsolid-heap-profile-stream.js:81-88
Timestamp: 2025-10-14T10:18:23.123Z
Learning: In nsolid.heapProfileStream() and nsolid.heapSamplingStream(), synchronous throws occur for argument validation errors and immediate state validation checks (like assets being disabled), while async error events are emitted for runtime errors that manifest during execution (like invalid threadId).
Applied to files:
test/parallel/test-nsolid-heap-sampling-stream.jslib/nsolid.js
🧬 Code graph analysis (15)
test/parallel/test-nsolid-heap-sampling.js (5)
test/common/nsolid-grpc-agent/index.js (2)
assert(4-4)common(3-3)test/common/nsolid-zmq-agent/index.js (2)
assert(4-4)common(3-3)lib/nsolid.js (2)
err(444-444)err(532-532)test/common/nsolid-grpc-agent/client.js (1)
nsolid(7-7)test/common/nsolid-zmq-agent/client.js (1)
nsolid(6-6)
test/parallel/test-nsolid-profile.js (4)
test/common/nsolid-grpc-agent/index.js (2)
assert(4-4)common(3-3)test/parallel/test-nsolid-heap-profile.js (3)
assert(4-4)common(3-3)nsolid(5-5)test/parallel/test-nsolid-heap-sampling.js (3)
assert(4-4)common(3-3)nsolid(5-5)test/common/nsolid-zmq-agent/client.js (1)
nsolid(6-6)
test/agents/test-zmq-heap-sampling.mjs (1)
test/agents/test-zmq-heap-profile.mjs (19)
playground(491-491)profile(43-43)profile(98-98)profile(252-252)profile(298-298)profile(347-347)profile(415-415)bootstrapOpts(51-54)bootstrapOpts(105-109)bootstrapOpts(259-262)bootstrapOpts(304-308)bootstrapOpts(356-359)bootstrapOpts(426-429)disabledConfig(362-362)enabledConfig(373-373)heapProf(75-75)heapProf(133-133)heapProf(395-395)currentConfig(400-400)
test/agents/test-grpc-heap-sampling.mjs (2)
test/agents/test-grpc-heap-profile.mjs (63)
grpcServer(88-88)grpcServer(113-113)grpcServer(127-127)grpcServer(154-154)grpcServer(169-169)grpcServer(183-183)grpcServer(197-197)grpcServer(218-218)grpcServer(229-229)grpcServer(252-252)grpcServer(263-263)grpcServer(283-283)grpcServer(302-302)grpcServer(333-333)grpcServer(348-348)grpcServer(377-377)env(90-90)env(129-129)env(171-171)env(199-199)env(231-231)env(265-268)env(304-304)env(350-350)env(408-408)env(446-446)opts(91-94)opts(130-133)opts(172-175)opts(200-203)opts(232-235)opts(269-272)opts(305-308)opts(351-354)opts(410-413)opts(447-450)child(95-95)child(134-134)child(176-176)child(204-204)child(236-236)child(273-273)child(309-309)child(355-355)child(414-414)child(451-451)agentId(96-96)agentId(135-135)agentId(177-177)agentId(205-205)agentId(237-237)agentId(274-274)agentId(310-310)agentId(356-356)agentId(415-415)agentId(460-460)profileOpts(311-317)profileOpts(357-363)disabledConfig(319-319)disabledResult(322-322)disabledResult(367-367)enabledConfig(330-330)currentConfig(380-380)test/agents/test-grpc-profile.mjs (31)
env(80-80)env(116-116)env(154-154)env(182-182)env(214-214)env(249-252)opts(81-84)opts(117-120)opts(155-158)opts(183-186)opts(215-218)opts(253-256)child(85-85)child(121-121)child(159-159)child(187-187)child(220-220)child(257-257)agentId(86-86)agentId(122-122)agentId(160-160)agentId(188-188)agentId(221-221)agentId(258-258)profileOpts(292-295)profileOpts(335-338)disabledConfig(297-297)disabledResult(300-300)disabledResult(342-342)enabledConfig(308-308)currentConfig(355-355)
test/parallel/test-nsolid-heap-profile.js (2)
test/parallel/test-nsolid-heap-profile-stream.js (3)
common(4-4)nsolid(6-6)assert(5-5)lib/nsolid.js (2)
err(444-444)err(532-532)
test/agents/test-grpc-profile.mjs (3)
test/agents/test-grpc-heap-profile.mjs (63)
grpcServer(88-88)grpcServer(113-113)grpcServer(127-127)grpcServer(154-154)grpcServer(169-169)grpcServer(183-183)grpcServer(197-197)grpcServer(218-218)grpcServer(229-229)grpcServer(252-252)grpcServer(263-263)grpcServer(283-283)grpcServer(302-302)grpcServer(333-333)grpcServer(348-348)grpcServer(377-377)env(90-90)env(129-129)env(171-171)env(199-199)env(231-231)env(265-268)env(304-304)env(350-350)env(408-408)env(446-446)opts(91-94)opts(130-133)opts(172-175)opts(200-203)opts(232-235)opts(269-272)opts(305-308)opts(351-354)opts(410-413)opts(447-450)child(95-95)child(134-134)child(176-176)child(204-204)child(236-236)child(273-273)child(309-309)child(355-355)child(414-414)child(451-451)agentId(96-96)agentId(135-135)agentId(177-177)agentId(205-205)agentId(237-237)agentId(274-274)agentId(310-310)agentId(356-356)agentId(415-415)agentId(460-460)profileOpts(311-317)profileOpts(357-363)disabledConfig(319-319)disabledResult(322-322)disabledResult(367-367)enabledConfig(330-330)currentConfig(380-380)test/agents/test-grpc-heap-sampling.mjs (31)
env(79-79)env(118-118)env(159-159)env(187-187)env(219-219)env(254-257)opts(80-83)opts(119-122)opts(160-163)opts(188-191)opts(220-223)opts(258-261)child(84-84)child(123-123)child(164-164)child(192-192)child(225-225)child(262-262)agentId(85-85)agentId(124-124)agentId(165-165)agentId(193-193)agentId(226-226)agentId(263-263)profileOpts(300-306)profileOpts(346-352)disabledConfig(308-308)disabledResult(311-311)disabledResult(356-356)enabledConfig(319-319)currentConfig(369-369)test/agents/test-grpc-snapshot.mjs (5)
disabledConfig(300-300)disabledResult(303-303)disabledResult(345-345)enabledConfig(311-311)currentConfig(358-358)
agents/grpc/src/grpc_agent.cc (1)
agents/zmq/src/zmq_agent.cc (10)
config(430-476)config(430-430)config(1263-1378)config(1263-1263)config(2760-2795)config(2760-2760)config(2888-2905)config(2888-2888)config(2983-3000)config(2983-2983)
test/parallel/test-nsolid-heap-sampling-stream.js (4)
test/common/nsolid-grpc-agent/client.js (1)
nsolid(7-7)test/common/nsolid-zmq-agent/client.js (1)
nsolid(6-6)test/parallel/test-nsolid-heap-profile-stream.js (2)
nsolid(6-6)common(4-4)test/parallel/test-nsolid-heap-sampling.js (2)
nsolid(5-5)common(3-3)
test/agents/test-grpc-snapshot.mjs (2)
test/agents/test-grpc-heap-profile.mjs (61)
grpcServer(88-88)grpcServer(113-113)grpcServer(127-127)grpcServer(154-154)grpcServer(169-169)grpcServer(183-183)grpcServer(197-197)grpcServer(218-218)grpcServer(229-229)grpcServer(252-252)grpcServer(263-263)grpcServer(283-283)grpcServer(302-302)grpcServer(333-333)grpcServer(348-348)grpcServer(377-377)env(90-90)env(129-129)env(171-171)env(199-199)env(231-231)env(265-268)env(304-304)env(350-350)env(408-408)env(446-446)opts(91-94)opts(130-133)opts(172-175)opts(200-203)opts(232-235)opts(269-272)opts(305-308)opts(351-354)opts(410-413)opts(447-450)child(95-95)child(134-134)child(176-176)child(204-204)child(236-236)child(273-273)child(309-309)child(355-355)child(414-414)child(451-451)agentId(96-96)agentId(135-135)agentId(177-177)agentId(205-205)agentId(237-237)agentId(274-274)agentId(310-310)agentId(356-356)agentId(415-415)agentId(460-460)disabledConfig(319-319)disabledResult(322-322)disabledResult(367-367)enabledConfig(330-330)currentConfig(380-380)test/agents/test-grpc-heap-sampling.mjs (29)
env(79-79)env(118-118)env(159-159)env(187-187)env(219-219)env(254-257)opts(80-83)opts(119-122)opts(160-163)opts(188-191)opts(220-223)opts(258-261)child(84-84)child(123-123)child(164-164)child(192-192)child(225-225)child(262-262)agentId(85-85)agentId(124-124)agentId(165-165)agentId(193-193)agentId(226-226)agentId(263-263)disabledConfig(308-308)disabledResult(311-311)disabledResult(356-356)enabledConfig(319-319)currentConfig(369-369)
test/agents/test-grpc-heap-profile.mjs (2)
test/agents/test-grpc-heap-sampling.mjs (53)
grpcServer(77-77)grpcServer(102-102)grpcServer(116-116)grpcServer(143-143)grpcServer(157-157)grpcServer(171-171)grpcServer(185-185)grpcServer(206-206)grpcServer(217-217)grpcServer(241-241)grpcServer(252-252)grpcServer(272-272)grpcServer(291-291)grpcServer(322-322)grpcServer(337-337)grpcServer(366-366)env(79-79)env(118-118)env(159-159)env(187-187)env(219-219)env(254-257)env(293-293)env(339-339)env(392-392)env(424-424)child(84-84)child(123-123)child(164-164)child(192-192)child(225-225)child(262-262)child(298-298)child(344-344)child(397-397)child(429-429)agentId(85-85)agentId(124-124)agentId(165-165)agentId(193-193)agentId(226-226)agentId(263-263)agentId(299-299)agentId(345-345)agentId(398-398)agentId(438-438)profileOpts(300-306)profileOpts(346-352)disabledConfig(308-308)disabledResult(311-311)disabledResult(356-356)enabledConfig(319-319)currentConfig(369-369)test/agents/test-grpc-profile.mjs (25)
env(80-80)env(116-116)env(154-154)env(182-182)env(214-214)env(249-252)child(85-85)child(121-121)child(159-159)child(187-187)child(220-220)child(257-257)agentId(86-86)agentId(122-122)agentId(160-160)agentId(188-188)agentId(221-221)agentId(258-258)profileOpts(292-295)profileOpts(335-338)disabledConfig(297-297)disabledResult(300-300)disabledResult(342-342)enabledConfig(308-308)currentConfig(355-355)
lib/nsolid.js (1)
agents/grpc/src/grpc_agent.cc (2)
config(1000-1186)config(1000-1000)
agents/grpc/src/proto/reconfigure.pb.h (2)
agents/grpc/src/proto/command.pb.h (3)
internal(44-47)ReconfigureBody(452-582)ReconfigureBody(837-843)agents/grpc/src/proto/reconfigure.pb.cc (4)
ReconfigureBody(53-53)ReconfigureBody(267-270)_impl_(59-60)_impl_(85-86)
test/parallel/test-nsolid-snapshot.js (2)
agents/grpc/src/grpc_errors.h (1)
nsolid(16-33)lib/nsolid.js (2)
err(444-444)err(532-532)
agents/grpc/src/proto/reconfigure.pb.cc (1)
agents/grpc/src/proto/reconfigure.pb.h (16)
ReconfigureBody(94-94)ReconfigureBody(95-96)ReconfigureBody(681-693)ReconfigureBody(694-698)ReconfigureBody(699-721)ReconfigureBody(722-726)ReconfigureBody(727-749)ReconfigureBody(750-754)ReconfigureBody(755-777)ReconfigureBody(778-782)ReconfigureBody(783-805)ReconfigureBody(806-810)ReconfigureBody(811-834)ReconfigureBody(836-841)ReconfigureBody(875-887)ReconfigureBody(890-903)
test/agents/test-grpc-continuous-profile.mjs (2)
test/agents/test-grpc-heap-profile.mjs (30)
env(90-90)env(129-129)env(171-171)env(199-199)env(231-231)env(265-268)env(304-304)env(350-350)env(408-408)env(446-446)opts(91-94)opts(130-133)opts(172-175)opts(200-203)opts(232-235)opts(269-272)opts(305-308)opts(351-354)opts(410-413)opts(447-450)agentId(96-96)agentId(135-135)agentId(177-177)agentId(205-205)agentId(237-237)agentId(274-274)agentId(310-310)agentId(356-356)agentId(415-415)agentId(460-460)test/common/nsolid-grpc-agent/server.mjs (2)
opts(20-27)agentId(67-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-tarball
- GitHub Check: lint-js-and-md
- GitHub Check: test-macOS
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-windows
🔇 Additional comments (20)
test/agents/test-zmq-heap-sampling.mjs (1)
135-201: LGTM! Comprehensive test coverage for assetsEnabled toggle.The test thoroughly validates the runtime asset control feature:
- Properly verifies that heap sampling is rejected with a 422 error when
assetsEnabledisfalse- Confirms successful sampling after re-enabling assets
- Validates profile structure and final configuration state
The
resolvedflag (lines 142, 170-171) is a good defensive pattern to prevent race conditions from multiple event emissions after test completion.agents/grpc/src/grpc_errors.h (1)
12-14: New error EAssetsDisabled wired correctly.Macro continuation and enum expansion look good; this aligns with assets gating behavior and code 1008.
lib/nsolid.js (8)
133-137: Public toggle APIs exposed on start object look good.enableAssets/disableAssets and enableTraces/disableTraces are clearly surfaced and follow existing conventions.
195-207: Config getters with sensible defaults.assetsEnabled defaulting to true and tracingEnabled to false matches initializeConfig. Good for pre-start behavior and user ergonomics.
497-499: heapSamplingStream gating matches behavior expectations.Early throw keeps semantics consistent with argument validation ordering in these APIs.
604-617: enableAssets/disableAssets helpers are idempotent and minimal.Early returns avoid redundant config churn. Nice.
620-633: enableTraces/disableTraces helpers mirror assets helpers.Looks consistent and straightforward.
761-766: Config normalization for assetsEnabled is correct.optionToBool with undefined guard prevents accidental clearing; good precedence handling.
947-957: Env/pkg precedence and default(true) for assetsEnabled are aligned with native.Good: env > pkg > default(true). Matches expected operator ergonomics.
1106-1116: optionToBool is robust across types.Covers undefined/null, boolean, number, and string via existing envToBool semantics.
Do we intentionally leave profile(), profileEnd(), and snapshot() ungated at the JS layer, relying solely on native errors? If parity is desired, we can mirror the early-throw pattern there as well. Happy to draft that if needed.
test/parallel/test-nsolid-config-tracing.js (1)
24-29: Runtime tracing helpers keep config in sync — LGTM.This verifies helper-driven toggles reflect in
nsolid.config.tracingEnabled, matching the tracer flag recompute in agents (see GrpcAgent config path).test/parallel/test-nsolid-snapshot.js (1)
82-108: Good: toggling chained to snapshot completion avoids in‑flight racesSequencing disableAssets() only after snapshot callback completes is correct and resolves prior race.
test/parallel/test-nsolid-heap-profile.js (1)
133-204: Asset toggle flow is correct and race‑safeEnable → run → end → disable sequencing with platformTimeouts looks good.
test/parallel/test-nsolid-heap-sampling.js (1)
177-248: Sequencing and assertions look solidRe-enable, run to completion, then disable with proper waits—LGTM.
test/agents/test-grpc-profile.mjs (2)
243-277: Env‑driven disablement test is clear and correctValidates NSOLID_ASSETS_ENABLED=0 returns 500 with expected message.
279-320: Config toggle path is well‑coveredAsserts disabled then enabled via config before invoking RPCs—LGTM.
agents/grpc/src/grpc_agent.cc (1)
379-382: LGTM: reconfigure payload round-trips assetsEnabled.PopulateReconfigureEvent and reconfigure() now propagate assetsEnabled. Matches proto additions.
Also applies to: 1739-1742
test/common/nsolid-grpc-agent/index.js (1)
346-355: config(): good consolidation via #sendAndWait.API looks fine; returns the mapped config payload.
agents/grpc/src/proto/reconfigure.pb.h (1)
233-235: LGTM: assetsEnabled field (tag 13) added consistently.Field number, accessors, has-bits mask, and parse table size updated coherently.
Also applies to: 392-402, 408-413, 1175-1202
agents/grpc/src/proto/reconfigure.pb.cc (1)
49-51: LGTM: generated code matches proto changes for assetsEnabled.Offsets, masks, serialization, merge, and size accounting include field 13 correctly.
Also applies to: 107-121, 339-349, 393-398, 624-661, 702-716
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
configure.py (2)
2322-2324: Consider verifying script existence before execution.While the bash availability check is good, the code doesn't verify that the
apply-patches.shscript actually exists before attempting to run it. If the script is missing,subprocess.runwill fail with a potentially unclear error message.Consider this improvement:
# Check if we can run bash scripts (available on Unix-like systems and Windows with git/bash) bash_available = shutil.which('bash') is not None -if bash_available: +if bash_available and script.is_file(): subprocess.run(['bash', str(script)], check=True) +elif bash_available: + warn(f'V8 patch script not found: {script}') else: warn('bash not found - skipping V8 patches. Make sure git bash or similar is available on Windows.')
2325-2326: Warning message could be more informative about consequences.The warning informs users that bash is missing but doesn't explain the potential impact of skipping V8 patches. Consider making it clearer what functionality might be affected.
For example:
else: - warn('bash not found - skipping V8 patches. Make sure git bash or similar is available on Windows.') + warn('bash not found - skipping V8 patches. V8 may not function correctly without patches. On Windows, install Git Bash or WSL to apply patches.')agents/grpc/src/grpc_utils.h (4)
4-6: Add missing standard headers for used symbols.
std::stringandstd::snprintfare used but<string>and<cstdio>are not included here. Avoid relying on transitive includes from other headers.#include "debug_utils-inl.h" #include "google/protobuf/util/json_util.h" #include "nlohmann/json.hpp" +#include <cstdio> +#include <string>
8-10: Avoid globalusingin headers; fully qualify protobuf types/functions.Placing
using google::protobuf::Messageandusing google::protobuf::util::MessageToJsonStringat global scope in a header pollutes all including translation units and risks symbol collisions. Prefer fully qualified names locally.Remove the global using-declarations:
-using google::protobuf::Message; -using google::protobuf::util::MessageToJsonString;Fully qualify in the API and call site:
-inline void DebugProtobufMsg(const char* format, - const Message& msg, +inline void DebugProtobufMsg(const char* format, + const google::protobuf::Message& msg, Args&&... args) { if (per_process::enabled_debug_list.enabled( DebugCategory::NSOLID_GRPC_AGENT)) { std::string json_payload; - const auto status = MessageToJsonString(msg, &json_payload); + const auto status = + google::protobuf::util::MessageToJsonString(msg, &json_payload);Also applies to: 29-36
21-26: Stabilize JSON string lifetime before logging.Store
msg.dump(4)in a localstd::stringbefore passing.c_str()to the logger to avoid any surprises if the logger defers formatting.inline void DebugJSON(const char* str, const nlohmann::json& msg) { if (per_process::enabled_debug_list.enabled( DebugCategory::NSOLID_GRPC_AGENT)) { - Debug(str, msg.dump(4).c_str()); + const std::string dumped = msg.dump(4); + Debug(str, dumped.c_str()); } }
35-49: Harden formatting and error path; handle truncation and null format.
snprintfresult is unchecked; long prefixes silently truncate to 512 bytes.- Assigning
formatdirectly tostd::stringwhen no args assumesformat != nullptr.- On protobuf->JSON failure, nothing is logged; add a clear failure log.
std::string json_payload; - const auto status = MessageToJsonString(msg, &json_payload); + const auto status = + google::protobuf::util::MessageToJsonString(msg, &json_payload); if (status.ok()) { // Build complete message with format args + JSON std::string formatted_prefix; if constexpr (sizeof...(args) > 0) { - char buffer[512]; - snprintf(buffer, sizeof(buffer), format, std::forward<Args>(args)...); - formatted_prefix = buffer; + char buffer[512]; + int n = std::snprintf(buffer, + sizeof(buffer), + format ? format : "", + std::forward<Args>(args)...); + if (n < 0) { + formatted_prefix.clear(); + } else if (static_cast<size_t>(n) >= sizeof(buffer)) { + buffer[sizeof(buffer) - 1] = '\0'; + formatted_prefix.assign(buffer); + formatted_prefix += "… (truncated)"; + } else { + formatted_prefix.assign(buffer); + } } else { - formatted_prefix = format; + formatted_prefix = (format != nullptr) ? format : ""; } Debug("%s%s\n", formatted_prefix.c_str(), json_payload.c_str()); return; - } + } else { + const std::string status_msg = status.ToString(); + Debug("Protobuf->JSON conversion failed for '%s': %s\n", + (format ? format : ""), + status_msg.c_str()); + }If the project already uses a safer formatter (e.g., {fmt}), I can switch this to
fmt::formatfor type‑safe, dynamic sizing. Confirm preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
agents/grpc/src/grpc_utils.h(1 hunks)configure.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
agents/grpc/src/grpc_utils.h (1)
src/debug_utils.h (1)
DebugCategory(67-71)
🪛 Ruff (0.14.1)
configure.py
2324-2324: subprocess call: check for execution of untrusted input
(S603)
2324-2324: Starting a process with a partial executable path
(S607)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-windows
🔇 Additional comments (2)
agents/grpc/src/grpc_utils.h (2)
15-19: Nice wrapper; forwards category consistently.The variadic
Debughelper cleanly scopes logs toNSOLID_GRPC_AGENT. No issues spotted.
28-33: Build standard check — OK (C++17+ enforced).agents/grpc sources are built from node.gyp (agents/grpc/* listed there) and the project gyp includes common_node.gypi which sets cflags_cc / CLANG_CXX_LANGUAGE_STANDARD to gnu++20, so this TU will be compiled with C++20 (>= C++17).
(node.gyp; common_node.gypi).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
configure.py (1)
2321-2333: LGTM! Defensive approach improves robustness.The availability checks for bash and git before running the V8 patches script are a good defensive programming practice, allowing the configure process to gracefully handle environments where these tools are missing.
Optional improvement: Consider using the full paths returned by
shutil.which()in the subprocess call for consistency and slightly better security posture:bash_available = shutil.which('bash') is not None git_available = shutil.which('git') is not None if bash_available and git_available: - subprocess.run(['bash', str(script)], check=True) + bash_path = shutil.which('bash') + subprocess.run([bash_path, str(script)], check=True) else:Note: The static analysis warnings (S603, S607) can be safely ignored here—the script path is from a known repository location, and using 'bash' from PATH is standard practice for configuration scripts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
configure.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.1)
configure.py
2326-2326: subprocess call: check for execution of untrusted input
(S603)
2326-2326: Starting a process with a partial executable path
(S607)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: build-tarball
- GitHub Check: test-macOS
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/agents/test-grpc-blocked-loop.mjs(2 hunks)test/common/nsolid-grpc-agent/index.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-macOS
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-windows
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
🔇 Additional comments (3)
test/common/nsolid-grpc-agent/index.js (2)
346-387: New API methods look good.The new methods (
config,enableAssets,disableAssets,enableTraces,disableTraces,trace) follow consistent patterns and properly check for the child process before sending messages. The refactoring ofconfigto use#sendAndWaitimproves code maintainability.Note: The
trace()method correctly uses a different pattern than the enable/disable methods because it waits for send completion rather than a response message.
18-18: The validation change for requestId is intentional and correct.Based on verification of the codebase, this change reflects proper request tracking. The test file generates requestIds as UUIDs (using
randomUUID()) and uses them to match responses. Validating thatrequestIdis a string (rather than asserting it must be empty) aligns with actual usage where requestIds contain unique identifiers for tracking operations like profiling, snapshots, and metrics collection.test/agents/test-grpc-blocked-loop.mjs (1)
72-72: Verify the validation relaxation for requestId is consistent with expected behavior.Both
checkBlockedLoopDataandcheckUnblockedLoopDatanow validaterequestIdas any string instead of asserting it equals an empty string. This matches the change intest/common/nsolid-grpc-agent/index.js(line 18).Please confirm this is the intended behavior across all command types (exit, loop_blocked, loop_unblocked). If these commands should still have empty requestIds, the validation has been weakened.
Also applies to: 122-122
santigimeno
force-pushed
the
node-v22.x-nsolid-v6.x
branch
from
d359fc9 to
d08dcb2
Compare
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
9facded to
7c6a746
Compare
santigimeno
changed the base branch from
node-v22.x-nsolid-v6.x
to
node-v24.x-nsolid-v6.x
santigimeno
force-pushed
the
santi/assets_enabled
branch
2 times, most recently
from
7c6a746 to
97e5eea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
agents/zmq/src/zmq_agent.h (1)
731-731: Initializeassets_enabled_to avoid undefined behavior.This atomic boolean is declared without initialization, leaving it in an indeterminate state until config explicitly sets it. If the config lacks an "assetsEnabled" key, reading this variable results in undefined behavior.
Per the previous review, initialize to
trueto match the JS default:- std::atomic<bool> assets_enabled_; + std::atomic<bool> assets_enabled_{true};
🧹 Nitpick comments (2)
test/parallel/test-nsolid-heap-profile-stream.js (1)
118-120: Remove redundant assertion.
assert.ok(true)is a no-op. Thecommon.mustCall()wrapper already verifies the callback executes.finalStream.on('end', common.mustCall(() => { - assert.ok(true); }));lib/nsolid.js (1)
416-419: Remove unusedoptionsparameter fromheapProfileStreamfunction.The
optionsparameter is declared in the function signature but never used in the function body. It's not validated, passed to any function calls, or referenced anywhere. Remove it unless it's explicitly intended for future use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(3 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(6 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- test/common/nsolid-zmq-agent/client.js
- test/common/nsolid-grpc-agent/client.js
- test/parallel/test-nsolid-config-tracing.js
- src/nsolid/nsolid_api.cc
- test/agents/test-zmq-tracing.mjs
- agents/zmq/src/zmq_agent.cc
- test/parallel/test-nsolid-heap-sampling-stream.js
- test/common/nsolid-zmq-agent/index.js
- test/agents/test-grpc-heap-sampling.mjs
- test/agents/test-grpc-profile.mjs
- test/common/nsolid-grpc-agent/index.js
- test/agents/test-zmq-heap-sampling.mjs
- agents/grpc/src/grpc_agent.h
- agents/grpc/proto/reconfigure.proto
- test/agents/test-zmq-profile.mjs
- agents/grpc/src/grpc_errors.h
- agents/grpc/src/grpc_agent.cc
- agents/grpc/src/proto/reconfigure.pb.h
🧰 Additional context used
🧬 Code graph analysis (8)
test/agents/test-grpc-snapshot.mjs (2)
test/agents/test-grpc-heap-sampling.mjs (5)
disabledConfig(308-308)disabledResult(311-311)disabledResult(356-356)enabledConfig(319-319)currentConfig(369-369)test/agents/test-grpc-profile.mjs (5)
disabledConfig(297-297)disabledResult(300-300)disabledResult(342-342)enabledConfig(308-308)currentConfig(355-355)
test/parallel/test-nsolid-heap-sampling.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/parallel/test-nsolid-heap-profile.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/agents/test-grpc-continuous-profile.mjs (2)
test/agents/test-grpc-heap-profile.mjs (22)
tests(81-81)env(90-90)env(129-129)env(171-171)env(199-199)env(231-231)env(265-268)env(304-304)env(350-350)env(408-408)env(446-446)child(95-95)child(134-134)child(176-176)child(204-204)child(236-236)child(273-273)child(309-309)child(355-355)child(414-414)child(451-451)currentConfig(380-380)test/agents/test-grpc-profile.mjs (2)
tests(72-72)currentConfig(355-355)
agents/grpc/src/proto/reconfigure.pb.cc (1)
agents/grpc/src/proto/reconfigure.pb.h (16)
ReconfigureBody(94-94)ReconfigureBody(95-96)ReconfigureBody(681-693)ReconfigureBody(694-698)ReconfigureBody(699-721)ReconfigureBody(722-726)ReconfigureBody(727-749)ReconfigureBody(750-754)ReconfigureBody(755-777)ReconfigureBody(778-782)ReconfigureBody(783-805)ReconfigureBody(806-810)ReconfigureBody(811-834)ReconfigureBody(836-841)ReconfigureBody(875-887)ReconfigureBody(890-903)
test/agents/test-grpc-tracing.mjs (1)
test/agents/test-zmq-tracing.mjs (1)
phase(411-411)
test/agents/test-grpc-heap-profile.mjs (2)
test/agents/test-grpc-profile.mjs (5)
disabledConfig(297-297)disabledResult(300-300)disabledResult(342-342)enabledConfig(308-308)currentConfig(355-355)test/agents/test-grpc-snapshot.mjs (5)
disabledConfig(300-300)disabledResult(303-303)disabledResult(345-345)enabledConfig(311-311)currentConfig(358-358)
test/agents/test-zmq-heap-profile.mjs (1)
test/agents/test-grpc-heap-profile.mjs (3)
disabledConfig(319-319)enabledConfig(330-330)currentConfig(380-380)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-macOS
- GitHub Check: coverage-linux-without-intl
- GitHub Check: coverage-linux
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
🔇 Additional comments (35)
agents/grpc/src/proto/reconfigure.pb.cc (1)
1-4: Auto-generated protobuf code - no manual edits required.This file is generated by the protocol buffer compiler from
reconfigure.proto. The changes correctly add the newassetsEnabledfield (tag 13) with proper has-bit handling, serialization, parsing, merge, and swap logic consistent with other boolean fields inReconfigureBody.test/agents/test-zmq-heap-profile.mjs (1)
342-408: Well-structured test for assetsEnabled toggle flow.The test correctly validates the full enable/disable cycle: disabling assets returns a 422 error for heap profile requests, re-enabling allows successful profiling, and the final state is verified. The use of the
resolvedflag prevents duplicate event handling, and the async/await pattern is properly applied.test/agents/test-zmq-snapshot.mjs (1)
235-296: Consistent test pattern for assetsEnabled toggle on snapshots.The test follows the established pattern from heap profile tests, properly validating the 422 error when assets are disabled, successful snapshot capture when enabled, and final state verification. The snapshot data accumulation and JSON parsing validation are correctly implemented.
test/parallel/test-nsolid-snapshot.js (1)
60-110: Correctly implements asset toggle tests with proper sequencing.The implementation properly addresses the race condition concern from the previous review by using
common.mustSucceed()callbacks to wait for snapshot completion before toggling the assets state. The nestedsetImmediatecalls ensure proper ordering of operations.test/parallel/test-nsolid-heap-sampling.js (2)
174-175: Good use ofplatformTimeoutfor cross-platform compatibility.Replacing hardcoded timeouts with
common.platformTimeout(100)ensures tests work reliably across different platforms with varying performance characteristics.
177-248: Proper sequencing of asset toggle tests for heap sampling.The test correctly validates the full asset enable/disable cycle for heap sampling operations. The implementation properly waits for sampling operations to complete via
common.mustSucceed()callbacks before toggling asset state, avoiding race conditions. The consistent use ofcommon.platformTimeout(100)ensures cross-platform reliability.test/parallel/test-nsolid-heap-profile-stream.js (2)
68-69: LGTM!Clean integration point - calling the new asset toggle tests after the existing test completes successfully.
72-126: LGTM! Comprehensive asset toggle test coverage.The test correctly exercises multiple toggle paths:
- Disabling via
nsolid.start({ assetsEnabled: false })- Re-enabling via
enableAssets()helper- Disabling via
disableAssets()helper- Final re-enable to confirm restoration
The nested
setImmediatepattern is appropriate for ensuring state propagation before assertions.test/parallel/test-nsolid-profile.js (2)
107-118: LGTM!Good changes:
- Reformatted assertion for readability
- Added integration point for asset toggle tests
- Using
common.platformTimeout(100)ensures cross-platform reliability
120-182: LGTM! Well-structured asset toggle tests for CPU profiling.The test correctly:
- Disables assets and verifies both synchronous throw and callback error paths for
profile()andprofileEnd()- Re-enables assets and completes a full profile cycle before toggling again
- Uses appropriate timeouts between state changes
test/agents/test-grpc-continuous-profile.mjs (7)
4-4: LGTM!Adding
setTimeoutfromnode:timers/promisesenables cleaner async/await timing in tests.
121-156: LGTM!The test correctly verifies that when
NSOLID_ASSETS_ENABLED='0'is set via environment, no continuous profiles are emitted. The 300ms wait appropriately covers multiple profile intervals.
158-201: LGTM!Well-designed test that:
- Verifies continuous profiling works initially
- Confirms profiles stop (or nearly stop) after disabling assets via config
- Validates that re-enabling assets resumes profiling
The
<= 1tolerance at line 189 correctly accounts for an in-flight profile that may complete after the disable call.
203-245: LGTM!This test complements the previous one by exercising the
enableAssets()/disableAssets()helper methods. The additional config verification at lines 236-237 is a nice touch.
255-266: LGTM!Refactored setup for consistency with other tests. Moving
env,opts, andstartTimeinitialization beforegrpcServer.on('profile', ...)improves code organization.
380-386: LGTM!Clean refactor from callback-style
setTimeouttoawait setTimeout()fromnode:timers/promises.
430-449: LGTM!Same async/await refactor pattern applied consistently.
test/agents/test-grpc-snapshot.mjs (3)
246-280: LGTM!The test correctly verifies that when
NSOLID_ASSETS_ENABLED='0'is set via environment, heap snapshot requests return a 500 error with the appropriate "Assets collection disabled(1008)" message.
282-323: LGTM!Well-structured test that:
- Disables assets and verifies the config change took effect
- Confirms snapshot request fails with the expected error
- Re-enables assets and verifies snapshot succeeds
325-367: LGTM!This test exercises the
enableAssets()/disableAssets()helper methods for snapshot operations, complementing the config-based toggle test above.test/parallel/test-nsolid-heap-profile.js (2)
124-131: LGTM!Clean integration point calling the new asset toggle tests. Using
common.platformTimeout(100)ensures cross-platform reliability.
133-204: LGTM! Thorough asset toggle coverage for heap profiling.The test correctly exercises the same toggle scenarios as the CPU profile test but for the heap profiling API. While there's structural similarity with
test-nsolid-profile.js, the tests target different APIs and keeping them separate maintains clarity.test/agents/test-grpc-heap-profile.mjs (3)
259-296: LGTM: Well-structured test for environment-based asset disabling.The test correctly sets
NSOLID_ASSETS_ENABLED: '0'and verifies the expected 500 error response with the'Assets collection disabled(1008)'message. The pattern is consistent with other error-handling tests in this file.
298-342: LGTM: Comprehensive test for config-based asset toggling.Good coverage of the toggle flow: disabling via
config({ assetsEnabled: false }), verifying the error, re-enabling viaconfig({ assetsEnabled: true }), and confirming successful profiling. Consistent with the pattern shown in the relevant snippets fromtest-grpc-profile.mjsandtest-grpc-snapshot.mjs.
344-389: LGTM: Good coverage for helper function API.The test exercises
disableAssets()andenableAssets()helpers and includes a final verification thatcurrentConfig.assetsEnabledistrueafter re-enabling, which provides additional confidence in the API consistency.lib/nsolid.js (7)
133-136: LGTM: Clean API surface additions.The new toggle functions are properly exposed alongside the existing profiling APIs.
201-214: LGTM: Sensible defaults for new getters.Using
?? trueforassetsEnabled(enabled by default) and?? falsefortracingEnabled(disabled by default) aligns with the initialization logic ininitializeConfig.
502-504: LGTM: Consistent assets check for heap sampling.The guard follows the same pattern as
heapProfileStream, throwing the appropriate error type when assets are disabled.
609-638: LGTM: Simple and idempotent toggle implementations.The early return when already in the desired state prevents unnecessary config updates. All four functions follow the same consistent pattern.
766-770: LGTM: Proper normalization in updateConfig.Using
optionToBoolensures consistent handling of various input types, and only updating whennormalized !== undefinedpreserves existing behavior when invalid input is provided.
952-961: LGTM: Correct precedence for assetsEnabled initialization.The env → pkg → default (true) precedence is properly implemented and consistent with other configuration options in this function.
1111-1121: LGTM: Well-designed type coercion utility.
optionToBoolcorrectly handles all expected input types (undefined, null, boolean, number, string) and returnsundefinedfor invalid types, allowing callers to distinguish between "not set" and "set to false".test/agents/test-grpc-tracing.mjs (3)
5-5: LGTM: Import addition is appropriate.The
setTimeout as delayimport is correctly added and used later in the test for timing control between phase transitions.
205-247: LGTM: Phase-based testing approach is well-structured.The phase state machine (
initial→disabled→reenabled→done) effectively validates the enable/disable traces functionality:
- Early return guard (lines 207-209) prevents post-completion processing
- Array reset via
resourceSpans.length = 0ensures clean state between phases- 200ms delay provides sufficient time to verify spans are not emitted when disabled
mustCallAtLeast(2)correctly reflects listener calls in bothinitialandreenabledphaseschild.shutdown(0)is appropriately deferred to the final phaseThe test comprehensively verifies that traces are collected, not collected when disabled, and collected again when re-enabled.
273-315: LGTM: Custom tracing test mirrors HTTP tracing approach.The custom tracing test correctly implements the same phase-based enable/disable verification as the HTTP tracing test, ensuring consistent coverage across different tracing types. The implementation maintains the same defensive patterns and timing controls.
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
97e5eea to
c1da77d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/agents/test-grpc-tracing.mjs (1)
107-135: FixcheckHttpSpansto validate the client span IDs, not the server span twice
checkHttpSpanscurrently callsvalidateId(serverSpan.traceId/spanId, ...)again when checking the client span (Lines 125–127), so the client span’s IDs are never actually validated. This is pre‑existing but worth correcting:const clientSpan = spans[1]; - validateId(serverSpan.traceId, 16); - validateId(serverSpan.spanId, 8); + validateId(clientSpan.traceId, 16); + validateId(clientSpan.spanId, 8);agents/grpc/src/grpc_agent.cc (1)
2076-2097: Fix data race onconfig_when accessed from JS threads indo_start_prof_init()
do_start_prof_init()is called from both GRPC handlers (start_*_profilevariants) running on the agent loop thread and from*_from_jsentry points that execute on arbitrary JS/V8 threads. The method readsconfig_at lines 2092-2093:auto it = config_.find("contCpuProfile"); if (it != config_.end() && it->get<bool>() == true) { return ErrorType::EInProgressError; }While
assets_enabled_is atomic and thread-safe,config_is a plainnlohmann::jsonmember with no synchronization mechanism. Although writes toconfig_only occur on the agent loop thread viaconfig_msg_cb_(), reads from JS threads create a data race and undefined behavior per C++11 memory model.To eliminate the race, snapshot the continuous-profile flag into an atomic variable:
// grpc_agent.h std::atomic<bool> cont_cpu_profile_enabled_{false};// In config() method after updating config_: { auto it = config_.find("contCpuProfile"); if (it != config_.end() && it->is_boolean()) { cont_cpu_profile_enabled_.store(*it, std::memory_order_relaxed); } else { cont_cpu_profile_enabled_.store(false, std::memory_order_relaxed); } }// In do_start_prof_init(): if (!assets_enabled_.load(std::memory_order_acquire)) { return ErrorType::EAssetsDisabled; } if (type == ProfileType::kCpu && cont_cpu_profile_enabled_.load(std::memory_order_acquire)) { return ErrorType::EInProgressError; }This keeps all
config_access confined to the agent thread and safely communicates the continuous-profile state to off-thread callers.
♻️ Duplicate comments (1)
test/common/nsolid-grpc-agent/index.js (1)
389-406: Tighten#sendAndWaittimeout behavior and avoid payload overridingtypeTwo follow-ups here:
Timeout mapping can throw for mapped calls (e.g.
config())
On timeout you callresolve(map ? map(null) : undefined). Forconfig(),mapismsg => msg.config, so a timeout will attemptnull.configand throw. Prefer to resolve a sentinel (e.g.null) without invokingmapon timeout:const timeout = setTimeout(() => {
- this.#child.off('message', handler);
- resolve(map ? map(null) : undefined);
- this.#child.off('message', handler);
- resolve(undefined); // or resolve(null), leaving mapping to the caller
}, 30000);Callers that care can wrap `config()` and handle `undefined`/`null` explicitly. 2. **Payload can still override `type`** *(previously flagged)* The object sent is `{ type, ...payload }`, so a `payload.type` field would override the intended message type if introduced later. To make this robust, flip the spread order: ```diff - this.#child.on('message', handler); - this.#child.send({ type, ...payload }); + this.#child.on('message', handler); + this.#child.send({ ...payload, type });That keeps the explicit
typeauthoritative.
🧹 Nitpick comments (7)
test/parallel/test-nsolid-heap-profile-stream.js (1)
80-125: Optional: consider flattening the nested async/control-flow for readability.The nested
setImmediateand stream callbacks are functionally fine but a bit hard to scan; if you touch this again, consider extracting small helpers (e.g.,expectHeapStreamFailsWhenDisabled,collectHeapProfile,withAssetsEnabled) or using a simple step-wise driver to reduce indentation while keeping the same assertions.agents/zmq/src/zmq_agent.cc (1)
2116-2118: Consider a more descriptive error message for disabled assets.The guard correctly prevents profiling when assets are disabled. However, returning
UV_EINVALmaps to the generic error message "Invalid arguments" (line 1546-1548), which doesn't clearly indicate that assets are disabled.Consider adding a specific error code for disabled assets to provide clearer feedback to users. This would help distinguish between "assets disabled" and actual invalid arguments:
int ZmqAgent::do_start_prof_init(const nlohmann::json& message, ProfileType type, ProfileOptions& options) { if (!assets_enabled_) { - return UV_EINVAL; + return UV_EPERM; // Or introduce a custom error code }Then update
create_command_error()to handle this case:ZmqAgent::ZmqCommandError ZmqAgent::create_command_error( const std::string& command, int err) const { ZmqCommandError cmd_error; switch (err) { + case UV_EPERM: + cmd_error.code = 403; + cmd_error.message = "Asset collection is disabled"; + break; case UV_EEXIST: cmd_error.code = 409;Alternatively, keep the current approach if consistency with existing error patterns is preferred.
test/agents/test-zmq-profile.mjs (1)
149-173: Avoidasyncevent handler to prevent silent promise rejectionsThe event callback passed to
playground.bootstrapisasyncand awaitsplayground.client.config()after the asset stream completes. Ifconfig()rejects or the assertions after theawaitfail, the resulting rejected promise may not be observed by the bootstrap/event machinery, depending on how it wires listeners, leading to an unhandled rejection rather than a clean test failure.You can keep the handler synchronous and move the
awaitwork into a fire-and-forget IIFE so assertion failures still throw synchronously before the firstawait, while post-awaitfailures at least surface as unhandled rejections you can manage at the harness level:- }), async (eventType, agentId, data) => { + }), (eventType, agentId, data) => { if (resolved) return; @@ - case 2: { + case 2: { assert.strictEqual(eventType, 'asset-received'); checkProfileData(requestId, options, agentId, data, true); JSON.parse(profile); resolved = true; - const currentConfig = await playground.client.config(); - assert.strictEqual(currentConfig.assetsEnabled, true); - resolve(); + (async () => { + const currentConfig = await playground.client.config(); + assert.strictEqual(currentConfig.assetsEnabled, true); + resolve(); + })(); }This keeps the external handler signature non-async while preserving the behavior. Please double-check how
playground.bootstraptreats async listeners; if it already awaits or handles returned promises, you may choose to keep the current form.test/parallel/test-nsolid-heap-sampling.js (1)
177-248: Good coverage of assetsEnabled toggling; consider tightening assertions and timers (optional)The new
runHeapSamplingAssetsToggleTests()function does a nice job exercising:
- disabling assets via
nsolid.start({ assetsEnabled: false }),- validating both sync (
assert.throws) and callback error paths forheapSampling/heapSamplingEnd,- re-enabling via
enableAssets(), running a full sampling cycle, then disabling again withdisableAssets(),- and finally re-enabling to confirm sampling still works.
Two optional improvements you might consider:
Assert on an assets-specific error code if available
Right now the tests only assert on the generic messages'Heap sampling could not be started'/'stopped'pluserr.code !== 0. If there is a stable errorcodethat uniquely identifies the “assets disabled” condition, checking that here would make the tests more targeted and avoid passing when some unrelated failure produces the same message.Wrap timers with
common.mustCallfor stricter async guarantees
UsingsetTimeout(common.mustCall(...), common.platformTimeout(100))for the new timers would surface clearer failures if a path is never reached (e.g., due to earlier unexpected errors), and is consistent with how many other async tests enforce callback execution.Both are nice-to-have and can be deferred if you prefer to keep the test light.
test/parallel/test-nsolid-profile.js (1)
120-182: LGTM! Comprehensive test for assets toggle lifecycle.The
runAssetsToggleTests()function thoroughly tests:
- Assets disabled via config → profile fails
- Assets enabled via
enableAssets()→ profile succeeds- Assets disabled via
disableAssets()→ profile fails- Assets re-enabled → profile succeeds
The deeply nested callbacks (6+ levels) could be flattened using async/await for improved readability:
-function runAssetsToggleTests() { - // Disable assets through config update - nsolid.start({ - command: 'localhost:9001', - data: 'localhost:9002', - assetsEnabled: false, - }); - - setTimeout(() => { - // ... deeply nested callbacks - }, common.platformTimeout(100)); -} +async function runAssetsToggleTests() { + const { promisify } = require('util'); + const delay = promisify(setTimeout); + + nsolid.start({ + command: 'localhost:9001', + data: 'localhost:9002', + assetsEnabled: false, + }); + + await delay(common.platformTimeout(100)); + // ... flattened async logic +}test/agents/test-zmq-heap-profile.mjs (1)
365-370: Consider aligning ZMQ error code for disabled assets with gRPC's more specific error.The ZMQ agent returns error code 422 "Invalid arguments" when assets are disabled, while the gRPC agent returns 500 "Assets collection disabled(1008)". This occurs because ZMQ maps the
assets_enabled_check indo_start_prof_init()toUV_EINVAL, which translates to code 422. Consider whether the ZMQ agent should return a dedicated error code (like 500) with a more descriptive message to match gRPC's approach and provide clearer API semantics.lib/nsolid.js (1)
715-772: Bool normalization for assetsEnabled viaoptionToBooland env/pkg precedence looks solid
updateConfig()now normalizesassetsEnabledfrom the JS config usingoptionToBool, only applying it when the value is interpretable as a boolean.initializeConfig()derivesnsolidConfig.assetsEnabledfrom:
NSOLID_ASSETS_ENABLEDenv (viaoptionToBool),package.json’snsolid.assetsEnabled(also viaoptionToBool),- falling back to
trueif both are absent.optionToBool()sensibly handles booleans, numbers, and strings (using the existingenvToBoolsemantics), returningundefinedfor unsupported types so callers can skip invalid values.This yields predictable precedence and avoids surprising behavior from odd config types.
Also applies to: 951-960, 1110-1120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(3 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(6 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- test/parallel/test-nsolid-heap-sampling-stream.js
- test/common/nsolid-zmq-agent/index.js
- test/parallel/test-nsolid-snapshot.js
- test/common/nsolid-grpc-agent/client.js
- agents/zmq/src/zmq_agent.h
- test/parallel/test-nsolid-config-tracing.js
- test/agents/test-zmq-heap-sampling.mjs
- agents/grpc/proto/reconfigure.proto
- src/nsolid/nsolid_api.cc
- test/agents/test-grpc-profile.mjs
🧰 Additional context used
🧬 Code graph analysis (11)
test/parallel/test-nsolid-heap-profile-stream.js (4)
test/common/nsolid-grpc-agent/client.js (1)
nsolid(7-7)test/common/nsolid-zmq-agent/client.js (1)
nsolid(6-6)test/parallel/test-nsolid-config-tracing.js (2)
nsolid(5-5)assert(4-4)test/parallel/test-nsolid-heap-profile.js (3)
nsolid(5-5)assert(4-4)common(3-3)
test/agents/test-zmq-tracing.mjs (1)
lib/internal/process/pre_execution.js (1)
threadId(726-726)
test/agents/test-grpc-snapshot.mjs (2)
test/agents/test-grpc-heap-sampling.mjs (3)
disabledConfig(308-308)enabledConfig(319-319)currentConfig(369-369)test/agents/test-grpc-profile.mjs (3)
disabledConfig(297-297)enabledConfig(308-308)currentConfig(355-355)
test/agents/test-grpc-continuous-profile.mjs (1)
test/agents/test-grpc-heap-profile.mjs (10)
env(90-90)env(129-129)env(171-171)env(199-199)env(231-231)env(265-268)env(304-304)env(350-350)env(408-408)env(446-446)
test/agents/test-zmq-heap-profile.mjs (1)
test/agents/test-zmq-heap-sampling.mjs (4)
playground(512-512)disabledConfig(154-154)enabledConfig(165-165)currentConfig(193-193)
test/agents/test-zmq-profile.mjs (1)
test/agents/test-zmq-heap-profile.mjs (10)
playground(491-491)profile(43-43)profile(98-98)profile(252-252)profile(298-298)profile(347-347)profile(415-415)disabledConfig(362-362)enabledConfig(373-373)currentConfig(400-400)
test/parallel/test-nsolid-heap-profile.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
agents/grpc/src/proto/reconfigure.pb.h (1)
agents/grpc/src/proto/reconfigure.pb.cc (4)
ReconfigureBody(53-53)ReconfigureBody(267-270)_impl_(59-60)_impl_(85-86)
agents/grpc/src/grpc_agent.cc (1)
agents/zmq/src/zmq_agent.cc (10)
config(430-476)config(430-430)config(1264-1379)config(1264-1264)config(2761-2796)config(2761-2761)config(2889-2906)config(2889-2889)config(2984-3001)config(2984-2984)
test/common/nsolid-grpc-agent/index.js (1)
test/common/nsolid-grpc-agent/client.js (1)
msg(89-89)
test/agents/test-grpc-heap-sampling.mjs (1)
test/agents/test-grpc-heap-profile.mjs (33)
tests(81-81)grpcServer(88-88)grpcServer(113-113)grpcServer(127-127)grpcServer(154-154)grpcServer(169-169)grpcServer(183-183)grpcServer(197-197)child(95-95)child(134-134)child(176-176)child(204-204)child(236-236)child(273-273)child(309-309)child(355-355)agentId(96-96)agentId(135-135)agentId(177-177)agentId(205-205)agentId(237-237)agentId(274-274)agentId(310-310)agentId(356-356)agentId(415-415)agentId(460-460)profileOpts(311-317)profileOpts(357-363)disabledConfig(319-319)disabledResult(322-322)disabledResult(367-367)enabledConfig(330-330)currentConfig(380-380)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-macOS
- GitHub Check: coverage-windows
- GitHub Check: coverage-linux
🔇 Additional comments (29)
test/parallel/test-nsolid-heap-profile-stream.js (1)
68-69: Assets toggle stream test flow and sync-throw expectations look correct.The added
runHeapProfileStreamAssetsToggleTests()sequence (disable viastart→ assert sync failure → enable and validate successful stream → disable and assert sync failure again → final enable + successful stream) exercises the intended runtime assets control thoroughly, and usingassert.throwsfor the disabled-assets cases aligns with the synchronous validation behavior ofheapProfileStream()(immediate state checks vs. async runtime errors). Based on learnings, this matches the established error-handling contract for this API.Also applies to: 72-126
agents/zmq/src/zmq_agent.cc (2)
662-662: LGTM!Initialization of
assets_enabled_totruepreserves backward compatibility by keeping asset collection enabled by default.
1283-1288: Verify whetherassetsEnabledreconfiguration is intentionally ZMQ-only limited.The implementation correctly reads
assetsEnabledfrom the configuration (lines 1283-1288). However,assetsEnabledis excluded from the ZMQ agent'svalidate_reconfigure()allowed options list (lines 2495-2507), meaning it cannot be changed via the ZMQreconfigurecommand at runtime.In contrast, the gRPC agent includes
assetsEnabledas an optional boolean field in its reconfigure protobuf definition (agents/grpc/proto/reconfigure.proto, line 20) and handles it in the reconfigure response (grpc_agent.cc, line 1740).Clarify whether this difference is intentional—that is, whether runtime reconfiguration of
assetsEnabledis only supported via the gRPC protocol and explicitly excluded from ZMQ, or if this represents a gap that should be addressed.test/agents/test-zmq-snapshot.mjs (1)
235-296: NewassetsEnabledtoggle snapshot test looks correct and consistentThis test cleanly exercises the “disabled → 422 error → re‑enable → successful snapshot” flow, reuses the existing snapshot event pattern (
eventscounter,checkSnapshotData,JSON.parse(snapshot)), and theresolvedguard safely handles the extra asyncclient.config()check at the end. The assertions ondisabledConfig,enabledConfig, andcurrentConfiggive good coverage of the runtime config state around the snapshot request.test/agents/test-zmq-profile.mjs (1)
133-148: Good coverage of assetsEnabled disable/enable flowThe sequence that disables assets, asserts the 422/
Invalid argumentsonagentProfileStart, then re-enables assets and starts a successful profile aligns well with the intended behavior and matches existing error-handling patterns in this file. No changes needed here.test/parallel/test-nsolid-heap-sampling.js (1)
164-176: Sequencing of the new assets-toggle tests and platformTimeout usage looks soundPlacing
runHeapSamplingAssetsToggleTests()inside the innermostheapSamplingEndcallback keeps the original error-path coverage intact and guarantees the new assets-toggling scenarios only run after the prior sequence completes. The switch tocommon.platformTimeout(100)on the surrounding timers is also in line with existing test patterns and should help with cross-platform stability.test/agents/test-zmq-tracing.mjs (1)
406-472: LGTM! Test correctly implements phase-based tracing toggle verification.The test properly:
- Fails immediately if spans arrive during the disabled phase (line 432)
- Sets
phase = 'reenabled'afterenableTraces()completes (lines 452-453), preventing misclassification of late spans- Includes a 200ms delay to allow any pending spans to flush before re-enabling
agents/grpc/src/grpc_agent.h (1)
351-351: LGTM! Thread-safe atomic flag for asset control.Using
std::atomic<bool>is appropriate for theassets_enabled_flag that will be accessed from multiple threads (configuration updates and profiling operations).test/common/nsolid-zmq-agent/client.js (1)
141-148: LGTM! IPC handlers for asset/trace toggles.The new handlers correctly delegate to the corresponding
nsolidAPI methods. These are synchronous toggles that don't require acknowledgment responses.test/agents/test-grpc-snapshot.mjs (3)
246-280: LGTM! Test correctly validates assets-disabled error via environment variable.The test properly verifies that when
NSOLID_ASSETS_ENABLED: '0'is set, heap snapshot requests return error code 500 with message'Assets collection disabled(1008)'.
282-323: LGTM! Test validates programmatic assetsEnabled toggle via nsolid.start().The test correctly:
- Disables assets via
child.config({ assetsEnabled: false })- Verifies snapshot request fails with the expected error
- Re-enables assets and confirms snapshot succeeds
325-367: LGTM! Test validates enableAssets()/disableAssets() helper methods.The test properly exercises the helper API and confirms the config state reflects the toggle after
enableAssets()(line 358-359).agents/grpc/src/grpc_errors.h (1)
12-13: LGTM! New error type for assets-disabled state.The
EAssetsDisablederror with HTTP code 500, message "Assets collection disabled", and runtime code 1008 correctly aligns with test expectations across the test suite.test/agents/test-zmq-heap-profile.mjs (1)
342-408: Test logic is sound with proper cleanup viaresolvedflag.The
resolvedflag (line 349, 378-379, 399) correctly prevents processing stale events after the test resolves, avoiding potential race conditions.test/agents/test-grpc-continuous-profile.mjs (3)
121-156: LGTM! Test validates no continuous profiles are emitted when assets disabled via environment.The test correctly waits 300ms (slightly more than two 100ms intervals) to verify no profiles arrive, providing reasonable margin for CI timing variability.
186-193: Timing tolerance handles in-flight profiles gracefully.The assertion
profileCount - countAfterDisable <= 1(line 189) correctly accounts for at most one in-flight profile that may arrive after disabling. This is a reasonable tolerance for async profiling systems.
203-245: LGTM! Test validates disableAssets()/enableAssets() for continuous profiling.The test properly verifies the complete enable/disable/re-enable cycle and confirms the final config state reflects
assetsEnabled: true(line 237).test/parallel/test-nsolid-profile.js (1)
117-118: Good use of platformTimeout for CI adaptability.Replacing fixed 100ms timeouts with
common.platformTimeout(100)ensures the test adapts to slower CI environments, reducing flakiness.test/agents/test-grpc-tracing.mjs (1)
185-247: Phase-based enable/disable tracing tests look correct and exercise the new API surface wellThe new
phasestate machine for HTTP and custom tracing correctly:
- Verifies initial spans content.
- Clears accumulated spans and disables traces.
- Confirms no spans arrive while disabled after an additional trace + delay.
- Re-enables traces, triggers another trace, and validates spans again before clean shutdown.
The guards for
phase === 'done'and use ofmustCallAtLeast(..., 2)should keep the tests stable while allowing extra span batches if they arrive. No functional issues spotted in the new logic.Also applies to: 253-315
test/parallel/test-nsolid-heap-profile.js (1)
124-204: Assets toggle coverage around heap profiling is thorough and consistent
runHeapProfileAssetsToggleTests()cleanly exercises:
- Starting with
assetsEnabled: false(start/stop both error, with and without callbacks).- Re-enabling assets and running a successful profile.
- Disabling again and re-validating start/stop errors.
- Final re-enable and successful profile/stop.
The use of
common.platformTimeout(100)is consistent with other tests and should keep timing robust across platforms.test/agents/test-grpc-heap-sampling.mjs (1)
248-378: Heap sampling assetsEnabled tests correctly mirror the intended error and toggle behaviorThe three added tests:
- Verify
NSOLID_ASSETS_ENABLED='0'yields a 500Assets collection disabled(1008)error.- Confirm
assetsEnabledtoggling viaconfig()affects subsequentheapSamplingcalls as expected.- Exercise the
enableAssets()/disableAssets()helpers and assert finalassetsEnabled === true.Assertions use the existing
checkProfileError/checkProfileDatahelpers, so coverage is consistent with the rest of this suite.test/agents/test-grpc-heap-profile.mjs (1)
259-389: Heap profile tests for assetsEnabled are well-aligned with heap sampling behaviorThe new tests:
- Cover the NSOLID_ASSETS_ENABLED=0 case and expect the 500
Assets collection disabled(1008)error.- Exercise dynamic toggling via
child.config({ assetsEnabled })and viadisableAssets()/enableAssets()helpers.- Validate that when re-enabled, heap profiles complete successfully, and that
assetsEnabledin the reported config istrue.They parallel the heap sampling tests and give good end-to-end coverage for asset gating.
agents/grpc/src/proto/reconfigure.pb.cc (1)
29-147: Generated ReconfigureBody gencode forassetsEnabledlooks consistentThe additions for
assetsenabled_(constructor init, memcpy/memset ranges, offsets table, descriptor string, parse table, Clear/Merge/Serialize/ByteSize, and InternalSwap ranges) are coherent with introducing anoptional bool assetsEnabled = 13onReconfigureBody. No manual inconsistencies jump out, so this file looks like correctly regenerated protobuf code.Also applies to: 153-195, 339-429, 440-472, 581-587, 624-625, 702-715, 739-744
agents/grpc/src/grpc_agent.cc (2)
320-383: assetsEnabled propagation through reconfigure events and back into config is coherent
PopulateReconfigureEvent()now emitsassetsEnabledwhen present in the JSON config.reconfigure()mapsbody.assetsenabled()back intoout["assetsEnabled"].send_reconfigure_event()still usesPopulateReconfigureEventso round-tripping works.Together with the JSON config handling in
config(), this ensures assetsEnabled flows cleanly between runtime config, GRPC requests, and events.Also applies to: 1695-1747, 1931-1947
421-432: assets_enabled_ initialization and JSON handling are sensible and type-safe
GrpcAgentctor now initializesassets_enabled_(true), matching the JS default andinitializeConfig()logic.config()readsassetsEnabledfromconfig_only when the JSON value is actually boolean, avoiding type issues.- The added debug JSON logs around
config_help trace configuration diffs.Assuming
assets_enabled_is declaredstd::atomic<bool>in the header (as previously suggested), these assignments are also thread-safe.Also applies to: 1000-1007, 1178-1183
test/common/nsolid-grpc-agent/index.js (1)
346-388: New TestClient helpers for config/assets/traces match the runtime toggles cleanlyThe added methods:
config(config = null)returningmsg.config,enableAssets()/disableAssets(),enableTraces()/disableTraces(), andtrace(kind, targetThreadId = 0)provide a clean, promise-based API that mirrors the new
assetsEnabledand tracing runtime controls inlib/nsolid.js. Call patterns in the GRPC tests (heap profile/sampling, tracing) are straightforward and consistent.lib/nsolid.js (2)
84-154: Public API surface and getters for assetsEnabled/tracingEnabled are well-designed
Exposing
enableAssets()/disableAssets()andenableTraces()/disableTraces()on the exportedstartobject matches the rest of the NSolid config API.The new getters
assetsEnabled: () => getConfig('assetsEnabled') ?? true, tracingEnabled: () => getConfig('tracingEnabled') ?? false,give sensible defaults while still reflecting runtime config, including env and package.json sources.
This keeps the JS-facing API coherent with the underlying GRPC/ZMQ behavior.
Also applies to: 156-215
416-453: heapProfileStream/heapSamplingStream correctly respect assetsEnabled at the JS layerBoth streaming APIs now short-circuit when assets are disabled:
if (getConfig('assetsEnabled') === false) throw new ERR_NSOLID_HEAP_PROFILE_START(); // or ERR_NSOLID_HEAP_SAMPLING_STARTThis:
- Gives immediate, deterministic failures for JS callers when assets are off, instead of relying on agent-side errors alone.
- Matches the parallel GRPC tests that expect asset operations to be blocked globally when assetsEnabled is false.
The rest of the stream wiring (validation, destroy handlers, error propagation via the callback status) remains unchanged.
Also applies to: 501-543
agents/grpc/src/proto/reconfigure.pb.h (1)
233-233: Generated protobuf code properly adds assetsEnabled field.The additions for the new
assetsEnabledfield (field number 13) are correctly generated and follow the established pattern of other boolean fields in the message. The has-bit mask0x00000800U, accessor/mutator methods, struct member declaration, and TcParseTable update are all consistent with protobuf conventions.Also applies to: 392-401, 408-408, 442-442, 1175-1201
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
c1da77d to
c879387
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/common/nsolid-grpc-agent/index.js (1)
389-407: Tighten #sendAndWait timeout behavior and payload constructionTwo minor points here:
Timeout →
map(null)can throw / leave Promise unsettledOn timeout you call
resolve(map ? map(null) : undefined). Forconfig(),mapismsg => msg.config, so a timeout would callmap(null)and throw beforeresolveruns, leaving the Promise pending and surfacing as an unhandled exception.Consider making the timeout path independent of
map, e.g.:- const timeout = setTimeout(() => { - this.#child.off('message', handler); - resolve(map ? map(null) : undefined); - }, 30000); + const timeout = setTimeout(() => { + this.#child.off('message', handler); + resolve(undefined); // or reject(new Error('config timeout')); + }, 30000);and let callers treat
undefined/nullas “no response” if they care.Payload can still override explicit
typeSending
{ type, ...payload }means apayload.typeproperty would overwrite the intendedtype. Swapping the order avoids this foot‑gun:
this.#child.on('message', handler);this.#child.send({ type, ...payload });
this.#child.on('message', handler);this.#child.send({ ...payload, type });That keeps the call‑site API unchanged but guarantees `type` is authoritative.
🧹 Nitpick comments (6)
test/parallel/test-nsolid-heap-sampling-stream.js (1)
220-222: Remove redundant assertion.The
assert.ok(true)assertion is redundant sincecommon.mustCall()already validates that the callback is invoked. Consider removing it or replacing it with a more meaningful check if additional validation is needed.Apply this diff:
finalStream.on('end', common.mustCall(() => { - assert.ok(true); + // Test passes if 'end' event fires (validated by mustCall) }));lib/nsolid.js (2)
416-419: Consider validating arguments before checking assetsEnabled in stream APIs
heapProfileStreamandheapSamplingStreamnow synchronously throwERR_NSOLID_HEAP_PROFILE_START/ERR_NSOLID_HEAP_SAMPLING_STARTwhenassetsEnabled === false, which matches the “immediate state check throws” semantics.Right now the assets check runs before the validators; this means invalid arguments will be masked by the generic “could not be started” error when assets are disabled. If you want to preserve the more precise argument errors regardless of assets state, consider moving the
assetsEnabledcheck after thevalidateNumber/validateBooleancalls:-function heapProfileStream(threadId, duration, trackAllocations, options) { - if (getConfig('assetsEnabled') === false) - throw new ERR_NSOLID_HEAP_PROFILE_START(); - - validateNumber(threadId, 'threadId'); - validateNumber(duration, 'duration'); - validateBoolean(trackAllocations, 'trackAllocations'); +function heapProfileStream(threadId, duration, trackAllocations, options) { + validateNumber(threadId, 'threadId'); + validateNumber(duration, 'duration'); + validateBoolean(trackAllocations, 'trackAllocations'); + if (getConfig('assetsEnabled') === false) + throw new ERR_NSOLID_HEAP_PROFILE_START();(and similarly for
heapSamplingStream).Also applies to: 501-504
765-769: optionToBool + assetsEnabled normalization: minor type-safety improvementThe
assetsEnabledhandling end‑to‑end is solid: env overrides package, defaulting totrue, andupdateConfigignores unparseable values. One small polish:
- In
optionToBool, the string branch currently returnsenvToBool(value)directly. For empty strings,envToBool('')returns'', soenvAssetsEnabledbecomes''and is treated as “set” (and truthy) even though it’s effectively “unset”.If you want to avoid ever storing non‑boolean values in config for these toggles, you can coerce the string branch explicitly, and treat empty strings as “unspecified”:
function optionToBool(value) { if (value === undefined || value === null) return undefined; if (typeof value === 'boolean') return value; if (typeof value === 'number') return value !== 0; - if (typeof value === 'string') - return envToBool(value); + if (typeof value === 'string') { + if (value === '') + return undefined; + return !!envToBool(value); + } return undefined; }This keeps
assetsEnabledstrictly boolean/undefined in bothinitializeConfigandupdateConfig.Also applies to: 951-960, 1110-1120
test/agents/test-grpc-continuous-profile.mjs (1)
158-245: Continuous profiling assets toggle tests are well-structured but timing-sensitiveThe tests that toggle assets via
child.config({ assetsEnabled: ... })andchild.enableAssets()/disableAssets()correctly:
- Assert at least one profile before disabling.
- Allow for one in‑flight profile after disable.
- Verify profiles resume after re‑enable and that
currentConfig.assetsEnabled === true.Given the 100 ms interval and 250 ms waits, the
<= 1post‑disable assertion could become flaky if disable propagation takes longer under load. If flakes appear, consider relaxing the bound slightly or increasing the delay rather than changing logic.test/parallel/test-nsolid-heap-sampling.js (1)
164-176: Heap sampling assets toggle flow is correct but quite timing-heavy
runHeapSamplingAssetsToggleTestsexercises:
- Disabling assets via
nsolid.start({ assetsEnabled: false })and confirming both syncheapSampling()and callback forms fail, andheapSamplingEndalso fails.- Re‑enabling via
nsolid.enableAssets(), running a full sampling + end cycle, then disabling again and re‑asserting the failures.- Finally re‑enabling and running one last successful sampling + end.
The nested
setTimeout(..., common.platformTimeout(100))calls are acceptable in this test suite, but they do make the flow a bit fragile to scheduling variance. If this ever turns flaky, consider slightly increasing the platform timeout or flattening the nesting rather than changing the logic.Also applies to: 177-248
test/common/nsolid-grpc-agent/index.js (1)
346-387: New TestClient config/assets/traces helpers are consistent with existing API
config(config = null)now centrally uses#sendAndWaitand returnsmsg.config, preserving previous semantics while reducing duplication.enableAssets()/disableAssets()andenableTraces()/disableTraces()correctly guard on#childand use#sendAndWaitso callers only resume once the child has acknowledged.trace(kind, targetThreadId = 0)mirrors the existingtracing()method’s behavior (fire‑and‑forget with anawaiton the send callback), making it straightforward for tests to trigger spans.The new helpers look correct and align with the rest of the TestClient surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(3 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(6 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- agents/grpc/proto/reconfigure.proto
- test/common/nsolid-zmq-agent/index.js
- test/agents/test-grpc-profile.mjs
- test/agents/test-zmq-profile.mjs
- test/agents/test-zmq-tracing.mjs
- agents/grpc/src/grpc_errors.h
- test/parallel/test-nsolid-config-tracing.js
- test/parallel/test-nsolid-snapshot.js
- test/agents/test-zmq-snapshot.mjs
- src/nsolid/nsolid_api.cc
- test/common/nsolid-zmq-agent/client.js
- agents/zmq/src/zmq_agent.cc
- test/agents/test-zmq-heap-profile.mjs
- agents/grpc/src/grpc_agent.h
- test/agents/test-grpc-snapshot.mjs
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-08T14:48:04.827Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
test/agents/test-zmq-heap-sampling.mjstest/parallel/test-nsolid-heap-profile.jstest/parallel/test-nsolid-heap-sampling.jstest/agents/test-grpc-continuous-profile.mjstest/agents/test-grpc-heap-sampling.mjstest/agents/test-grpc-heap-profile.mjstest/parallel/test-nsolid-heap-sampling-stream.js
📚 Learning: 2025-07-08T14:46:47.806Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Applied to files:
test/agents/test-zmq-heap-sampling.mjstest/parallel/test-nsolid-heap-profile.jstest/parallel/test-nsolid-heap-sampling.jstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile-stream.jstest/common/nsolid-grpc-agent/client.jstest/agents/test-grpc-heap-sampling.mjstest/agents/test-grpc-heap-profile.mjstest/parallel/test-nsolid-heap-sampling-stream.js
📚 Learning: 2025-10-14T10:18:23.123Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/parallel/test-nsolid-heap-profile-stream.js:81-88
Timestamp: 2025-10-14T10:18:23.123Z
Learning: In nsolid.heapProfileStream() and nsolid.heapSamplingStream(), synchronous throws occur for argument validation errors and immediate state validation checks (like assets being disabled), while async error events are emitted for runtime errors that manifest during execution (like invalid threadId).
Applied to files:
test/agents/test-zmq-heap-sampling.mjstest/parallel/test-nsolid-heap-profile.jstest/parallel/test-nsolid-heap-sampling.jstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile-stream.jslib/nsolid.jstest/agents/test-grpc-heap-profile.mjstest/parallel/test-nsolid-heap-sampling-stream.js
📚 Learning: 2025-07-08T14:47:34.724Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
Applied to files:
test/agents/test-zmq-heap-sampling.mjstest/parallel/test-nsolid-heap-profile.jstest/parallel/test-nsolid-heap-sampling.jstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile-stream.jstest/common/nsolid-grpc-agent/client.jstest/agents/test-grpc-heap-sampling.mjstest/agents/test-grpc-heap-profile.mjstest/parallel/test-nsolid-heap-sampling-stream.js
📚 Learning: 2025-07-15T16:09:04.521Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 327
File: test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js:47-73
Timestamp: 2025-07-15T16:09:04.521Z
Learning: In the nsolid test suite, hardcoded timeouts are acceptable for timing-sensitive tests. The preferred approach when tests become flaky is to increase timeout values rather than implementing more complex synchronization mechanisms.
Applied to files:
test/parallel/test-nsolid-heap-profile.jstest/parallel/test-nsolid-heap-sampling.jstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile-stream.js
📚 Learning: 2025-06-10T09:52:49.401Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Applied to files:
test/parallel/test-nsolid-heap-sampling-stream.js
📚 Learning: 2025-08-20T11:03:11.870Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 354
File: test/agents/test-zmq-handshake.mjs:55-57
Timestamp: 2025-08-20T11:03:11.870Z
Learning: In test files, suggestions for general robustness improvements like try/finally blocks for teardown should be considered out of scope if they're not directly related to the main PR changes, and should be deferred to separate PRs or issues.
Applied to files:
test/parallel/test-nsolid-heap-sampling-stream.js
🧬 Code graph analysis (11)
test/agents/test-zmq-heap-sampling.mjs (2)
test/agents/test-grpc-heap-sampling.mjs (3)
disabledConfig(308-308)enabledConfig(319-319)currentConfig(369-369)test/agents/test-zmq-heap-profile.mjs (10)
playground(491-491)profile(43-43)profile(98-98)profile(252-252)profile(298-298)profile(347-347)profile(415-415)disabledConfig(362-362)enabledConfig(373-373)currentConfig(400-400)
test/agents/test-grpc-tracing.mjs (1)
test/agents/test-zmq-tracing.mjs (1)
phase(411-411)
test/parallel/test-nsolid-heap-profile.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/parallel/test-nsolid-heap-sampling.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/agents/test-grpc-continuous-profile.mjs (2)
test/agents/test-grpc-heap-profile.mjs (22)
child(95-95)child(134-134)child(176-176)child(204-204)child(236-236)child(273-273)child(309-309)child(355-355)child(414-414)child(451-451)currentConfig(380-380)workers(136-136)workers(238-238)wid(137-137)wid(239-239)options(97-111)options(138-152)options(178-181)options(206-209)options(240-243)options(275-281)options(416-419)test/agents/test-grpc-profile.mjs (6)
currentConfig(355-355)workers(123-123)workers(222-222)wid(124-124)wid(223-223)options(87-98)
test/parallel/test-nsolid-profile.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/parallel/test-nsolid-heap-profile-stream.js (1)
test/parallel/test-nsolid-heap-profile.js (3)
nsolid(5-5)assert(4-4)common(3-3)
agents/grpc/src/proto/reconfigure.pb.h (1)
agents/grpc/src/proto/reconfigure.pb.cc (4)
ReconfigureBody(53-53)ReconfigureBody(267-270)_impl_(59-60)_impl_(85-86)
test/agents/test-grpc-heap-sampling.mjs (1)
test/agents/test-grpc-heap-profile.mjs (32)
grpcServer(88-88)grpcServer(113-113)grpcServer(127-127)grpcServer(154-154)grpcServer(169-169)grpcServer(183-183)grpcServer(197-197)child(95-95)child(134-134)child(176-176)child(204-204)child(236-236)child(273-273)child(309-309)child(355-355)agentId(96-96)agentId(135-135)agentId(177-177)agentId(205-205)agentId(237-237)agentId(274-274)agentId(310-310)agentId(356-356)agentId(415-415)agentId(460-460)profileOpts(311-317)profileOpts(357-363)disabledConfig(319-319)disabledResult(322-322)disabledResult(367-367)enabledConfig(330-330)currentConfig(380-380)
test/common/nsolid-grpc-agent/index.js (1)
test/common/nsolid-grpc-agent/client.js (1)
msg(89-89)
agents/grpc/src/proto/reconfigure.pb.cc (1)
agents/grpc/src/proto/reconfigure.pb.h (16)
ReconfigureBody(94-94)ReconfigureBody(95-96)ReconfigureBody(681-693)ReconfigureBody(694-698)ReconfigureBody(699-721)ReconfigureBody(722-726)ReconfigureBody(727-749)ReconfigureBody(750-754)ReconfigureBody(755-777)ReconfigureBody(778-782)ReconfigureBody(783-805)ReconfigureBody(806-810)ReconfigureBody(811-834)ReconfigureBody(836-841)ReconfigureBody(875-887)ReconfigureBody(890-903)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test-macOS
- GitHub Check: coverage-linux
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
- GitHub Check: build-tarball
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
🔇 Additional comments (15)
test/parallel/test-nsolid-heap-sampling-stream.js (1)
171-228: Well-structured asset toggle test coverage.The test comprehensively exercises the asset enable/disable functionality through multiple toggle cycles, validating both synchronous error behavior when disabled and successful profile generation when enabled. The nested
setImmediatecalls appropriately sequence the async state transitions.agents/zmq/src/zmq_agent.h (1)
728-732: New assets flag member placement looks correctAdding
assets_enabled_alongside the profiling state is consistent with the new assets gating. As long as the constructor initializes it to a defined default (e.g.,true) and config paths only flip it explicitly, this header-side change is fine.lib/nsolid.js (2)
83-154: Public assets/tracing toggles are coherent with config plumbingExporting
enableAssets/disableAssetsandenableTraces/disableTraceson thestartobject, backed byupdateConfig({ assetsEnabled|tracingEnabled: ... }), gives a clear and symmetric API surface. The early‑return guards avoid redundant config updates and look good.Also applies to: 609-639
156-215: Getters for assetsEnabled/tracingEnabled have sensible defaults
assetsEnabled: () => getConfig('assetsEnabled') ?? trueandtracingEnabled: () => getConfig('tracingEnabled') ?? falsecorrectly preserve explicitfalsewhile defaulting when unset. This matches the intended “assets on by default, tracing off by default” behavior across config/env/package.test/agents/test-grpc-continuous-profile.mjs (2)
4-4: Using timers/promises for timing in async tests looks goodSwitching to
await setTimeout(...)fromnode:timers/promisesfor the new “assets disabled via env” test is clean and consistent with the existing async style. The test asserts zero profiles, waits slightly >2 intervals, and correctly shuts down the child and GRPC server; no leaks apparent.Also applies to: 121-156
255-267: Refactors in worker and “continuous during manual profile” tests are benign
- Hoisting
env,opts,child, andworkerssetup in the worker test keeps logic clearer without changing behavior.- Replacing callback‑style
setTimeoutwithawait setTimeout(...)in the last two tests simplifies flow and doesn’t affect semantics.These changes look safe.
Also applies to: 380-381, 429-431
test/common/nsolid-grpc-agent/client.js (1)
59-81: gRPC test client: OTEL registration guard and assets/traces toggles look correct
- Wrapping
nsolid.otel.register(api)in a try/catch that only swallowsERR_NSOLID_OTEL_API_ALREADY_REGISTEREDavoids noisy failures when the API has already been wired, while still surfacing real errors.- The new message types (
enable_assets,disable_assets,enable_traces,disable_traces) cleanly proxy tonsolid.enableAssets()/disableAssets()/enableTraces()/disableTraces()and synchronously echo back an acknowledgement, which matches how the other commands in this client behave.No issues spotted.
Also applies to: 120-137
test/agents/test-grpc-heap-sampling.mjs (1)
248-378: Assets-disabled heap sampling tests are consistent with GRPC error semanticsThe three new tests:
- Enforce that
NSOLID_ASSETS_ENABLED=0yields a 500 withAssets collection disabled(1008).- Verify that toggling
assetsEnabledviachild.config()and viaenableAssets()/disableAssets()produces the same 500 error when disabled and valid data when re‑enabled.- Assert
disabledConfig.assetsEnabled === falseand finalcurrentConfig.assetsEnabled === true, exercising the config surface end‑to‑end.The expectations line up with the new
EAssetsDisabledbehavior and look thorough.test/parallel/test-nsolid-profile.js (1)
107-119: CPU profiling assets toggle test mirrors heap-sampling behavior appropriately
runAssetsToggleTests:
- Disables assets via
nsolid.start({ assetsEnabled: false })and validates both sync and callbackprofile()start failures plusprofileEnd()stop failure.- Re‑enables with
nsolid.enableAssets(), runs a full profile + end, then disables again and verifies start/stop failures once more before re‑enabling and doing a final successful cycle.Using
common.platformTimeout(100)for the staged delays is consistent with other timing tests. The logic matches the intended “assets gate all CPU profiling” semantics.Also applies to: 120-182
test/parallel/test-nsolid-heap-profile.js (1)
122-132: Heap profile assets toggle helper is consistent and covers key paths
runHeapProfileAssetsToggleTests:
- Starts by disabling assets via config and confirming both direct and callback
heapProfilestart failures plusheapProfileEndstop failure.- Re‑enables via
nsolid.enableAssets(), performs a full profile cycle, then disables again and re‑asserts failures for start/stop.- Finally re‑enables and runs a last successful profile + end, with all staged via
common.platformTimeout(100)delays.This gives good coverage of the heap profile API’s behavior under assets toggling, aligned with the rest of the suite.
Also applies to: 133-204
test/agents/test-grpc-tracing.mjs (1)
5-5: Phase-based tracing toggle logic looks correct and exercises disable/enable pathsThe new
phasestate machine plusenableTraces()/disableTraces()andtrace()calls are wired coherently: initial spans are validated, spans buffer is cleared before disabling, you assert that no spans arrive while disabled, and then you re-enable and validate spans again. UsingmustCallAtLeast(..., 2)matches the two expected observation points and the'done'phase guard avoids extra work on late events. The same pattern is correctly duplicated for custom tracing.No functional issues spotted here.
Also applies to: 205-247, 273-315
test/agents/test-grpc-heap-profile.mjs (1)
259-389: Heap profile assetsEnabled tests are well‑structured and consistentThe three added tests cover the key surfaces:
- Env flag
NSOLID_ASSETS_ENABLED=0→500with"Assets collection disabled(1008)".config({ assetsEnabled: false/true })→ error when disabled, valid profile when re‑enabled.disableAssets()/enableAssets()→ same behavior plus a finalconfig()check thatassetsEnabledis true.Options, requestId plumbing, and shutdown/close flows match existing tests, so this gives good coverage of the new behavior without regressions.
agents/grpc/src/grpc_agent.cc (1)
379-383: Assets and continuous-CPU gating are wired correctly and concurrency‑safe
assets_enabled_andcont_cpu_profile_enabled_are now atomics, defaulting totrueandfalserespectively, withstore(..., std::memory_order_release)inconfig()andload(std::memory_order_acquire)indo_start_prof_init, which is appropriate for cross‑thread reads from*_from_js.config()snapshotsassetsEnabledandcontCpuProfilefrom the JSON config with a boolean check before writing the atomics, avoiding the prior JSON data race for these flags indo_start_prof_init.PopulateReconfigureEvent/reconfigure()correctly propagate the newassetsEnabledfield in both directions, keeping the proto in sync with the runtime config.do_start_prof_initnow:
- Short‑circuits with
EAssetsDisabledwhenassets_enabled_is false, before touching profile state, and- Refuses manual CPU profiles with
EInProgressErrorwhencont_cpu_profile_enabled_is true,
while leaving the rest of the profile bookkeeping unchanged.This matches the intended semantics without introducing new synchronization issues.
Also applies to: 421-433, 1179-1191, 1747-1749, 2084-2151
agents/grpc/src/proto/reconfigure.pb.h (1)
219-234: Generated ReconfigureBody header correctly exposes assetsEnabledThe new
assetsEnabledfield is wired consistently:
- Enum constant
kAssetsEnabledFieldNumber = 13matches the proto tag.- Public/inline accessors (
has_assetsenabled,clear_assetsenabled,assetsenabled,set_assetsenabled) and internal variants use a new has‑bit (0x00000800U) and the_impl_.assetsenabled_storage.- The parse table and layout (
TcParseTable<4, 13, ...>, field entries) are updated to include this field.Everything here looks mechanically consistent with the new proto definition.
Also applies to: 392-443, 1175-1202
agents/grpc/src/proto/reconfigure.pb.cc (1)
29-51: Generated ReconfigureBody implementation matches the new assetsEnabled field
_impl_.assetsenabled_is initialized tofalseand included in the memcpy/memset ranges used in the copy ctor,SharedCtor,Clear(), andInternalSwap.- Descriptor data and schema include
"assetsEnabled"with tag13and its presence bit is folded into the has‑bit masks (0x00000de0U).- The parse table and field metadata add an entry for field 13 that reads/writes
_impl_.assetsenabled_, and_InternalSerializewrites it when the corresponding has‑bit is set.This is all consistent with the header and proto and should serialize/parse assetsEnabled correctly.
Also applies to: 103-147, 153-194, 328-439, 440-472, 581-587, 611-625, 663-718
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
c879387 to
a7459d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/common/nsolid-grpc-agent/index.js (1)
405-405: Payload spreading order still allows type override.The previous review flagged that spreading
payloadaftertypeallowspayload.typeto override the intended message type. This issue remains unresolved.Apply this diff to ensure the explicit type cannot be overridden:
- this.#child.send({ type, ...payload }); + this.#child.send({ ...payload, type });
🧹 Nitpick comments (6)
test/parallel/test-nsolid-snapshot.js (2)
78-80: Optionally assert a specific error code for the assets‑disabled pathYou currently assert
err.code !== 0and the error message. If the assets‑disabled snapshot path has (or will have) a stable, specific error code, consider also assertingassert.strictEqual(err.code, <EXPECTED_CODE>)here to make the test more precise and resilient to message changes. If there is no dedicated code, you can keep it as is.
68-109: Consider flattening nestedsetImmediate/callback pyramid for readabilityThe nested
setImmediate+snapshotcallbacks form a fairly deep pyramid. Not required, but you might consider extracting a small helper or sequencing via a simple async wrapper (e.g., promise‑wrappedsnapshot) to make the control flow easier to follow, especially if more cases are added later.test/agents/test-zmq-tracing.mjs (1)
5-5: ZMQ tracing toggle test now robustly enforces the disabled window.The phase-driven flow plus
assert.notStrictEqual(phase, 'disabled')ensures any spans emitted while tracing is disabled will immediately fail the test, and movingphase = 'reenabled'afterenableTraces()avoids misclassifying late spans. The 200ms delay should be OK, though bumping it slightly (e.g. 300ms) would add margin against CI timing jitter if this ever flakes.Also applies to: 406-472
test/agents/test-grpc-continuous-profile.mjs (1)
4-4: Continuous profiling tests correctly exercise assets gating and contCpuProfile interactions.The new cases for env-based disable, config-based
assetsEnabledtoggling, anddisableAssets()/enableAssets()all assert the expected presence/absence ofprofileevents, with leeway for at most one in-flight sample after disabling and verification that profiles resume once assets are re-enabled. Migrating toawait setTimeout(...)improves clarity over nested timers. Only minor nit: the comment in the “enable continuous profiling during an active CPU profile” test still mentions starting without continuous profiling even though the shared test env enables it from the outset.Also applies to: 121-245, 255-266, 380-386, 430-446
agents/grpc/src/grpc_agent.cc (1)
379-382: Assets and continuous-CPU flags are now wired through config/reconfigure with safe cross-thread checks.Including
assetsEnabledinPopulateReconfigureEvent/reconfigure(), initializing the new atomics in the ctor, and snapshotting JSONassetsEnabled/contCpuProfileintoassets_enabled_/cont_cpu_profile_enabled_inconfig()removes the earlier race for start_*_from_js and letsdo_start_prof_init()cheaply gate:
- all asset starts when assets are disabled (
EAssetsDisabled), and- manual CPU profiles when continuous CPU profiling is enabled (
EInProgressError).The explicit
is_boolean()guards before storing into the atomics are a nice touch to keep malformed config from throwing.Also applies to: 421-433, 1179-1191, 1747-1750, 2084-2103
test/common/nsolid-grpc-agent/index.js (1)
391-393: Timeout resolves with undefined rather than rejecting.When the timeout fires, the promise resolves with
map(null)orundefinedrather than rejecting. Callers must explicitly check for null/undefined to detect timeout, which is easy to miss.Consider rejecting the promise on timeout for clearer error signaling:
const timeout = setTimeout(() => { this.#child.off('message', handler); - resolve(map ? map(null) : undefined); + reject(new Error(`Timeout waiting for message type: ${expect}`)); }, 30000); // 30 second timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(3 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(6 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- test/common/nsolid-grpc-agent/client.js
- test/common/nsolid-zmq-agent/client.js
- agents/grpc/proto/reconfigure.proto
- test/parallel/test-nsolid-heap-sampling-stream.js
- agents/grpc/src/grpc_errors.h
- test/parallel/test-nsolid-heap-profile-stream.js
- test/common/nsolid-zmq-agent/index.js
- test/parallel/test-nsolid-config-tracing.js
- agents/zmq/src/zmq_agent.h
- test/agents/test-grpc-heap-sampling.mjs
- test/agents/test-zmq-snapshot.mjs
- test/agents/test-grpc-profile.mjs
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-14T10:18:23.123Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/parallel/test-nsolid-heap-profile-stream.js:81-88
Timestamp: 2025-10-14T10:18:23.123Z
Learning: In nsolid.heapProfileStream() and nsolid.heapSamplingStream(), synchronous throws occur for argument validation errors and immediate state validation checks (like assets being disabled), while async error events are emitted for runtime errors that manifest during execution (like invalid threadId).
Applied to files:
test/parallel/test-nsolid-heap-sampling.jstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jslib/nsolid.jstest/agents/test-zmq-heap-sampling.mjs
📚 Learning: 2025-07-15T16:09:04.521Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 327
File: test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js:47-73
Timestamp: 2025-07-15T16:09:04.521Z
Learning: In the nsolid test suite, hardcoded timeouts are acceptable for timing-sensitive tests. The preferred approach when tests become flaky is to increase timeout values rather than implementing more complex synchronization mechanisms.
Applied to files:
test/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-tracing.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/parallel/test-nsolid-snapshot.js
📚 Learning: 2025-07-08T14:48:04.827Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
test/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-tracing.mjstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/agents/test-grpc-snapshot.mjstest/agents/test-zmq-heap-sampling.mjstest/parallel/test-nsolid-snapshot.jstest/agents/test-zmq-heap-profile.mjstest/agents/test-zmq-profile.mjs
📚 Learning: 2025-07-08T14:47:34.724Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
Applied to files:
test/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-tracing.mjstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/agents/test-grpc-snapshot.mjstest/agents/test-zmq-heap-sampling.mjstest/parallel/test-nsolid-snapshot.jstest/agents/test-zmq-heap-profile.mjstest/agents/test-grpc-heap-profile.mjs
📚 Learning: 2025-07-08T14:46:47.806Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Applied to files:
test/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-tracing.mjstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/agents/test-grpc-snapshot.mjstest/agents/test-zmq-heap-sampling.mjstest/parallel/test-nsolid-snapshot.jstest/agents/test-zmq-heap-profile.mjstest/agents/test-zmq-profile.mjstest/agents/test-grpc-heap-profile.mjs
📚 Learning: 2025-06-10T09:52:49.401Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Applied to files:
test/agents/test-zmq-tracing.mjstest/parallel/test-nsolid-snapshot.js
📚 Learning: 2025-12-02T14:38:52.884Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/agents/test-zmq-heap-sampling.mjs:135-201
Timestamp: 2025-12-02T14:38:52.884Z
Learning: In ZMQ agent tests, `playground.client.config(config)` exercises the nsolid.start() config surface by sending an IPC message to the child process, which then calls `nsolid.start(config)` before returning the current config. There is no `playground.child` API in the ZMQ test helpers.
Applied to files:
test/agents/test-zmq-tracing.mjstest/agents/test-zmq-heap-sampling.mjstest/agents/test-zmq-heap-profile.mjstest/agents/test-zmq-profile.mjs
📚 Learning: 2025-08-20T11:38:09.253Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 354
File: test/parallel/test-nsolid-config-saas.js:121-122
Timestamp: 2025-08-20T11:38:09.253Z
Learning: In the N|Solid codebase, when nsolid.js sets nsolidConfig.command = null, this null value is specifically designed to work with the C++ nsolid_api.cc UpdateConfig() method that uses nlohmann::json::merge_patch(). The merge patch operation with null results in the command field being undefined/removed in the final configuration, so tests should expect undefined rather than null for the final config values.
Applied to files:
test/agents/test-zmq-heap-sampling.mjs
🧬 Code graph analysis (9)
test/parallel/test-nsolid-heap-sampling.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/parallel/test-nsolid-heap-profile.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
lib/nsolid.js (2)
lib/internal/process/pre_execution.js (1)
threadId(726-726)agents/grpc/src/grpc_agent.cc (2)
config(1001-1194)config(1001-1001)
agents/grpc/src/proto/reconfigure.pb.h (1)
agents/grpc/src/proto/reconfigure.pb.cc (4)
ReconfigureBody(53-53)ReconfigureBody(267-270)_impl_(59-60)_impl_(85-86)
test/agents/test-grpc-tracing.mjs (1)
test/agents/test-zmq-tracing.mjs (1)
phase(411-411)
test/parallel/test-nsolid-snapshot.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/agents/test-zmq-heap-profile.mjs (2)
test/agents/test-grpc-heap-profile.mjs (3)
disabledConfig(319-319)enabledConfig(330-330)currentConfig(380-380)lib/nsolid.js (2)
err(447-447)err(537-537)
agents/grpc/src/proto/reconfigure.pb.cc (1)
agents/grpc/src/proto/reconfigure.pb.h (16)
ReconfigureBody(94-94)ReconfigureBody(95-96)ReconfigureBody(681-693)ReconfigureBody(694-698)ReconfigureBody(699-721)ReconfigureBody(722-726)ReconfigureBody(727-749)ReconfigureBody(750-754)ReconfigureBody(755-777)ReconfigureBody(778-782)ReconfigureBody(783-805)ReconfigureBody(806-810)ReconfigureBody(811-834)ReconfigureBody(836-841)ReconfigureBody(875-887)ReconfigureBody(890-903)
test/agents/test-zmq-profile.mjs (1)
test/agents/test-zmq-snapshot.mjs (4)
playground(309-309)disabledConfig(253-253)enabledConfig(264-264)currentConfig(288-288)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: coverage-linux
- GitHub Check: test-macOS
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: build-tarball
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
🔇 Additional comments (21)
test/parallel/test-nsolid-heap-sampling.js (3)
164-167: LGTM: Improved readability.The multi-line formatting improves code readability without changing semantics.
168-168: LGTM: Proper test orchestration and timeout handling.The new asset toggle test is integrated at the appropriate point after existing error path validation, and the
platformTimeoutwrapper correctly follows the established pattern for timing-sensitive tests.Also applies to: 174-175
177-248: LGTM: Comprehensive asset toggle test coverage.The new test function thoroughly validates the runtime asset control feature:
- Correctly tests both synchronous throws and async callback errors when assets are disabled (consistent with the learning about immediate state validation)
- Properly sequences operations to avoid race conditions by waiting for heap sampling completion before toggling state
- Uses
platformTimeoutconsistently for timing-sensitive operations- Validates the complete cycle: disable → verify errors → enable → successful operation → disable → verify errors → enable → final success
The deep nesting is consistent with the existing test style in this file.
agents/zmq/src/zmq_agent.cc (1)
662-662: LGTM!The initialization of
assets_enabled_totrueis correct, ensuring asset collection is enabled by default. Usingstd::atomic_boolprovides thread-safe access.test/parallel/test-nsolid-snapshot.js (1)
60-109: Snapshot/asset toggle sequencing now looks race‑free and aligned with intent
runSnapshotAssetsToggleTestsenables/disables assets only after each precedingsnapshot()call has either thrown synchronously or invoked its callback (mustCall/mustSucceed). This ordering should avoid the prior “snapshot already in progress” races while still exercising both error and success paths for assets being toggled at runtime. LGTM.test/agents/test-zmq-heap-sampling.mjs (1)
135-201: Well-structured test for assetsEnabled toggle via nsolid.start().The test correctly exercises the toggle flow: disabling assets returns a 422 error on heap sampling start, then re-enabling allows successful profiling. The use of
playground.client.config()is appropriate as it sends an IPC message to the child process which callsnsolid.start(). Based on learnings, this is the correct API for ZMQ agent tests.test/agents/test-zmq-profile.mjs (1)
120-177: Consistent test for assetsEnabled toggle in CPU profiling.The test follows the same pattern as the heap sampling test, correctly validating that disabling assets returns a 422 error and re-enabling allows successful profiling. The implementation is consistent with other ZMQ agent tests.
src/nsolid/nsolid_api.cc (1)
1164-1190: Correct gating of continuous profiler by assetsEnabled.The logic correctly ensures that when
assetsEnabledis false, continuous CPU profiling is disabled (contCpuProfile defaults to false). Only whenassetsEnabledis true are the actualcontCpuProfileandcontCpuProfileIntervalvalues read from the configuration. This properly integrates the new asset control with the existing continuous profiler functionality.lib/nsolid.js (5)
133-136: New public API surface for asset and trace control.The four new helper methods (
enableAssets,disableAssets,enableTraces,disableTraces) are appropriately added to the exported start object, providing a clean programmatic API for runtime control.
201-214: Correct getter implementations with appropriate defaults.The
assetsEnabledgetter defaults totrue(via nullish coalescing) andtracingEnableddefaults tofalse, which aligns with the expected behavior that assets are enabled by default while tracing must be explicitly enabled.
416-419: Synchronous throws for immediate state validation.The checks throw synchronously when
assetsEnabledisfalse, which is consistent with the established pattern where immediate state validation errors are thrown synchronously while runtime errors are emitted asynchronously. Based on learnings, this is the expected behavior for these stream APIs.Also applies to: 502-504
609-638: Enable/disable helper implementations are idempotent.Each helper checks the current state before calling
updateConfig, preventing unnecessary configuration updates when the state is already as desired. This is a good practice for avoiding redundant work and potential side effects.
1110-1120: Robust type normalization with optionToBool.The
optionToBoolhelper correctly handles various input types: booleans pass through, numbers check for non-zero, strings delegate toenvToBool, and null/undefined return undefined to signal "not specified". This enables flexible configuration from multiple sources.test/agents/test-zmq-heap-profile.mjs (1)
342-408: Consistent assetsEnabled toggle test for heap profiling.The test follows the established pattern across ZMQ agent tests, correctly validating the toggle flow for heap profiles. The explicit
trackAllocations: falseand corresponding assertions fortrace_function_count,trace_function_infos, andtrace_treebeing zero/empty are appropriate for this test case.test/parallel/test-nsolid-profile.js (2)
117-118: Good use of platformTimeout for cross-platform reliability.Replacing hardcoded
100withcommon.platformTimeout(100)improves test reliability across different platforms with varying performance characteristics. Based on learnings, this is the preferred approach.
120-182: Comprehensive assets toggle tests for CPU profiling.The
runAssetsToggleTestsfunction thoroughly exercises the assetsEnabled control:
- Disables assets via
nsolid.start()config and verifies sync/async errors- Re-enables via
nsolid.enableAssets()helper and verifies successful profiling- Disables via
nsolid.disableAssets()helper and verifies errors return- Re-enables and completes a final successful profile cycle
This covers both configuration surfaces (start config and helper methods) and both error patterns (sync throws and async callbacks).
test/parallel/test-nsolid-heap-profile.js (1)
124-204: Heap profile assets toggle test flow looks correct and well-integrated.The new
runHeapProfileAssetsToggleTests()sequence cleanly validates sync/async error behavior withassetsEnabledoff, re-enables viaenableAssets(), and only toggles back to disabled after an ongoing profile completes. Usingcommon.platformTimeout(100)for all waits should keep this stable across platforms. Based on learnings, this matches the pattern that immediate state validation (assets disabled) throws synchronously while runtime failures surface via the callback.agents/grpc/src/grpc_agent.h (1)
351-352: Atomic profiling flags are appropriate here.Declaring
assets_enabled_andcont_cpu_profile_enabled_as atomics is consistent with other flags and supports the cross-thread checks indo_start_prof_initwithout further locking.test/agents/test-grpc-snapshot.mjs (1)
246-367: Snapshot tests cover assetsEnabled across env, config, and helpers effectively.The three new cases correctly assert the 500 /
Assets collection disabled(1008)error when assets are off, and they verify that bothconfig({ assetsEnabled })anddisableAssets()/enableAssets()round-trip the flag and restore successful snapshots afterwards.test/agents/test-grpc-heap-profile.mjs (1)
259-389: Heap-profile assets gating tests are consistent and symmetric with snapshot coverage.These three tests correctly validate that heap profiling returns
500 / Assets collection disabled(1008)when assets are disabled via env,config({ assetsEnabled: false }), ordisableAssets(), and that re-enabling restores normal profile data. The use ofheapProfile.trackAllocationskeeps behavior explicit but simple.test/agents/test-grpc-tracing.mjs (1)
5-5: gRPC tracing tests now validate disabled and reenabled phases explicitly.The added phase logic around
disableTraces()/enableTraces()ensures HTTP and custom tracing each prove three properties: initial spans are valid, nospansarrive during a 200ms disabled window (resourceSpansremains empty), and spans resume with the same structure after re-enabling. Importingdelayfrom timers/promises keeps the flow readable and consistent with the ZMQ tracing toggle test.Also applies to: 205-247, 273-315
Add comprehensive runtime control for asset collection (CPU/heap
profiling, snapshots) with multiple configuration methods.
Implements assetsEnabled flag with full control surface:
- NSOLID_ASSETS_ENABLED environment variable
- nsolid.start({ assetsEnabled: true/false }) configuration
- Runtime helpers: enableAssets() and disableAssets()
- Dynamic updates via gRPC reconfigure protocol
- Validation in both gRPC and ZMQ agents with proper errors
- Continuous profiler integration respects assetsEnabled state
This allows operators to disable expensive profiling operations
at runtime without process restarts, improving resource management
and production safety.
Also adds missing enableTraces()/disableTraces() helper methods
to complete the tracing API surface.
Comprehensive test coverage added for all asset types across gRPC,
ZMQ, and parallel test suites.
santigimeno
force-pushed
the
santi/assets_enabled
branch
from
a7459d3 to
8a8eb19
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
agents/zmq/src/zmq_agent.cc (1)
1264-1288: Consider adding a type guard when readingassetsEnabledfromconfig_.The new block correctly wires the top‑level
assetsEnabledconfig intoassets_enabled_, but it assigns the JSON value directly without checkingis_boolean(). If an invalid type ever sneaks in via the initial config (as opposed to the reconfigure path, which you now validate), this could throw or yield surprising behavior depending on the JSON library’s conversion rules.You might want to mirror the
validate_reconfigure()check here, e.g. ignore/log non‑boolean values and only updateassets_enabled_when the value is actually a boolean.test/common/nsolid-grpc-agent/index.js (1)
346-407: Centralized send/wait helper is good; keep payload/type ordering and timeout behavior in mindThe new helpers (
config, enable/disable assets & traces,trace) built on#sendAndWaitmake the TestClient much easier to use and extend, and the 30s timeout prevents hung tests.Two caveats (one previously noted):
Payload spread order (previously flagged):
this.#child.send({ type, ...payload });still letspayload.typeoverride the explicittype. Today your payloads don’t containtype, but for robustness it’s safer to send{ ...payload, type }so the call site’stypecan’t be clobbered.Timeout behavior (optional):
For methods without amap(e.g.,enableAssets()), a timeout resolves the promise silently after 30s. If you ever need stronger guarantees, consider rejecting with an Error on timeout so tests fail fast when the client stops responding, and optionally guardthis.#childin the timeout/handler to avoidoffonnullafter child exit.These are refinements; the overall structure and new API surface look solid.
🧹 Nitpick comments (5)
test/parallel/test-nsolid-heap-sampling.js (1)
177-248: Assets toggle coverage looks solid; consider reducing nesting if it grows furtherThe new
runHeapSamplingAssetsToggleTests()nicely exercises the key transitions:
- Disabling assets via
nsolid.start({ assetsEnabled: false })and validating both the syncheapSampling()throw path and async error callback path.- Re‑enabling with
nsolid.enableAssets()and confirming a full successfulheapSampling/heapSamplingEndcycle.- Disabling again with
nsolid.disableAssets()and re‑validating the error behavior, then finally re‑enabling and confirming success.The sequencing (waiting for callbacks before toggling, and using
common.platformTimeout(100)between config changes and assertions) keeps race windows small and matches existing patterns in related asset tests. From a correctness standpoint this looks good.If you touch this again, you might consider a small optional refactor (e.g., extracting the repeated “assert start/stop fail” block into a tiny helper or using named inner functions) to shave some callback/timeout nesting, but it’s not required for this PR.
Based on learnings, this matches the expected sync/async error patterns for state validation in these APIs.
test/agents/test-grpc-tracing.mjs (2)
185-248: HTTP tracing phase machine correctly validates enable/disable behaviorThe new
phase‑based logic plusmustCallAtLeast(..., 2)cleanly exercises:
- Initial tracing on (
phase === 'initial') and validates the HTTP spans.- Runtime
disableTraces()plus an extratrace('http'), asserting no spans arrive after a 200ms delay.- Re‑enabling traces and confirming spans are emitted again before shutting down the client.
Control flow and use of
resourceSpans.length = 0avoids stale data, and thephase === 'done'guard prevents post‑completion noise. Consider (optional) using acommon.platformTimeout(200)-style helper instead of a hard-coded 200ms if these ever get flaky.
253-315: Custom tracing toggle flow mirrors HTTP path and looks soundThe custom tracing test mirrors the HTTP test’s
phasemachine:
- Validates the expected single custom span when tracing is initially enabled.
- Disables traces and confirms no spans arrive after emitting another custom trace and waiting 200ms.
- Re‑enables, sends another trace, and validates spans again before shutdown.
The symmetry with the HTTP case makes behavior easy to reason about and keeps tests consistent. Same optional note as above about potentially using a platform‑scaled delay helper if timing ever proves flaky.
agents/grpc/src/grpc_agent.cc (1)
320-383: gRPC agent assets/tracing gating is wired correctly across config and profiling
Constructor +
config():
- Initializing
assets_enabled_totrueandcont_cpu_profile_enabled_tofalse, then updating them inconfig()viais_boolean()checks andstore(..., std::memory_order_release)is a solid pattern for safely reflecting JSON config in atomics.- Reads in
do_start_prof_init()useload(std::memory_order_acquire), which is appropriate for this simple flag scenario.Reconfigure/propagation:
PopulateReconfigureEvent()now includesassetsEnabledfromGetConfig(), andreconfigure()mirrors that by propagatingbody.assetsenabled()back into the JS‑visible config viaUpdateConfig. This keeps console reconfigure, global config, and agent state consistent for the new flag.Profiling gating (
do_start_prof_init):
- Early return with
EAssetsDisabledwhenassets_enabled_is false correctly prevents all manual asset profiles from starting and will surface as the expected JS/console errors.- The continuous CPU guard (
type == kCpu && cont_cpu_profile_enabled_.load(...) → EInProgressError) avoids overlapping manual CPU profiles with the continuous profiler, and importantly no longer touchesconfig_off the agent thread.Overall this closes the previously noted race on
assetsEnabled/contCpuProfilewhile providing a clear, centralized gate for asset collection. Only minor nit (optional):PopulateReconfigureEventsetsassetsenabledwithout anis_boolean()check, unlikeconfig(); if malformed config is a concern there too, you could mirror the same type guard.Also applies to: 421-433, 1001-1194, 1703-1750, 2085-2152
lib/nsolid.js (1)
133-137: JS‑level assets/tracing toggles and stream gating are coherent with agent behavior
Public API & getters:
- Exposing
enableAssets/disableAssetsandenableTraces/disableTracesonstartand wiring them throughupdateConfigis the right abstraction, and the cheapgetConfig(...) === true/falseshort‑circuit avoids redundant config updates.- The
assetsEnabledandtracingEnabledgetters with??defaults (true/false) line up with the initialization logic and keepnsolid.config/info consumers simple.Heap stream gating:
- The early checks in
heapProfileStream/heapSamplingStreamthat throwERR_NSOLID_HEAP_PROFILE_START/ERR_NSOLID_HEAP_SAMPLING_STARTwhenassetsEnabled === falsematch the documented behavior that immediate state validation errors (like assets disabled) are synchronous, while runtime failures still surface via the stream’serrorpath. This is consistent with the existing test-suite expectations. Based on learnings.Config normalization:
initializeConfig’s tri‑state handling ofNSOLID_ASSETS_ENABLEDandpkgConfig.nsolid.assetsEnabledviaoptionToBool, plus a default oftruewhen unset, ensures explicit false is preserved rather than overridden by||chains.- The
updateConfigbranch forassetsEnabledreusesoptionToBoolso JS APIs accept booleans, numbers, and env‑style strings without leaking odd types into the native config.Overall the wiring between JS config, the global config store, and the C++ agents looks correct. Only tiny nit:
heapProfileStream’s newoptionsparameter is currently unused; you can drop it or hook it up later when you actually need extra options.Also applies to: 201-215, 416-419, 501-504, 609-639, 951-961, 1110-1120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/proto/reconfigure.pb.cc(18 hunks)agents/grpc/src/proto/reconfigure.pb.h(4 hunks)agents/zmq/src/zmq_agent.cc(5 hunks)agents/zmq/src/zmq_agent.h(1 hunks)lib/nsolid.js(8 hunks)src/nsolid/nsolid_api.cc(1 hunks)test/agents/test-grpc-continuous-profile.mjs(6 hunks)test/agents/test-grpc-heap-profile.mjs(1 hunks)test/agents/test-grpc-heap-sampling.mjs(1 hunks)test/agents/test-grpc-profile.mjs(1 hunks)test/agents/test-grpc-snapshot.mjs(1 hunks)test/agents/test-grpc-tracing.mjs(3 hunks)test/agents/test-zmq-heap-profile.mjs(1 hunks)test/agents/test-zmq-heap-sampling.mjs(1 hunks)test/agents/test-zmq-profile.mjs(1 hunks)test/agents/test-zmq-snapshot.mjs(1 hunks)test/agents/test-zmq-tracing.mjs(2 hunks)test/common/nsolid-grpc-agent/client.js(2 hunks)test/common/nsolid-grpc-agent/index.js(1 hunks)test/common/nsolid-zmq-agent/client.js(1 hunks)test/common/nsolid-zmq-agent/index.js(1 hunks)test/parallel/test-nsolid-config-tracing.js(1 hunks)test/parallel/test-nsolid-heap-profile-stream.js(1 hunks)test/parallel/test-nsolid-heap-profile.js(1 hunks)test/parallel/test-nsolid-heap-sampling-stream.js(2 hunks)test/parallel/test-nsolid-heap-sampling.js(1 hunks)test/parallel/test-nsolid-profile.js(1 hunks)test/parallel/test-nsolid-snapshot.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- test/parallel/test-nsolid-heap-profile-stream.js
- test/common/nsolid-zmq-agent/client.js
- test/agents/test-zmq-heap-profile.mjs
- test/agents/test-zmq-profile.mjs
- test/parallel/test-nsolid-config-tracing.js
- agents/zmq/src/zmq_agent.h
- test/agents/test-zmq-tracing.mjs
- test/agents/test-grpc-profile.mjs
- test/common/nsolid-zmq-agent/index.js
- agents/grpc/src/grpc_agent.h
- test/agents/test-grpc-heap-sampling.mjs
- test/parallel/test-nsolid-heap-sampling-stream.js
- test/agents/test-grpc-heap-profile.mjs
- test/common/nsolid-grpc-agent/client.js
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-07-08T14:46:47.806Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Applied to files:
test/agents/test-grpc-snapshot.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-snapshot.jstest/agents/test-zmq-snapshot.mjstest/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-heap-sampling.mjs
📚 Learning: 2025-07-08T14:47:34.724Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
Applied to files:
test/agents/test-grpc-snapshot.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-snapshot.jstest/agents/test-zmq-snapshot.mjstest/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-heap-sampling.mjs
📚 Learning: 2025-07-08T14:48:04.827Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
test/agents/test-grpc-snapshot.mjstest/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/agents/test-grpc-continuous-profile.mjstest/parallel/test-nsolid-snapshot.jstest/agents/test-zmq-snapshot.mjstest/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-heap-sampling.mjs
📚 Learning: 2025-07-15T16:09:04.521Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 327
File: test/addons/nsolid-code-event-hook/nsolid-code-event-hook.js:47-73
Timestamp: 2025-07-15T16:09:04.521Z
Learning: In the nsolid test suite, hardcoded timeouts are acceptable for timing-sensitive tests. The preferred approach when tests become flaky is to increase timeout values rather than implementing more complex synchronization mechanisms.
Applied to files:
test/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jstest/parallel/test-nsolid-snapshot.jstest/parallel/test-nsolid-heap-sampling.js
📚 Learning: 2025-10-14T10:18:23.123Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/parallel/test-nsolid-heap-profile-stream.js:81-88
Timestamp: 2025-10-14T10:18:23.123Z
Learning: In nsolid.heapProfileStream() and nsolid.heapSamplingStream(), synchronous throws occur for argument validation errors and immediate state validation checks (like assets being disabled), while async error events are emitted for runtime errors that manifest during execution (like invalid threadId).
Applied to files:
test/parallel/test-nsolid-profile.jstest/parallel/test-nsolid-heap-profile.jslib/nsolid.jstest/parallel/test-nsolid-heap-sampling.jstest/agents/test-zmq-heap-sampling.mjs
📚 Learning: 2025-10-27T13:06:17.506Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 0
File: :0-0
Timestamp: 2025-10-27T13:06:17.506Z
Learning: In the nodesource/nsolid repository, when reviewing PRs that add or modify V8 patches (e.g., in deps/v8/patches/), focus review comments on the patch management infrastructure (configure.py, apply-patches.sh, README.md, etc.) and not on the V8 code changes contained within the .patch files themselves, as the patch content is out of scope for the patch management PR.
Applied to files:
lib/nsolid.js
📚 Learning: 2025-06-10T09:52:49.401Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Applied to files:
test/parallel/test-nsolid-snapshot.js
📚 Learning: 2025-12-02T14:38:52.884Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/agents/test-zmq-heap-sampling.mjs:135-201
Timestamp: 2025-12-02T14:38:52.884Z
Learning: In ZMQ agent tests, `playground.client.config(config)` exercises the nsolid.start() config surface by sending an IPC message to the child process, which then calls `nsolid.start(config)` before returning the current config. There is no `playground.child` API in the ZMQ test helpers.
Applied to files:
test/agents/test-zmq-snapshot.mjstest/agents/test-zmq-heap-sampling.mjs
📚 Learning: 2025-08-20T11:38:09.253Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 354
File: test/parallel/test-nsolid-config-saas.js:121-122
Timestamp: 2025-08-20T11:38:09.253Z
Learning: In the N|Solid codebase, when nsolid.js sets nsolidConfig.command = null, this null value is specifically designed to work with the C++ nsolid_api.cc UpdateConfig() method that uses nlohmann::json::merge_patch(). The merge patch operation with null results in the command field being undefined/removed in the final configuration, so tests should expect undefined rather than null for the final config values.
Applied to files:
test/agents/test-zmq-heap-sampling.mjs
🧬 Code graph analysis (9)
test/parallel/test-nsolid-profile.js (2)
lib/nsolid.js (2)
err(447-447)err(537-537)agents/grpc/src/grpc_errors.h (1)
nsolid(16-33)
agents/grpc/src/grpc_agent.cc (1)
agents/zmq/src/zmq_agent.cc (10)
config(430-476)config(430-430)config(1264-1379)config(1264-1264)config(2767-2802)config(2767-2767)config(2895-2912)config(2895-2895)config(2990-3007)config(2990-2990)
test/agents/test-grpc-tracing.mjs (1)
test/agents/test-zmq-tracing.mjs (1)
phase(411-411)
test/common/nsolid-grpc-agent/index.js (1)
test/common/nsolid-grpc-agent/client.js (1)
msg(89-89)
lib/nsolid.js (2)
agents/grpc/src/grpc_agent.cc (2)
config(1001-1194)config(1001-1001)agents/zmq/src/zmq_agent.cc (10)
config(430-476)config(430-430)config(1264-1379)config(1264-1264)config(2767-2802)config(2767-2767)config(2895-2912)config(2895-2895)config(2990-3007)config(2990-2990)
agents/grpc/src/proto/reconfigure.pb.h (1)
agents/grpc/src/proto/reconfigure.pb.cc (4)
ReconfigureBody(53-53)ReconfigureBody(267-270)_impl_(59-60)_impl_(85-86)
test/agents/test-zmq-snapshot.mjs (1)
test/agents/test-grpc-snapshot.mjs (3)
disabledConfig(300-300)enabledConfig(311-311)currentConfig(358-358)
test/parallel/test-nsolid-heap-sampling.js (1)
lib/nsolid.js (2)
err(447-447)err(537-537)
test/agents/test-zmq-heap-sampling.mjs (1)
test/agents/test-grpc-heap-sampling.mjs (4)
options(86-100)options(127-141)disabledConfig(308-308)enabledConfig(319-319)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: coverage-linux
- GitHub Check: test-macOS
- GitHub Check: build-tarball
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
🔇 Additional comments (23)
test/agents/test-zmq-snapshot.mjs (1)
235-296: New ZMQ snapshot assetsEnabled toggle test correctly exercises config and event flowThe test wiring looks solid and consistent with the existing patterns:
- Uses
playground.client.config({ assetsEnabled: false/true })to drive thensolid.start()config surface, matching how ZMQ tests exercise config elsewhere (per prior heap-sampling tests, this is the expected API surface in ZMQ tests, notplayground.child).- Validates the disabled path via a single
agentSnapshotRequestwith amustCallerror callback, asserting the expected 422/'Invalid arguments'contract that other ZMQ snapshot error tests in this file also rely on.- After re-enabling, it issues a snapshot without a callback and uses the familiar
eventscounter +checkSnapshotData+snapshotconcatenation pattern already used in the earlier snapshot tests, then confirmsassetsEnabledstaystruevia a finalplayground.client.config()call.- The
resolvedguard in the async event handler is a reasonable extra safety to avoid double-resolving the outer Promise if any unexpected extra events arrive.If you want to tighten things further, you could optionally assert some property of the parsed
snapshotJSON (beyond just that it parses) to make the success path slightly stronger, but as-is this test already gives good coverage of the assetsEnabled gating behavior and config surface.test/parallel/test-nsolid-heap-sampling.js (2)
164-168: Good integration point for assets toggle sub-testsTriggering
runHeapSamplingAssetsToggleTests()only after the final'Heap sampling could not be stopped'assertion ensures the earlier sampling lifecycle tests complete before you start exercising the assetsEnabled transitions, which keeps state interactions predictable. The expanded multi-lineassert.strictEqualis also clearer and consistent with nearby style.
174-176: Usingcommon.platformTimeout(100)improves cross-platform stabilitySwitching from literal
100tocommon.platformTimeout(100)keeps the original intent while respecting the test timeout scaling used in this suite, which should reduce flakiness on slower environments.agents/zmq/src/zmq_agent.cc (3)
646-664: Defaultingassets_enabled_to true preserves existing behavior.Initializing
assets_enabled_totruein the ctor matches the previous implicit “assets always on” behavior and makes the new flag opt‑out only, which is consistent with the PR intent.
2113-2118: Central assets gating indo_start_prof_initlooks good.Putting the
assets_enabled_check at the top ofdo_start_prof_init()cleanly gates all profile/snapshot entry points (both console‑initiated and*_from_jshelpers) behind a single flag, which aligns with the PR’s runtime control objective.
2494-2519: Reconfigure support and boolean validation forassetsEnabledare consistent.Adding
"assetsEnabled"toallowed_optsand enforcingval.is_boolean()invalidate_reconfigure()ensures the flag is actually reconfigurable over ZMQ and prevents bad types from being applied, returning a clear 422 error with a specific message when invalid.test/agents/test-zmq-heap-sampling.mjs (1)
135-201: LGTM! Well-structured test for assetsEnabled toggle behavior.The test correctly exercises the assetsEnabled toggle via
playground.client.config(), which properly invokesnsolid.start()in the child process via IPC (based on learnings). The flow appropriately verifies:
- Assets disabled → heap sampling fails with 422
- Assets re-enabled → heap sampling succeeds
- Final config state is validated
The
resolvedguard prevents double-resolution race conditions.src/nsolid/nsolid_api.cc (1)
1164-1190: LGTM! Correct gating of continuous profiler settings behind assetsEnabled.The logic properly:
- Triggers on changes to
assetsEnabled,contCpuProfile, orcontCpuProfileInterval- Defaults
assetsEnabledtotruefor backward compatibility- Only reads profiler settings when assets are enabled; otherwise uses defaults that disable profiling
- Calls
update_continuous_profiler()with the resolved valuesThis ensures the continuous profiler is stopped when assets are disabled and respects profiler settings when assets are enabled.
agents/grpc/proto/reconfigure.proto (1)
20-20: LGTM! Clean proto field addition.The
optional bool assetsEnabledfield follows the established pattern in the message and uses an appropriate sequential field number.agents/grpc/src/grpc_errors.h (1)
12-13: LGTM! Appropriate error definition for assets-disabled state.The new
EAssetsDisablederror follows the established macro pattern, uses a sequential runtime code (1008), and aligns with similar errors likeESnapshotDisabledin using HTTP status 500.test/parallel/test-nsolid-snapshot.js (1)
60-110: LGTM! Well-sequenced test for assets toggle with snapshots.The test properly addresses the previous race condition concern by:
- Waiting for snapshot completion callbacks before toggling assets state
- Using
common.mustSucceed()to verify successful operations- Chaining operations through callbacks to ensure deterministic ordering
The nested structure, while deep, correctly ensures each snapshot completes before the next asset state change.
test/agents/test-grpc-continuous-profile.mjs (4)
121-156: LGTM! Good test for environment-variable-based asset disabling.The test correctly verifies that no continuous profiles are emitted when
NSOLID_ASSETS_ENABLED: '0'is set in the environment. The 300ms wait (slightly longer than two 100ms intervals) is appropriate for validating no emissions occur.
158-201: LGTM! Well-designed test for config-based asset toggle.The test properly validates:
- Initial profiling emits at least once
- After disabling via config, at most one additional profile arrives (in-flight tolerance)
- After re-enabling, profiling resumes with increased count
The
<= 1tolerance at line 189 appropriately accounts for an in-flight profile during the transition.
203-245: LGTM! Comprehensive test for helper-based asset toggle.This test exercises
disableAssets()/enableAssets()helpers and validates both the profiling behavior and final config state verification at line 237.
247-294: LGTM! Clean refactor of worker threads test.Inlining the environment setup and removing duplicated initialization improves readability while maintaining the same test coverage.
test/agents/test-grpc-snapshot.mjs (3)
246-280: LGTM! Proper test for env-based asset disabling.The test correctly validates that heap snapshots return error code 500 with the expected "Assets collection disabled(1008)" message when
NSOLID_ASSETS_ENABLED: '0'is set.
282-323: LGTM! Good test for config-based asset toggle with snapshots.The test properly validates both the disabled and enabled paths, verifying config state assertions at lines 301 and 312.
325-367: LGTM! Complete test for helper-based asset toggle with snapshots.The test exercises the full cycle: disable → verify error → enable → verify success → verify final config state at line 359.
test/parallel/test-nsolid-profile.js (2)
117-118: Good improvement: usingplatformTimeoutfor CI stability.Replacing hardcoded timeouts with
common.platformTimeout(100)improves test reliability across different CI environments and platforms.
120-182: LGTM! Well-structured test for profile assets toggle.The test properly sequences operations:
- Disables assets via config → verifies profile/profileEnd fail
- Enables via helper → starts and completes profile before next toggle
- Disables via helper → verifies profile fails
- Re-enables → verifies profile succeeds
The nested callbacks with
common.mustSucceed()ensure each profile operation completes before toggling asset state, avoiding race conditions.test/parallel/test-nsolid-heap-profile.js (1)
124-204: Assets toggle heap-profile flow looks correct and race‑safeThe new
runHeapProfileAssetsToggleTests()sequence correctly:
- Disables assets via
nsolid.start({ assetsEnabled: false })and asserts both sync and callback error paths forheapProfile/heapProfileEnd.- Re‑enables assets before starting a profile, waits for
heapProfile+heapProfileEndto succeed, then disables assets only after completion.- Verifies errors again with assets disabled, then re‑enables and runs a final successful profile.
Usingcommon.platformTimeout(100)between config changes and operations is consistent with existing tests and provides reasonable slack for agent reconfiguration. I don’t see correctness or flakiness risks beyond the usual timeout tuning.agents/grpc/src/proto/reconfigure.pb.h (1)
219-235: GeneratedassetsEnabledfield wiring in ReconfigureBody is consistentThe additions for
assetsEnabled(field number 13) inReconfigureBody—enum constant, public/ internal accessors, has‑bit mask0x00000800U, and the newassetsenabled_member inImpl_—are all consistent with the existing pattern for other optional bools likecontCpuProfile. The parse table template and inline methods look mechanically generated and match the corresponding.cclayout.Also applies to: 392-447, 1175-1201
agents/grpc/src/proto/reconfigure.pb.cc (1)
29-51: Protobuf implementation forassetsEnabledis consistent with the header and protoThe
.ccupdates forReconfigureBody.assetsEnabled—Impl_ initialization, offsets/has‑bit layout, descriptor protodef string, parse table entry for field 13, and the plumbing inClear,_InternalSerialize,ByteSizeLong,MergeImpl, andInternalSwap—all align with the new header definitions and appear mechanically generated. I don’t see any inconsistencies in offsets, masks, or serialization logic.Also applies to: 103-147, 153-173, 329-439, 440-472, 581-587, 611-625, 663-717, 728-745
Add comprehensive runtime control for asset collection (CPU/heap
profiling, snapshots) with multiple configuration methods.
Implements assetsEnabled flag with full control surface:
- NSOLID_ASSETS_ENABLED environment variable
- nsolid.start({ assetsEnabled: true/false }) configuration
- Runtime helpers: enableAssets() and disableAssets()
- Dynamic updates via gRPC reconfigure protocol
- Validation in both gRPC and ZMQ agents with proper errors
- Continuous profiler integration respects assetsEnabled state
This allows operators to disable expensive profiling operations
at runtime without process restarts, improving resource management
and production safety.
Also adds missing enableTraces()/disableTraces() helper methods
to complete the tracing API surface.
Comprehensive test coverage added for all asset types across gRPC,
ZMQ, and parallel test suites.
PR-URL: #377
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
|
Landed in 75faeaa |
Add comprehensive runtime control for asset collection (CPU/heap
profiling, snapshots) with multiple configuration methods.
Implements assetsEnabled flag with full control surface:
- NSOLID_ASSETS_ENABLED environment variable
- nsolid.start({ assetsEnabled: true/false }) configuration
- Runtime helpers: enableAssets() and disableAssets()
- Dynamic updates via gRPC reconfigure protocol
- Validation in both gRPC and ZMQ agents with proper errors
- Continuous profiler integration respects assetsEnabled state
This allows operators to disable expensive profiling operations
at runtime without process restarts, improving resource management
and production safety.
Also adds missing enableTraces()/disableTraces() helper methods
to complete the tracing API surface.
Comprehensive test coverage added for all asset types across gRPC,
ZMQ, and parallel test suites.
PR-URL: #377
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Add comprehensive runtime control for asset collection (CPU/heap
profiling, snapshots) with multiple configuration methods.
Implements assetsEnabled flag with full control surface:
- NSOLID_ASSETS_ENABLED environment variable
- nsolid.start({ assetsEnabled: true/false }) configuration
- Runtime helpers: enableAssets() and disableAssets()
- Dynamic updates via gRPC reconfigure protocol
- Validation in both gRPC and ZMQ agents with proper errors
- Continuous profiler integration respects assetsEnabled state
This allows operators to disable expensive profiling operations
at runtime without process restarts, improving resource management
and production safety.
Also adds missing enableTraces()/disableTraces() helper methods
to complete the tracing API surface.
Comprehensive test coverage added for all asset types across gRPC,
ZMQ, and parallel test suites.
PR-URL: #377
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
3 participants
Add comprehensive runtime control for asset collection (CPU/heap profiling, snapshots) with multiple configuration methods.
Implements assetsEnabled flag with full control surface:
This allows operators to disable expensive profiling operations at runtime without process restarts, improving resource management and production safety.
Also adds missing enableTraces()/disableTraces() helper methods to complete the tracing API surface.
Comprehensive test coverage added for all asset types across gRPC, ZMQ, and parallel test suites.
Summary by CodeRabbit
New Features
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.