Repository navigation
feat(workspace-client,desktop): relay WS outage telemetry + version-stamped desktop events - #5653
Merged
Merged
Conversation
…tamped desktop events Production terminal_connect_failed events showed the expired-JWT redial loop but couldn't be attributed to app builds, and the eventBus relay socket (the workspace "disconnected" surface) emitted no telemetry at all. This adds: - outageReporter: pure episode state machine emitting access_denied / degraded / recovered at most once per outage, via a pluggable process-wide sink (package stays analytics-vendor-free) - createRelaySocket wires dial outcomes into the reporter; sockets are labeled (event-bus, web-terminal) for queryability - desktop registers app_version as a PostHog super property and forwards relay socket health to relay_ws_* events
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit fc1911c on Jul 13, 2026 6:10am UTC. |
Contributor
📝 WalkthroughWalkthroughRelay socket connections now track outage episodes, emit sanitized telemetry for degradation, access denial, and recovery, and expose socket names for reporting. Desktop PostHog wiring captures these events and registers the application version. ChangesRelay socket telemetry
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RelaySocket
participant OutageReporter
participant PostHog
RelaySocket->>OutageReporter: report dial, failure, denial, or open
OutageReporter->>OutageReporter: aggregate outage episode
OutageReporter->>PostHog: capture relay_ws_<kind> event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds telemetry for relay WebSocket outages and stamps desktop analytics with the app version. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/posthog.ts | Registers desktop app version analytics and installs the relay socket telemetry sink. |
| packages/workspace-client/src/lib/relaySocket/outageReporter.ts | Adds the outage episode state machine and telemetry event shape. |
| packages/workspace-client/src/lib/relaySocket/relaySocket.ts | Connects relay socket attempts, failures, access denials, and opens to the outage reporter. |
| packages/workspace-client/src/index.ts | Exports the new relay socket telemetry APIs from the package entrypoint. |
| packages/workspace-client/src/lib/relaySocket/index.ts | Re-exports outage reporter telemetry APIs from the relay socket module. |
| packages/workspace-client/src/lib/eventBus.ts | Labels event-bus relay sockets for telemetry attribution. |
| apps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/TerminalConnection.ts | Labels web-terminal relay sockets for telemetry attribution. |
Reviews (1): Last reviewed commit: "feat(workspace-client,desktop): relay WS..." | Re-trigger Greptile
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/posthog.ts (1)
37-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap the telemetry callback in a try-catch to protect the relay socket lifecycle.
If
posthogFull.capturethrows, the error propagates throughtelemetrySink→emit→accessDenied/failed/opened→ the WebSocket event handlers or the asyncproviderinrelaySocket.ts. In theaccessDeniedpath specifically, this would preventopts.onAccessDenied?.()from being called and prevent the intendedthrow new Error("relay access denied")from executing, altering the connection shutdown behavior.🛡️ Proposed defensive wrapper
setRelaySocketTelemetry((event) => { - posthogFull.capture(`relay_ws_${event.kind}`, { - socket_name: event.socketName, - endpoint: event.endpoint, - preflight_status: event.preflightStatus, - tunnel_region: event.tunnelRegion, - close_code: event.closeCode, - close_reason: event.closeReason, - reconnect_attempts: event.failedAttempts, - outage_ms: event.outageMs, - }); + try { + posthogFull.capture(`relay_ws_${event.kind}`, { + socket_name: event.socketName, + endpoint: event.endpoint, + preflight_status: event.preflightStatus, + tunnel_region: event.tunnelRegion, + close_code: event.closeCode, + close_reason: event.closeReason, + reconnect_attempts: event.failedAttempts, + outage_ms: event.outageMs, + }); + } catch { + // Telemetry must never disrupt the relay socket lifecycle. + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/posthog.ts` around lines 37 - 48, Wrap the callback registered with setRelaySocketTelemetry in a try-catch so failures from posthogFull.capture do not propagate into relay socket lifecycle handlers. Keep the existing event payload and ensure the caught telemetry error is handled without interrupting accessDenied, failed, or opened processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/renderer/lib/posthog.ts`:
- Around line 37-48: Wrap the callback registered with setRelaySocketTelemetry
in a try-catch so failures from posthogFull.capture do not propagate into relay
socket lifecycle handlers. Keep the existing event payload and ensure the caught
telemetry error is handled without interrupting accessDenied, failed, or opened
processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b6320bc-484a-4cdf-a364-1e5ea0de7d7e
📒 Files selected for processing (9)
apps/desktop/src/renderer/lib/posthog.tsapps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/TerminalConnection.tspackages/workspace-client/src/index.tspackages/workspace-client/src/lib/eventBus.tspackages/workspace-client/src/lib/relaySocket/index.tspackages/workspace-client/src/lib/relaySocket/outageReporter.test.tspackages/workspace-client/src/lib/relaySocket/outageReporter.tspackages/workspace-client/src/lib/relaySocket/relaySocket.test.tspackages/workspace-client/src/lib/relaySocket/relaySocket.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/workspace-client/src/lib/relaySocket/relaySocket.ts">
<violation number="1" location="packages/workspace-client/src/lib/relaySocket/relaySocket.ts:65">
P3: `relay_ws_access_denied` reports zero attempts on the first denied dial, so denial telemetry undercounts the episode that triggered it. Pass the current dial as `retryCount + 1` here; later retries have the same off-by-one before provider rejection is handled.</violation>
<violation number="2" location="packages/workspace-client/src/lib/relaySocket/relaySocket.ts:87">
P2: The `error` event listener on the socket fires before the `close` event listener for abnormal connection drops (per WebSocket spec, error fires first). Since both handlers call `reporter.failed()`, the error handler fires first, marks the outage as reported, and emits a `degraded` event with `closeCode: null`. When the `close` handler fires moments later with a meaningful code (e.g., 1006) and reason, it finds `reported = true` and returns early, discarding the diagnostic details. This reduces the telemetry value for connection drops, especially for differentiating abnormal closures from silent failures. Consider having the close handler be the primary signal (passing full `CloseInfo`) and the error handler be a fallback that only calls `reporter.failed()` if `reported` is still false after a short microtask delay, or restructure the reporter to accept optional close info that supersedes the error-only call.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Renderer as Desktop Renderer (initPostHog)
participant PostHog as PostHog (capture)
participant TelemSink as Telemetry Sink (setRelaySocketTelemetry)
participant EventBus as EventBus (getOrCreateConnection)
participant CreateSocket as createRelaySocket (with name)
participant Reporter as outageReporter (state machine)
participant Relay as Remote Relay (WS / _whoowns)
Note over Renderer, Relay: NEW: Relay WS telemetry + version-stamped events
Renderer->>PostHog: register({app_version, app_name, platform})
Note over Renderer,PostHog: CHANGED: app_version super property added
Renderer->>TelemSink: setRelaySocketTelemetry(sinkFn)
Note over Renderer,TelemSink: Process-wide sink, forwards to PostHog
EventBus->>CreateSocket: createRelaySocket(name="event-bus", ...)
CreateSocket->>Reporter: createOutageReporter("event-bus")
Note over CreateSocket, Relay: --- Degraded episode (≥5 failed dials) ---
CreateSocket->>Relay: attempt(url+signedToken)
CreateSocket->>Reporter: attempt(url, probe)
CreateSocket->>Relay: _whoowns preflight
Relay-->>CreateSocket: 200 OK (or null)
CreateSocket->>Reporter: failed(attempts=1)
loop Failed dials 2..4
CreateSocket->>Reporter: failed(attempts)
end
CreateSocket->>Reporter: failed(attempts=5)
alt attempts >= DEGRADED_AFTER_ATTEMPTS (5)
Reporter->>TelemSink: emit({kind:"degraded", socketName:"event-bus", failedAttempts:5, ...})
TelemSink->>PostHog: capture("relay_ws_degraded", { ... })
end
CreateSocket->>Reporter: failed(attempts=6) -- dedup, no emission
Note over CreateSocket, Relay: --- Recovery ---
Relay-->>CreateSocket: connect success
CreateSocket->>Reporter: opened(attempts=7)
alt was reported (degraded)
Reporter->>TelemSink: emit({kind:"recovered", outageMs, failedAttempts:7})
TelemSink->>PostHog: capture("relay_ws_recovered", { ... })
end
Note over CreateSocket, Relay: --- Access denied episode ---
CreateSocket->>Relay: attempt(url+signedToken)
CreateSocket->>Reporter: attempt(url, probe)
CreateSocket->>Relay: _whoowns preflight
Relay-->>CreateSocket: 403 Forbidden (preflight_status:403)
CreateSocket->>Reporter: accessDenied(attempts=0)
alt not yet reported
Reporter->>TelemSink: emit({kind:"access_denied", preflightStatus:403, ...})
TelemSink->>PostHog: capture("relay_ws_access_denied", { ... })
end
CreateSocket->>Reporter: failed(attempts=1) -- no degraded emission, suppressed
CreateSocket->>Reporter: accessDenied(attempts=2) -- dedup, no emission
CreateSocket->>Reporter: accessDenied(attempts=3) -- dedup
Note over CreateSocket, Relay: --- Regrant ends episode ---
Relay-->>CreateSocket: 200 OK + connect success
CreateSocket->>Reporter: opened(attempts=10)
Reporter->>TelemSink: emit({kind:"recovered", outageMs, ...})
TelemSink->>PostHog: capture("relay_ws_recovered", { ... })
Note over Renderer, PostHog: Existing events (terminal_connect_failed) now carry app_version via super property
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| maxEnqueuedMessages: opts.maxEnqueuedMessages ?? 0, | ||
| }); | ||
|
|
||
| socket.addEventListener("close", (event) => { |
Contributor
There was a problem hiding this comment.
P2: The error event listener on the socket fires before the close event listener for abnormal connection drops (per WebSocket spec, error fires first). Since both handlers call reporter.failed(), the error handler fires first, marks the outage as reported, and emits a degraded event with closeCode: null. When the close handler fires moments later with a meaningful code (e.g., 1006) and reason, it finds reported = true and returns early, discarding the diagnostic details. This reduces the telemetry value for connection drops, especially for differentiating abnormal closures from silent failures. Consider having the close handler be the primary signal (passing full CloseInfo) and the error handler be a fallback that only calls reporter.failed() if reported is still false after a short microtask delay, or restructure the reporter to accept optional close info that supersedes the error-only call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/workspace-client/src/lib/relaySocket/relaySocket.ts, line 87:
<comment>The `error` event listener on the socket fires before the `close` event listener for abnormal connection drops (per WebSocket spec, error fires first). Since both handlers call `reporter.failed()`, the error handler fires first, marks the outage as reported, and emits a `degraded` event with `closeCode: null`. When the `close` handler fires moments later with a meaningful code (e.g., 1006) and reason, it finds `reported = true` and returns early, discarding the diagnostic details. This reduces the telemetry value for connection drops, especially for differentiating abnormal closures from silent failures. Consider having the close handler be the primary signal (passing full `CloseInfo`) and the error handler be a fallback that only calls `reporter.failed()` if `reported` is still false after a short microtask delay, or restructure the reporter to accept optional close info that supersedes the error-only call.</comment>
<file context>
@@ -74,5 +83,15 @@ export function createRelaySocket(opts: RelaySocketOptions): RelaySocket {
maxEnqueuedMessages: opts.maxEnqueuedMessages ?? 0,
});
+
+ socket.addEventListener("close", (event) => {
+ // 1000 = deliberate close (cleanup, access-denied shutdown), not a failure.
+ if (event.code !== 1000) {
</file context>
…ket synthetic closes, harden sink Review fixes + CDP-verified fixes: - failuresSoFar() passes retryCount+1 so the first denied/failed dial counts itself (cubic P3) - keep the outage's last observed CloseInfo so degraded isn't emitted with null close details when the error listener crosses the threshold (cubic P2) - wrap the telemetry sink in try/catch inside the reporter so a throwing sink can never break the socket lifecycle (coderabbit) - ignore partysocket synthetic close events (browser cloneEvent mangles them: code becomes the string "close"); found live via CDP — a deliberate close() after >=4 failures in the uptime window emitted a spurious degraded, and close_code/close_reason carried garbage
Member
Author
|
Addressed all review findings in fc1911c, then verified the full telemetry surface live over CDP against the dev app (with #5652 merged in): Review dispositions
Found and fixed a real bug during CDP verification (browser-only, invisible to bun tests which take partysocket's Node CDP verification (all against the wired production code path, patched
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/workspace-client/src/lib/relaySocket/outageReporter.test.ts (1)
70-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
failed(6)never reaches the throwing sink in this test.
accessDenied(1)setsreported = truebeforefailed(6)is called, sofailedhits the earlyreturnatif (reported || ...) returnwithout invokingemit. The assertionexpect(() => reporter.failed(6)).not.toThrow()passes trivially — the throwing sink is never exercised through thefailed→emitpath. If thetry/catchinemitwere removed, this test would still pass forfailed.Use a fresh reporter so
failedcrosses the threshold withreported === falseand actually calls the sink:🛠️ Proposed fix
it("never lets a throwing sink escape into the caller", () => { setRelaySocketTelemetry(() => { throw new Error("analytics exploded"); }); const reporter = createOutageReporter("bus"); reporter.attempt("ws://relay/hosts/h/events", { status: 403, region: null, }); expect(() => reporter.accessDenied(1)).not.toThrow(); - expect(() => reporter.failed(6)).not.toThrow(); expect(() => reporter.opened(7)).not.toThrow(); + + const failedReporter = createOutageReporter("bus"); + failedReporter.attempt("ws://relay/hosts/h/events", null); + expect(() => failedReporter.failed(6)).not.toThrow(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workspace-client/src/lib/relaySocket/outageReporter.test.ts` around lines 70 - 82, Update the throwing-sink test around createOutageReporter so failed(6) is invoked on a fresh reporter whose reported state is still false and whose failure count crosses the emission threshold, ensuring it reaches emit. Keep the existing non-throwing assertions for accessDenied, failed, and opened while exercising the failed-to-sink path rather than the early return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/workspace-client/src/lib/relaySocket/outageReporter.test.ts`:
- Around line 70-82: Update the throwing-sink test around createOutageReporter
so failed(6) is invoked on a fresh reporter whose reported state is still false
and whose failure count crosses the emission threshold, ensuring it reaches
emit. Keep the existing non-throwing assertions for accessDenied, failed, and
opened while exercising the failed-to-sink path rather than the early return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 712cb7ae-0df7-469e-9268-ab44ab5bf057
📒 Files selected for processing (3)
packages/workspace-client/src/lib/relaySocket/outageReporter.test.tspackages/workspace-client/src/lib/relaySocket/outageReporter.tspackages/workspace-client/src/lib/relaySocket/relaySocket.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workspace-client/src/lib/relaySocket/relaySocket.ts
- packages/workspace-client/src/lib/relaySocket/outageReporter.ts
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
createRelaySocket(the eventBus / workspace "disconnected" surface, previously silent):relay_ws_access_denied,relay_ws_degraded, andrelay_ws_recoveredPostHog events, at most one per outage plus one on recovery.app_versionas a PostHog super property in the desktop renderer so every event (including the existingterminal_connect_failed) is attributable to the build that emitted it.Why / Context
Investigating the remote-workspace disconnect reports (relay 401 redial loop, fixed by #5628/#5637 but not yet released) hit two observability gaps:
terminal_connect_failedevents carry no app version — once 1.14.4 ships there's no way to segment pre/post-fix builds and confirm the 401 curve dies. The person-profiledesktop_versionreflects the current install, not the build at event time.How It Works
outageReporter.ts(co-located inrelaySocket/) is a pure state machine: callers feed it dial outcomes (attempt,accessDenied,failed,opened) and it collapses connection churn into episodes — oneaccess_denied(preflight 403) ordegraded(≥5 consecutive failed dials) event per episode, onerecoveredwithoutage_msand attempt count when the socket reconnects. partysocket'sretryCountonly resets after 5s of stable uptime, so flapping accumulates into one episode instead of spamming pairs.setRelaySocketTelemetry), keeping@superset/workspace-clientanalytics-vendor-free; the desktop renderer wires it to PostHog ininitPostHog. Web consumers are unaffected (sink defaults to null).namelabel (event-bus,web-terminal) so consumers are distinguishable in queries. The reported endpoint is the attempt URL minus its query string — the token never leaves the module.Manual QA Checklist
app_version(CDP-verified: super property registered,1.14.1in dev)relay_ws_degradedafter 5 failed dials → healed endpoint → onerelay_ws_recoveredwith saneoutage_ms; access-denied and terminal give-up paths verified too (see verification comment)Testing
bun run typecheck(workspace-client, desktop, web)bun run lint(exits 0)bun testinpackages/workspace-client— 18 pass: new pure unit tests for the reporter policy (threshold boundary, once-per-episode dedup, silent short blips, episode reset on regrant, token stripping) plus WS integration tests proving the socket wiring end-to-end against a scriptable local relaydegradedemission and breaking the access-denied dedup each made exactly the expected test failDesign Decisions
RelaySocketOptions: everygetEventBuscall site would need plumbing for no gain; a process-wide analytics hook is the same shape posthog itself uses.Known Limitations
terminal_connect_failedin the desktop terminal transport keeps its own capture path; it now gainsapp_versionvia the super property but doesn't use the outage reporter (it has its own max-attempts/give-up semantics). Unifying them is possible follow-up.Summary by cubic
Adds relay WebSocket outage telemetry and version-stamped desktop analytics to improve visibility into remote workspace disconnects. Emits
relay_ws_*events once per outage and recordsapp_versionon every desktop event; fixes attempt counting and filters synthetic closes for accurate stats.@superset/workspace-client: added outage reporter and process-wide telemetry sink (setRelaySocketTelemetry,RelaySocketTelemetryEvent).createRelaySocketnow reportsaccess_denied,degraded(≥5 failed dials), andrecoveredwithoutage_msand attempt count; tokens are stripped from endpoints. Hardened reporting: correct attempt ordinals, ignore partysocket synthetic closes, include last close info when errors cross the threshold, and guard the sink with try/catch.nameoption oncreateRelaySocket; event bus usesevent-bus, web terminal usesweb-terminal.relay_ws_${kind}events with endpoint, region, close info, and attempt counts. Registersapp_versionas a super property so all desktop events are attributed to the emitting build.Written for commit fc1911c. Summary will update on new commits.
Summary by CodeRabbit