src,lib,test: make metrics interval updates thread-safe#472
Open
santigimeno wants to merge 4 commits into
Open
src,lib,test: make metrics interval updates thread-safe#472santigimeno wants to merge 4 commits into
santigimeno wants to merge 4 commits into
Conversation
Also, move metrics interval state into EnvList.
Refactor NSolid metrics interval handling so configuration and updates are managed in a single place in JS, and the native API no longer exposes a separate setMetricsInterval entry point. This reduces duplicated logic between JS and C++ and makes interval changes easier to reason about.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughMetrics interval control moved from a JS binding to EnvList configuration: EnvList stores an atomic gen_ptiles_interval_ updated in UpdateConfig(), the ptiles timer uses that atomic value and restarts when changed, JS parses/stores top-level ChangesMetrics Interval Reconfiguration Migration
Sequence Diagram(s)sequenceDiagram
participant GRPCClient
participant GRPCServer
participant EnvList
participant gen_ptiles_timer
GRPCClient->>GRPCServer: reconfigure(agentId, { interval: NEW })
GRPCServer->>EnvList: UpdateConfig({ interval: NEW })
EnvList->>EnvList: gen_ptiles_interval_.store(NEW)
EnvList->>gen_ptiles_timer: uv_timer_get_repeat() vs gen_ptiles_interval_.load()
alt intervals differ
EnvList->>gen_ptiles_timer: stop & start with NEW repeat
end
gen_ptiles_timer->>GRPCServer: emit metrics at new cadence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/nsolid.js`:
- Around line 773-775: The code assigns nsolidConfig.interval = +config.interval
without validation, allowing NaN/Infinity/0/negative values; update the branch
that handles key === 'interval' to parse and validate config.interval (e.g.,
Number(config.interval) or parseFloat) and only assign if Number.isFinite(value)
&& value > 0, otherwise skip assignment or leave the existing
nsolidConfig.interval unchanged (or set a safe default); reference the
'interval' branch, the nsolidConfig object, and config.interval when making this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b6bf2c7-87ad-443b-9cdc-ced5b188e705
📒 Files selected for processing (4)
lib/nsolid.jssrc/nsolid/nsolid_api.ccsrc/nsolid/nsolid_api.htest/agents/test-grpc-metrics-interval-reconfigure.mjs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/nsolid.js (1)
1172-1195: ⚡ Quick winUse this parser in startup initialization too for consistent interval semantics.
parsePositiveFiniteNumber()is solid, but startup still uses permissive coercion ininitializeConfig()(Line 915), so invalid/negative env or package values can bypass this validation path. Reusing this helper there keeps startup and runtime behavior aligned.Proposed minimal diff
- nsolidConfig.interval = - +process.env.NSOLID_INTERVAL || - pkgConfig.nsolid.interval || - DEFAULT_INTERVAL; + const envInterval = parsePositiveFiniteNumber(process.env.NSOLID_INTERVAL); + const pkgInterval = parsePositiveFiniteNumber(pkgConfig.nsolid.interval); + nsolidConfig.interval = envInterval ?? pkgInterval ?? DEFAULT_INTERVAL;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/nsolid.js` around lines 1172 - 1195, Startup initialization currently coerces env/package interval values permissively; update initializeConfig to call parsePositiveFiniteNumber when interpreting numeric configuration (e.g., interval, timeout) instead of using direct +value or permissive coercion so negative, NaN, empty-string or non-finite inputs return undefined; locate initializeConfig and replace the existing numeric coercion logic with a call to parsePositiveFiniteNumber(value) and handle its undefined return the same way startup treats missing config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/nsolid.js`:
- Around line 1172-1195: Startup initialization currently coerces env/package
interval values permissively; update initializeConfig to call
parsePositiveFiniteNumber when interpreting numeric configuration (e.g.,
interval, timeout) instead of using direct +value or permissive coercion so
negative, NaN, empty-string or non-finite inputs return undefined; locate
initializeConfig and replace the existing numeric coercion logic with a call to
parsePositiveFiniteNumber(value) and handle its undefined return the same way
startup treats missing config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4441d74e-5b09-4087-9135-c409c9898791
📒 Files selected for processing (2)
lib/nsolid.jstest/parallel/test-nsolid-config-interval.js
santigimeno
force-pushed
the
santi/interval_thread_safe
branch
from
b5397f8 to
fcf7417
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/parallel/test-nsolid-config-interval-env.js (1)
33-72: ⚡ Quick winConsider adding test cases for standalone negative interval and env-package precedence.
The current test coverage is solid, but two additional scenarios would strengthen it:
Standalone negative
NSOLID_INTERVAL(withoutNSOLID_PACKAGE_JSON): Currently tested with package.json (lines 66-72), but not alone. Adding a test forNSOLID_INTERVAL: '-1'without package should verify fallback to 5000, similar to the zero test (lines 52-57).Valid
NSOLID_INTERVALoverriding package value: Test precedence when bothNSOLID_PACKAGE_JSON(3000) and a validNSOLID_INTERVAL(e.g., '2500') are provided to clarify which source takes priority.📋 Suggested additional test cases
// Test standalone negative interval falls back to default { const config = runWithEnv({ NSOLID_INTERVAL: '-1', }); assert.strictEqual(config.interval, 5000); } // Test valid env interval overrides package-derived value { const config = runWithEnv({ NSOLID_PACKAGE_JSON: pkgJson, NSOLID_INTERVAL: '2500', }); assert.strictEqual(config.interval, 2500); // or 3000 if package wins }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/parallel/test-nsolid-config-interval-env.js` around lines 33 - 72, Add two tests to cover missing negative-interval and env-vs-package precedence: create a case calling runWithEnv with NSOLID_INTERVAL: '-1' only and assert config.interval === 5000 to ensure negative env values fall back to default, and another case calling runWithEnv with both NSOLID_PACKAGE_JSON: pkgJson and NSOLID_INTERVAL: '2500' and assert config.interval === 2500 to verify a valid NSOLID_INTERVAL overrides the package-derived interval; reference runWithEnv, NSOLID_INTERVAL, NSOLID_PACKAGE_JSON and the config.interval assertions when adding these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/parallel/test-nsolid-config-interval-env.js`:
- Around line 33-72: Add two tests to cover missing negative-interval and
env-vs-package precedence: create a case calling runWithEnv with
NSOLID_INTERVAL: '-1' only and assert config.interval === 5000 to ensure
negative env values fall back to default, and another case calling runWithEnv
with both NSOLID_PACKAGE_JSON: pkgJson and NSOLID_INTERVAL: '2500' and assert
config.interval === 2500 to verify a valid NSOLID_INTERVAL overrides the
package-derived interval; reference runWithEnv, NSOLID_INTERVAL,
NSOLID_PACKAGE_JSON and the config.interval assertions when adding these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1332c28-d5ec-4f0d-bf25-c1f5ae7f69c9
📒 Files selected for processing (5)
lib/nsolid.jstest/fixtures/nsolid-interval-package.jsontest/fixtures/test-nsolid-config-interval-env-script.jstest/parallel/test-nsolid-config-interval-env.jstest/parallel/test-nsolid-config-interval.js
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/nsolid-interval-package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- test/parallel/test-nsolid-config-interval.js
- lib/nsolid.js
Validate interval updates in lib/nsolid.js before assigning them to nsolidConfig.interval. Ignore invalid values such as 0, negative numbers, NaN, Infinity, and non-numeric strings so the previous valid interval is preserved. Add a parallel test covering valid coercion, invalid updates, and partial updates that omit interval.
santigimeno
force-pushed
the
santi/interval_thread_safe
branch
from
fcf7417 to
da61279
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes runtime metrics interval updates thread-safe and adds regression coverage for the gRPC reconfigure path.
Before this change, interval updates used a separate native setMetricsInterval path. This PR moves interval handling into the normal config update flow and stores the interval state on EnvList, so the metrics timer can safely observe and apply runtime updates.
This keeps interval updates in a single configuration path and avoids updating timer state through a separate unsynchronized mechanism. It also gives us an end-to-end test for the real behavior we care about: changing interval at runtime affects metrics export cadence without restarting the process.
Summary by CodeRabbit