Add timeout option for telegram notifier - #5127
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesTelegram notifier timeout
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🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
📥 Commits
Reviewing files that changed from the base of the PR and between 19e792f and c627cec84bda5cf54b29c05ae8eed08c8480e775.
📒 Files selected for processing (4)
config/notifiers.godocs/configuration.mdnotify/telegram/telegram.gonotify/telegram/telegram_test.go
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
📥 Commits
Reviewing files that changed from the base of the PR and between c627cec84bda5cf54b29c05ae8eed08c8480e775 and 881b187fe9e0ce10bc714457a664ff012d44a79a.
📒 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
881b187 to
11013db
Compare
11013db to
730f944
Compare
|
@siavashs @SoloJacobs PTAL. |
|
Please resolve the conflicts. |
0789a1e to
9ac3c69
Compare
Done. |
|
Note The previously reviewed commits are no longer reachable (likely due to a force-push or rebase), so CodeRabbit is performing a full review instead of an incremental one. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@notify/telegram/config.go`:
- Around line 49-52: Reject negative Timeout values during both configuration
validation and telegram notifier construction, treating only zero as the
documented “no timeout” value. Update the validation logic and telegram.New so
Timeout values below zero return an appropriate configuration error instead of
being silently accepted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1a036e1a-f420-4e65-b076-5106b287e273
📒 Files selected for processing (4)
docs/configuration.mdnotify/telegram/config.gonotify/telegram/telegram.gonotify/telegram/telegram_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/configuration.md
- notify/telegram/telegram.go
- notify/telegram/telegram_test.go
f22ed9b to
c8bc78d
Compare
|
@siavashs @SoloJacobs, could you please take a look at this PR? |
06f2e14 to
aff09ed
Compare
|
Thanks for the links. I tried to use the same pattern here. It does not work for Telegram. The Telegram notifier sends messages through the telebot library. Telebot does not accept a context from the caller. Its Thus |
|
Ok, very good analysis, thanks. Do you know whether the error is displayed somewhat reasonably? I think this is the main thing, where the inconsistency matters. Perhaps you could add it to the test? |
SoloJacobs
left a comment
SoloJacobs
left a comment
There was a problem hiding this comment.
Another thing I noticed: I think the error will leak the secret telegram url. We need to handle that too. Then the PR is ready to go.
aff09ed to
7217f41
Compare
Signed-off-by: Ali Afsharzadeh <afsharzadeh8@gmail.com>
Signed-off-by: Ali Afsharzadeh <afsharzadeh8@gmail.com>
7217f41 to
c0293d8
Compare
Signed-off-by: Ali Afsharzadeh <afsharzadeh8@gmail.com>
already done in #5254 |
Done. |
|
Thanks and nice 👍 |
4 participants
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?