-
Notifications
You must be signed in to change notification settings - Fork 33
fix: isolate mediaDevices traces #1779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request aims to isolate mediaDevices tracing to individual call instances rather than using a single shared Tracer. Key changes include:
- Removing the global patching of browser mediaDevices APIs.
- Propagating a Tracer instance through various modules (stats, devices, and call) to enable scoped tracing.
- Updating trace handling in the SfuStatsReporter and device management modules.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/client/src/stats/utils.ts | Added a helper function dumpStream for serializing MediaStream data. |
packages/client/src/stats/rtc/mediaDevices.ts | Removed global patching of mediaDevices APIs. |
packages/client/src/stats/SfuStatsReporter.ts | Replaced mediaStatsTracer with call-scoped tracers and updated snapshot and rollback logic. |
packages/client/src/rtc/BasePeerConnection.ts | Modified tracing tag composition and trace payload to include SFU edge information. |
packages/client/src/devices/devices.ts | Updated media stream acquisition functions to accept a tracer parameter and log success/failure traces. |
packages/client/src/devices/tests/* | Updated tests to expect tracer parameters and error message adjustments. |
packages/client/src/devices/SpeakerState.ts | Changed tracer reference from a legacy import to instance injection. |
packages/client/src/devices/SpeakerManager.ts | Instantiated a Tracer and passed it into SpeakerState. |
packages/client/src/devices/ScreenShareManager.ts | Updated screen share stream requests to include the tracer parameter and adjusted error messages. |
packages/client/src/devices/MicrophoneManager.ts | Propagated the tracer to the getAudioStream call. |
packages/client/src/devices/InputMediaDeviceManager.ts | Injected a new tracer instance and adjusted the constructor signature. |
packages/client/src/devices/CameraManager.ts | Updated getVideoStream to pass the tracer instance through. |
packages/client/src/Call.ts | Aggregated multiple tracer instances and updated tracing enabling logic. |
packages/client/index.ts | Removed the side-effect global mediaDevices patching import. |
Comments suppressed due to low confidence (1)
packages/client/src/devices/InputMediaDeviceManager.ts:34
- [nitpick] The inline initialization of 'tracer' with 'null' may be confusing; consider clarifying its intended usage or ensuring it is properly overridden via constructor injection when a valid tracer is available.
tracer = new Tracer(null);
💡 Overview
Continuation of #1765.
mediaDevice
traces were tracked in a single sharedTracer
instance.While this is ok for the majority of the integrations, this can be problematic for customers allowing one to be connected to two separate
call
instances from a single browser tab.This PR scopes the traces to the
call
instance.📝 Implementation notes
tracer
instance in the lowest level APIsgetUserMedia
invocations triggered outside of our SDK anymorecreate
trace - we include the SFU name in it now🎫 Ticket: https://linear.app/stream/issue/REACT-365/rtc-tracing-and-performance-stats-tracking