-
Notifications
You must be signed in to change notification settings - Fork 217
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
VIDEO-7728 | Better Preflight Errors #1689
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.
@charliesantos, This is looking very good. I have just one suggestion about adding preflight progress events along with their timing to insights.
lib/preflight/preflighttest.ts
Outdated
@@ -110,6 +115,7 @@ export class PreflightTest extends EventEmitter { | |||
private _connectTiming = new Timer(); | |||
private _sentBytesMovingAverage = new MovingAverageDelta(); | |||
private _packetLossMovingAverage = new MovingAverageDelta(); | |||
private _progressEvents: string[] = []; |
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.
How about changing _progressEvents to be Map<time, event>
and also reporting it to insights.
lib/preflight/preflighttest.ts
Outdated
@@ -347,19 +365,19 @@ export class PreflightTest extends EventEmitter { | |||
const updatedAnswer = answer; |
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.
looks like updatedAnswer
is not needed
also please hold off on merging to develop - we need to decide if we want to release |
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.
looking good, but have couple more suggestions
@@ -163,13 +169,13 @@ export class PreflightTest extends EventEmitter { | |||
}, | |||
selectedIceCandidatePairStats: collectedStats ? collectedStats.selectedIceCandidatePairStats : null, | |||
iceCandidateStats: collectedStats ? collectedStats.iceCandidateStats : [], | |||
progressEvents: this._progressEvents, |
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.
I think insights does not like arrays. please check if this works
lib/preflight/preflighttest.ts
Outdated
@@ -438,6 +456,11 @@ export class PreflightTest extends EventEmitter { | |||
} | |||
return collectedStats; | |||
} | |||
|
|||
private _updateProgress(name: string): void { | |||
this._progressEvents.push({ timestamp: Date.now(), name }); |
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.
instead of timestamp
we should use timeFromStart as it would be much useful that absolute time.
* VIDEO-7728 | Better Preflight Errors (#1689) * Adding better errors on preflight * lint * Update docs * Changelog * Update docs * Convert error to string * Adding timestamp to progress events * Properly raising signaling errors * lint * Update tests * Mak's feedback * Update test * Feature/remove plan b (#1697) * VIDEO-6587 | Remove Plan B in SDK (#1656) * Initial implementation. * Fixing integration tests * Updating unit tests * Adding back unit test job * Removing unneeded case * Fix build * Adding rename suggestion Co-authored-by: Manjesh Malavalli <[email protected]> * Adding changelog * Update CHANGELOG.md Co-authored-by: Manjesh Malavalli <[email protected]> * Adding ticket number Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> * Move changelog to 2.20.0 section * 2.20.0-rc1 * 2.20.0-dev Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Fix unit test from merge * 2.20.0-rc2 * 2.20.0-dev * Update doc * Update changelog Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]>
* Merge Preflight and PlanB removal feature branch (#1702) * VIDEO-7728 | Better Preflight Errors (#1689) * Adding better errors on preflight * lint * Update docs * Changelog * Update docs * Convert error to string * Adding timestamp to progress events * Properly raising signaling errors * lint * Update tests * Mak's feedback * Update test * Feature/remove plan b (#1697) * VIDEO-6587 | Remove Plan B in SDK (#1656) * Initial implementation. * Fixing integration tests * Updating unit tests * Adding back unit test job * Removing unneeded case * Fix build * Adding rename suggestion Co-authored-by: Manjesh Malavalli <[email protected]> * Adding changelog * Update CHANGELOG.md Co-authored-by: Manjesh Malavalli <[email protected]> * Adding ticket number Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> * Move changelog to 2.20.0 section * 2.20.0-rc1 * 2.20.0-dev Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Fix unit test from merge * 2.20.0-rc2 * 2.20.0-dev * Update doc * Update changelog Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Prep for release Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]>
* Merge Preflight and PlanB removal feature branch (#1702) * VIDEO-7728 | Better Preflight Errors (#1689) * Adding better errors on preflight * lint * Update docs * Changelog * Update docs * Convert error to string * Adding timestamp to progress events * Properly raising signaling errors * lint * Update tests * Mak's feedback * Update test * Feature/remove plan b (#1697) * VIDEO-6587 | Remove Plan B in SDK (#1656) * Initial implementation. * Fixing integration tests * Updating unit tests * Adding back unit test job * Removing unneeded case * Fix build * Adding rename suggestion Co-authored-by: Manjesh Malavalli <[email protected]> * Adding changelog * Update CHANGELOG.md Co-authored-by: Manjesh Malavalli <[email protected]> * Adding ticket number Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> * Move changelog to 2.20.0 section * 2.20.0-rc1 * 2.20.0-dev Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Fix unit test from merge * 2.20.0-rc2 * 2.20.0-dev * Update doc * Update changelog Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Removing framework tests (#1700) * Convert createLocalTracks to TS (#1694) * VIDEO-7714 - Convert createLocalTracks to TypeScript Co-authored-by: joma <[email protected]> Co-authored-by: charliesantos <[email protected]> * Update @twilio/webrtc to 4.6.0-rc1 (#1705) * Update @twilio-webrtc to RC Co-authored-by: joma <[email protected]> * 2.21.0-rc1 * 2.21.0-dev * merge from master * VIDEO-8791 - Adding extra information into CHANGELOG (#1716) * Adding extra information into CHANGELOG * 2.21.0-rc2 * 2.21.0-dev * VIDEO-8647 - Fixing Chrome Docker Image (#1718) * VIDEO-8647 Fixing docker images build for Chrome * VIDEO-8954: Adding in extra links and details to the CHANGELOG entry (#1722) * VIDEO-8954 - Adding in extra links and details to the CHANGELOG entry * VIDEO-8647 - Fixing Chrome Docker Image (#1718) (#1721) (#1728) * VIDEO-8647 Fixing docker images build for Chrome * prep 2.21.0 release Co-authored-by: Charlemagne Santos <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> Co-authored-by: Makarand Patwardhan <[email protected]>
* Merge Preflight and PlanB removal feature branch (#1702) * VIDEO-7728 | Better Preflight Errors (#1689) * Adding better errors on preflight * lint * Update docs * Changelog * Update docs * Convert error to string * Adding timestamp to progress events * Properly raising signaling errors * lint * Update tests * Mak's feedback * Update test * Feature/remove plan b (#1697) * VIDEO-6587 | Remove Plan B in SDK (#1656) * Initial implementation. * Fixing integration tests * Updating unit tests * Adding back unit test job * Removing unneeded case * Fix build * Adding rename suggestion Co-authored-by: Manjesh Malavalli <[email protected]> * Adding changelog * Update CHANGELOG.md Co-authored-by: Manjesh Malavalli <[email protected]> * Adding ticket number Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> * Move changelog to 2.20.0 section * 2.20.0-rc1 * 2.20.0-dev Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Fix unit test from merge * 2.20.0-rc2 * 2.20.0-dev * Update doc * Update changelog Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Removing framework tests (#1700) * Convert createLocalTracks to TS (#1694) * VIDEO-7714 - Convert createLocalTracks to TypeScript Co-authored-by: joma <[email protected]> Co-authored-by: charliesantos <[email protected]> * Update @twilio/webrtc to 4.6.0-rc1 (#1705) * Update @twilio-webrtc to RC Co-authored-by: joma <[email protected]> * 2.21.0-rc1 * 2.21.0-dev * merge from master * VIDEO-8791 - Adding extra information into CHANGELOG (#1716) * Adding extra information into CHANGELOG * 2.21.0-rc2 * 2.21.0-dev * VIDEO-8647 - Fixing Chrome Docker Image (#1718) * VIDEO-8647 Fixing docker images build for Chrome * VIDEO-8954: Adding in extra links and details to the CHANGELOG entry (#1722) * VIDEO-8954 - Adding in extra links and details to the CHANGELOG entry * VIDEO-8647 - Fixing Chrome Docker Image (#1718) (#1721) (#1728) * VIDEO-8647 Fixing docker images build for Chrome * VIDEO-8939 - Migrating twilio/webrtc (#1731) * VIDEO-8939 - Folding twilio/webrtc into SDK, installing util and events packages. * Prep for 2.21.1 RC (#1739) * Adding CHANGELOG entry for 2.21.1 RC * 2.21.1-rc1 * 2.21.1-dev * Edit changelog and readme for 2.21.1 release Co-authored-by: Charlemagne Santos <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> Co-authored-by: Makarand Patwardhan <[email protected]>
* Merge Preflight and PlanB removal feature branch (#1702) * VIDEO-7728 | Better Preflight Errors (#1689) * Adding better errors on preflight * lint * Update docs * Changelog * Update docs * Convert error to string * Adding timestamp to progress events * Properly raising signaling errors * lint * Update tests * Mak's feedback * Update test * Feature/remove plan b (#1697) * VIDEO-6587 | Remove Plan B in SDK (#1656) * Initial implementation. * Fixing integration tests * Updating unit tests * Adding back unit test job * Removing unneeded case * Fix build * Adding rename suggestion Co-authored-by: Manjesh Malavalli <[email protected]> * Adding changelog * Update CHANGELOG.md Co-authored-by: Manjesh Malavalli <[email protected]> * Adding ticket number Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> * Move changelog to 2.20.0 section * 2.20.0-rc1 * 2.20.0-dev Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Fix unit test from merge * 2.20.0-rc2 * 2.20.0-dev * Update doc * Update changelog Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Removing framework tests (#1700) * Convert createLocalTracks to TS (#1694) * VIDEO-7714 - Convert createLocalTracks to TypeScript Co-authored-by: joma <[email protected]> Co-authored-by: charliesantos <[email protected]> * Update @twilio/webrtc to 4.6.0-rc1 (#1705) * Update @twilio-webrtc to RC Co-authored-by: joma <[email protected]> * 2.21.0-rc1 * 2.21.0-dev * merge from master * VIDEO-8791 - Adding extra information into CHANGELOG (#1716) * Adding extra information into CHANGELOG * 2.21.0-rc2 * 2.21.0-dev * VIDEO-8647 - Fixing Chrome Docker Image (#1718) * VIDEO-8647 Fixing docker images build for Chrome * VIDEO-8954: Adding in extra links and details to the CHANGELOG entry (#1722) * VIDEO-8954 - Adding in extra links and details to the CHANGELOG entry * VIDEO-8647 - Fixing Chrome Docker Image (#1718) (#1721) (#1728) * VIDEO-8647 Fixing docker images build for Chrome * VIDEO-8939 - Migrating twilio/webrtc (#1731) * VIDEO-8939 - Folding twilio/webrtc into SDK, installing util and events packages. * Prep for 2.21.1 RC (#1739) * Adding CHANGELOG entry for 2.21.1 RC * 2.21.1-rc1 * 2.21.1-dev * Marking unstable integration tests (#1740) * Marking unstable integration tests * merge from master (#1745) * VIDEO-9511 - do not configure encodings for stopped tracks (#1768) * do not update encodings for stopped tracks * Emit dimensionsChanged event on videoTrack start * Add test for videoTrack _start method * Add changelog entry for VIDEO-3576 * VIDEO-9282 - Migrate SDK from using node.js util. (#1752) * VIDEO-9282 - Removing all references to node dependencies. Co-authored-by: Makarand Patwardhan <[email protected]> * 2.21.2-rc1 * 2.21.2-dev * VIDEO-8282 - Adding iPad Detection (#1779) * VIDEO-8282 - Adding iPad and iPhone detection logic * 2.21.2-rc2 * 2.21.2-dev * Prep for 2.21.2 release, updating changelog Co-authored-by: Charlemagne Santos <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> Co-authored-by: Makarand Patwardhan <[email protected]> Co-authored-by: Tim Mendoza <[email protected]> Co-authored-by: timmydoza <[email protected]>
* Merge Preflight and PlanB removal feature branch (#1702) * VIDEO-7728 | Better Preflight Errors (#1689) * Adding better errors on preflight * lint * Update docs * Changelog * Update docs * Convert error to string * Adding timestamp to progress events * Properly raising signaling errors * lint * Update tests * Mak's feedback * Update test * Feature/remove plan b (#1697) * VIDEO-6587 | Remove Plan B in SDK (#1656) * Initial implementation. * Fixing integration tests * Updating unit tests * Adding back unit test job * Removing unneeded case * Fix build * Adding rename suggestion Co-authored-by: Manjesh Malavalli <[email protected]> * Adding changelog * Update CHANGELOG.md Co-authored-by: Manjesh Malavalli <[email protected]> * Adding ticket number Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> * Move changelog to 2.20.0 section * 2.20.0-rc1 * 2.20.0-dev Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Fix unit test from merge * 2.20.0-rc2 * 2.20.0-dev * Update doc * Update changelog Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> * Removing framework tests (#1700) * Convert createLocalTracks to TS (#1694) * VIDEO-7714 - Convert createLocalTracks to TypeScript Co-authored-by: joma <[email protected]> Co-authored-by: charliesantos <[email protected]> * Update @twilio/webrtc to 4.6.0-rc1 (#1705) * Update @twilio-webrtc to RC Co-authored-by: joma <[email protected]> * 2.21.0-rc1 * 2.21.0-dev * merge from master * VIDEO-8791 - Adding extra information into CHANGELOG (#1716) * Adding extra information into CHANGELOG * 2.21.0-rc2 * 2.21.0-dev * VIDEO-8647 - Fixing Chrome Docker Image (#1718) * VIDEO-8647 Fixing docker images build for Chrome * VIDEO-8954: Adding in extra links and details to the CHANGELOG entry (#1722) * VIDEO-8954 - Adding in extra links and details to the CHANGELOG entry * VIDEO-8647 - Fixing Chrome Docker Image (#1718) (#1721) (#1728) * VIDEO-8647 Fixing docker images build for Chrome * VIDEO-8939 - Migrating twilio/webrtc (#1731) * VIDEO-8939 - Folding twilio/webrtc into SDK, installing util and events packages. * Prep for 2.21.1 RC (#1739) * Adding CHANGELOG entry for 2.21.1 RC * 2.21.1-rc1 * 2.21.1-dev * Marking unstable integration tests (#1740) * Marking unstable integration tests * merge from master (#1745) * VIDEO-9511 - do not configure encodings for stopped tracks (#1768) * do not update encodings for stopped tracks * Emit dimensionsChanged event on videoTrack start * Add test for videoTrack _start method * Add changelog entry for VIDEO-3576 * VIDEO-9282 - Migrate SDK from using node.js util. (#1752) * VIDEO-9282 - Removing all references to node dependencies. Co-authored-by: Makarand Patwardhan <[email protected]> * 2.21.2-rc1 * 2.21.2-dev * VIDEO-8282 - Adding iPad Detection (#1779) * VIDEO-8282 - Adding iPad and iPhone detection logic * 2.21.2-rc2 * 2.21.2-dev * Added details in changelog entry and updated insights payload constants (#1789) * 2.21.2-rc3 * 2.21.2-dev * Merge from master, resolving conflicts, adding new changelog entry (#1796) * VIDEO-10199 Update tooling back to working versions (#1798) * Adding platformName into insights payload (#1799) * Changelog for RC * 2.21.3-rc1 * 2.21.3-dev * Update changelog and readme Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: Manjesh Malavalli <[email protected]> Co-authored-by: twilio-ci <[email protected]> Co-authored-by: Joyce Ma <[email protected]> Co-authored-by: joma <[email protected]> Co-authored-by: Makarand Patwardhan <[email protected]> Co-authored-by: Tim Mendoza <[email protected]> Co-authored-by: timmydoza <[email protected]>
Contributing to Twilio
Pull Request Details
Description
See changelog.
I will update the tests once these changes are approved.
Burndown
Before review
npm test