chore(llc): improved sfu stats #1122
Conversation
WalkthroughIntroduces zoned and base tracing across Call and Session paths (CallSession.getTrace() now returns multiple TraceSlice), appends media-device traces to SFU stats, renames trace/event keys to camelCase, updates many SFU toJson() serializations to camelCase/value enums, adds lifecycle guards, and exposes RtcMediaDevice.toJson(). Changes
Sequence Diagram(s)sequenceDiagram
participant Call as Call / CallSession
participant Tracer as Tracers (_tracer, _zonedTracer)
participant Zone as TracerZone
participant Stats as SfuStatsReporter
participant Media as RtcMediaDeviceNotifier
participant SFU as SFU/Backend
Call->>Tracer: getTrace() -> List<TraceSlice> ([_tracer.take(), _zonedTracer.take()])
Zone->>Call: run zoned ops (setCamera/setMicrophone) using _zonedTracer
Call->>Tracer: zoned tracer logs events
Stats->>Call: request session traces
Call-->>Stats: returns expanded traces
Stats->>Media: getTrace()
Media-->>Stats: media-device TraceSlice
Stats->>Stats: assemble traces (...sessionTrace, mediaDeviceTrace)
Stats->>SFU: send SFU stats payload (includes traces)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1122 +/- ##
=====================================
Coverage 6.29% 6.29%
=====================================
Files 593 593
Lines 40849 40887 +38
=====================================
+ Hits 2570 2574 +4
- Misses 38279 38313 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/stream_video/lib/src/call/call.dart (1)
1489-1552: Verify trace payload forstrategyand downstream expectationsThe reconnect traces now emit:
'callReconnect','callReconnectSuccess','callReconnectFailed'(camelCase)- payload
{ 'strategy': strategy, ... }wherestrategyisSfuReconnectionStrategy.If
TraceRecord→ JSON serialization does not special-case this enum/Protobuf type,jsonEncodeof SFU stats may fail. Also, changing event names is a telemetry contract change.Consider reverting payload to a plain string if needed:
- _session?.trace('callReconnect', { - 'strategy': strategy, - }); + _session?.trace('callReconnect', { + 'strategy': strategy.name, + }); ... - _session?.trace('callReconnectSuccess', { - 'strategy': strategy, - }); + _session?.trace('callReconnectSuccess', { + 'strategy': strategy.name, + }); ... - _session?.trace('callReconnectFailed', { - 'strategy': strategy, - 'error': error.toString(), - }); + _session?.trace('callReconnectFailed', { + 'strategy': strategy.name, + 'error': error.toString(), + });And please double‑check that backend consumers are ready for the new camelCase event names before deploying.
packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart (1)
2-3: RtcMediaDeviceNotifier tracing is well-scoped and non-intrusive
_tracer+getTrace()provide a clean hook for SFU stats to pull device‑level traces.enumerateDevices()logs a JSON‑friendly view of devices.pauseAudioPlayout,resumeAudioPlayout, andregainAndroidAudioFocusemit concise trace events without altering behavior.This is a good level of observability for debugging audio/device issues.
If you later need to reduce noise, you could gate
_tracer.trace(...)behind a simple “device tracing enabled” flag, similar to the SFU stats flag.Also applies to: 7-8, 45-50, 155-158, 197-216
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/stream_video/CHANGELOG.md(1 hunks)packages/stream_video/lib/src/call/call.dart(4 hunks)packages/stream_video/lib/src/call/session/call_session.dart(11 hunks)packages/stream_video/lib/src/call/session/call_session_factory.dart(1 hunks)packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart(1 hunks)packages/stream_video/lib/src/call/stats/stats_reporter.dart(2 hunks)packages/stream_video/lib/src/sfu/sfu_extensions.dart(19 hunks)packages/stream_video/lib/src/webrtc/rtc_manager.dart(2 hunks)packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device.dart(1 hunks)packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart(5 hunks)packages/stream_video/test/src/call/fixtures/call_test_helpers.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: analyze
🔇 Additional comments (26)
packages/stream_video/lib/src/sfu/sfu_extensions.dart (11)
14-31: LGTM!New imports and camelCase key naming in
CodecX.toJson()are consistent with the PR objectives for standardizing JSON serialization.
85-98: LGTM!camelCase keys and proper enum
.valueaccess forqualityare consistent with the standardization effort.
110-119: LGTM!Conditional field emission using
has*()checks is a clean approach that avoids emitting unset protobuf fields.
121-130: LGTM!Proper enum
.valueaccess for SDK type.
160-187: LGTM!Consistent camelCase keys and enum
.valueaccess across these request extensions.
376-385: LGTM!The
reconnectStrategy.toDto().valuepattern is appropriate when the domain model needs conversion to a DTO before accessing the protobuf value.
406-414: Verify intentional use of.indexforconnectionQuality.
connectionQuality.indexreturns the enum's ordinal position (0, 1, 2...) rather than its semantic value. Other enums in this file use.valuewhich typically returns the protobuf field value. Confirm this is the intended serialization format.
387-393: Same concern asgoAwayReason: potential.toString()vs.valueinconsistency.Verify whether
callEndedReasonshould use.valuefor consistency with other enums.
416-458: LGTM!Both track event extensions follow consistent patterns with
toDTO().valuefor trackType and properly structured nested participant objects.
484-504: LGTM!SDP fields are properly emitted directly, and the camelCase key naming is consistent throughout the join request serialization.
360-366: Remove this comment — the code is correct.
SfuGoAwayReasonis a plain Dart enum without a.valueproperty. The customtoString()override returns the enum name directly, which is the correct serialization approach for this enum type. The file's mixed use of.valueand.toString()reflects different enum sources: protobuf-generated enums use.value, while plain Dart enums use.toString().packages/stream_video/CHANGELOG.md (1)
1-4: Changelog entry matches implementation scopeThe “Improved SFU stats implementation.” note is accurate and scoped correctly to the telemetry/tracing changes in this PR.
packages/stream_video/lib/src/call/stats/stats_reporter.dart (1)
46-55: Mounted guards in stats pipeline look correctEarly returns on
!mountedinrun()and_processStats()cleanly avoid post‑dispose work while still yielding the latest stats snapshot.Also applies to: 97-99
packages/stream_video/test/src/call/fixtures/call_test_helpers.dart (1)
303-312: MockedgetTracecorrectly updated toList<TraceSlice>The mock now matches the new
CallSession.getTrace()contract by returning a single emptyTraceSlicein a list, which is sufficient for exercising callers that flatten snapshots.packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device.dart (1)
47-54:RtcMediaDevice.toJsonis minimal and sufficient for telemetryThe JSON shape (
id,label,groupId,kindas alias) is consistent and safe to feed into tracing/SFU stats.packages/stream_video/lib/src/call/session/call_session_factory.dart (1)
75-85: Tracer naming and leftover record IDs look consistentIncluding
sessionSeqandsfuNamein both tracer name and leftover record IDs will make multi‑SFU/session traces easier to correlate without changing runtime behavior.packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart (1)
186-199: Media-device notifier trace integration into SFU stats looks correctAggregating:
- subscriber/publisher tracer slices
- all
callSession.getTrace()slices (...sessionTrace)- the
RtcMediaDeviceNotifiertrace sliceinto a single
traceslist, then expanding snapshots and rolling back on failure, fits the existing trace model without changing semantics.packages/stream_video/lib/src/call/session/call_session.dart (9)
48-48: Good addition of lifecycle guard.The
_isLeavingOrClosedflag helps prevent operations during shutdown states, which is a solid defensive programming practice.
108-108: LGTM - dual tracing facility added.The addition of
_zonedTracersupports the PR's goal of introducing dual tracing mechanisms for better SFU stats instrumentation.
141-144: LGTM - both tracers controlled consistently.Correctly enables/disables both tracing facilities together.
595-632: LGTM - consistent lifecycle state management.The
_isLeavingOrClosedflag is properly set in all shutdown paths (leave,close, anddispose), ensuring consistent state tracking during session teardown.
697-699: LGTM - enhanced observability.Adding explicit tracing for connection quality changes improves debugging and monitoring capabilities.
920-924: LGTM - appropriate guard against renegotiation during shutdown.The guard correctly prevents renegotiation when the session is leaving/closed or disconnected. Note that the log message "call is disconnected" doesn't fully reflect the
_isLeavingOrClosedcase, but this is a minor clarity issue.
1043-1064: LGTM - enhanced tracing for camera operations.Wrapping camera enable/disable in a
TracerZoneprovides better instrumentation for debugging media device issues, aligning with the PR's goal of improved SFU stats.
1066-1087: LGTM - consistent tracing for microphone operations.Microphone operations now have the same detailed tracing as camera operations, providing comprehensive media device instrumentation.
137-139: All callers ofCallSession.getTrace()have been properly updated to handle the newList<TraceSlice>return type:
sfu_stats_reporter.dartuses the spread operator (...sessionTrace) to add items to the traces listcall.dartcorrectly processes the list with.expand((slice) => slice.snapshot).toList()- Test fixtures in
call_test_helpers.darthave been updated to mock the list return typeNo breaking usages remain in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
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/session/call_session.dart (1)
619-622: Missing_zonedTracer.dispose()call.The base
_traceris disposed at line 621, but_zonedTraceris not disposed. This could lead to a resource leak ifTracer.dispose()releases resources.Apply this diff:
statsReporter?.dispose(); statsReporter = null; await rtcManager?.dispose(); rtcManager = null; _tracer.dispose(); + _zonedTracer.dispose(); _peerConnectionCheckTimer?.cancel();
♻️ Duplicate comments (1)
packages/stream_video/lib/src/call/session/call_session.dart (1)
465-470: Past issue resolved and timeout tracing added.The duration formatting fix (
joinResponseTimeout.inMilliseconds) correctly addresses the previous review comment. The addedjoinRequestTimeouttrace improves visibility into connection failures.
🧹 Nitpick comments (3)
packages/stream_video/lib/src/sfu/sfu_extensions.dart (3)
395-404: Rename loop variable inconnectionQualityUpdatesmap for clarity.Here the lambda uses
codecas the variable name, but the list isconnectionQualityUpdates, which likely containsSfuConnectionQualityInfoitems, not codecs. Renaming improves readability and reduces confusion:- 'connectionQualityUpdates': connectionQualityUpdates - .map((codec) => codec.toJson()) - .toList(), + 'connectionQualityUpdates': connectionQualityUpdates + .map((update) => update.toJson()) + .toList(),
360-366: Align remaining enumtoString()usages with the new value‑based convention (if desired).Most SFU enums are now serialized via
.value(ortoDto().value), but these still use.toString():
- Line 363:
goAwayReason.toString()- Line 380:
code.toString()- Line 390:
callEndedReason.toString()- Lines 500–502:
source.toString()andcapabilities.map((c) => c.toString())If the goal is a consistent, stable JSON schema (and to avoid including Dart enum type prefixes in the payload), you may want to switch these to the same value representation you use elsewhere (or introduce a dedicated DTO/value mapping). If they are intentionally using full
toString()for backwards compatibility, consider adding a short comment to document that.Also applies to: 376-385, 387-392, 484-503
416-458: Consider normalizingpublishedTracksserialization away fromtoString().Inside the
participantpayload forSfuTrackPublishedEventJsonX/SfuTrackUnpublishedEventJsonX:'publishedTracks': participant.publishedTracks .map((track) => track.toString()) .toList(),Other track‑type fields in this file use
.valueortoDTO().value. IfpublishedTrackscontains the same enum type (or something similar), using.toString()may produce verbose implementation‑specific strings instead of the stable values the rest of the API emits.If there’s a corresponding
.value/DTO or other stable identifier for these track entries, consider switching to that for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/stream_video/lib/src/call/session/call_session.dart(11 hunks)packages/stream_video/lib/src/sfu/sfu_extensions.dart(19 hunks)packages/stream_video/lib/src/webrtc/rtc_manager.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/lib/src/webrtc/rtc_manager.dart
🧰 Additional context used
🪛 GitHub Actions: legacy_version_analyze
packages/stream_video/lib/src/sfu/sfu_extensions.dart
[warning] 14-14: Unused import: '../utils/extensions.dart'. Try removing the import directive.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build
- GitHub Check: analyze
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video
- GitHub Check: iOS Build (Dogfooding)
🔇 Additional comments (6)
packages/stream_video/lib/src/call/session/call_session.dart (5)
107-107: LGTM on the new tracing and lifecycle fields.The dual tracer pattern (
_tracerfor base events,_zonedTracerfor zoned UI operations) and the_isLeavingOrClosedlifecycle guard are well-designed additions that improve tracing granularity and prevent race conditions during shutdown.Also applies to: 125-125
137-144: LGTM on trace management updates.The
getTrace()now correctly aggregates traces from both tracers, andsetTraceEnabledproperly synchronizes the enabled state across both.
697-699: LGTM on ConnectionQualityChanged trace.Adding this trace maintains consistency with other SFU event tracing and improves visibility into connection quality changes.
920-924: LGTM on the renegotiation guard enhancement.Checking
_isLeavingOrClosedfirst provides a fast path to prevent renegotiation attempts during session teardown, avoiding potential race conditions with the async shutdown sequence.
1052-1061: LGTM on zoned tracer integration for media device operations.Using
_zonedTracerforsetCameraEnabledandsetMicrophoneEnabledappropriately separates user-initiated media device traces from base SFU event traces, aligning with the dual-tracing architecture introduced in this PR.Also applies to: 1075-1084
packages/stream_video/lib/src/sfu/sfu_extensions.dart (1)
406-413: Double‑checkconnectionQualityserialization via enumindex.
connectionQualityis exported asconnectionQuality.index, which ties the wire format to the enum’s declaration order. If the backend or analytics expect a named value (string) or a specific numeric mapping, this could be fragile.Confirm that
indexis the agreed wire representation; if not, consider a stable value (e.g.,.valueor an explicit mapping).
There was a problem hiding this comment.
Actionable comments posted: 1
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/sfu/sfu_extensions.dart (1)
359-392: Use.valuefor protobuf enumsgoAwayReasonandcallEndedReason.
GoAwayReasonandCallEndedReasonextend$pb.ProtobufEnumand should use.valuefor consistency with other protobuf enums throughout this file (e.g.,trackType.value,peerType.value). Note:codeis a Dart enum (SfuErrorCode), so.toString()is appropriate and should not be changed.extension SfuGoAwayEventJsonX on SfuGoAwayEvent { Map<String, dynamic> toJson() { return { - 'goAwayReason': goAwayReason.toString(), + 'goAwayReason': goAwayReason.value, }; } }extension SfuCallEndedEventJsonX on SfuCallEndedEvent { Map<String, dynamic> toJson() { return { - 'callEndedReason': callEndedReason.toString(), + 'callEndedReason': callEndedReason.value, }; } }
♻️ Duplicate comments (1)
packages/stream_video/lib/src/call/call.dart (1)
969-975: Fix null-safety on_previousSessionwhen flattening trace slices
_previousSession?.getTrace().expand(...)is not null-safe: the result of_previousSession?.getTrace()is nullable, so calling.expandon it fails static analysis and would throw if_previousSessionis null. This matches the earlier review concern and will break first join/reconnect flows where_previousSessionis legitimately null.Apply this diff to make the access null-safe and keep types aligned:
- leftoverTraceRecords: - _previousSession - ?.getTrace() - .expand((slice) => slice.snapshot) - .toList() ?? - const [], + leftoverTraceRecords: + _previousSession == null + ? const [] + : _previousSession! + .getTrace() + .expand((slice) => slice.snapshot) + .toList(),
🧹 Nitpick comments (1)
packages/stream_video/lib/src/call/call.dart (1)
1489-1491: Consider logging the effective reconnect strategy instead of the initial oneThe new traces use the method argument
strategy.name:
callReconnectcallReconnectSuccesscallReconnectFailedHowever,
_reconnectStrategycan change inside the loop (e.g. fromfasttorejoinafter several failed attempts). If you want traces to reflect the actual strategy used for that attempt, consider switching these to_reconnectStrategy.name.For example:
- _session?.trace('callReconnect', { - 'strategy': strategy.name, - }); + _session?.trace('callReconnect', { + 'strategy': _reconnectStrategy.name, + }); ... - _session?.trace('callReconnectSuccess', { - 'strategy': strategy.name, - }); + _session?.trace('callReconnectSuccess', { + 'strategy': _reconnectStrategy.name, + }); ... - _session?.trace('callReconnectFailed', { - 'strategy': strategy.name, + _session?.trace('callReconnectFailed', { + 'strategy': _reconnectStrategy.name,Also applies to: 1539-1541, 1549-1552
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/stream_video/CHANGELOG.md(1 hunks)packages/stream_video/lib/src/call/call.dart(4 hunks)packages/stream_video/lib/src/sfu/sfu_extensions.dart(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: analyze
- GitHub Check: build
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video
- GitHub Check: stream_video_flutter
🔇 Additional comments (8)
packages/stream_video/CHANGELOG.md (1)
7-7: LGTM!The changelog entry accurately reflects the PR's improvements to SFU stats. The placement under "🐞 Fixed" is consistent with the codebase's style for enhancements, and the wording is appropriately concise for a user-facing changelog.
packages/stream_video/lib/src/sfu/sfu_extensions.dart (7)
17-31: LGTM!New imports support the added extensions, and
CodecX.toJson()correctly uses camelCase keys.
67-97: LGTM!The previous inconsistency with
trackType.toString()has been addressed. BothtrackType.valueandquality.valuenow follow the consistent.valuepattern for enum serialization.
109-129: LGTM!Good use of
hasSdk(),hasOs(), etc., guards for conditional field emission inClientDetailsX, and consistent.valueenum serialization throughout.
188-265: LGTM!Consistent camelCase key naming and
.valueenum serialization acrossUpdateMuteStatesRequestX,TrackMuteStateX,UpdateSubscriptionsRequestX,TrackSubscriptionDetailsX, andReconnectDetailsX.
287-348: LGTM!The
trackType.toDTO().valuepattern correctly converts domain model enums to DTOs before accessing.value, maintaining consistency with protobuf serialization.
415-457: LGTM!Both
SfuTrackPublishedEventJsonXandSfuTrackUnpublishedEventJsonXfollow consistent patterns with camelCase keys andtrackType.toDTO().valuefor enum serialization.
459-504: LGTM with minor note onsource.toString()andcapabilitiesserialization.
PublishOptionX,SubscribeOptionX, andJoinRequestXcorrectly use.valuefortrackType. Thesource.toString()andcapabilities.map((c) => c.toString())at lines 499 and 501 follow the same pattern as other non-protobuf enums flagged earlier.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart (1)
198-198: Consider tracing after operation completion for consistency.The
pauseAudioPlayout(),resumeAudioPlayout(), andregainAndroidAudioFocus()methods trace before the asynchronous operations complete. This differs fromenumerateDevices()which traces after success.While tracing invocations is useful, consider whether you want to:
- Trace after the Future completes (to confirm success)
- Trace on both invocation and completion
- Keep the current pattern if invocation tracking is sufficient
The current approach means traces will be recorded even if the underlying operations fail.
Also applies to: 205-205, 215-215
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: analyze_legacy_version
- GitHub Check: iOS Build (Dogfooding)
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video
- GitHub Check: analyze
- GitHub Check: build
🔇 Additional comments (3)
packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart (3)
2-3: LGTM!The import additions support the new tracing infrastructure. The
metapackage provides the@internalannotation, and the tracer import enables tracing functionality.Also applies to: 8-8
156-159: RtcMediaDevice.toJson() is properly implemented.The
toJson()method exists on RtcMediaDevice (line 47 of rtc_media_device.dart) and returnsMap<String, dynamic>. The tracing code correctly serializes devices using this method.
46-51: Tracer initialization withTracer(null)is valid and intentional.The Tracer constructor accepts a nullable String parameter (
Tracer(this.id)whereidisString?). UsingTracer(null)is the correct pattern for singleton components like RtcMediaDeviceNotifier that aren't tied to specific call sessions. This same pattern is already used elsewhere in the codebase (e.g.,CallSession._zonedTracer).
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.