feat(search-indexer): Wait before doing another request to Elasticsearch in case of 429 status code #16215
Conversation
WalkthroughThe changes in the Changes
Possibly related PRs
Suggested labels
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
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
🧹 Outside diff range and nitpick comments (2)
libs/cms/src/lib/search/contentful.service.ts (2)
486-488: LGTM: Retry mechanism implemented for handling 429 errors.The implementation of a retry mechanism with exponential backoff for handling 429 (Too Many Requests) errors from Elasticsearch is a good approach. This aligns well with the PR objective and helps prevent overwhelming the Elasticsearch service.
Consider extracting delay and retry values to configurable constants.
For better flexibility across different environments, consider extracting the initial delay (500ms) and the number of retries (3) to configurable constants. This would allow easier adjustment of these values without modifying the code.
Here's a suggested implementation:
const INITIAL_RETRY_DELAY_MS = 500; const MAX_RETRY_ATTEMPTS = 3; // In the method: let delay = INITIAL_RETRY_DELAY_MS; let retries = MAX_RETRY_ATTEMPTS;
525-533: Add logging for retry attempts.To improve observability and aid in debugging, consider adding log statements for each retry attempt. This will help track the frequency and duration of retries in production.
Here's a suggested implementation:
if (error?.statusCode === 429 && retries > 0) { logger.warn(`Received 429 error. Retrying in ${delay}ms. Attempts left: ${retries}`); await new Promise((resolve) => { setTimeout(resolve, delay) }) retries -= 1; delay *= 2; continue; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/cms/src/lib/search/contentful.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/cms/src/lib/search/contentful.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/cms/src/lib/search/contentful.service.ts (2)
486-536: Summary: Effective implementation with room for minor improvementsThe changes successfully implement a retry mechanism for handling 429 errors from Elasticsearch, addressing the main objective of the PR. The use of exponential backoff is a good practice. Consider the following improvements:
- Extract retry-related values to configurable constants for better flexibility.
- Add logging for retry attempts to improve observability.
- Verify the impact of the retry mechanism on overall system performance, especially during high load scenarios.
These minor enhancements will further improve the robustness and maintainability of the implementation.
486-536: Verify the impact of retry mechanism on system performance.While the retry mechanism is a good addition, it's important to verify its impact on the overall system performance, especially during high load scenarios.
Run the following script to check for any performance-related logs or metrics:
This will help identify if there are any existing performance monitoring mechanisms in place and if additional instrumentation is needed for the new retry logic.
✅ Verification successful
Performance Monitoring Verified
Existing performance monitoring mechanisms are in place, utilizing metrics and monitoring tools to track system performance. The current setup should adequately capture the impact of the retry mechanism on system performance during high load scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for performance-related logs or metrics # Test: Search for performance-related logs rg --type typescript "performance|latency|duration" # Test: Check for any existing metrics collection rg --type typescript "metrics|monitoring"Length of output: 167
Script:
#!/bin/bash # Description: Check for performance-related logs or metrics # Test: Search for performance-related logs in TypeScript files rg --glob '*.ts' --glob '*.tsx' "performance|latency|duration" # Test: Check for any existing metrics collection in TypeScript files rg --glob '*.ts' --glob '*.tsx' "metrics|monitoring"Length of output: 58260
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16215 +/- ##
==========================================
- Coverage 36.77% 36.77% -0.01%
==========================================
Files 6784 6785 +1
Lines 139840 139849 +9
Branches 39764 39741 -23
==========================================
Hits 51431 51431
- Misses 88409 88418 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 12 Total Test Services: 0 Failed, 11 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5) |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
libs/content-search-toolkit/src/services/elastic.service.ts (1)
117-159: Add unit tests for the new retry mechanismTo ensure the reliability of the retry logic in
bulkRequest, consider adding unit tests that simulate different scenarios, such as successful retries after a 429 error and failures after exceeding the maximum retry count.Would you like assistance in writing these unit tests or opening a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/cms/src/lib/search/contentful.service.ts (2 hunks)
- libs/content-search-toolkit/src/services/elastic.service.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/cms/src/lib/search/contentful.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/content-search-toolkit/src/services/elastic.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
oddsson
added
automerge
kodiakhq
bot
deleted the
feat/search-indexer-backoff-when-too-many-requests
branch
…rch in case of 429 status code (#16215) * Wait in case of 429 status code from ES * Add constants * Add retry logic to ES calls --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Wait before doing another request to Elasticsearch in case of 429 status code
Why
Checklist:
Summary by CodeRabbit