notify/telegram: classify failure reasons from Telegram Bot API errors#5375
notify/telegram: classify failure reasons from Telegram Bot API errors#5375softho0n wants to merge 2 commits into
Conversation
231702b to
ab85926
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Telegram notifier now wraps message-send errors using a new ChangesTelegram failure reason wrapping
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Notifier
participant TelegramAPI
participant wrapWithFailureReason
Notifier->>TelegramAPI: Send message
TelegramAPI-->>Notifier: error response
Notifier->>wrapWithFailureReason: wrap(err)
wrapWithFailureReason-->>Notifier: ErrorWithReason(reason)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
notify/telegram/telegram_test.go (1)
217-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a fallback-path test case.
All cases exercise the FloodError/
*telebot.Errorbranches; none cover the default path inwrapWithFailureReasonwhere the error is returned unchanged (unstructured error). Adding one would close coverage on that branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@notify/telegram/telegram_test.go` around lines 217 - 290, Add a fallback-path test to TestTelegramNotifyFailureReason that exercises the default branch in wrapWithFailureReason, where a non-structured/unrecognized error is returned unchanged. Use the existing Telegram notifier setup in New and Notify, but have the test server return or trigger an error that is neither FloodError nor *telebot.Error, then assert the returned error does not wrap a notify.ErrorWithReason and is passed through as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@notify/telegram/telegram_test.go`:
- Around line 217-290: Add a fallback-path test to
TestTelegramNotifyFailureReason that exercises the default branch in
wrapWithFailureReason, where a non-structured/unrecognized error is returned
unchanged. Use the existing Telegram notifier setup in New and Notify, but have
the test server return or trigger an error that is neither FloodError nor
*telebot.Error, then assert the returned error does not wrap a
notify.ErrorWithReason and is passed through as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eebe29b8-5a26-404d-a8f0-969662677ef2
📒 Files selected for processing (3)
CHANGELOG.mdnotify/telegram/telegram.gonotify/telegram/telegram_test.go
aab51c4 to
a39291e
Compare
The telegram integration returned errors from the telebot client without classification, so all failed notifications were counted under the default reason in alertmanager_notifications_failed_total. Classify structured telebot errors before returning them: flood-control errors map to rateLimited, and API errors with a known description map via their error code (401/403 to authError, 429 to rateLimited, other 4xx to clientError, 5xx to serverError). Errors that telebot does not surface in a structured form are returned unchanged and keep the default reason. Part of prometheus#3231 Signed-off-by: Seunghun Shin <18shshin@gmail.com>
a39291e to
39f37c7
Compare
Pull Request Checklist
TestTelegramNotifyFailureReason)Which user-facing changes does this PR introduce?
Summary
The telegram notifier returned
client.Send()errors unclassified, so every failure was counted asreason="other"inalertmanager_notifications_failed_total.Telegram goes through the telebot client, so there is no raw
resp.StatusCodeto classify at the transport level. Instead this classifies the structured errors telebot returns:telebot.FloodError(429 withretry_after) →rateLimited*telebot.Error(carries the Telegramerror_code, which mirrors HTTP status) →notify.GetFailureReasonFromStatusCodeSame approach as discord/webex (#5332). Retry behavior is unchanged (always
retry=trueon error, as before).Tests
TestTelegramNotifyFailureReason: 401/403/400/500/429 against an httptest Bot API stub, assertingErrorWithReasonclassification (same pattern as notify: classify Discord and Webex failure reasons #5324)make lint: 0 issues