✨ support telemetry from the renderer process - #205
Conversation
7ae8c30 to
4410046
Compare
![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: 4410046d73
ℹ️ 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".
| * budget so neither stream can starve the other: a chatty renderer must not crowd out the Electron | ||
| * SDK's own error reporting, and vice versa. | ||
| */ | ||
| const MAX_RELAYED_TELEMETRY_EVENTS_PER_SESSION = 100; |
There was a problem hiding this comment.
This was to align with MAX_TELEMETRY_EVENTS_PER_SESSION, but happy to discuss this number. From what I checked, Android SDK doesn't have telemetry from WebViews, and iOS doesn't define a limit (e.g. relies on browser-sdk`s limit). Still, thought it was good to add this because:
- Browser's limit is 15 events per kind per page load, which can grow to a large number easily.
- We have a safe limit well-defined on electron side.
- Events here are not accounted for growing the
MAX_TELEMETRY_EVENTS_PER_SESSIONnumber.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
4410046 to
9a68e2c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a68e2c73e
ℹ️ 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".
|
@codex review |
![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.8 / 5
This PR closes the long-standing gap where renderer internal_telemetry events were dropped on the floor, and it does so with careful boundary design: IPC validation, source-aware hook enrichment, separate sampling/cap budgets for main vs renderer streams, and session re-attribution that avoids phantom sessions from the browser SDK stub. Prior Codex feedback on payload validation and transport registration ordering is addressed on the current head, unit and e2e coverage is strong, and docs/ARCHITECTURE.md documents the ownership model clearly. I would approve.
Why 4.8: Thoughtful cross-platform alignment (iOS WebView parity), robust validation at the IPC boundary including compile-time-checked KNOWN_TELEMETRY_TYPES, the transport-before-bridge fix, scoped e2e predicates that prevent flaky assertions once renderer telemetry flows, and thorough tests for cap/renewal/host-rate independence.
Why not 5: A documented schema-sync window drops unknown telemetry.type values until rum-events-format is bumped; relay-cap enforcement is unit-tested only (no e2e); and playground manual verification remains unchecked in the PR checklist.
Findings
- [Nit] Schema sync window — A new browser-core telemetry kind is silently dropped at the bridge until
rum-events-formatsyncs; the tradeoff is documented but worth tracking on submodule bumps.
Architectural flow
sequenceDiagram
participant Browser as Browser RUM SDK
participant IPC as DatadogEventBridge
participant RP as RendererPipeline
participant Hooks as Format Hooks
participant EM as EventManager
participant T as Transport
Browser->>IPC: internal_telemetry assembled event
IPC->>RP: BRIDGE_CHANNEL message
RP->>RP: validate type date telemetry.kind
RP->>Hooks: triggerTelemetry source RENDERER
Note over Hooks: commonContext adds application.id<br/>SessionContext adds session.id or DISCARDED<br/>ViewContext SKIPPED keeps renderer view.id
RP->>RP: enforce relay cap
RP->>EM: ServerTelemetryEvent on RUM track
EM->>T: batch and upload
Before: RendererPipeline ignored internal_telemetry bridge events (TODO RUM-15253), so any telemetry the browser SDK reported inside a renderer was lost.
After: Pre-assembled renderer telemetry is validated at the IPC boundary, enriched only with main-process-owned fields (application/session), capped independently of the main-process telemetry budget, and forwarded on the RUM track without re-sampling — preserving the renderer's own sampling decision and identity fields (source, service, version, view.id, ddtags).
Sent by Cursor Automation: electron-sdk reviews
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c45540181
ℹ️ 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".
| rumApi = rum.getApi(); | ||
| setDurationVitalApi(rumApi); | ||
|
|
||
| new RendererPipeline(eventManager, hooks, config); |
There was a problem hiding this comment.
When init() is called while an already-open renderer is producing events, placing RendererPipeline here leaves BRIDGE_CHANNEL without a listener throughout Transport.create() and RumCollection.start(). In particular, the RUM transport handler is registered partway through Transport.create(), so during subsequent awaited track setup the previous ordering could deliver renderer events but the new ordering makes Electron discard their IPC messages. Register the listener earlier and queue events until transport initialization completes rather than shifting the loss window. docs/REVIEW.mdL10-L18
Useful? React with 👍 / 👎.
| // Relayed renderer telemetry arrives with the browser SDK's own ddtags, so without these the | ||
| // Electron SDK's telemetry would be the only stream that cannot be filtered by env. | ||
| ddtags: buildDdtags(configuration), |
There was a problem hiding this comment.
❗ warning: it will be the values configured by the customer though, we should not collect them in the sdk telemetry.
There was a problem hiding this comment.
I believe the browser SDK also adds these ddtags on its own telemetry. link
So should we keep this?
0c45540 to
b790937
Compare
Boundary validation now requires a `telemetry` payload carrying a `type`
discriminator, which rejects the empty shells a bridge/SDK mismatch can produce
(`{}`, `[]`). The kind itself is deliberately not checked against the schema: the
browser SDK can add one before our `rum-events-format` submodule is synced, and
telemetry is indexed whatever its kind, so a check would drop valid telemetry for
the length of that window and buy nothing — renderer RUM events are relayed
whatever their type too. The identity fields stay unchecked as well: they are the
browser SDK's to report.
A relayed event whose date no main-process session covers now has its
`session.id` nulled rather than being dropped. The browser SDK's stub id must not
survive, but the event still reports on that SDK, and `null` is what the backend
reads as absent — the device `ProfilingCollection` already uses for
`_dd.profiling`. Main-process telemetry is likewise sent without a session.
`RendererPipeline` moves below `Transport.create` and `RumCollection.start`. It
accepts renderer IPC the moment it exists, `Transport` registers a track's
handler only once `createBatchManager` resolves, and `EventManager.notify` drops
events no handler claims — so a renderer event arriving in that window was lost
with no trace. One-shot events were the exposure: the browser SDK's
configuration telemetry is emitted once per page load and never retried.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b790937 to
5c533dc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c533dc46e
ℹ️ 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".
| rumApi = rum.getApi(); | ||
| setDurationVitalApi(rumApi); | ||
|
|
||
| new RendererPipeline(eventManager, hooks, config); |
There was a problem hiding this comment.
When session replay is enabled and a window loads while Transport.create() or RumCollection.start() is pending, constructing RendererPipeline only afterward means setBridgeConfig() has not replaced the fallback yet. That fallback deliberately omits the records capability (src/common/bridgeConfig.ts), and src/preload/bridge.ts captures the response in a constant, so this renderer never advertises record support and loses session replay for the lifetime of the page. Publish the real bridge configuration before these asynchronous setup calls, independently of when event forwarding becomes ready.
AGENTS.md reference: AGENTS.md:L3-L6
Useful? React with 👍 / 👎.
2 participants
Motivation
The bridge has always carried
internal_telemetryevents from the renderer's browser SDK, andRendererPipelinedropped them on the floor behind aTODO(RUM-15253). Any telemetry the browser SDK reported inside an Electron renderer was lost.Ticket: RUM-15253 (follow-up to #195)
Changes
RendererPipeline.handleTelemetryEventforwards the event as aServerTelemetryEventon the RUM track withsource: RENDERER, parallel tohandleRumEvent— noRawEventdetour, since the browser SDK sends it fully assembled. The payload is validated at the boundary — atelemetryevent with a numericdateand atelemetrypayload carrying atypediscriminator — and reported as a telemetry error otherwise, matching theprofile/recordcases. The kind itself is deliberately not checked against the schema: the browser SDK can add one before ourrum-events-formatsubmodule is synced, and telemetry is indexed whatever its kind, so a check would only drop valid telemetry for the length of that window — renderer RUM events are relayed whatever their type too. The identity fields (service,source,version,_dd) are deliberately left unchecked: they are the browser SDK's to report, and pinning their shape here would drop valid telemetry the day browser-core reshapes its envelope.Because the event arrives assembled, the telemetry hooks become source-aware and the hook result is merged over it, contributing only what the main process owns:
date,source,service,versionddtags,action.idapplication.id,session.idstartSessionManagerStub(), so its session id is a random UUID nothing else knowsview.idcontainerto hold the main-process view tooNot re-sampled or deduplicated. The browser SDK already applied the rate the renderer's
init()configured, and compounding two rates would be silently unrecoverable —effective_sample_rateis type-only in browser-core, never set, so nothing downstream could scale the survivors back up. This follows iOS'sWebViewEventReceiver; Android has no webview telemetry path.The two streams have separate rates, the same way they have separate caps. The Electron SDK's
telemetrySampleRateis not consulted for relayed events at all, including when it is0: a renderer's rate is configured in its owninit()and governs what crosses the bridge, while the main process's rate governs what the SDK reports about itself. Reaching across would let the host's rate silently override a decision the renderer already made. iOS draws the same line —RUMFeature.swiftgivesTelemetryReceiveraSampler(samplingRate: configuration.telemetrySampleRate)for native telemetry, whileWebViewEventReceivergets no sampler at all.Volume is still bounded, by a per-session cap of its own rather than the main process's
SessionBudget, so neither stream can starve the other. The browser SDK's ceiling is 15 events per kind per page load and lives in the SDK instance, so it resets on every window — it bounds a page, not a desktop session that outlives dozens of them. A cap is not a rate, so it cannot compound with the renderer's sampling.An event with no main-process session covering its date has its
session.idnulled rather than being dropped: the browser SDK's stub session id must not survive — it would mint phantom sessions in telemetry — but the event still reports on that SDK, andnullis what the backend reads as absent (the deviceProfilingCollectionalready uses for_dd.profiling). Main-process telemetry keeps its current behaviour and is still sent without a session, since an SDK error before the first session still reports a bug.Riding along: main-process telemetry now carries
ddtags(it was the only stream that could not be filtered by env), andTelemetryTypeis exported publicly.Existing e2e scenarios needed scoping, not just new tests —
session.scenario.tsindexed log telemetry positionally and the cap test counts to exactly 100/102, both with the main window's browser SDK running at its default 20% telemetry, so a relayed event could have landed in either.byMainProcessTelemetryType/byRendererTelemetryTypemake those reads mean what they say.Test instructions
New unit coverage in
src/assembly/RendererPipeline.spec.ts(internal telemetry events) replaces the test that pinned the old drop-everything behaviour. It covers the properties most likely to be "fixed" by a future reviewer: five identical events all going through, the per-session cap and its reset on session renewal, relaying regardless of the host's owntelemetrySampleRate, and a kind the schema does not define being relayed rather than dropped.Two new e2e scenarios in
e2e/scenarios/telemetry.scenario.ts(renderer telemetry) drive a real bridge window through the real assembly path: one asserts the re-attribution, one that the renderer's view survives. Both the bridge window and the playground pin their telemetry rates to 100, since the browser SDK samples before sending and nothing would arrive deterministically otherwise.Manually,
cd playground && yarn startagainst a real intake: loading the window emits the browser SDK'sconfigurationevent over the bridge, which should arrive with the playground's application and session id,source: browser,service: browser-rum-sdk.Checklist
docs/ARCHITECTURE.md→ "Renderer telemetry")🤖 Generated with Claude Code