fix: add extra logging on media state change #4149
Conversation
📝 WalkthroughWalkthroughThis pull request introduces multiple changes across several packages. It updates the version of the Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
k-wasniowski
added
the
validated
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
k-wasniowski
force-pushed
the
fix-extra-logging-on-media-state-change
branch
from
e42b5f8 to
47c1ebd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/src/media/properties.ts (1)
291-329: Excellent implementation of the timeout mechanism!The timeout implementation ensures that
getCurrentConnectionInfodoesn't wait indefinitely forgetStats()to resolve, improving application responsiveness. The changes include:
- Setting a 1-second timeout
- Properly clearing the timeout in both success and error cases
- Returning sensible default values when the operation times out or fails
This directly addresses the PR objective of limiting the wait time for connection information retrieval.
A small suggestion: consider using Promise.race() for a more declarative approach:
- await new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - reject(new Error('timed out')); - }, 1000); - - this.webrtcMediaConnection - .getStats() - .then((statsResult) => { - clearTimeout(timeout); - statsResult.forEach((report) => allStatsReports.push(report)); - resolve(allStatsReports); - }) - .catch((error) => { - clearTimeout(timeout); - reject(error); - }); - }); + await Promise.race([ + this.webrtcMediaConnection.getStats().then((statsResult) => { + statsResult.forEach((report) => allStatsReports.push(report)); + return allStatsReports; + }), + new Promise((_, reject) => + setTimeout(() => reject(new Error('timed out')), 1000) + ) + ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/@webex/media-helpers/package.json(1 hunks)packages/@webex/plugin-meetings/package.json(1 hunks)packages/@webex/plugin-meetings/src/media/MediaConnectionAwaiter.ts(1 hunks)packages/@webex/plugin-meetings/src/media/properties.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/media/properties.ts(1 hunks)packages/calling/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test - Integration
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (7)
packages/@webex/media-helpers/package.json (1)
25-25: Dependency version update looks good.The update of
@webex/internal-media-corefrom version 2.14.3 to 2.14.4 is consistent with the PR objective to enhance logging capabilities during media state changes.packages/calling/package.json (1)
40-40: LGTM - Dependency version update is consistent.The update of
@webex/internal-media-coreto version 2.14.4 is consistent with the same dependency update in the other packages.packages/@webex/plugin-meetings/package.json (1)
66-66: Consistent dependency update.The update of
@webex/internal-media-coreto version 2.14.4 aligns with the updates in the other packages, maintaining consistency across the codebase.packages/@webex/plugin-meetings/src/media/MediaConnectionAwaiter.ts (1)
114-115: Additional logging enhances observability.This log statement provides valuable visibility into the media connection resolution process, which aligns with the PR objective to add extra logging on media state changes. The placement right before the promise resolution is appropriate.
packages/@webex/plugin-meetings/test/unit/spec/media/properties.ts (1)
69-82: Great addition of a timeout test case!This test case effectively validates the new timeout mechanism in
getCurrentConnectionInfo. It ensures that whengetStats()doesn't resolve, the method will still return after the timeout period with appropriate default values instead of hanging indefinitely.packages/@webex/plugin-meetings/src/meeting/index.ts (2)
3354-3374: Good addition of logging for media state changesThe additional logging statements around media status change events provide valuable visibility during call initialization. This will help with troubleshooting media-related issues by clearly showing when media status changes are received and processed.
7474-7475: Helpful log message for media connection completionThis informative log statement clearly indicates when media has successfully connected and the process is being finalized, improving observability during the crucial media connection phase.
k-wasniowski
requested review from
SomeBody16,
antsukanova,
brycetham,
edvujic and
szotrabh
3 participants
COMPLETES #SPARK-635466
This pull request addresses
Additional logging to get better understanding what is going on during the call initialization.
Also, modifies
getCurrentConnectionInfonot to wait for getStats call longer than 1 second.Change Type
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.