feat(meetings): incremental log uploads #3899
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
marcin-bazyl
force-pushed
the
incremental-logs
branch
from
3c77ffa to
e5c5cbd
Compare
WalkthroughThis pull request introduces several changes across multiple files, primarily focused on enhancing logging functionalities and configurations within the Webex platform. Key additions include a new Changes
Possibly related PRs
Suggested reviewers
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
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 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
CodeRabbit Configuration File (
|
marcin-bazyl
force-pushed
the
incremental-logs
branch
from
e5c5cbd to
bb2dd38
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: 2
🧹 Outside diff range and nitpick comments (6)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
706-706: Use consistent timer type declarationsThe
uploadLogsTimerproperty is declared with typeReturnType<typeof setTimeout>, whereas other timers in the class, such askeepAliveTimerId, are declared asNodeJS.Timeout. For consistency and to avoid potential type mismatches across environments, consider usingNodeJS.Timeoutfor all timer properties.
4068-4126: Add documentation comments to new methods for clarityTo enhance code maintainability and readability, please add JSDoc comments to the newly introduced methods
startPeriodicLogUpload,stopPeriodicLogUpload, andsetLogUploadTimer. Documenting these methods will help other developers understand their purpose and usage.packages/@webex/plugin-logger/src/logger.js (1)
255-268: Consider resettinggroupLevelwhen clearing log buffersWhen using
clearBuffers()to reset the log buffers, consider also resetting thegroupLevelproperty to0. This prevents any unintended indentation in future logs due to residual group levels, ensuring consistent log formatting.packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (1)
Additional test coverage needed for periodic log upload feature
The implementation shows several untested aspects of the periodic log upload feature:
startPeriodicLogUpload()method with configuration vialogUploadIntervalMultiplicationFactorsetLogUploadTimer()method's interval calculation and scheduling- Log upload behavior when media connection is lost
- Automatic log upload triggers during reconnection scenarios
Suggested test cases to add:
- Test initialization of periodic uploads with different interval configurations
- Verify log upload stops when media connection is lost
- Test the progressive interval increase through
logUploadIntervalIndex- Verify automatic upload triggers during connection failures and reconnections
🔗 Analysis chain
Line range hint
42-68: Additional test coverage needed for new features.While the cleanup scenario is well tested, consider adding test coverage for other aspects of the incremental log upload feature:
- Starting periodic log uploads
- Differential vs. full log upload behavior
- Upload interval configuration
Let me check the content of these files to see if they contain the test coverage we're looking for.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing test coverage of the new features # Look for test files that might contain related tests fd -e js -e ts . | rg -i "test|spec" | xargs rg -l "periodic.*log|incremental.*log|log.*upload"Length of output: 226
Script:
#!/bin/bash # Check content of the identified files echo "=== Content of packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js ===" rg -A 5 "periodic.*log|incremental.*log|log.*upload" "packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js" echo -e "\n=== Content of packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js ===" rg -A 5 "periodic.*log|incremental.*log|log.*upload" "packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js" # Look for implementation of these features to understand what should be tested echo -e "\n=== Looking for implementation of periodic/incremental log upload features ===" rg -A 5 "startPeriodicLogUpload|stopPeriodicLogUpload|uploadLogs" "packages/@webex/plugin-meetings/src/"Length of output: 13927
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint
4-4: Reminder: Address the TODO comment.Please ensure that tests are added for this function to verify the behavior with the additional parameter.
Would you like me to help generate unit tests for this function?
Line range hint
12-24: Consider adjusting the fee structure or discount policy.The current implementation has two issues:
Indentation is inconsistent in the elif/else blocks. They should align with the if statement.
The business logic may be problematic:
- A flat $20 fee is added to any discounted total
- For small purchases with 10% discount (3-4 years loyalty), the $20 fee can exceed the discount benefit
- Example: $100 purchase with 10% discount = $90 + $20 = $110 (more than original price)
Would you like me to propose an alternative implementation that ensures discounts always benefit the customer?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/@webex/internal-plugin-support/src/config.js(1 hunks)packages/@webex/internal-plugin-support/src/support.js(1 hunks)packages/@webex/plugin-logger/src/logger.js(1 hunks)packages/@webex/plugin-meetings/src/config.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(5 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/@webex/plugin-meetings/src/meeting/util.ts
🔇 Additional comments (6)
packages/@webex/internal-plugin-support/src/config.js (1)
20-20: Addition of incrementalLogs property appears correct
The new incrementalLogs property is properly added to the support configuration object. This allows for control over incremental log submissions.
packages/@webex/plugin-meetings/src/config.ts (1)
99-99: Addition of logUploadIntervalMultiplicationFactor property appears correct
The new logUploadIntervalMultiplicationFactor property is appropriately added to control periodic log uploads. The accompanying comment clearly explains its usage, ensuring that if set to 0 or undefined, logs won't be uploaded periodically.
packages/@webex/internal-plugin-support/src/support.js (1)
110-113: Clearing log buffers after log submission
The conditional check for this.config.incrementalLogs and subsequent call to this.webex.logger.clearBuffers() correctly ensure that log buffers are cleared after logs are submitted when incremental logging is enabled. This prevents duplicate logs in future submissions.
packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js (2)
42-42: LGTM: Stub for new periodic log upload cleanup.
The addition of the stopPeriodicLogUpload stub aligns with the PR's objective to implement incremental log uploads, ensuring proper cleanup testing.
68-68: LGTM: Assertion verifies periodic log upload cleanup.
The assertion correctly verifies that stopPeriodicLogUpload is called once during meeting cleanup when LLM is enabled, which is essential for proper resource management.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint 1-2: LGTM!
The subtraction function is implemented correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (3)
4068-4084: Consider moving LOG_UPLOAD_INTERVALS to a constant.The array of intervals could be moved to a class constant or external configuration to improve maintainability and reusability.
+ private static readonly LOG_UPLOAD_INTERVALS = [0.1, 1, 15, 15, 30, 30, 30, 60]; private setLogUploadTimer() { - const LOG_UPLOAD_INTERVALS = [0.1, 1, 15, 15, 30, 30, 30, 60]; + const intervals = Meeting.LOG_UPLOAD_INTERVALS;
4106-4113: Add null check for config property.The config check could be more robust by explicitly checking if the multiplication factor is a positive number.
public startPeriodicLogUpload() { - if (this.config.logUploadIntervalMultiplicationFactor && !this.uploadLogsTimer) { + if (typeof this.config.logUploadIntervalMultiplicationFactor === 'number' && + this.config.logUploadIntervalMultiplicationFactor > 0 && + !this.uploadLogsTimer) {
4120-4126: Consider adding cleanup in the destructor.The stopPeriodicLogUpload method should be called when the meeting is destroyed to ensure proper cleanup.
Add a call to stopPeriodicLogUpload() in the class destructor or cleanup method:
public clearMeetingData = () => { + this.stopPeriodicLogUpload(); this.audio = null; this.video = null; // ... rest of the cleanup code };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/meeting/index.ts(5 hunks)packages/@webex/plugin-meetings/src/meeting/util.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/@webex/plugin-meetings/src/meeting/util.ts
- packages/@webex/plugin-meetings/test/unit/spec/meeting/utils.js
🧰 Additional context used
📓 Learnings (1)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
Learnt from: marcin-bazyl
PR: webex/webex-js-sdk#3899
File: packages/@webex/plugin-meetings/src/meeting/index.ts:4068-4126
Timestamp: 2024-12-03T13:21:58.600Z
Learning: In the `Meeting` class (`packages/@webex/plugin-meetings/src/meeting/index.ts`), the `stopPeriodicLogUpload()` method is called during meeting cleanup via `MeetingUtil.cleanUp()`, so it's already invoked when the meeting ends.
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (2)
706-707: LGTM! Private properties added for managing periodic log uploads.
The new private properties are well-typed and appropriately scoped for managing the periodic log upload functionality.
7312-7312: LGTM! Appropriate placement of startPeriodicLogUpload call.
The periodic log upload is started after the media connection is successfully established, which is a good point to begin collecting logs.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
2468-2523: LGTM! Well-structured test for periodic log uploads
The test case thoroughly verifies the periodic log upload functionality:
- Uses fake timers to control time progression
- Verifies log upload triggers at expected intervals (100ms, 1s, 15s, 30s, 60s)
- Checks that uploads stop when media connection is removed
- Validates counter increments at the right times
- Good use of helper function
checkLogCounterto avoid code duplication
This pull request addresses
Some existing issues with logging:
by making the following changes
Backend allows us to upload logs multiple times for same meeting, so instead of uploading the full log once, let's upload them periodically in smaller chunks - just the diff from the previous successful upload.
Added also config 2 options:
Change Type
The following scenarios were tested
manual run with the web app
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.
Summary by CodeRabbit
Release Notes
New Features
incrementalLogsandlogUploadIntervalMultiplicationFactor.Bug Fixes
Tests