Repository navigation
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 5ef2033 | Docs | View more details | Give us feedback! |
Bundles Sizes Evolution
|
a488fea to
197a42b
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
bcaudan
left a comment
There was a problem hiding this comment.
LGTM but could be impacted by #5093 (comment)
197a42b to
2e6497f
Compare
The browser fires no event when the closing handshake starts, so close() is instrumented. A call on a socket already closing or closed is a no-op and notifies nothing. Co-authored-by: Cursor <cursoragent@cursor.com>
send() on a socket that is not OPEN never reaches the wire: it throws before the handshake completed, and is silently dropped once the socket is closing or closed. Co-authored-by: Cursor <cursoragent@cursor.com>
…cket The closing vital carries the closing date and the client as initiator. It takes no snapshot version, and the closed vital that follows keeps the closing date. A close() during the handshake reports the closing phase without an open date. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
2e6497f to
5ef2033
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ef20335e3
ℹ️ 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.notify({ | ||
| state: 'closing', | ||
| instance, | ||
| at: clocksNow(), |
There was a problem hiding this comment.
Capture the closing clock before invoking close
issue: When WebSocket.prototype.close was already wrapped by customer code or another SDK that performs synchronous work, this samples the clock only after that wrapper returns. The resulting closing_date and view attribution can therefore be delayed—or assigned to a view started inside the wrapper—instead of representing when the application called close(). Capture the clock in the pre-call callback and use it from onPostCall, which still avoids reporting calls that throw.
Useful? React with 👍 / 👎.
Motivation
The browser fires no event when the closing handshake starts, so without instrumenting
close()the backend cannot tell a connection the application closed from one the server or the network ended.Part of the WebSocket vitals stack. Every PR targets the previous one; the first one targets the
boris.dibon/websocket-vitalsintegration branch, which is merged intomainin one go once the whole stack is approved. #5055 stays open as the reference implementation: the tip of this stack is content-identical to it, rebased on the currentmain.Changes
Commit by commit:
close()is instrumented; a call on a socket already closing or closed is a no-op and notifies nothing.send()outsideOPENnever reaches the wire (it throws before the handshake, and is silently dropped once closing or closed), so it is no longer counted as an outbound message.closing_dateandclose_initiator: "client", takes no snapshot version, and the closed vital that follows keeps the closing date. Aclose()during the handshake reports the closing phase without an open date.close()called twice.Test instructions
yarn test:unit --spec "packages/browser-rum-core/src/domain/webSocket/*.spec.ts" --spec packages/browser-rum-core/src/browser/webSocketObservable.spec.tsyarn test:e2e -g "rum websockets"Note
check-staging-mergeis expected to fail:staging-40already contains the earlier merge of #5055, which touches the same files. It clears at the next staging bump; the other checks should be green.Checklist