feat: clear stale queues #1952
feat: clear stale queues #1952
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![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 (1)
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts (1)
Based on the search results, I can now provide a definitive response about the commented queue initialization code:
The commented code should be uncommented as it's a critical part of queue initialization.
The evidence shows that:
- The
clientDestinationsReadysignal is used across the codebase to coordinate queue starts, particularly inEventRepository.tswhich starts both destination and DMT queues when this flag becomes true.- The commented code follows the same pattern as other queue initializations in the codebase, ensuring queues only start processing after destinations are ready.
- Without this initialization, the queue might start processing events before the destinations are ready to handle them.
🔗 Analysis chain
Line range hint
116-123: Investigate the commented out queue initialization code.The commented TODO and effect hook seem relevant to queue management. If this PR is about clearing stale queues, this initialization logic might be important.
Let's check the RetryQueue implementation to understand how it handles queue initialization and stale queues:
Let me analyze the RetryQueue implementation and the commented code to understand the queue initialization behavior.
From the search results, I can see that:
- The RetryQueue has a
start()method that initializes queue processing by starting the batch flush timer, ack timer, reclaim check, and head processing.- The commented code in the review is trying to start the queue when
clientDestinationsReadybecomes true.Let me check how this queue is used in the codebase to understand the implications:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for queue initialization and cleanup logic in RetryQueue ast-grep --pattern 'class RetryQueue { $$$ start() { $$$ } $$$ }' # Look for any stale queue handling logic rg -i "stale|cleanup|clear" --type tsLength of output: 31160
Script:
#!/bin/bash # Find where this queue is instantiated and used rg -A 5 "new RetryQueue" --type ts # Look for clientDestinationsReady usage rg -A 5 "clientDestinationsReady" --type tsLength of output: 16370
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
.github/workflows/deploy-beta.ymlis excluded by!**/*.yml.github/workflows/deploy.ymlis excluded by!**/*.yml.github/workflows/security-code-quality-and-bundle-size-checks.ymlis excluded by!**/*.ymlpackage.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts(1 hunks)packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts(6 hunks)packages/analytics-js-plugins/src/utilities/retryQueue/logMessages.ts(1 hunks)
🔇 Additional comments (5)
packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts (3)
370-374: Correct handling of queue size when maxItems is less than or equal to 1
The updated logic correctly ensures that when maxItems is less than or equal to 1, the queue is reset to an empty array, preventing any items from being enqueued beyond the limit.
549-558: Enhanced error logging for processing failures
Adding detailed logs in the catch block significantly improves debugging capabilities by providing comprehensive information about the failure context, including the queue item, instance state, and global variables.
669-677: Ensure consistent updating of ourQueue when reclaiming items
When updating ourQueue with reclaimedQueueItems, ensure that the logic correctly handles cases where the number of reclaimed items exceeds the available room in the queue, and that the items are sorted properly after the merge.
Please verify that no items are unintentionally dropped due to the maxItems limit and that the sorting ensures correct processing order.
packages/analytics-js-plugins/src/nativeDestinationQueue/index.ts (2)
113-113: LGTM: Logger parameter addition is correct.
The addition of the logger parameter to RetryQueue is consistent with the function signature and enables better logging capabilities.
Line range hint 1-150: Implementation of stale queue clearing appears to be missing.
Based on the PR objectives to "clear stale queues", I don't see any implementation of stale queue clearing logic in this file. While adding logging is valuable, the core feature appears to be missing.
Let's verify if the stale queue clearing logic exists elsewhere:
| // Instead of reclaiming stale queues, clear them | ||
| findOtherQueues(this.name).forEach(store => this.clearQueueEntries(store, 0)); |
There was a problem hiding this comment.
Potential data loss by clearing stale queues instead of reclaiming
Changing the checkReclaim method to clear stale queues (clearQueueEntries) rather than reclaiming them could lead to loss of unprocessed queue items, resulting in possible data loss.
Consider evaluating whether it's acceptable to discard these items. If not, you might need to implement a mechanism to reclaim unprocessed items from stale queues to ensure data integrity.
| @@ -1,7 +1,7 @@ | |||
| import { LOG_CONTEXT_SEPARATOR } from '../../shared-chunks/common'; | |||
|
|
|||
| const RETRY_QUEUE_PROCESS_ERROR = (context: string): string => | |||
| `${context}${LOG_CONTEXT_SEPARATOR}Process function threw an error.`; | |||
| `${context}${LOG_CONTEXT_SEPARATOR}Process function threw an error while processing the queue item. The item is be dropped.`; | |||
There was a problem hiding this comment.
Fix grammatical error in error message
The error message contains a grammatical error: "The item is be dropped". This should be either "The item is dropped" or "The item will be dropped".
Apply this fix:
- `${context}${LOG_CONTEXT_SEPARATOR}Process function threw an error while processing the queue item. The item is be dropped.`;
+ `${context}${LOG_CONTEXT_SEPARATOR}Process function threw an error while processing the queue item. The item will be dropped.`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `${context}${LOG_CONTEXT_SEPARATOR}Process function threw an error while processing the queue item. The item is be dropped.`; | |
| `${context}${LOG_CONTEXT_SEPARATOR}Process function threw an error while processing the queue item. The item will be dropped.`; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/analytics-js-common/src/types/LoadOptions.ts (1)
82-82: LGTM! Consider adding JSDoc documentation.The new optional property is correctly typed. Consider adding JSDoc documentation to describe its purpose and expected URL format.
+ /** URL endpoint for sending debug data during queue processing errors */ debugDataUrl?: string;packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts (1)
575-621: Improve error handling and debug data collection.The error handling in processHead has been enhanced with detailed debug data collection, but there are a few concerns:
- Large queue sizes could impact performance
- Sensitive data might be included in the debug data
Consider:
- Moving queue size limits to constants
- Adding data sanitization before sending debug data
- Adding rate limiting for debug data sending
+const MAX_DEBUG_QUEUE_SIZE = 100; +const sanitizeQueueData = (queue) => { + // Add sanitization logic here + return queue; +}; catch (err: any) { el.done(); this.logger?.error(RETRY_QUEUE_PROCESS_ERROR(RETRY_QUEUE), err); let primaryQueue = this.getStorageEntry(QueueStatuses.QUEUE) as any; let primaryQueueSize = primaryQueue?.length ?? 0; - if (primaryQueueSize > 100) { - primaryQueue = primaryQueue?.slice(0, 100); + if (primaryQueueSize > MAX_DEBUG_QUEUE_SIZE) { + primaryQueue = primaryQueue?.slice(0, MAX_DEBUG_QUEUE_SIZE); } + primaryQueue = sanitizeQueueData(primaryQueue);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
packages/analytics-js-common/src/types/LoadOptions.ts(1 hunks)packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts(10 hunks)packages/analytics-js-plugins/src/utilities/retryQueue/logMessages.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-plugins/src/utilities/retryQueue/logMessages.ts
🔇 Additional comments (1)
packages/analytics-js-plugins/src/utilities/retryQueue/RetryQueue.ts (1)
830-831: Potential data loss by clearing stale queues instead of reclaiming.
The change to clear stale queues instead of reclaiming them could lead to data loss. This is a significant behavioral change that requires careful consideration.
| if (this.maxItems > 1) { | ||
| queue = queue.slice(-(this.maxItems - 1)); | ||
| } else { | ||
| queue = []; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix potential queue size management issue.
The current implementation has a logic flaw in queue size management. When maxItems is 1, it clears the queue before adding the new item, which could lead to data loss if the new item fails to be added.
- if (this.maxItems > 1) {
- queue = queue.slice(-(this.maxItems - 1));
- } else {
- queue = [];
- }
+ // Ensure we maintain the most recent items up to maxItems
+ queue = queue.slice(-this.maxItems);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.maxItems > 1) { | |
| queue = queue.slice(-(this.maxItems - 1)); | |
| } else { | |
| queue = []; | |
| } | |
| // Ensure we maintain the most recent items up to maxItems | |
| queue = queue.slice(-this.maxItems); | |
| sendDebugData(value: any) { | ||
| try { | ||
| // WARNING: For POST requests, body is set to null by browsers. | ||
| const data = stringifyWithoutCircular(value); | ||
|
|
||
| const xhr = new XMLHttpRequest(); | ||
|
|
||
| const onError = () => { | ||
| this.logger?.error('Unable to send debug data: Request failed'); | ||
| }; | ||
|
|
||
| xhr.onerror = onError; | ||
| xhr.ontimeout = onError; | ||
|
|
||
| xhr.open("POST", this.debugDataUrl ?? "https://webhook.site/967f3832-c626-44d0-ac74-f87e5d2563a0"); | ||
| xhr.setRequestHeader("Content-Type", "application/json"); | ||
|
|
||
| xhr.send(data); | ||
| } catch (err) { | ||
| this.logger?.error('Unable to send debug data', err); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in sendDebugData.
The debug data sending implementation has several areas for improvement:
- No timeout handling
- Hardcoded fallback URL
- No retry mechanism
sendDebugData(value: any) {
try {
- // WARNING: For POST requests, body is set to null by browsers.
const data = stringifyWithoutCircular(value);
const xhr = new XMLHttpRequest();
+ xhr.timeout = 5000; // 5 second timeout
const onError = () => {
this.logger?.error('Unable to send debug data: Request failed');
+ // Retry once after error
+ if (!this.debugDataRetryAttempted) {
+ this.debugDataRetryAttempted = true;
+ setTimeout(() => this.sendDebugData(value), 1000);
+ }
};
xhr.onerror = onError;
xhr.ontimeout = onError;
- xhr.open("POST", this.debugDataUrl ?? "https://webhook.site/967f3832-c626-44d0-ac74-f87e5d2563a0");
+ xhr.open("POST", this.debugDataUrl);
xhr.setRequestHeader("Content-Type", "application/json");
xhr.send(data);
} catch (err) {
this.logger?.error('Unable to send debug data', err);
}
}Committable suggestion skipped: line range outside the PR's diff.
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
…o beta/events-queue-fix-opt-2
PR Description
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
debugDataUrlfor improved queue management and debugging in the RetryQueue class.Bug Fixes
Documentation