fix: refactored how incremental logs work #4076
Conversation
|
Warning Rate limit exceeded@marcin-bazyl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the log submission and formatting functionality across multiple packages. In the Possibly related PRs
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
|
marcin-bazyl
added
the
validated
![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 (3)
packages/@webex/internal-plugin-support/src/support.js (2)
41-50: Enhance JSDoc with specific type values.The JSDoc is comprehensive but could be more specific about the allowed values for
options.type.* @param {Object} options additional options - * @param {Boolean} options.type 'full' or 'diff', if not specified then the config.incrementalLogs value is used to determine the type, + * @param {('full'|'diff')} options.type If not specified, config.incrementalLogs value is used to determine the type,
54-64: Add type validation for the options.type parameter.The type parameter should be validated to ensure only valid values ('full' or 'diff') are provided.
- const {type} = options; + const {type} = options; + if (type !== undefined && !['full', 'diff'].includes(type)) { + throw new Error("options.type must be either 'full' or 'diff'"); + } const diff = type !== undefined ? type === 'diff' : this.config.incrementalLogs;packages/@webex/plugin-logger/src/logger.js (1)
281-285: Enhance JSDoc for formatLogs options.The JSDoc could be more specific about the default value of the diff option.
* @param {Object} options - * @param {boolean} options.diff whether to only format the diff from last call to formatLogs(), false by default + * @param {boolean} [options.diff=false] Whether to only format the diff from last call to formatLogs()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/@webex/internal-plugin-support/src/support.js(3 hunks)packages/@webex/internal-plugin-support/test/unit/spec/support.js(2 hunks)packages/@webex/plugin-logger/src/logger.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (6)
packages/@webex/internal-plugin-support/src/support.js (1)
Line range hint
94-121: LGTM! Good variable renaming practice.The renaming of
optionstouploadOptionsis a good practice to avoid naming conflicts with the method parameter.packages/@webex/plugin-logger/src/logger.js (2)
110-110: LGTM! Well-structured buffer index tracking.The new buffer index properties are well-defined and properly initialized.
Also applies to: 124-124, 131-131
Line range hint
289-321: Consider buffer overflow and memory management.The buffer indices keep increasing without bounds, which could lead to integer overflow in long-running sessions. Consider resetting indices when buffers are cleared.
Add index reset to clearBuffers():
clearBuffers() { this.clientBuffer = []; this.sdkBuffer = []; this.buffer = []; + this.clientBufferNextIndex = 0; + this.sdkBufferNextIndex = 0; + this.bufferNextIndex = 0; }packages/@webex/internal-plugin-support/test/unit/spec/support.js (3)
165-173: LGTM! Well-structured test setup.The test setup with mocks and stubs is clean and comprehensive.
180-207: LGTM! Excellent test coverage with parameterized tests.The test matrix thoroughly covers all combinations of type and incrementalLogsConfig settings.
208-221: LGTM! Good separation of concerns in tests.The separate test for provided logs ensures that the logger's formatLogs isn't called unnecessarily.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
marcin-bazyl
force-pushed
the
incremental-logs-refactor
branch
from
ff0857f to
77420cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/@webex/plugin-logger/test/unit/spec/logger.js (1)
1079-1148: Consider adding edge case tests for buffer truncationWhile the current tests cover basic truncation scenarios, consider adding tests for:
- Edge case where truncation exactly matches nextIndex
- Multiple consecutive truncations
- Truncation with empty buffers
packages/@webex/internal-plugin-support/src/support.js (2)
41-51: Enhance options.type documentationThe JSDoc comment should explicitly list the allowed values for options.type.
* @param {Object} metadata metadata about the logs * @param {Array} logs logs to send, if undefined, SDK's logs will be sent * @param {Object} options additional options - * @param {Boolean} options.type 'full' or 'diff', if not specified then the config.incrementalLogs value is used to determine the type, + * @param {('full'|'diff')} options.type The type of logs to send. If not specified, defaults to 'diff' when config.incrementalLogs is true, 'full' otherwise. * this option only applies if logs parameter is undefined
54-64: Add type validation for options.typeConsider validating the type parameter to ensure it only accepts 'full' or 'diff'.
const {type} = options; + if (type !== undefined && type !== 'full' && type !== 'diff') { + throw new Error('options.type must be either "full" or "diff"'); + } if (packages/@webex/plugin-logger/src/logger.js (3)
Line range hint
105-135: Consider DRYing up buffer initializationThe buffer initialization logic is duplicated across three properties. Consider extracting it to a helper function.
+ function createBuffer() { + return { + buffer: [], + nextIndex: 0, + }; + } + buffer: { type: 'object', - default() { - return { - buffer: [], - nextIndex: 0, - }; - }, + default: createBuffer, }, // ... sdkBuffer: { type: 'object', - default() { - return { - buffer: [], - nextIndex: 0, - }; - }, + default: createBuffer, }, clientBuffer: { type: 'object', - default() { - return { - buffer: [], - nextIndex: 0, - }; - }, + default: createBuffer, },
277-320: Optimize date comparison in buffer merge logicThe current implementation creates new Date objects in each iteration. Consider converting dates to timestamps once at the start of the merge.
function getDate(log) { - return log[1]; + return new Date(log[1]).getTime(); } const {diff = false} = options; let buffer = []; let clientIndex = diff ? this.clientBuffer.nextIndex : 0; let sdkIndex = diff ? this.sdkBuffer.nextIndex : 0; + + // Pre-compute timestamps if using separate buffers + let clientDates, sdkDates; + if (this.config.separateLogBuffers) { + clientDates = this.clientBuffer.buffer.map(getDate); + sdkDates = this.sdkBuffer.buffer.map(getDate); + } if (this.config.separateLogBuffers) { while ( clientIndex < this.clientBuffer.buffer.length || sdkIndex < this.sdkBuffer.buffer.length ) { if ( sdkIndex < this.sdkBuffer.buffer.length && (clientIndex >= this.clientBuffer.buffer.length || - new Date(getDate(this.sdkBuffer.buffer[sdkIndex])) <= - new Date(getDate(this.clientBuffer.buffer[clientIndex]))) + sdkDates[sdkIndex] <= clientDates[clientIndex]) ) {
426-437: Add safety checks to buffer truncation logicConsider adding validation to prevent potential issues with buffer manipulation.
+ // Ensure historyLength is valid + if (historyLength <= 0) { + throw new Error('historyLength must be positive'); + } + bufferRef.buffer.push(stringified); if (bufferRef.buffer.length > historyLength) { const deleteCount = bufferRef.buffer.length - historyLength; + + // Ensure we don't delete more than we have + if (deleteCount > bufferRef.buffer.length) { + throw new Error('Invalid deleteCount in buffer truncation'); + } bufferRef.buffer.splice(0, deleteCount); bufferRef.nextIndex -= deleteCount; if (bufferRef.nextIndex < 0) { bufferRef.nextIndex = 0; } + // Ensure nextIndex doesn't exceed buffer length + if (bufferRef.nextIndex > bufferRef.buffer.length) { + bufferRef.nextIndex = bufferRef.buffer.length; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/@webex/internal-plugin-support/src/support.js(3 hunks)packages/@webex/internal-plugin-support/test/unit/spec/support.js(2 hunks)packages/@webex/plugin-logger/src/logger.js(5 hunks)packages/@webex/plugin-logger/test/unit/spec/logger.js(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-support/test/unit/spec/support.js
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-logger/test/unit/spec/logger.js
[error] 96-96: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (1)
packages/@webex/plugin-logger/test/unit/spec/logger.js (1)
892-1078: Well-structured test coverage for diff logs functionality!The test suite thoroughly covers critical scenarios:
- Differential and full log formatting
- Buffer management with size limits
- Proper index tracking
- Timestamp ordering
- Both separate and combined buffer configurations
marcin-bazyl
force-pushed
the
incremental-logs-refactor
branch
from
77420cc to
0f509a7
Compare
marcin-bazyl
force-pushed
the
incremental-logs-refactor
branch
from
0f509a7 to
1673361
Compare
| // we've gone over the buffer limit, trim it down | ||
| const deleteCount = bufferRef.buffer.length - historyLength; | ||
|
|
||
| bufferRef.buffer.splice(0, deleteCount); |
There was a problem hiding this comment.
when incremental logs config is enabled we could potentially upload them here instead of just deleting old ones when history limit is reached, but this is out of scope for this PR, I've raised https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-610060 for this
This pull request addresses
We want to have incremental logs, but also in some cases we want to upload the full logs:
by making the following changes
Changed how incremental logs work. Instead of clearing the buffers on each upload we now keep track of the last uploaded positions from the buffers. This allows us to upload a full log if we want to and if we want a diff log, we just upload the subset of the buffers (from the last sent position). Sending/requesting a full log does not affect the next diff log.
Change Type
The following scenarios where tested
unit tests, tested manually with 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.