Improving error subscription log#427
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 26 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe error handling logic in the SNS subscriber's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sns/lib/utils/snsSubscriber.ts`:
- Around line 118-120: The error log in the update-failure branch currently only
calls logger.error('Failed to update subscription') which loses context and the
original SNS error; change that else branch to log the failure message together
with errMessage and the subscription context (e.g., topic ARN / queue name or
the subscription object available in this scope) using the existing logger
(replace the bare message with something like logger.error('Failed to update
subscription', { subscription: <subscriptionContext>, err: errMessage }) or a
formatted string) so the log retains topic/queue and original SNS error details;
update the else branch where logger.error(errMessage) is used to include both
the descriptive message and the contextual fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ee2363d-69d5-440c-b672-162c69e27403
📒 Files selected for processing (1)
packages/sns/lib/utils/snsSubscriber.ts
| logger.error('Failed to update subscription') | ||
| } else { | ||
| logger.error(errMessage) |
There was a problem hiding this comment.
Preserve the subscription context in the update-failure error log.
When the update path fails, the only error-level log is the generic "Failed to update subscription", so logs lose the queue/topic and original SNS error details from errMessage. Keep the contextual message here too.
🪵 Proposed logging fix
- logger.error('Failed to update subscription')
+ logger.error(`${errMessage}. Failed to update subscription`)📝 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.
| logger.error('Failed to update subscription') | |
| } else { | |
| logger.error(errMessage) | |
| logger.error(`${errMessage}. Failed to update subscription`) | |
| } else { | |
| logger.error(errMessage) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sns/lib/utils/snsSubscriber.ts` around lines 118 - 120, The error
log in the update-failure branch currently only calls logger.error('Failed to
update subscription') which loses context and the original SNS error; change
that else branch to log the failure message together with errMessage and the
subscription context (e.g., topic ARN / queue name or the subscription object
available in this scope) using the existing logger (replace the bare message
with something like logger.error('Failed to update subscription', {
subscription: <subscriptionContext>, err: errMessage }) or a formatted string)
so the log retains topic/queue and original SNS error details; update the else
branch where logger.error(errMessage) is used to include both the descriptive
message and the contextual fields.
There was a problem hiding this comment.
@kibertoad, agreed! Without the fix, when the update path fails, the log entry "Failed to update subscription" provides no actionable context — operators won't know which queue or topic was affected, or what the original SNS error was.
The one-line fix pulls the already-constructed errMessage (which includes queue name, topic name, and the original error) into the failure log:
- logger.error('Failed to update subscription')
+ logger.error(`${errMessage}. Failed to update subscription`)This ensures the log entry for the update-failure case is as informative as the one in the else branch added in this PR.
There was a problem hiding this comment.
@kibertoad In this flow we will have:
warn -> Error while creating subscription for queue "XXX", topic "YYYY": ${errMessage}. Trying to update subscription
error -> Failed to update subscription
I feel it is meaningful enough and adding errMessage again in the error can make it confusing, what do you think?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
3 participants
Summary by CodeRabbit
New Features
Bug Fixes