Skip to content
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

Add WebRTC media stats collector #3205

Merged
merged 21 commits into from
Mar 20, 2023

Conversation

EnricoSchw
Copy link
Contributor

@EnricoSchw EnricoSchw commented Mar 9, 2023

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@EnricoSchw EnricoSchw requested a review from a team as a code owner March 9, 2023 14:44
@EnricoSchw EnricoSchw self-assigned this Mar 9, 2023
@EnricoSchw EnricoSchw requested a review from a team as a code owner March 9, 2023 14:44
@EnricoSchw EnricoSchw marked this pull request as draft March 9, 2023 14:44
@EnricoSchw EnricoSchw changed the base branch from develop to matthew/sfu March 9, 2023 14:45
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing the review request as this is still a draft.

@EnricoSchw EnricoSchw removed request for a team and kerryarchibald March 10, 2023 09:25
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments but this looks like a good approach. Thanks!

src/webrtc/stats/groupCallStats.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/stats/groupCallStats.spec.ts Outdated Show resolved Hide resolved
spec/unit/webrtc/stats/groupCallStats.spec.ts Outdated Show resolved Hide resolved
src/webrtc/stats/connectionStats.ts Outdated Show resolved Hide resolved
src/webrtc/stats/groupCallStats.ts Outdated Show resolved Hide resolved
src/webrtc/stats/statsCollector.ts Show resolved Hide resolved
src/webrtc/stats/statsCollector.ts Outdated Show resolved Hide resolved
src/webrtc/stats/transportStats.ts Outdated Show resolved Hide resolved
@EnricoSchw EnricoSchw added the T-Task Tasks for the team like planning label Mar 14, 2023
spec/unit/webrtc/stats/groupCallStats.spec.ts Outdated Show resolved Hide resolved
src/webrtc/stats/media/mediaTrackStats.ts Outdated Show resolved Hide resolved
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good. Do we want to keep the stats logging to the console for now? If so, I'd change that to use the logger wrapper. Otherwise looks great!

@EnricoSchw
Copy link
Contributor Author

EnricoSchw commented Mar 17, 2023

Thanks, looks good. Do we want to keep the stats logging to the console for now? If so, I'd change that to use the logger wrapper. Otherwise looks great!

@dbkr
The console log is only for debugging and to be able to see the structure of the metrics. I turned off the metric logs for now. I'm currently thinking about two options:

  1. Broadcast metrics data events from SDK to ElementCall like you did it with device msg Events. So in EC we can decide how to display metrics (OTEL, Console, Posthog, Html-View )
  • You would prefer this option?
  1. Make it configurable via MatrxClient, how to display or where we send the metrics
  • You would prefer this option?

(I prefer the first option, but with the option to disable the metrics as well)

The next steps for the metrics are:
-- Augment logs with feed data
-- Include pkg send/receive metric data for feed -tracks.
-- Add transceiver data in feed metrics

@dbkr
Copy link
Member

dbkr commented Mar 17, 2023

I vote option 1 personally. I think the opentelemetry stuff will probably move in to the js-sdk in the long run, but we can get it out into EC with an event in the same way, that sounds ideal for now.

@EnricoSchw EnricoSchw merged commit 363b910 into matthew/sfu Mar 20, 2023
@EnricoSchw EnricoSchw deleted the enricoschw/real-time-media-statistics branch March 20, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants