feat(low-code): improve logging on async retriever errors #307
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThe pull request introduces a modification in the Changes
The modification maintains the existing exception handling logic while improving error visibility through more comprehensive logging. The change does not alter the fundamental control flow or job orchestration process. Would you like to explore more about how this logging enhancement could impact debugging processes? Wdyt? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (8)
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: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/async_job/job_orchestrator.py (1)
440-442: The logging addition looks good! Would you consider enhancing it further?The new warning log for caught exceptions improves visibility into async retriever errors, which aligns well with the PR's objective. However, we could make it even more helpful for debugging purposes.
What do you think about including the traceback information? Here's a suggestion, wdyt?
- LOGGER.warning( - f"Caught exception that stops the processing of the jobs: {exception}" - ) + LOGGER.warning( + f"Caught exception that stops the processing of the jobs: {exception}. Traceback: {traceback.format_exc()}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/async_job/job_orchestrator.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
|
@coderabbitai I'm good with your suggestion here. Can you make the change? |
|
@maxi297 I'll help you implement the suggested change to include traceback information in the warning log. Here's the code: LOGGER.warning(
f"Caught exception that stops the processing of the jobs: {exception}. Traceback: {traceback.format_exc()}"
)This change will provide better visibility into async retriever errors by including the full stack trace in the log message. Would you like me to make this change? 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 2146 Script: Length of output: 21056 ✅ Actions performedReview triggered.
|
|
@coderabbitai yes, please commit the change |
|
Tip For best results, initiate chat on the files or code changes. @maxi297 I apologize, but I am not able to directly commit changes to the repository. However, I can provide you with the code that you can use to make the change: LOGGER.warning(
f"Caught exception that stops the processing of the jobs: {exception}. Traceback: {traceback.format_exc()}"
)You can copy this code and implement it in the |
|
Is the concern that we log too much? The user was having an issue and was not seeing the issue with the current logging solution. |
|
No, i believe we could have a specific case to catch first than catch the general Exception so we start having the errors base we might face in the future, instead of just Expection. This is basically a part of the re-factoring so no blocking you and will do this in my PR |
See this Slack thread for more information
Summary by CodeRabbit