✨ add global context to the main process - #192
Conversation
|
@codex review |
There was a problem hiding this comment.
Stale comment
PR Review — Score: 4.6 / 5
This is a well-structured feature addition that mirrors the Browser SDK surface, reuses
ContextManagercleanly (including thesetFlatContext/extraInfoedge case), and DRYs the duplicated RUM hook registration. Test coverage spans unit, assembly, and e2e, documentation is thorough, and the PR description documents the intentional tradeoffs clearly. I would approve.Why 4.6: Solid API design with validation/sanitization at the public boundary, thoughtful renderer merge semantics, crash-attribution history parity with user/account, and comprehensive tests plus README updates.
Why not 5: Disk-backed history rewrites on every property mutation may be costly for high-frequency global-context updates (acknowledged in the PR body), pre-init calls are silently ignored without feedback (tracked as RUM-17397), and
nullproperty semantics intentionally diverge from the Browser SDK (also documented).
Findings
- [Minor] Disk history on every mutation —
setProperty/removePropertytrigger a full history close-and-persist cycle, which may be expensive if global context is updated frequently.- [Nit] Silent pre-init no-ops — API calls before
init()are dropped without warning; consistent with duration vitals but easy to misconfigure during startup.
Architectural flow
LoadingsequenceDiagram participant App as Main app participant API as Public API participant GC as GlobalContext participant Disk as DiskValueHistory participant Hooks as FormatHooks participant Pipe as RendererPipeline participant Intake as EventManager App->>API: setGlobalContext / setGlobalContextProperty API->>GC: sanitize then setFlatContext / setProperty GC->>Disk: closeAndAdd / pruneAndPersist Note over Hooks: Main-process RUM events Hooks->>GC: getContext via registerRumHook GC-->>Hooks: context attributes Hooks->>Intake: RumEvent with context field Note over Pipe: Renderer RUM events Pipe->>Hooks: triggerRum for main-process context Hooks->>GC: getContext GC-->>Pipe: main-process context Note over Pipe: Per-key merge, renderer wins on conflict Pipe->>Intake: ServerRumEventBefore: Only renderer processes could attach free-form
contextattributes via the Browser SDK; main-process RUM events had no equivalent, and renderer events did not inherit main-process attributes.After: The main process exposes five Browser-SDK-aligned APIs backed by
GlobalContext(disk-persisted history for crash attribution). A sharedregisterRumHookinjectscontextinto all RUM events. For renderer events,RendererPipelinemerges main-process and renderercontextper key, with renderer values taking precedence on conflicts.Note: GitLab CI jobs were still pending when this review was posted.
Sent by Cursor Automation: electron-sdk reviews
There was a problem hiding this comment.
PR Review — Score: 4.6 / 5
This is a well-executed feature addition that mirrors the Browser SDK global-context API, reuses the existing ContextManager infrastructure cleanly, and includes solid unit, pipeline, and e2e coverage plus documentation. I would approve.
Why 4.6: The implementation follows established SDK patterns (callMonitored, input validation/sanitization, disk-backed crash attribution), extracts the duplicated RUM hook into the base class, and handles the renderer merge with the right per-key precedence. Tests exercise the important edge cases (extraInfo key handling, historical attribution, renderer-wins merge).
Why not 5: Two known trade-offs remain — frequent property updates will hit disk on every mutation (acceptable for crash attribution but worth watching), and pre-init calls are silently dropped (tracked separately in RUM-17397). Global context also intentionally skips span enrichment, which is documented but leaves a small internal inconsistency with user/account.
CI had several dd-gitlab/* jobs still pending when this review was posted; that is noted separately and not factored into the score.
Findings
- [Minor] History rewrite on every mutation — Each
setGlobalContextPropertycloses the active history entry and rewrites the disk file; fine for login-time user/account updates but potentially costly for high-frequency callers. - [Minor] Pre-init calls silently dropped —
globalContextApi?.…no-ops beforeinit()with no warning; same pattern as duration vitals, tracked in RUM-17397. - [Nit] RUM-only enrichment — Global context is intentionally omitted from spans to match Browser/mobile SDKs, unlike user/account in this SDK.
Architectural flow
sequenceDiagram
participant App as Main process app
participant API as Public API
participant GC as GlobalContext
participant Hooks as FormatHooks
participant RP as RendererPipeline
participant EM as EventManager
App->>API: setGlobalContext / setGlobalContextProperty
API->>GC: setContext / setProperty
GC->>GC: record history and persist
GC->>Hooks: registerRumHook injects context
Note over RP: Renderer RUM event arrives
RP->>Hooks: triggerRum
Hooks-->>RP: main-process context override
RP->>RP: per-key merge renderer wins on conflicts
RP->>EM: ServerRumEvent with combined context
Before: Only user/account context lived in the main process; renderer global context was Browser SDK-only and was not merged with main-process attributes.
After: Main process exposes five global-context APIs backed by GlobalContext (disk history + RUM hook). Main-process events get context from the hook. Renderer events still carry Browser SDK context, but RendererPipeline now merges main-process attributes in per-key with renderer precedence — unlike usr/account, which are replaced wholesale when the renderer already has them.
Sent by Cursor Automation: electron-sdk reviews
| */ | ||
| protected setProperty(key: string, value: unknown): void { | ||
| this.standardFields = pickNonNullish({ ...this.standardFields, [key]: value }); | ||
| this.recordCurrentContext(); |
There was a problem hiding this comment.
[Minor] Disk I/O on every property change
setProperty (and removeProperty) call recordCurrentContext(), which closes the active history entry and runs pruneAndPersist() — rewriting the history file on every mutation. That is appropriate for user/account (login/logout cadence) but global context is more likely to be updated frequently (feature flags, build metadata refreshes).
Not blocking — crash attribution parity is a reasonable reason to keep history — but worth monitoring if customers adopt this for high-churn attributes. A future optimization could batch or throttle persistence for global context without losing current-state injection.
| display.error('setGlobalContext: context must be an object. The context will not be updated.'); | ||
| return; | ||
| } | ||
| globalContextApi?.setContext(sanitize(context) as Context); |
There was a problem hiding this comment.
[Minor] Silent no-op before init()
globalContextApi?.setContext(…) silently drops calls made before init() resolves. The PR description calls this out and links RUM-17397 for a broader fix (same pattern as duration vitals).
Consider a one-time display.warn here once the broader ticket is addressed, so mis-ordered startup code is easier to spot.
|
|
||
| constructor(hooks: FormatHooks, history?: ContextHistory) { | ||
| super('global context', {}, history); | ||
| this.registerRumHook(hooks); |
There was a problem hiding this comment.
[Nit] RUM-only by design
GlobalContext registers only the RUM hook — no registerSpan equivalent. That matches Browser/mobile SDK behavior and is documented in the PR, but it is inconsistent with UserContext/AccountContext in this SDK. Fine as an intentional parity choice; just flagging for anyone expecting span tags from global context.
| } | ||
| if (hasContext(data.account)) delete overrides.account; | ||
| if (hasContext(data.context) && hasContext(overrides.context)) { | ||
| overrides.context = combine(overrides.context, data.context); |
There was a problem hiding this comment.
Per-key merge with renderer precedence looks correct: combine(overrides.context, data.context) lets the renderer win on conflicting keys while preserving main-process-only attributes like build. This is the right boundary behavior for free-form attributes (vs. wholesale replacement for usr/account). Unit test coverage for both directions (renderer-only keys, main-only keys) is good.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |


Motivation
Renderer processes can attach custom attributes through the Browser SDK's
setGlobalContext, but themain process has no equivalent, so main-process RUM events cannot carry app-level attributes such as a
build id or a feature flag.
Changes
Adds five APIs mirroring the Browser SDK, since customers already use those names in the renderer:
Attributes are sent in the event's
contextfield.GlobalContextreusesContextManager, so it getsthe same disk-backed history as user and account, meaning a crash from a previous run is enriched with
the attributes that were active at the time. The RUM hook that user, account and global context all
register was identical three times, so it moved to the base class.
Renderer events also receive the main-process context.
combinealready merged the two objects; thissets the precedence so the renderer wins on a conflicting key.
A few decisions worth a look:
setGlobalContextProperty(key, null)removes the property, matchingaddUserExtraInfo. The BrowserSDK keeps the
null.account do enrich spans, so this is inconsistent within the SDK.
usrandaccountare replaced wholesale. Free-form attributeshave no single owner, and a wholesale rule would drop main-process attributes from any event that
happens to carry event-level context.
main-process attributes too.
init()are dropped silently, as with duration vitals. Rationalising that isRUM-17397.
account, which change at login, but global context is likely to be called far more often.
Test instructions
In the playground, use the Global Context buttons and check that following events carry the attributes
in
context. Setting a key in a renderer throughdatadogRum.setGlobalContextshould win over the samekey set in the main process.
Checklist