agents,lib,src,test: add traceSampleRate support#430
Open
santigimeno wants to merge 1 commit intonode-v24.x-nsolid-v6.xfrom
Open
agents,lib,src,test: add traceSampleRate support#430santigimeno wants to merge 1 commit intonode-v24.x-nsolid-v6.xfrom
santigimeno wants to merge 1 commit intonode-v24.x-nsolid-v6.xfrom
Conversation
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a new traceSampleRate (double, 0–1) configuration propagated through protobufs, gRPC reconfigure, runtime C++ APIs, and JS tracing; implements validation, runtime propagation to per-env bindings, probabilistic root-span sampling, and accompanying tests and fixtures. Changes
Sequence DiagramsequenceDiagram
participant App as JavaScript App
participant Config as nsolid.config
participant gRPC as gRPC Agent
participant EnvList as EnvList (C++)
participant Binding as binding.trace_sample_rate
participant Tracer as OpenTelemetry Tracer
participant Span as Root Span
App->>Config: updateConfig({ traceSampleRate: X })
Config->>Config: parseTraceSampleRate(X)
Config->>gRPC: send reconfigure (traceSampleRate: X)
gRPC->>EnvList: receive reconfigure
EnvList->>EnvList: validate_trace_sample_rate -> sanitize
EnvList->>EnvList: update_tracing_sample_rate(X)
EnvList->>Binding: write trace_sample_rate (shared buffer)
App->>Tracer: start root span
Tracer->>Binding: read trace_sample_rate
Tracer->>Span: decide sampled = Math.random() < trace_sample_rate
alt sampled
Span->>Span: set traceFlags = SAMPLED
else not sampled
Span->>Span: set traceFlags = NONE
end
Span-->>Tracer: return span
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (1)
test/agents/test-grpc-reconfigure.mjs (1)
171-208: Consider addingNaN/Infinityinvalid-rate cases in this gRPC path test.This block currently checks only out-of-range finite numbers. Extending it with
Number.NaNand±Infinitywould better guard the gRPC reconfigure edge cases already covered in non-gRPC tests.✅ Minimal test extension
- const invalidRates = [2, -0.5]; + const invalidRates = [2, -0.5, Number.NaN, Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/agents/test-grpc-reconfigure.mjs` around lines 171 - 208, Update the test "should preserve previous traceSampleRate for invalid values" to include NaN and Infinity cases: add Number.NaN, Number.POSITIVE_INFINITY and Number.NEGATIVE_INFINITY (or +/-Infinity) to the invalidRates array used with grpcServer.reconfigure and client.config assertions so grpcServer.reconfigure(agentId, { traceSampleRate: invalidRate }) is exercised for NaN/Infinity and the existing assert.strictEqual checks that nsolidConfig.traceSampleRate remained 0.4 still apply.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/grpc/src/grpc_agent.cc`:
- Around line 1761-1763: PopulateReconfigureEvent currently omits the
traceSampleRate field when building the outgoing reconfigure event body; update
PopulateReconfigureEvent to set body.traceSampleRate from the internal config so
the outgoing event mirrors inbound updates (i.e., when you previously read
body.tracesamplerate() on incoming updates, ensure PopulateReconfigureEvent
calls the corresponding setter to populate body.traceSampleRate in the event).
Locate the PopulateReconfigureEvent function in grpc_agent.cc and add the
traceSampleRate assignment using the same source/field used for other mapped
settings so reconfigure responses include traceSampleRate.
In `@lib/nsolid.js`:
- Around line 1144-1154: The parseTraceSampleRate function currently coerces
unintended types via unary +; update parseTraceSampleRate to only accept inputs
that are typeof 'number' or 'string', explicitly reject booleans and other
types, trim string inputs and return undefined for empty/whitespace-only
strings, then convert the trimmed numeric string (or the number input) to a
numeric value and validate with NumberIsFinite and range checks (>=0 && <=1);
ensure the function returns undefined for invalid types, whitespace-only
strings, NaN, non-finite numbers, or values outside the 0–1 range while
returning the numeric rate for valid inputs.
In `@test/parallel/test-nsolid-config-trace-sample-rate-env.js`:
- Around line 15-18: The test currently spreads process.env into the child env
(env: {...process.env, ...envVars}), which can leak ambient NSOLID_* variables
and make the test nondeterministic; update the env construction in
test/parallel/test-nsolid-config-trace-sample-rate-env.js to explicitly filter
out any keys starting with "NSOLID_" (e.g., build a filteredEnv =
Object.fromEntries(Object.entries(process.env).filter(([k]) =>
!k.startsWith('NSOLID_'))) and then use env: { ...filteredEnv, ...envVars } so
the child process is isolated from ambient NSOLID_* variables while still
allowing envVars to override.
---
Nitpick comments:
In `@test/agents/test-grpc-reconfigure.mjs`:
- Around line 171-208: Update the test "should preserve previous traceSampleRate
for invalid values" to include NaN and Infinity cases: add Number.NaN,
Number.POSITIVE_INFINITY and Number.NEGATIVE_INFINITY (or +/-Infinity) to the
invalidRates array used with grpcServer.reconfigure and client.config assertions
so grpcServer.reconfigure(agentId, { traceSampleRate: invalidRate }) is
exercised for NaN/Infinity and the existing assert.strictEqual checks that
nsolidConfig.traceSampleRate remained 0.4 still apply.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
agents/grpc/proto/reconfigure.protoagents/grpc/src/grpc_agent.ccagents/grpc/src/proto/reconfigure.pb.ccagents/grpc/src/proto/reconfigure.pb.hlib/internal/otel/trace.jslib/nsolid.jssrc/nsolid/nsolid_api.ccsrc/nsolid/nsolid_api.htest/addons/nsolid-tracing/test-otel-basic2.jstest/agents/test-grpc-reconfigure.mjstest/fixtures/nsolid-trace-sample-rate-package.jsontest/fixtures/test-nsolid-config-trace-sample-rate-env-script.jstest/parallel/test-nsolid-config-trace-sample-rate-env.jstest/parallel/test-nsolid-config-trace-sample-rate.jstest/parallel/test-nsolid-trace-sample-rate-sampling.js
santigimeno
force-pushed
the
santi/tracing_rate
branch
from
5048dab to
85d36e4
Compare
Add end-to-end traceSampleRate handling across config, runtime propagation, tracing decisions, and regression tests. Why: - Enable configurable probabilistic trace sampling with predictable behavior. - Ensure consistent semantics across all config entry points. - Prevent invalid updates from corrupting current sampling behavior. - Keep transaction consistency by deciding sampling at the root span only. What changed: - Added traceSampleRate parsing and normalization in JS config paths with explicit default fallback and finite/range validation in [0, 1]. - Added native config sanitization for traceSampleRate to reject invalid values before merge, preserving previous valid configuration. - Ensured runtime sampling state is synchronized from effective current config after updates to avoid stale shared-memory sample rates. - Added gRPC reconfigure support for traceSampleRate in proto and agent mapping, including generated protobuf updates. - Updated tracing logic so root spans perform the sampling decision and child spans inherit parent traceFlags. - Extended tests for: - invalid value handling (including NaN/Infinity) - env/package bootstrap behavior - partial updates preserving existing traceSampleRate - gRPC invalid-update fallback behavior - sampling behavior at 0%, 50% (tolerance), and 100% - worker-thread sampling behavior - explicit parent/child trace consistency assertions
santigimeno
force-pushed
the
santi/tracing_rate
branch
from
85d36e4 to
d4333fe
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
2 participants
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.
Add end-to-end traceSampleRate handling across config, runtime propagation, tracing decisions, and regression tests.
Why:
What changed:
Summary by CodeRabbit
New Features
Bug Fixes
Tests