fix(calling): set mediaEngine logger with webex logs for upload logs #4115
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes focused on logging and testing improvements across multiple modules. In 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) 📜 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 (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![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/calling/src/common/Utils.ts (1)
1557-1561: Improved uploadLogs function by removing redundant parameter.Refactored the function to directly retrieve the Webex instance from SDKConnector rather than requiring it as a parameter. This simplifies the function signature and centralizes the WebexSDK instance retrieval approach.
Consider adding error type information for better error handling:
-export async function uploadLogs(data = {}) { +export async function uploadLogs(data: Record<string, unknown> = {}): Promise<void> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/calling/src/CallingClient/CallingClient.test.ts(3 hunks)packages/calling/src/CallingClient/CallingClient.ts(1 hunks)packages/calling/src/SDKConnector/types.ts(1 hunks)packages/calling/src/common/Utils.test.ts(2 hunks)packages/calling/src/common/Utils.ts(1 hunks)packages/calling/src/common/testUtil.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (10)
packages/calling/src/SDKConnector/types.ts (1)
47-47: Improved parameter naming for consistency.Renaming the parameter from
messtopayloadenhances code readability and consistency, as it now matches the parameter naming used in other logging methods in the same interface.packages/calling/src/CallingClient/CallingClient.test.ts (3)
2-2: Added Media module import for logger testing.The import provides access to the media core module for testing the new logger integration.
57-57: Added spy for Media.setLogger to verify integration.This spy allows for verification that the logger is correctly integrated with the media engine during tests.
102-102: Verified logger integration in invalid URL scenario.The assertion confirms that the media engine logger is properly set up even when the Mobius service URL is invalid, ensuring robust logging across error conditions.
packages/calling/src/CallingClient/CallingClient.ts (1)
123-133: Added media engine logger integration.Implements the core fix for SPARK-616868 by integrating Webex logger with the media engine. The
adaptedLoggerobject correctly maps each log level method to the corresponding Webex logger method, ensuring that logs from the media layer are now included in log uploads to MATS.The implementation properly joins multiple arguments into a single string before passing to the Webex logger, which handles the various logging format requirements.
packages/calling/src/common/testUtil.ts (1)
64-66: Enhanced mock structure for webex support object.The change from using a simple mock function to a structured object with
submitLogsmethod better represents the actual implementation structure, which will improve test fidelity for log submission tests.packages/calling/src/common/Utils.test.ts (4)
60-60: Added SDKConnector import for centralized Webex instance management.This import allows tests to use a singleton pattern for accessing the Webex instance, making test management more consistent.
67-67: Setting Webex instance in SDKConnector for centralized access.This ensures that tests uniformly access the same Webex instance through SDKConnector rather than using direct references.
1563-1567: Refactored uploadLogs test to use centralized Webex instance.The test now retrieves the Webex instance via SDKConnector instead of passing it directly, which aligns with the approach used in the implementation code. This is a good pattern for ensuring consistency across the codebase.
1571-1575: Updated error handling test to use SDKConnector.The test now correctly mocks the rejection through the centralized SDKConnector instance, ensuring that the error handling test is properly validating the implementation.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
@sreenara we are completely replacing the logger(using setLogger). so the level also gets effected in the media core logs. so in the console we see whatever we pass in config while initialising webex/callingclient. But for uploading logs we upload complete buffer(irrespective of level) as per current logic in webex logger package. |
| const adaptedLogger: Media.Logger = { | ||
| log: (...args) => webex.logger.log(args.join(' ')), | ||
| error: (...args) => webex.logger.error(args.join(' ')), | ||
| warn: (...args) => webex.logger.warn(args.join(' ')), | ||
| info: (...args) => webex.logger.info(args.join(' ')), | ||
| trace: (...args) => webex.logger.trace(args.join(' ')), | ||
| debug: (...args) => webex.logger.debug(args.join(' ')), | ||
| }; |
There was a problem hiding this comment.
As discussed, Let's avoid space separated logs
There was a problem hiding this comment.
media layer uses console log which prints space separated. so using the same.
attached the downloaded log for reference.
There was a problem hiding this comment.
@sreenara @mkesavan13 What's your thought on this?
COMPLETES #SPARK-616868
This pull request addresses
The uploaded logs from calling sdk to mats does not have Media layer(webrtc-media-core) logs.
by making the following changes
called setLogger of Media layer to assign webex logger so that upload logs will have logs of media layer.
webex-js-sdk_4288e710-ec5a-4895-adc5-80f339eec445.txt
webex-js-sdk_ea67d93f-7651-43c5-952c-16e36f86061a.txt
Change Type
The following scenarios were tested
login to calling sdk with token and make a call.
end the call and click upload logs.
Expected: the logs should be uploaded to mats. and it should contain webrtc-media-core logs eg: initiateOffer() called
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.