[RUM-17903] Integrate tracing sample rate - #193
Conversation
|
@codex review |
There was a problem hiding this comment.
PR Review — Score: 4.7 / 5
This is a well-designed trace sampling feature that mirrors the existing session replay and profiling child-rate model. The split between fast instrumentation gates (isTraceSampled) and the span assembly hook safety net is sound, session renewal/expiry handling is correct, and preserving unlinked HTTP RUM resources when tracing is sampled out is the right product behavior. I would approve this PR.
Why 4.7: Deterministic session-based sampling with correctedChildSampleRate, cross-bundle global state for the instrument bundle, thorough unit coverage (config validation, instrumentation, span assembly, transport), and clear architecture documentation. The SKIPPED/DISCARDED hook inversion in TraceSampling correctly composes with existing session/view enrichment hooks.
Why not 5: Two small, intentional tradeoffs remain: (1) isTraceSampled() defaults to true before init() completes, so a brief pre-init startup window may still trace even when traceSampleRate is 0; (2) HTTP instrumentation still creates local dd-trace spans for RUM resource timing when a session is trace-sampled out — documented, but it means some tracing overhead remains on unsampled sessions.
Findings
- [Nit] Pre-init tracing default —
isTraceSampled()returnstrueuntilTraceSamplingruns, so very early main-process traffic may trace briefly even withtraceSampleRate: 0.
Architectural flow
sequenceDiagram
participant App as App code
participant Inst as Instrumentation
participant TS as TraceSampling
participant SP as SpanProcessor
participant Hooks as FormatHooks
participant Transport as Transport
Note over TS: init sets per-session trace decision
TS->>Inst: setTraceSampled
App->>Inst: HTTP or IPC call
Inst->>Inst: isTraceSampled check
alt trace sampled
Inst->>Inst: create span and inject HTTP headers
else not trace sampled
Inst->>Inst: skip IPC spans and header injection
Note over Inst: HTTP keeps local span for RUM timing
end
Inst->>SP: dd-trace export
SP->>Hooks: triggerSpan by startTime
alt hooks DISCARDED
SP->>SP: drop APM span
SP->>SP: emit unlinked HTTP RUM resource
else hooks pass
SP->>Transport: linked span envelope
end
Before: All sampled RUM sessions produced main-process HTTP/IPC spans, injected distributed tracing headers, and linked RUM resources to traces.
After: traceSampleRate gates tracing as a child of sessionSampleRate. Trace-sampled sessions behave as before. Non-trace-sampled sessions skip IPC spans and HTTP header injection, discard APM spans at assembly time, and still emit unlinked HTTP RUM resources. The SPANS transport is omitted when traceSampleRate is 0 as an idle-work optimization.
Sent by Cursor Automation: electron-sdk reviews
|
|
||
| function getState(): TraceSamplingState { | ||
| const store = globalThis as unknown as Record<symbol, TraceSamplingState | undefined>; | ||
| return (store[TRACE_SAMPLING_STATE] ??= { sampled: true }); |
There was a problem hiding this comment.
Pre-init default (sampled: true) — This preserves pre-init() backward compatibility (also covered by the unit test), but it means a narrow startup window can still trace/inject headers even when the user configures traceSampleRate: 0. If that window ever shows up in the field, consider initializing from config in the instrument prelude; for now the tradeoff seems reasonable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d6ae4ed66
ℹ️ 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".
| // so it is authoritative for the rates reported on every RUM event. | ||
| const ddConfiguration = { | ||
| session_sample_rate: configuration.sessionSampleRate, | ||
| trace_sample_rate: configuration.traceSampleRate, |
There was a problem hiding this comment.
Avoid overriding renderer trace sampling config
When a renderer uses the Browser SDK with its own tracing configuration and Electron init sets traceSampleRate differently (for example 0 to disable only main-process HTTP/IPC tracing), this value is applied to renderer RUM events too. RendererPipeline merges hook overrides over the renderer event, so the renderer’s own _dd.configuration.trace_sample_rate is replaced even though the new config is documented as controlling main-process requests and IPC operations. This makes renderer RUM metadata report the wrong trace sampling decision; keep this field main-only or preserve the renderer-provided value for renderer events.
Useful? React with 👍 / 👎.


Motivation
The Electron SDK did not expose trace sampling, so main-process HTTP and IPC tracing was effectively all-or-nothing.
This change adds session-based trace sampling consistent with the existing session replay and profiling sampling model. It reduces APM volume and distributed trace propagation while preserving native HTTP requests as RUM resources, even when tracing is sampled out.
Changes
traceSampleRateconfiguration option, defaulting to100.sessionSampleRateusing deterministic session-based sampling.trace_sample_rateto_dd.configurationon RUM events.SPANStransport whentraceSampleRateis0as an idle-work optimization.Test instructions
Checklist