[RUM-15788] Add User & Account info#135
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 473ab60ab8
ℹ️ 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".
cdn34dd
force-pushed
the
carlosnogueira/RUM-15788/add-user-account-info
branch
from
473ab60 to
bbea694
Compare
There was a problem hiding this comment.
Pull request overview
Adds user/account identity context APIs to the Electron SDK so main-process code can attach usr and account attributes to subsequent RUM events (including renderer-originated events), with interactive playground controls and new unit/e2e coverage.
Changes:
- Introduces new public APIs for user/account context management (
set/get/clearplus property-level setters/removers) and exportsUserInfo/AccountInfotypes. - Adds
UserContext/AccountContexthook-based context injection and forwardsusr/accountwhen assembling renderer RUM events. - Extends playground UI + IPC and adds a new e2e scenario file for user/account context behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Exposes new public user/account context APIs and wires them into SDK initialization. |
| src/assembly/contextManager.ts | Adds shared context storage/validation + flatten/unflatten helpers for info objects. |
| src/assembly/userContext.ts | Implements usr context injection via format hooks. |
| src/assembly/userContext.spec.ts | Unit tests for user context behavior, validation, and cloning semantics. |
| src/assembly/accountContext.ts | Implements account context injection via format hooks. |
| src/assembly/accountContext.spec.ts | Unit tests for account context behavior, validation, and cloning semantics. |
| src/assembly/index.ts | Re-exports new context classes/types for consumption by the root entrypoint. |
| src/assembly/Assembly.ts | For renderer RUM events, forwards main-process usr/account (when set) along with session/app/view overrides. |
| playground/src/index.html | Adds UI buttons for exercising new user/account APIs. |
| playground/src/renderer.ts | Wires new UI buttons to preload-exposed API calls. |
| playground/src/preload.ts | Exposes new IPC invocations for user/account demo actions. |
| playground/src/main.ts | Adds IPC handlers that call the new SDK APIs for the playground demo. |
| e2e/scenarios/user-account.scenario.ts | Adds end-to-end scenarios validating user/account propagation through intake. |
| e2e/lib/mainPage.ts | Adds page-object helpers for new user/account IPC calls in e2e tests. |
| e2e/app/src/preload.ts | Exposes new IPC calls from renderer to main in the e2e app. |
| e2e/app/src/main.ts | Implements IPC handlers calling the new SDK user/account APIs in the e2e app. |
cdn34dd
force-pushed
the
carlosnogueira/RUM-15788/add-user-account-info
branch
from
bbea694 to
e52ef2b
Compare
cdn34dd
force-pushed
the
carlosnogueira/RUM-15788/add-user-account-info
branch
from
e52ef2b to
bb127d6
Compare
| setContextProperty(key: string, value: unknown): void { | ||
| if (this.protectedKeys?.has(key)) return; | ||
|
|
||
| if (this.isStandardKey(key)) { | ||
| const candidate = { ...this.standardFields, [key]: deepClone(value) }; | ||
| if (!this.validateProperties(candidate)) return; | ||
| this.standardFields = candidate; | ||
| } else { | ||
| this.extraInfo = { ...this.extraInfo, [key]: deepClone(value) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
❗ warning: Non-standard keys (extraInfo) can be set even when no context exists, producing { usr: { plan: 'premium' } } with no id violates the RUM schema.
There was a problem hiding this comment.
Yes, that is a valid concern, this is a regression due to all the changes, I'll add a test to prevent this.
There was a problem hiding this comment.
Actually this is allowed on mobile, we recently needed to make this change on react-native to replicate the other mobile platforms in order to address a customer issue: https://github.com/DataDog/dd-sdk-reactnative/pull/1288/changes.
Their issue was the following:
- SDKs generates anonymous_id -> backend feature flag copies it to user id field
- User logs in -> you want to call addUserExtraInfo to add the authenticated id without overwriting the user id field
But if you were to call setUserInfo({ id: consumerId }) it would break your setup, since user id field is auto set by the feature flag.
And looking at the schema id seems to be optional for user but required for account: https://github.com/DataDog/rum-events-format/blob/8dc61166ee818608892d13b6565ff04a3f2a7fe9/schemas/rum/_common-schema.json#L127
There was a problem hiding this comment.
I've addressed this in the latest commit.
| /** | ||
| * Filters out entries whose value is `undefined`, so unset standard fields are not stored as | ||
| * explicit `undefined` keys. | ||
| * | ||
| * @param context - The object to filter. | ||
| * @returns A shallow copy of `context` keeping only entries whose value is defined. | ||
| */ | ||
| function pickDefined(context: Context): Context { | ||
| const result: Context = {}; | ||
| for (const [key, value] of Object.entries(context)) { | ||
| if (value !== undefined) result[key] = value; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
❓ question: do we care though? won't those values be removed when serialized for transport anyway?
There was a problem hiding this comment.
We do, because it's not about transport only, for transport they are indeed removed when serialized, but some public APIs like getUserInfo/getAccountInfo preserve the undefined keys, if not removed by this function.
There was a problem hiding this comment.
What issue do you see with preserving undefined keys with those APIs?
There was a problem hiding this comment.
It's not an issue, it's about correctness, if you get you user/account data, you get a clean object without any undefined values inside. This seems like a very minor thing, is it really that important that we're striping away undefineds instead of leaving them ?
cdn34dd
force-pushed
the
carlosnogueira/RUM-15788/add-user-account-info
branch
from
6127a87 to
0d40d55
Compare
cdn34dd
force-pushed
the
carlosnogueira/RUM-15788/add-user-account-info
branch
from
9cf58d7 to
643a83f
Compare
Persist user and account context history so events can be enriched using their start time, including crash events from a previous process. Also propagate customer context to span metadata as meta.usr.* and meta.account.*.
Treat null values in user/account extra info updates as removals, matching mobile SDK behavior. Add coverage for removed properties across stored info, RUM payloads, and span metadata.
cdn34dd
force-pushed
the
carlosnogueira/RUM-15788/add-user-account-info
branch
from
643a83f to
281f65a
Compare
This comment has been minimized.
This comment has been minimized.
bcaudan
force-pushed
the
carlosnogueira/RUM-15788/add-user-account-info
branch
from
a9f72b2 to
b9a664d
Compare
Replace `displayWarn` import with `display.warn` in contextManager.ts and userContext.ts - the named export was removed in main and replaced by the `display` object from `createDisplay`.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d74008e753
ℹ️ 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".
| for (const [key, value] of Object.entries(context)) { | ||
| const metaValue = toSpanMetaValue(value); | ||
| if (metaValue !== undefined) { | ||
| meta[`meta.${prefix}.${key}`] = metaValue; |
There was a problem hiding this comment.
When a user or account context is set and dd-trace spans are emitted, SpanProcessor merges this result into the span's existing meta object, so these keys are sent literally as meta.usr.id/meta.account.id rather than the Datadog span tags usr.id/account.id. In that scenario, RUM events are enriched, but span user/account attribution won't be recognized because the tags are nested under span.meta already.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review — Score: 4.4 / 5
This is a well-structured feature PR that adds user/account identity APIs with solid internals (ContextManager, disk-backed history, renderer precedence) and strong unit + e2e coverage for the happy paths. Prior review feedback (shared context manager, renderer precedence, deep cloning, standard-field protection, mobile SDK parity for id-less user extra info) has been addressed thoughtfully. I would approve with minor follow-ups on customer-facing docs and crash-attribution e2e.
Why 4.4: Clean domain separation, thorough validation/copy semantics, hook-based enrichment for RUM and spans, renderer injection that respects existing renderer context, and broad test coverage including parametrized e2e scenarios.
Why not 5: README/customer docs are not updated for eight new public APIs, crash-time context attribution (a stated motivation) lacks an end-to-end scenario, and log enrichment is explicitly deferred.
Findings
- [Minor] Missing README docs — Eight new public APIs are exported but README.md has no User & Account section; PR checklist docs item is still unchecked.
- [Minor] Crash attribution untested e2e — Disk-backed history is motivated by post-crash relaunch enrichment, but only unit tests cover it;
crash.scenario.tscould assertusr/accounton the crash error after relaunch. - [Nit] Public JSDoc lacks
@example— New APIs insrc/index.tsfollow the descriptive style of existing exports but omit@exampleblocks called for indocs/REVIEW.md.
Architectural flow
sequenceDiagram
participant App as Main-process app
participant API as setUserInfo / setAccountInfo
participant CM as ContextManager
participant Disk as DiskValueHistory
participant Hooks as FormatHooks
participant Pipe as MainAssembly / RendererPipeline
participant Intake as EventManager
App->>API: setUserInfo / addUserExtraInfo
API->>CM: setContext / addExtraInfo
CM->>Disk: closeActive + add (timestamped snapshot)
Note over Pipe: RUM event assembly
Pipe->>Hooks: triggerRum({ startTime: event.date })
Hooks->>CM: getContext(startTime)
CM->>Disk: find(startTime) when historical
CM-->>Hooks: flat usr / account object
Hooks-->>Pipe: hook overrides
Note over Pipe: Renderer: skip usr/account override when renderer already has non-empty context
Pipe->>Intake: ServerRumEvent with usr.* / account.*
Before: Main-process code had no API to attach usr or account identity to RUM events; renderer events were enriched only with session/application/view context.
After: Customer context is stored in ContextManager (standard fields + extraInfo, validated and deep-cloned), snapshotted to disk for crash attribution, injected into main-process and span assembly via format hooks, and conditionally merged into renderer-originated RUM events when the renderer event does not already carry non-empty usr/account context.
Sent by Cursor Automation: electron-sdk reviews
| * user whose `id` is managed elsewhere (e.g. derived from `anonymous_id`), use `addUserExtraInfo`. | ||
| * @param user - The user information, including an `id`. | ||
| */ | ||
| export function setUserInfo(user: UserInfo & { id: string }): void { |
There was a problem hiding this comment.
[Nit] Missing @example blocks
docs/REVIEW.md asks new public APIs to include at least one @example. These exports have good param docs but no usage sample. Even a short example (set user → add extra info → clear) would help IDE tooltips and generated API docs, matching the README pattern used for Operation Monitoring.
| /** | ||
| * Creates a crash-attribution history for customer context. | ||
| * | ||
| * The active entry from the previous process is closed during SDK startup so it can still enrich |
There was a problem hiding this comment.
[Minor] Crash attribution lacks e2e coverage
The disk-backed history and startup closeActive logic exist specifically so crash events processed after relaunch get the context that was active at crash time. Unit tests mock find() and contextHistory.spec.ts covers startup closure, but there is no scenario that sets user/account, crashes, relaunches, and asserts usr/account on the recovered crash error.
Extending e2e/scenarios/crash.scenario.ts (or a sibling scenario) would protect the full persistence → lookup → hook injection path that is called out in the PR motivation.
| break; | ||
| case 'log': | ||
| // TODO(RUM-15047) | ||
| // TODO(RUM-15047): when Logs are implemented, enrich them with user/account context |
There was a problem hiding this comment.
[Nit] Log enrichment deferred
The TODO correctly notes that logs should eventually get the same usr.* / account.* enrichment as mobile SDKs. Not blocking for this RUM-focused PR, but worth tracking so log support does not ship without parity.
| "dist/index.chunk.cjs", | ||
| "dist/index.chunk.cjs.map", | ||
| "dist/index.chunk.mjs", | ||
| "dist/index.chunk.mjs.map", | ||
| "dist/contextHistory.chunk.cjs", | ||
| "dist/contextHistory.chunk.cjs.map", | ||
| "dist/contextHistory.chunk.mjs", | ||
| "dist/contextHistory.chunk.mjs.map", |
There was a problem hiding this comment.
❓ question: what is the reasoning for going this way?
| */ | ||
| getContext(startTime?: TimeStamp): Context { | ||
| if (startTime !== undefined && this.history) { | ||
| return this.history.find(startTime) ?? {}; |
There was a problem hiding this comment.
💬 suggestion: do we have a test covering this case: no context was active at that time → emit nothing?
| expect(error.usr).toEqual({ id: 'user-1', name: 'Alice' }); | ||
| expect(error.account).toEqual({ id: 'account-1', name: 'Acme Corp' }); | ||
| }); |
There was a problem hiding this comment.
💬 suggestion: what about testing the span enrichment as well?
| static async init(hooks: FormatHooks): Promise<AccountContext> { | ||
| const { initContextHistory } = await import('./contextHistory'); | ||
| const history = await initContextHistory(ACCOUNT_CONTEXT_HISTORY_FILE_NAME); | ||
| return new AccountContext(hooks, history); |
There was a problem hiding this comment.
💬 suggestion: it seems to be pretty similar on the UserContext as well, could it be pulled in the base class?
with something like:
static init(hooks: FormatHooks) {
return ContextManager.init(AccountContext, hooks, ACCOUNT_CONTEXT_HISTORY_FILE_NAME);
}
Motivation
Adds user and account identity context APIs to the Electron SDK, closing RUM-15788. Previously there was no way to attach user or account information to RUM events from the main process. This is a common requirement for any app that has authenticated users and multi-tenant/account structures.
Changes
New public API (
src/index.ts)Eight new customer-context functions are exported from the SDK.
User info
setUserInfo(user: UserInfo & { id: string })— sets{ id, name?, email?, extraInfo? }for subsequent supported events.getUserInfo()— returns a copy of the current user info, with custom fields underextraInfo.clearUserInfo()— removes user info from subsequent supported events.addUserExtraInfo(extraInfo)— merges custom attributes intoextraInfo.id,name,email) can only be set viasetUserInfo.nullfor a custom attribute removes it.Account info
setAccountInfo(accountInfo)— sets{ id, name?, extraInfo? }for subsequent supported events.getAccountInfo()— returns a copy of the current account info.clearAccountInfo()— removes account info from subsequent supported events.addAccountExtraInfo(extraInfo)— merges custom attributes intoextraInfo.id,name) can only be set viasetAccountInfo.nullfor a custom attribute removes it.Event enrichment
User and account context are attached to RUM events as
usr.*andaccount.*. CustomextraInfofields are flattened into those objects; noextraInfokey is emitted.User and account context are also attached to spans as
meta.usr.*andmeta.account.*, matching mobile SDK naming.Historical context
User/account context is persisted in a small disk-backed history so events can be enriched using their
startTime. This allows crash events processed after relaunch to receive the context that was active when the crash happened.Renderer pipeline
Renderer-originated RUM events are enriched with main-process context through
RendererPipeline. If the renderer event already contains non-emptyusroraccountcontext, the renderer value is preserved; otherwise the main-process context is injected.Log enrichment is explicitly left as a TODO until Logs are implemented.
Playground (
playground/src/)Added a "User & Account Context" section with buttons to exercise:
E2E tests (
e2e/scenarios/customer-context.scenario.ts)Ten scenarios cover the full pipeline:
usr/accountfield is emitted when unsetusr/accountUnit tests
Added focused coverage for:
nullremoval for user/account extra infoChecklist