⚗️ Add webSocketObservable for WebSocket lifecycle events#4715
Conversation
Bundles Sizes Evolution
|
|
bdibon
force-pushed
the
boris.dibon/ws-1-instrument-method-constructor
branch
from
c445e3c to
e2bdea2
Compare
bdibon
force-pushed
the
boris.dibon/ws-2-observable
branch
2 times, most recently
from
9763a7b to
7b7708d
Compare
bdibon
changed the base branch from
boris.dibon/ws-1-instrument-method-constructor
to
graphite-base/4715
bdibon
force-pushed
the
boris.dibon/ws-2-observable
branch
from
7b7708d to
8c31a3b
Compare
bdibon
changed the base branch from
graphite-base/4715
to
boris.dibon/ws-1-instrument-method-constructor
bdibon
changed the base branch from
boris.dibon/ws-1-instrument-method-constructor
to
graphite-base/4715
bdibon
force-pushed
the
boris.dibon/ws-2-observable
branch
from
8c31a3b to
55471a3
Compare
bdibon
changed the base branch from
graphite-base/4715
to
boris.dibon/ws-1-instrument-method-constructor
bdibon
changed the base branch from
boris.dibon/ws-1-instrument-method-constructor
to
graphite-base/4715
bdibon
force-pushed
the
boris.dibon/ws-2-observable
branch
from
55471a3 to
d3cac5a
Compare
bdibon
force-pushed
the
graphite-base/4715
branch
from
201cc19 to
0e51273
Compare
bdibon
changed the title
![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: d3cac5ab73
ℹ️ 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".
| }) | ||
| }) | ||
|
|
||
| stopListeners.push(stopOpen, stopMessage, stopClose) |
There was a problem hiding this comment.
issue: Each constructed socket contributes three stop closures to the singleton stopListeners array, and those closures retain the WebSocket instance until the whole observable is unsubscribed (which is typically never during an SDK session). On pages that create many short-lived WebSockets, closed sockets remain strongly referenced after their close event; the close handler should stop/remove the instance listeners after reporting the close.
Useful? React with 👍 / 👎.
| observable: Observable<WebSocketContext>, | ||
| stopListeners: Array<() => void> | ||
| ) { | ||
| const { stop: stopOpen } = addEventListener({ allowUntrustedEvents }, instance, 'open', () => { |
There was a problem hiding this comment.
issue: Passing { allowUntrustedEvents } here snapshots the current value when the WebSocket is constructed, so if an early caller initializes the singleton with allowUntrustedEvents: true and the customer configuration later calls initWebSocketObservable({ allowUntrustedEvents: false }), any sockets created in between will still report synthetic open/message/close events. That contradicts the singleton policy described above and leaks untrusted events for pre-existing sockets; the listener should read the mutable latest policy instead of a copied primitive.
Useful? React with 👍 / 👎.
| globalObject, | ||
| 'WebSocket', | ||
| ({ parameters, onPostCall }) => { | ||
| const url = String(parameters[0]) |
There was a problem hiding this comment.
issue: Capturing String(parameters[0]) reports the raw constructor argument, so common calls like new WebSocket('/socket') produce url: '/socket' even though the browser connects to an absolute ws:///wss:// URL. Since the observable emits after construction, using instance.url there would preserve the browser's canonical URL and avoid under-identifying relative WebSocket endpoints.
Useful? React with 👍 / 👎.
| observable: Observable<WebSocketContext>, | ||
| stopListeners: Array<() => void> | ||
| ) { | ||
| const { stop: stopOpen } = addEventListener({ allowUntrustedEvents }, instance, 'open', () => { |
There was a problem hiding this comment.
issue: In a Dedicated Worker, globalObject.WebSocket is available so this observable proceeds, but constructing a socket then calls addEventListener, whose implementation dereferences the page-only window.EventTarget. That makes initWebSocketObservable() crash on the first worker WebSocket instead of collecting or no-oping; this path should avoid the window-based helper or guard worker globals explicitly.
Useful? React with 👍 / 👎.
bdibon
force-pushed
the
boris.dibon/ws-2-observable
branch
from
d3cac5a to
a1a9d9e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1a9d9eb65
ℹ️ 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".
| observable: Observable<WebSocketContext>, | ||
| stopListeners: Array<() => void> | ||
| ) { | ||
| const { stop: stopOpen } = addEventListener({ allowUntrustedEvents }, instance, 'open', () => { |
There was a problem hiding this comment.
When a WebSocket is constructed, this calls addEventListener with the trust-policy object as the event target and the socket as the event name, so addEventListener ends up trying to install a listener on a plain object instead of on the WebSocket. In practice the monitored post-constructor hook fails while attaching listeners, so the observable never receives open, message, or close events for new sockets; the socket instance needs to be the first argument here (and similarly for the other listeners).
Useful? React with 👍 / 👎.
|
|
||
| function computePayloadSize(data: unknown): number { | ||
| if (typeof data === 'string') { | ||
| return new TextEncoder().encode(data).byteLength |
There was a problem hiding this comment.
For text-heavy WebSocket traffic, this allocates a new TextEncoder output buffer as large as each incoming and outgoing string just to count bytes, so enabling this observable can add CPU and memory pressure proportional to all message payloads. The core package already has computeBytesCount() with an ASCII fast path for this exact sizing case, which would preserve UTF-8 accuracy without copying common text frames.
Useful? React with 👍 / 👎.
| if (typeof data === 'string') { | ||
| return new TextEncoder().encode(data).byteLength | ||
| } | ||
| if (data instanceof ArrayBuffer) { |
There was a problem hiding this comment.
If an app sends or receives an ArrayBuffer created in another same-origin frame, this realm-specific instanceof ArrayBuffer check is false even though the native WebSocket still transmits the bytes, so the observable reports size: 0 for that binary frame; the same applies to cross-realm Blobs in the branch below. This undercounts WebSocket traffic for iframe-based apps unless the detection uses realm-agnostic byteLength/size checks.
Useful? React with 👍 / 👎.
Co-authored-by: Cursor <cursoragent@cursor.com>
bdibon
force-pushed
the
boris.dibon/ws-2-observable
branch
from
a1a9d9e to
158b6ae
Compare
1 participant
Motivation
Provide a low-level observable that emits WebSocket lifecycle events (connecting, open, message in/out, closed). This is the browser-level building block consumed by the future WebSocket collection layer.
Changes
webSocketObservable(initWebSocketObservable), instrumenting theWebSocketconstructor andsend, and listening toopen/message/close/errorevents.WebSocketEventMapsupport toaddEventListener.@datadog/browser-core.Test instructions
yarn test:unit --spec packages/core/src/browser/webSocketObservable.spec.tsChecklist