[RUM-15525] Add sessionSampleRate configuration option#139
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7346c3fd45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7346c3f to
292d50b
Compare
…Rate Cherry-picked from #139. Drop this commit once PR#139 lands on main.
292d50b to
bc009dc
Compare
There was a problem hiding this comment.
Pull request overview
Adds client-side session sampling to the Electron SDK via a new sessionSampleRate config option and deterministic sampling based on the session UUID, with propagation into session lifecycle handling and unit tests.
Changes:
- Added
sessionSampleRateto configuration and validation logic. - Introduced a deterministic sampler utility (
isSessionSampled) and unit tests for sampling behavior. - Wired session sampling into session creation so non-sampled sessions are discarded from RUM attribution.
Architectural Flow
buildConfiguration() resolves sessionSampleRate → init() passes resolved config into SessionManager.start() → SessionManager.createNewSession() generates a UUID and makes a deterministic sampling decision → session history (SessionContext/DiskValueHistory) is updated to drive hook enrichment (RUM/telemetry/spans) based on history lookups.
Score
2.6 / 5.0
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/Sampler.ts | Adds deterministic session sampling + helper rate math. |
| src/tools/Sampler.spec.ts | Adds unit tests for sampler determinism and boundary conditions. |
| src/tools/DiskValueHistory.ts | Minor typing/cleanup in disk history restore logic. |
| src/mocks.specUtil.ts | Updates test configuration helper with sessionSampleRate. |
| src/index.ts | Passes resolved configuration into SessionManager.start(). |
| src/domain/session/SessionManager.ts | Computes sampling decision on session creation and gates session history updates. |
| src/domain/session/SessionManager.spec.ts | Updates SessionManager tests and adds sampling-related scenarios. |
| src/domain/session/SessionContext.ts | Minor refactor of hook code (variable naming). |
| src/config.ts | Adds sessionSampleRate and updates sample-rate validation/build logic. |
| src/config.spec.ts | Adds config validation tests for sessionSampleRate and updates telemetry sample-rate invalid cases. |
bc009dc to
8f3aa3b
Compare
|
It could be nice to document the new option in the README.md |
- getInternalContext() returned the active session's id regardless of sampling, so a non-sampled session (which produces no RUM) could still leak a session_id into cross-product correlation — pointing consumers at a RUM session that was never ingested. Back getInternalContext() with a single source of truth: SessionContext now exposes getActiveSessionId(), which looks the session up in the same history the RUM/span/telemetry hooks consult. A non-sampled session is absent (never added) and an expired one is closed, so both resolve to undefined, keeping internal context consistent with what the hooks actually attribute. Removes the parallel status/isSampled check on the Session object.
8f3aa3b to
ebddc93
Compare
This comment has been minimized.
This comment has been minimized.
sessionSampleRate configuration optionsessionSampleRate configuration option
Motivation
Adds support for client-side session sampling in the Electron SDK. Without this, every session is always collected. Implements
sessionSampleRate(0–100) analogous to the other SDKs.Tracked in RUM-15525.
Changes
New
sessionSampleRateconfiguration option (src/config.ts)InitConfiguration, required (defaults to100) in the resolvedConfiguration.telemetrySampleRate— invalid values log an error and the validator returnsundefined, which abortsinit()(no configuration is built,init()returnsfalse).Deterministic sampler (
src/tools/Sampler.ts)isSessionSampleduses the Knuth-factor consistent-sampling method, aligned with the Datadog cross-language spec (and the browser-core implementation):(uuid_node * knuthFactor) % 2^64compared against the rate, using BigInt to avoid 64-bit overflow.sessionReplaySampleRateis added.0.1) are supported.correctedChildSampleRate(parentRate, childRate)ported from browser-core, in preparation for the upcoming Session Replay PR. It is currently unused (no caller yet) but lands here since the SR PR follows immediately.Session sampling propagation (
src/domain/session/SessionManager.ts)isSampledis computed once at session creation viaisSessionSampled(id, sessionSampleRate).Hook behavior for non-sampled sessions (
src/domain/session/SessionContext.ts)DISCARDED, and telemetry returnsSKIPPED(telemetry still sends, without session attribution).SessionContextalso exposesgetActiveSessionId()(see below); remaining changes are a cosmetic local rename.Internal context (
src/index.ts,src/domain/session/SessionManager.ts,src/domain/session/SessionContext.ts)getInternalContext()now returnsundefinedfor any session that isn't currently tracked — including non-sampled (and expired) sessions — instead of always returning the active session's id. This prevents leaking asession_id(used for cross-product correlation) for a session that produces no RUM.SessionContext.getActiveSessionId()resolves the session from the same history the RUM/span/telemetry hooks consult, so internal context stays consistent with what those hooks attribute (a non-sampled session is absent; an expired one is closed).Test instructions
Checklist