feat(alerts): Include screenshots in alert/report failure emails #35652
+392
−25
Conversation
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
When an alert or report fails, capture a screenshot of the current state and include it in the error notification email sent to owners. This helps admins/owners quickly identify issues without needing to navigate to the dashboard or chart. Key changes: - Modified send_error() to attempt screenshot capture before sending error notifications - Updated email notification templates to include inline screenshot images - Added comprehensive tests for error emails with and without screenshots - Implemented best-effort approach: screenshots are captured if possible, but failures don't prevent error notifications from being sent 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Inefficient string concatenation in loop ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/reports/notifications/email.py | ✅ |
| superset/commands/report/execute.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
Comment on lines
126
to
129
| for msgid in images.keys(): | ||
| img_tag_str += ( | ||
| f'<div class="image"><img width="1000" src="cid:{msgid}"></div>' | ||
| ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Modify Selenium webdriver to capture screenshots of whatever state exists even when timeouts occur. This is especially useful for error notifications where we want to show admins the state of the page at the time of failure, even if the page didn't fully load. Changes: - TimeoutExceptions no longer immediately raise - instead we try to capture the current page state - If the target element isn't found, fall back to full page screenshot - Improved logging to distinguish between successful loads and partial captures This only affects Selenium (not Playwright) as Selenium maintains browser state after timeouts while Playwright does not. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Don't retry screenshot capture in send_error() since we may not capture the same state. Selenium webdriver now handles partial screenshot capture on timeout automatically. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When Selenium timeouts occur during screenshot capture, we now properly raise an error with the partial screenshot attached. This ensures: - Timeout failures are logged as errors in execution logs - Error notifications are sent to owners - Partial screenshots are included in error emails Changes: - ReportScheduleScreenshotTimeout now accepts and stores screenshot data - Selenium webdriver tracks timeouts and raises exception with screenshots - Error handlers extract screenshots from timeout exceptions - send_error() accepts screenshots parameter to include in notifications 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add detailed diagnostic logging when chart containers fail to render: - Log number of chart/grid/loading elements found - Extract and log chart identifiers for debugging - Include DOM preview (first 2000 chars) to help diagnose issues This telemetry helps identify which specific charts are causing timeouts and what state the page is in when timeouts occur. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Address korbit-ai review feedback - replace O(n²) string concatenation with O(n) list append and join pattern for better performance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…rror emails When a dashboard/chart times out during screenshot capture: - Display how long the page was loading before timeout - Show a screenshot of the partial rendering - Provide actionable performance recommendations: - Optimize queries (indexes, reduce data, simplify) - Enable and utilize cached data - Break complex dashboards into smaller views - Review database performance The elapsed time is tracked from the start of screenshot capture and passed through the exception chain to the error notification. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Improve code readability by breaking long HTML strings in error template across multiple lines. HTML rendering is unaffected by whitespace in the source strings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Include elapsed time in all internal timeout warning logs to help with debugging performance issues. This complements the timing info added to error emails. Changes: - Element location timeout: Shows elapsed time when element not found - Chart container timeout: Shows elapsed time in diagnostic logs - Chart loading timeout: Shows elapsed time when charts don't finish loading Example log: "Selenium timed out waiting for charts to load at url http://example.com/dashboard/1 after 15.32 seconds" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update test_get_screenshot_logs_chart_timeout_details to mock time() function and add required config keys. The test now properly validates that elapsed time is included in timeout warning logs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
… to exception - Move time import to module level in webdriver.py for proper test mocking - Fix test mock setup for screenshot_as_png property - Update test assertions to check lazy-formatted log messages with chart IDs - Add working_timeout parameter to ReportSchedulePreviousWorkingError exception - Pass working_timeout value when raising the exception 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
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.
SUMMARY
When an alert or report fails, this PR adds the ability to capture a screenshot of the current state and include it in the error notification email sent to owners. This helps admins/owners quickly identify issues without needing to navigate to the dashboard or chart.
Key changes:
send_error()insuperset/commands/report/execute.pyto attempt screenshot capture before sending error notificationssuperset/reports/notifications/email.pyto include inline screenshot imagesBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Error emails only contained text describing the error
After: Error emails include a screenshot showing the state of the dashboard/chart at the time of failure (when screenshot capture is successful)
TESTING INSTRUCTIONS
pytest tests/unit_tests/reports/notifications/email_tests.py -vADDITIONAL INFORMATION
🤖 Generated with Claude Code