✨ add beforeSend event filtering and scrubbing - #180
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
@codex review |
There was a problem hiding this comment.
PR Review — Score: 4.6 / 5
This is a well-scoped addition of main-process beforeSend filtering and scrubbing. RumEventMapper is wired at the right boundary — after format hooks assemble the event and before ServerRumEvent emission — with an allowlist + clone pattern that protects identity fields, blocks view/crash drops, and fails open on callback errors. Documentation, playground controls, and unit/E2E coverage are thorough. I would approve.
Why 4.6: Correct pipeline placement, strong allowlist enforcement with sanitization, protected view/crash semantics, clear public API typing (RumBeforeSend, MainRumEvent), and meaningful tests across mapper, assembly, config, e2e, and playground.
Why not 5: Minor consistency gap when scrubbing service/version (ddtags not updated), plus small UX/perf nits around string clearing and per-event cloning.
Findings
- [Minor] ddtags not synced with scrubbed service/version — Allowlisted
service/versionchanges do not update theddtagsstring set bycommonContext. - [Nit] String fields cannot be cleared — Setting an allowlisted string to
null/undefinedsilently keeps the original value. - [Nit] Per-event deepClone cost — Every main-process RUM event is deep-cloned when
beforeSendis configured.
Architectural flow
sequenceDiagram
participant Coll as RUM collection
participant EM as EventManager
participant MA as MainAssembly
participant Hooks as FormatHooks
participant Map as RumEventMapper
participant VC as ViewCollection
participant T as Transport
Coll->>EM: RawRumEvent
EM->>MA: handle
MA->>Hooks: triggerRum
Hooks-->>MA: session, view, service, ddtags
MA->>MA: combine raw + hooks
MA->>Map: map assembled event
Map->>Map: deepClone and beforeSend
alt discarded
Map-->>MA: undefined
MA-->>EM: no ServerEvent
else kept or scrubbed
Map-->>MA: modified event
MA->>EM: ServerRumEvent
EM->>VC: increment counters
EM->>T: batch for intake
end
Before: Main-process RawRumEvents were enriched via format hooks and emitted directly as ServerRumEvents. Renderer events already used the Browser SDK beforeSend before crossing the bridge.
After: MainAssembly runs RumEventMapper on fully assembled main-process RUM events. Customers can scrub allowlisted fields or return false to drop non-view, non-crash events. Discarded events never become ServerRumEvents, so view counters and transport stay consistent. Telemetry, profiles, spans, and renderer RUM events are unchanged.
Sent by Cursor Automation: electron-sdk reviews
![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: d879cfa0f2
ℹ️ 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".
| Use `beforeSend` to inspect fully assembled main-process RUM events before they are sent to Datadog: | ||
|
|
||
| ```ts | ||
| await init({ | ||
| // ... | ||
| beforeSend: (event) => { | ||
| if (event.type === 'error') { |
There was a problem hiding this comment.
❓ question: For the browser-sdk, there is one beforeSend for RUM events and another one for logs events.
What is the strategy for supporting logs in the future?
There was a problem hiding this comment.
logs should have a separate callback like the browser sdk I guess 👍
There was a problem hiding this comment.
It could be nice to have a clear strategy on that
|
|
||
| ### Event Filtering and Scrubbing | ||
|
|
||
| Use `beforeSend` to inspect fully assembled main-process RUM events before they are sent to Datadog: |
There was a problem hiding this comment.
💭 thought: we are also collecting some spans, it could be interesting to see if we should do something here
There was a problem hiding this comment.
Do we plan to handle things differently about spans ? beforeSend in the browser sdk doesn't allow to modify spans
There was a problem hiding this comment.
We don't collect spans on the browser-sdk, we do collect some on the electron-sdk.
I think mobile are collecting some as well, it could be worth comparing our strategies and see what makes sense.
| `MainAssembly` applies `RumEventMapper` after enrichment and before emitting the final `ServerRumEvent`. Renderer | ||
| events use the Browser SDK's `beforeSend` before crossing the bridge and are not mapped again by the Electron SDK. | ||
| Telemetry, profiles, and spans are not mapped. |
There was a problem hiding this comment.
💭 thought: From past discussion, I though we were more leaning towards handling renderer events too.
I think it is fine to only target main events first but it could be nice to have some thoughts on how we can extend the current API to support those events later.
See if it can influence the current API shape or not.
There was a problem hiding this comment.
I added a note about this in the PR description when I opened it. The choice was intentional: renderer events already pass through the Browser SDK’s beforeSend, and Electron currently only enriches them with protected fields, so a second callback would mostly duplicate filtering. After our WebView discussion, I followed the mobile SDK approach and scoped Electron’s beforeSend to main-process events only.
What do you think ?
There was a problem hiding this comment.
cf my comment
I think it is fine to only target main events first
My points was mostly about:
it could be nice to have some thoughts on how we can extend the current API to support those events later.
See if it can influence the current API shape or not.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3ae277cbe
ℹ️ 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".
|
🔄 Datadog auto-retried 1 job - 1 passed on retry 🔗 Commit SHA: b3ae277 | Docs | Datadog PR Page | Give us feedback! |
2 participants
Motivation
Allow Electron applications to filter or scrub main-process RUM events before they are sent.
Changes
beforeSendcallback for fully assembled main-process RUM events.Note :
I kept beforeSend separate because renderer events already go through the Browser SDK callback before crossing the bridge
Running the Electron callback afterward would add a second filtering step . Browser therefore handles renderer events, while Electron handles main-process events. Android follow the same path with webview, there is no "global" before send
Test instructions
yarn test:unityarn typecheckyarn buildyarn playwright test -c e2e --project=e2e e2e/scenarios/before-send.scenario.tsyarn --cwd playground test before-send.scenario.ts