fix: send rtc metrics at the beginning every 5 seconds #3893
Conversation
k-wasniowski
force-pushed
the
fix-send-rtc-metrics-every-5-seconds-at-the-beginning
branch
from
52f0ecb
to
8b71ce3
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| @@ -151,7 +152,7 @@ export default class RtcMetrics { | |||
| */ | |||
| private resetConnection() { | |||
| this.connectionId = uuid.v4(); | |||
| this.shouldSendMetricsOnNextStatsReport = true; | |||
| this.shouldSendMetricsOnNextStatsReport = 1; | |||
There was a problem hiding this comment.
we should set this to 30000/5000 here too, right? This is called from constructor so this is the default used when we join the meeting straight in, without lobby.
| @@ -3213,7 +3213,8 @@ export default class Meeting extends StatelessWebexPlugin { | |||
| options: {meetingId: this.id}, | |||
| }); | |||
| } | |||
| this.rtcMetrics?.sendNextMetrics(); | |||
| const numberOfReportsToSend = 30000 / 5000; // 30 seconds divided by 5 seconds | |||
There was a problem hiding this comment.
it's really annoying that the constant for calling getStats every 5s is buried deep in WCME and not even exported from it, but at least the 30s timer for uploading is defined in rtcMetrics/index.ts in SDK so we could define a constant and use it here instead of hardcoding 30000. Maybe define a constant for the 5s inside rtcMetrics/index.ts too so that both are kept together? Or don't bother with the math and just set numberOfReportsToSend=5 or 6, after all we just want "a few" reports with data from initial meeting phase, don't care about the exact number of seconds the data is from. If you go with that, then maybe define a constant so the same value is used also when we don't go through lobby (see my other comment)
| @@ -95,7 +96,7 @@ export default class RtcMetrics { | |||
| // this is the first useful set of data (WCME gives it to us after 5s), send it out immediately | |||
| // in case the user is unhappy and closes the browser early | |||
| this.sendMetricsInQueue(); | |||
| this.shouldSendMetricsOnNextStatsReport = false; | |||
| this.shouldSendMetricsOnNextStatsReport -= 1; | |||
There was a problem hiding this comment.
maybe worth changing the condition in line 95 to this.shouldSendMetricsOnNextStatsReport > 0 just in case if that variable ever became negative somehow?
… seconds
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.