feat(llc): Client call event reporting (join telemetry)#1268
Open
Brazol wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds a client-side telemetry subsystem for tracking call join lifecycle events. New modules define event data models, error-to-failure mapping, HTTP transport with retry, and a ChangesClient Event Telemetry
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Call
participant ClientEventReporter
participant CallSession
participant CoordinatorWS
participant ClientEventTransport
Call->>ClientEventReporter: registerCall(cid)
Call->>ClientEventReporter: reportEvent(joinInitiated)
CoordinatorWS->>ClientEventReporter: beginStage(coordinatorWs)
CoordinatorWS-->>ClientEventReporter: completeStage/failStage
Call->>ClientEventReporter: beginStage(coordinatorJoin)
Call-->>ClientEventReporter: completeStage(setCallSessionId)
CallSession->>ClientEventReporter: beginStage(wsJoin)
CallSession-->>ClientEventReporter: completeStage/failStageWithError
ClientEventReporter->>ClientEventTransport: send(events) [fire-and-forget]
ClientEventTransport-->>ClientEventTransport: POST with retry/backoff
Call->>ClientEventReporter: newJoinAttempt(reason) [on reconnect]
Call->>ClientEventReporter: abort(code) / unregisterCall(cid) [on leave]
Possibly related PRs
Suggested reviewers: 🚥 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
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_video/lib/src/call/call.dart (1)
2271-2288: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Call.end()never unregisters/aborts the call inClientEventReporter.The new
abort(...)/unregisterCall(...)cleanup was added only toleave(). Howeverend()(Lines 941-970) calls_disconnect(...)directly and never callsleave(), so ending a call viaCall.end()(e.g. host ending the call for everyone) never callsclientEventReporter.abort/unregisterCall. The_onStateChangedhandler'sCallStatusDisconnectedbranch also only calls_clear(...), not the reporter.Per the cited reporter snippet,
unregisterCallremoves the call's entry from its internal_callsmap stage_id -> pending pair (both call- and connection-scoped) ... void unregisterCall(StreamCallCid cid) { abort(cid, ClientEventStandardCode.clientAborted); _calls.remove(cid.value); — skipping it means the entry (and any still-pending stages) is never cleaned up for calls ended viaend(), leaking state and leaving telemetry stages unresolved.Move the cleanup into the shared
_disconnect()helper (called by bothleave()andend()) instead of onlyleave().🔧 Proposed fix
Future<Result<None>> leave({DisconnectReason? reason}) async { _logger.i(() => '[leave] reason: $reason'); final abortCode = switch (reason) { DisconnectReasonEnded() || DisconnectReasonCallEnded() => ClientEventStandardCode.backendLeave, // Reconnection gave up — treat as a device-offline DisconnectReasonReconnectionFailed() => ClientEventStandardCode.networkOffline, _ => ClientEventStandardCode.clientAborted, }; - _streamVideo.clientEventReporter - ..abort(callCid, abortCode) - ..unregisterCall(callCid); try { final didDisconnect = await _disconnect( - sfuLeaveReason: _sfuLeaveReason(reason), + sfuLeaveReason: _sfuLeaveReason(reason), + abortCode: abortCode, );- Future<bool> _disconnect({required String sfuLeaveReason}) async { + Future<bool> _disconnect({ + required String sfuLeaveReason, + ClientEventStandardCode abortCode = ClientEventStandardCode.clientAborted, + }) async { if (_leaveCallTriggered) { _logger.i(() => '[disconnect] rejected (already disconnecting)'); return false; } _leaveCallTriggered = true; + _streamVideo.clientEventReporter + ..abort(callCid, abortCode) + ..unregisterCall(callCid);cid value -> call state. final Map<String, _CallState> _calls = {}; ... void unregisterCall(StreamCallCid cid) { abort(cid, ClientEventStandardCode.clientAborted); _calls.remove(cid.value); _logger.d(() => '[unregisterCall] cid: ${cid.value}');
🤖 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/stream_video/lib/src/call/call.dart` around lines 2271 - 2288, `Call.end()` is bypassing the `ClientEventReporter` cleanup that `leave()` now performs. Move the `clientEventReporter.abort(...)` and `clientEventReporter.unregisterCall(...)` calls into the shared `_disconnect(...)` path in `Call`, so both `leave()` and `end()` always clean up reporter state. Keep the abort-code selection tied to the existing `leave({reason})` logic, and ensure `_onStateChanged`/`CallStatusDisconnected` does not rely on `_clear(...)` alone for reporter cleanup.
🧹 Nitpick comments (2)
packages/stream_video/test/src/telemetry/media_frame_reporter_test.dart (1)
1-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the video first-frame path.
All three tests exercise only
onRemoteAudioTrack/onSubscriberStats(FirstAudioFrame).onRemoteVideoTrack/_reportVideo(renderer creation,onFirstFrameRenderedfiring, single-shot behavior, and renderer disposal) has no test coverage in this file.🤖 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/stream_video/test/src/telemetry/media_frame_reporter_test.dart` around lines 1 - 91, Add test coverage for the video first-frame flow in MediaFrameReporter, since this file only verifies the audio path through onRemoteAudioTrack and onSubscriberStats. Create tests that exercise onRemoteVideoTrack and the _reportVideo behavior, asserting that onFirstFrameRendered emits the expected first-frame event once, that it does not fire before a remote video track is registered, and that the renderer is disposed after the event. Use the existing ClientEventReporter/MediaFrameReporter setup and mirror the single-shot assertions already used for FirstAudioFrame.packages/stream_video/lib/src/call/session/call_session.dart (1)
452-569: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
fastReconnect()has nowsJoinstage telemetry, unlikestart().
start()wraps the SFU WS connect/join-response flow withbeginStage/completeStage/failStageforClientEventStage.wsJoin, butfastReconnect()performs an equivalent SFU WS join (sfuWS.recreate(), sending the join request, waiting forSfuJoinResponseEvent) with no telemetry at all — including its own explicit failure path at Lines 482-487. Since fast-reconnect is a common, failure-prone path, consider wiring the same stage reporting here for consistent join-telemetry coverage.🤖 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/stream_video/lib/src/call/session/call_session.dart` around lines 452 - 569, `fastReconnect()` is missing `wsJoin` stage telemetry for the SFU WS recreate/join flow, unlike `start()`. Update `CallSession.fastReconnect` to wrap the `sfuWS.recreate()`, join request send, and `SfuJoinResponseEvent` wait with the same `beginStage`/`completeStage`/`failStage` reporting used in `start()` for `ClientEventStage.wsJoin`, including the explicit failure return path when `wsResult.isFailure` is true.
🤖 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.
Inline comments:
In `@packages/stream_video/lib/src/telemetry/client_event_transport.dart`:
- Around line 60-111: The `ClientEventTransport.send` flow only catches failures
around `_httpClient.post`, so exceptions from `jsonEncode` or
`authHeadersProvider?.call()` can escape as unhandled Future errors. Move the
body/header preparation into the same protected section as the request, or wrap
the entire `send()` method in try/catch, so any encoding or auth-header failure
is logged and treated like a dropped telemetry attempt rather than breaking
`ClientEventReporter._dispatch`.
In `@packages/stream_video/lib/src/telemetry/media_frame_reporter.dart`:
- Around line 60-82: The duplicate-setup guard in media_frame_reporter.dart’s
onRemoteVideoTrack is only checked before awaiting initialize(), so a concurrent
second call for the same trackId can still overwrite _renderers and leak the
first renderer. After await renderer.initialize(), re-check _videoReported,
_disposed, and whether _renderers already contains trackId before assigning to
_renderers; if another renderer is already present, dispose the newly created
RTCVideoRenderer and return. Keep the fix localized to onRemoteVideoTrack and
preserve the existing cleanup path.
- Around line 50-56: The FirstAudioFrame check in onSubscriberStats is using
any(RtcInboundRtpAudioStream with packetsReceived > 0), which can fire for the
wrong remote audio stream when multiple tracks exist. Update the
MediaFrameReporter logic to correlate on the WebRTC receiver id from
RtcInboundRtpAudioStream.trackId instead of relying on _audioTrackId alone, and
thread that receiver id through the matching path so _reportAudio() only runs
for the intended stream.
---
Outside diff comments:
In `@packages/stream_video/lib/src/call/call.dart`:
- Around line 2271-2288: `Call.end()` is bypassing the `ClientEventReporter`
cleanup that `leave()` now performs. Move the `clientEventReporter.abort(...)`
and `clientEventReporter.unregisterCall(...)` calls into the shared
`_disconnect(...)` path in `Call`, so both `leave()` and `end()` always clean up
reporter state. Keep the abort-code selection tied to the existing
`leave({reason})` logic, and ensure `_onStateChanged`/`CallStatusDisconnected`
does not rely on `_clear(...)` alone for reporter cleanup.
---
Nitpick comments:
In `@packages/stream_video/lib/src/call/session/call_session.dart`:
- Around line 452-569: `fastReconnect()` is missing `wsJoin` stage telemetry for
the SFU WS recreate/join flow, unlike `start()`. Update
`CallSession.fastReconnect` to wrap the `sfuWS.recreate()`, join request send,
and `SfuJoinResponseEvent` wait with the same
`beginStage`/`completeStage`/`failStage` reporting used in `start()` for
`ClientEventStage.wsJoin`, including the explicit failure return path when
`wsResult.isFailure` is true.
In `@packages/stream_video/test/src/telemetry/media_frame_reporter_test.dart`:
- Around line 1-91: Add test coverage for the video first-frame flow in
MediaFrameReporter, since this file only verifies the audio path through
onRemoteAudioTrack and onSubscriberStats. Create tests that exercise
onRemoteVideoTrack and the _reportVideo behavior, asserting that
onFirstFrameRendered emits the expected first-frame event once, that it does not
fire before a remote video track is registered, and that the renderer is
disposed after the event. Use the existing
ClientEventReporter/MediaFrameReporter setup and mirror the single-shot
assertions already used for FirstAudioFrame.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c03b46e1-51ec-49ab-9be2-dad45c846a8c
📒 Files selected for processing (20)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/call.dartpackages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/coordinator/open_api/coordinator_client_open_api.dartpackages/stream_video/lib/src/coordinator/open_api/coordinator_ws.dartpackages/stream_video/lib/src/stream_video.dartpackages/stream_video/lib/src/telemetry/client_event.dartpackages/stream_video/lib/src/telemetry/client_event_error_mapper.dartpackages/stream_video/lib/src/telemetry/client_event_reporter.dartpackages/stream_video/lib/src/telemetry/client_event_transport.dartpackages/stream_video/lib/src/telemetry/client_event_types.dartpackages/stream_video/lib/src/telemetry/media_frame_reporter.dartpackages/stream_video/lib/src/telemetry/peer_connection_connect_reporter.dartpackages/stream_video/lib/src/webrtc/peer_connection.dartpackages/stream_video/lib/src/webrtc/rtc_manager.dartpackages/stream_video/lib/src/webrtc/rtc_manager_factory.dartpackages/stream_video/test/src/telemetry/client_event_reporter_test.dartpackages/stream_video/test/src/telemetry/media_frame_reporter_test.dartpackages/stream_video/test/src/telemetry/peer_connection_connect_reporter_test.dartpackages/stream_video/test/test_helpers.dart
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
1 participant
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.
resolving: FLU-504
This PR adds client-side call event reporting to the core client, a telemetry layer that tracks the full call-join lifecycle and reports each stage (initiated → completed with success/failure) to the backend. It gives us end-to-end visibility into where and why joins fail, correlated across the coordinator connection, the join attempt, and each peer connection.
Each stage emits an initiated event and, once resolved, a paired completed event (outcome: success|failure); the two share a stage_id.
call.join()is called (single-shot)Call.join()coordinator_connect_id, nojoin_attempt_id)CoordinatorWebSocketconnection statejoinCallrequestCall._performJoinCallRequestCallSession.startICE_CONNECTIVITY_FAILED, PC→DTLS_CONNECTIVITY_FAILED; trackswas_previously_connected+ice_state)RtcManager/TracedStreamPeerConnectionRTCVideoRenderer.onFirstFrameRendered)RtcManagerpacketsReceived > 0, fed from the existing periodic stats loop)RtcManagerNot included / follow-ups
Summary by CodeRabbit
New Features
Bug Fixes