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

Expose WebRTC peerconn stats #983

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

marcovidonis
Copy link
Collaborator

I revisited the work on #803 and improved on it. By saving a pointer to the open peer connections, we get stats more reliably. Stats now also work when seeding.

A note on the Pion/webrtc version. While some stats are available in the current latest version (v3), the project greatly improved on them in v4, currently in beta. I'd recommend upgrading to v4 as soon as it's convenient when it gets out of beta.

@anacrolix
Copy link
Owner

I'm happy to delegate this to you entirely. I don't imagine it's easy to add some tests here? It's probably too much work to spin up a fake websocket server, but that's about my only suggestion.

Let me know when you're happy with the state of the PR and I'll merge it (including now if you think it's ready now).

I also don't really know much and am not particularly familiar with WebRTC, so if you are interested in a fork or long-lived branch to test using v4 of pion, I'm open to that.

Thanks!

@marcovidonis
Copy link
Collaborator Author

That's great, thanks! Yeah I don't really see how to make unit tests for this... we'd likely need an integration test of some kind.

I think the PR is almost ready now, as it does expose the stats we get from Pion. I noticed the wasm test fails because Pion doesn't yet support stats on wasm. Do you know of a way I could avoid calling GetStats() if we're on a wasm architecture?

@anacrolix
Copy link
Owner

I'd probably put the GetStats behind a build tag that matches whatever pion uses. For the unsupported tags, return something appropriate.

@marcovidonis
Copy link
Collaborator Author

Thanks for that, @anacrolix! All checks pass now, so I think you can merge if you're happy with this.

Also, I think a long-lived branch for Pion v4 could be useful, so I'd be happy to make one!

@anacrolix anacrolix merged commit d112dd8 into anacrolix:master Oct 3, 2024
10 checks passed
@anacrolix
Copy link
Owner

Go for it, thanks!

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

Successfully merging this pull request may close these issues.

2 participants