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

Stats: improve JSON support, add missing structs #2513

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

aalekseevx
Copy link
Member

Hi there! We are delighted to have the opportunity to utilize pion's webrtc-stats implementation for unmarshalling JSON stats at our backend. I have put in some effort to make it feasible and would love to contribute it.

Description:

@AbsoluteNikola
Copy link

Amazing feature. We really need this

@Sean-Der
Copy link
Member

This looks great @aalekseevx lets get it merged!

I will review tonight. If you don’t hear from me please ping here and on https://pion.ly/slack

Lots of stuff going on sometimes get pulled away :/

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage: 62.08% and project coverage change: -0.42% ⚠️

Comparison is base (6453346) 77.63% compared to head (ec5f6f2) 77.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2513      +/-   ##
==========================================
- Coverage   77.63%   77.22%   -0.42%     
==========================================
  Files          87       87              
  Lines        9378     9674     +296     
==========================================
+ Hits         7281     7471     +190     
- Misses       1666     1745      +79     
- Partials      431      458      +27     
Flag Coverage Δ
go 78.88% <62.08%> (-0.52%) ⬇️
wasm 64.51% <0.00%> (-5.72%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
icegatherer.go 81.13% <ø> (-0.14%) ⬇️
stats.go 64.35% <59.20%> (-35.65%) ⬇️
datachannelstate.go 100.00% <100.00%> (ø)
dtlstransportstate.go 100.00% <100.00%> (ø)
icecandidatetype.go 81.81% <100.00%> (+2.87%) ⬆️
icerole.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aalekseevx
Copy link
Member Author

@Sean-Der, hi! I made some fixes for CI checks, do you have some time to complete the review now?

@aalekseevx
Copy link
Member Author

Any updates?

@Sean-Der
Copy link
Member

Sean-Der commented Aug 5, 2023

Fantastic work @aalekseevx! sorry I didn't get a chance to review sooner

  • Can we drop the dependency? Outside of tests we have 0 3rd party dependencies, would be nice to keep that :)

  • Can you move the testdata into the files that use them? I don't feel strongly either way, I just would like to follow the established pattern.

Thank you so much for doing this work. It is a very important contribution. Sorry I did't respond sooner, personal life has just kept me busy. thank you!

- Fix json marshalling of stats containing enums
- Add UnmarshalStatsJSON helper
- Add marshalling/unmarshalling tests
- Add missing AudioSourceStats, VideoSourceStats AudioPlayoutStats
  defined in https://www.w3.org/TR/webrtc-stats
- Deprecate ICECandidateStats' NetworkType, use plain string
  instead of enum which does not suite the definition:
  https://clck.ru/354H9r
@aalekseevx
Copy link
Member Author

aalekseevx commented Aug 8, 2023

@Sean-Der, thanks a lot for the review! I'm glad you liked this piece of work. I have updated the PR to meet the requirements. Is it possible to make a release as soon as it is merged?

@aalekseevx
Copy link
Member Author

@Sean-Der, are there any blockers to merge left? Is golangci-lint timeout a problem?

@Sean-Der
Copy link
Member

@aalekseevx nope you did a great job! I will merge and tag today.

thank you for pinging me. This is super important contribution. Sorry I have been distracted with OBS dev.

@Sean-Der Sean-Der merged commit 2926b99 into pion:master Aug 17, 2023
17 of 18 checks passed
@Sean-Der
Copy link
Member

Merged and tagged with v3.2.15 thank you @aalekseevx !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants