fix(reports): make webhook notification request timeout configurable - #43018
Conversation
Both requests.post() calls in webhook notifications hardcoded timeout=60, unlike the SMTP/CSV/Selenium timeouts made configurable in #41250. Add ALERT_REPORTS_WEBHOOK_TIMEOUT (default 60) and wire it into both calls so operators can tune it consistently with the other network timeouts. Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #6d1052Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
alert-reports
github-actions
Bot
added
doc
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43018 +/- ##
==========================================
- Coverage 66.59% 66.54% -0.05%
==========================================
Files 2863 2864 +1
Lines 161681 161884 +203
Branches 37249 37298 +49
==========================================
+ Hits 107678 107733 +55
- Misses 51961 52105 +144
- Partials 2042 2046 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR makes webhook notification HTTP request timeouts configurable for alert/report delivery, aligning the webhook path with prior work that added network timeouts to prevent report schedules from getting stuck in WORKING.
Changes:
- Adds new config
ALERT_REPORTS_WEBHOOK_TIMEOUT(default60) and uses it for webhookrequests.post()calls. - Adds a unit test to assert the configured timeout is forwarded to
requests.post. - Documents the new configuration option in the alerts/reports admin docs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
superset/reports/notifications/webhook.py |
Reads ALERT_REPORTS_WEBHOOK_TIMEOUT from config and forwards it to both webhook request paths. |
tests/unit_tests/reports/notifications/webhook_tests.py |
Extends existing mocks and adds a test asserting the configured timeout is passed to requests.post. |
superset/config.py |
Introduces ALERT_REPORTS_WEBHOOK_TIMEOUT with default value and explanatory comment. |
docs/admin_docs/configuration/alerts-reports.mdx |
Documents how to configure the webhook request timeout and notes disabling via None. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The documentation in Suggested update for the documentation: Retries are bounded to roughly 120 seconds of cumulative wall-clock time (worst case ~210 seconds, because the bound is checked against the time elapsed before each attempt, so the final request can begin just under the limit and still run its full configured timeout), after which the delivery is abandoned.docs/admin_docs/configuration/alerts-reports.mdx |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #219201Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
LGTM. Default keeps the current 60s behavior and the SSRF/HTTPS validation is untouched; the timeout just becomes configurable. CI green.
Follow-up to #41250. That PR made SMTP, CSV/dataframe, and Selenium network timeouts configurable so a hung network call can't wedge a report schedule in
WORKINGforever. It missed the webhook notification path: bothrequests.post()calls insuperset/reports/notifications/webhook.pystill hardcodedtimeout=60.SUMMARY
Adds
ALERT_REPORTS_WEBHOOK_TIMEOUT(default60, matching the prior hardcoded value) and wires it into both webhookrequests.post()calls, consistent with the other timeouts #41250 introduced. Also documents the new setting alongside the existingALERT_REPORTS_WEBHOOK_HTTPS_ONLYdocs.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend config change).
TESTING INSTRUCTIONS
pytest tests/unit_tests/reports/notifications/webhook_tests.pytest_send_forwards_configured_timeoutasserts the configured value is passed through torequests.post.ADDITIONAL INFORMATION
🤖 Generated with Claude Code