Add timeout option for telegram notifier#5127
Open
guoard wants to merge 1 commit intoprometheus:mainfrom
Open
Conversation
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant Notifier as Notifier (New / Notify)
participant HTTP as HTTP Client (with Timeout)
participant API as Telegram API (test server)
Notifier->>HTTP: create client (apply conf.Timeout if >0)
Notifier->>HTTP: POST /bot<token>/sendMessage (alert)
HTTP->>API: request
API-->>HTTP: delayed response (latency)
alt latency < Timeout
HTTP-->>Notifier: 200 OK (forward success)
else latency >= Timeout
HTTP--x API: timeout occurs (client cancels)
HTTP-->>Notifier: timeout error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/configuration.md`:
- Around line 1805-1809: Update the documentation for the timeout setting to
remove or fix the misleading note about group_interval: clarify that the
Telegram receiver applies timeout directly to the HTTP client (symbol: timeout)
and does not compare or enforce it against group_interval, so setting timeout
higher than group_interval does not get automatically capped; rephrase the
sentence to state this behavior explicitly and ensure the note references the
Telegram receiver behavior rather than implying an enforced relationship with
group_interval.
In `@notify/telegram/telegram_test.go`:
- Around line 227-236: The "success" test case in telegram_test.go has a tight
timing window (latency: 100ms vs timeout: 120ms) which can be flaky on CI;
update the test table entries (the cases with name "success" and "timeout" that
set the latency and timeout fields) to increase the headroom—e.g., raise the
"success" timeout to a safer value (200ms) or reduce the simulated latency—so
the success case has a comfortable margin while keeping the "timeout" case still
below the latency.
🪄 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: eebc574c-57db-4a14-938a-6138ed179007
📒 Files selected for processing (4)
config/notifiers.godocs/configuration.mdnotify/telegram/telegram.gonotify/telegram/telegram_test.go
guoard
force-pushed
the
telegram_timeout
branch
from
c627cec to
881b187
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
notify/telegram/telegram_test.go (1)
240-276: Test logic is sound; consider verifying error type for the timeout case.The test structure is clean: proper server lifecycle management, correct context setup for testing HTTP client timeout (not context deadline), and appropriate use of table-driven tests.
One optional enhancement: the timeout case could verify the error is specifically a timeout rather than any error, which would catch false positives if the notifier fails for unrelated reasons.
💡 Optional: Verify timeout error type
- _, err = notifier.Notify(ctx, alert) - require.Equal(t, tc.wantErr, err != nil) + _, err = notifier.Notify(ctx, alert) + if tc.wantErr { + require.Error(t, err) + var netErr net.Error + require.ErrorAs(t, err, &netErr) + require.True(t, netErr.Timeout(), "expected timeout error") + } else { + require.NoError(t, err) + }Would require adding
"net"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notify/telegram/telegram_test.go` around lines 240 - 276, Update the timeout test case to assert the error is specifically a timeout: after calling notifier.Notify(ctx, alert) check that err is non-nil and then assert it satisfies net.Error && err.Timeout() (or use errors.Is for context.DeadlineExceeded if appropriate); modify the table-case that sets tc.timeout/wantErr to also expect a timeout and add the "net" import to the test file; locate this change around the test loop that constructs cfg, calls New(...) and notifier.Notify(...) in notify/telegram/telegram_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@notify/telegram/telegram_test.go`:
- Around line 240-276: Update the timeout test case to assert the error is
specifically a timeout: after calling notifier.Notify(ctx, alert) check that err
is non-nil and then assert it satisfies net.Error && err.Timeout() (or use
errors.Is for context.DeadlineExceeded if appropriate); modify the table-case
that sets tc.timeout/wantErr to also expect a timeout and add the "net" import
to the test file; locate this change around the test loop that constructs cfg,
calls New(...) and notifier.Notify(...) in notify/telegram/telegram_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6083ffc-5fd0-4014-84a7-4ec6e119c915
📒 Files selected for processing (4)
config/notifiers.godocs/configuration.mdnotify/telegram/telegram.gonotify/telegram/telegram_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/configuration.md
🚧 Files skipped from review as they are similar to previous changes (2)
- notify/telegram/telegram.go
- config/notifiers.go
guoard
force-pushed
the
telegram_timeout
branch
from
881b187 to
11013db
Compare
Signed-off-by: Ali Afsharzadeh <afsharzadeh8@gmail.com>
guoard
force-pushed
the
telegram_timeout
branch
from
11013db to
730f944
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change introduces a
Timeoutoption for the Telegram notifier. When set, the HTTP client used by the notifier will time out if a request to the Telegram API takes longer than the configured duration.Pull Request Checklist
Please check all the applicable boxes.
benchstatto compare benchmarksWhich user-facing changes does this PR introduce?
Summary by CodeRabbit
New Features
Documentation
Tests