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 ICE candidate errors sending to a server #151

Merged
merged 19 commits into from
Feb 19, 2024

Conversation

evdokimovs
Copy link
Contributor

@evdokimovs evdokimovs commented Feb 14, 2024

Synopsis

Sometimes ICE candidate error can happen on PeerConnection, but it will be just client side error and we will not know that this is happened, and what exactly happened. This can lead to the silent bugs (bugs we don't know about) or hardly fixable bugs (because we're not having any info about them). So we need to somehow monitor this kind of errors (and similar errors in the future).

Solution

Firstly, we need to introduce new PeerMetrics which will be used for monitoring errors which can happen on a PeerConnection. It will be called PeerMetrics::PeerConnectionError.

Then we need to listen for onicecandidateerror events on client and send this errors to the server in this PeerMetrics. At this moment, server will just print them in logs, but in the future some additional mechanisms can be added (like displaying count of them in metrics, or using some backup methods to interconnect users somehow differently).

When this PR was started, web-sys had no support for RTCPeerConnectionIceEvent type, so it was implemented, but currently it's not released feature, so it's required at this moment to use git version of wasm-bindgen and related crates. wasm-bindgen releases every month at 6th, so most likely we can switch to the crates.io version after 06.03.2024.

Checklist

  • Created PR:
    • In draft mode
    • Name contains issue reference
    • Has type and k:: labels applied
    • Has assignee
  • Before review:
    • Documentation is updated (if required)
    • Tests are updated (if required)
    • Changes conform code style
    • CHANGELOG entry is added (if required)
    • FCM (final commit message) is posted or updated
    • Draft mode is removed
  • Review is completed and changes are approved
    • FCM (final commit message) is approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • All temporary labels are removed

@evdokimovs evdokimovs added enhancement Improvement of existing features or bugfix k::api Related to API (application interface) k::refactor Refactoring, technical debt elimination and other improvements of existing code base k::monitoring Related to monitoring capabilities labels Feb 14, 2024
@evdokimovs evdokimovs self-assigned this Feb 14, 2024
@evdokimovs
Copy link
Contributor Author

evdokimovs commented Feb 15, 2024

FCM

Add ICE candidate errors sending to server (#151)

Additionally:
- temporary patch `wasm-bindgen` crates to use Git revision
- temporary pin `ffigen` Dart package to `9.0.0` version

@evdokimovs evdokimovs marked this pull request as ready for review February 15, 2024 14:52
@evdokimovs evdokimovs requested a review from tyranron February 15, 2024 14:52
Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

After #152 I was unable to run make cargo.gen.bridge due to Dart dependencies errors:

2024/02/16 18:42:04 [INFO] Phase: Generate Dart code
2024/02/16 18:42:04 [ERROR] Fatal error encountered. Rerun with RUST_BACKTRACE=1 or RUST_BACKTRACE=full for more details.
Error: External command failure.
Please update version of ffigen in your dev_dependencies. (version >=8.0.0, <10.0.0)

Caused by:
    Please update version of ffigen in your dev_dependencies. (version >=8.0.0, <10.0.0)
make: *** [cargo.gen.bridge] Error 1

@evdokimovs please, resolve and regenerate the files properly.

@evdokimovs
Copy link
Contributor Author

@tyranron ,

Temporary pinned version of ffigen to 9.0.0 (supported both by flutter_gherkin and flutter_rust_bridge) until FRB updates. PR in FRB for this is already exists, and should be merged soon.

@evdokimovs evdokimovs requested a review from tyranron February 19, 2024 11:09
@tyranron tyranron merged commit 3667edc into master Feb 19, 2024
51 checks passed
@tyranron tyranron deleted the send-ice-candidate-errors-to-server branch February 19, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::api Related to API (application interface) k::monitoring Related to monitoring capabilities k::refactor Refactoring, technical debt elimination and other improvements of existing code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants