fix(reports): downgrade chart-container timeout log level and fix tiling veto on unknown height#42153
fix(reports): downgrade chart-container timeout log level and fix tiling veto on unknown height#42153eschutho wants to merge 1 commit into
Conversation
…ing veto on unknown height The chart-container locate-wait timeout was logged via logger.exception (ERROR + traceback), inconsistent with the WARNING level already used for the sibling spinner-wait timeouts in the same method. Bring it in line and add rendered/total chart-container progress to the message; the timeout still re-raises, so report generation already failed loudly here. Also fix a real bug in the tiled-screenshot heuristic: when the DOM query for a dashboard's height comes back empty (returns 0), the code treated that as "small enough for a single tile", vetoing tiling even when chart_count alone showed the dashboard was large. That could let a large dashboard skip the scroll-driven tiling pass and ship with below-the-fold charts that never rendered. An unknown height no longer overrides a chart_count-based large-dashboard signal, and the warning is now only logged when it's actually consequential (downgraded to debug otherwise). Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #6b7e95Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42153 +/- ##
==========================================
- Coverage 65.17% 65.06% -0.12%
==========================================
Files 2753 2758 +5
Lines 154657 155205 +548
Branches 35484 35560 +76
==========================================
+ Hits 100801 100982 +181
- Misses 51940 52308 +368
+ Partials 1916 1915 -1
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:
|
Summary
WebDriverPlaywright.get_screenshot()waits for.chart-containerelements to render before deciding whether to use tiled or standard screenshot capture for a dashboard. Two issues in that logic:ERROR(logger.exception), inconsistent with theWARNINGlevel already used for the sibling spinner-wait timeouts a few lines below in the same method.0(the query found no matching element, or the layout hadn't settled yet), the code treated that the same as "the dashboard is small enough for a single tile" — which could veto tiled capture even when the chart count alone indicated a large dashboard. That could let a large, virtualized dashboard skip the scroll-driven tiling pass and ship with below-the-fold charts that never rendered.Fix
WARNING, matching the convention already used for the other Playwright locate-wait timeouts in this method (customer-side chart loading is usually just slow, not a Superset bug). The screenshot still fails — the exception is still re-raised — only the log level and message changed; the message now also reports how many chart containers rendered before the timeout, for easier debugging.0) is now treated as "unknown" rather than "small". Whether tiling is attempted is decided by the chart-count signal alone when height can't be measured, instead of chart count being silently overridden. The corresponding log line is only emitted atWARNINGwhen it's actually consequential (i.e. the dashboard looks large by chart count but tiling would otherwise have been skipped); it'sDEBUGfor the common, benign case of a small dashboard with an unmeasured height.Not changed
Testing
Added to
tests/unit_tests/utils/webdriver_test.py:WARNINGwith rendered/total progress and still raises (never logs vialogger.exception).DEBUG, notWARNING, for the small-dashboard case where that's the expected/benign path.Full relevant test suite passing;
ruff check,ruff format, andmypyclean on both changed files.Additional information