feat(reports): structured chart-holder diagnostics for capture logging - #42816
feat(reports): structured chart-holder diagnostics for capture logging#42816eschutho wants to merge 2 commits into
Conversation
… logging Enriches the report readiness/capture log lines with the per-state holder breakdown proven out in production: - ChartHolderDiagnostics (report_execution.py): structured counts separating capture readiness from semantic success, with TERMINAL_CHART_HOLDER_STATES and the explicit CHART_HOLDER_SEMANTIC_POLICY=deliver_terminal_errors_with_warning. - report_readiness_poll/_ready/_terminal now carry rendered/empty/ error/virtualized/unready counts and semantic_success on both the Playwright and Selenium paths. - report_readiness_tile: per-tile readiness diagnostics during tiled capture, with each holder's strongest observed terminal state aggregated across tiles for the final ready line. - report_semantic_status: a WARNING whenever capture readiness is satisfied but the artifact contains terminal chart errors — delivery is not semantic completeness, and this makes the distinction operator-visible (and alertable) per tile and per capture. - Restores the explicit zero-holder thumbnail diagnostic on the no-context path. Logging-only: no readiness predicate, budget, or delivery behavior changes. Unit tests pin the diagnostics counts and the enriched line formats. Co-Authored-By: Claude <noreply@anthropic.com>
dosubot
Bot
added
the
alert-reports
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #6153b4
Actionable Suggestions - 2
-
tests/unit_tests/utils/webdriver_test.py - 2
- Out-of-bounds array access · Line 923-923
- Out-of-bounds array access · Line 1860-1860
Additional Suggestions - 1
-
superset/utils/screenshot_utils.py - 1
-
Missing holder state recording on zero-holders timeout · Line 472-472The zero-holders timeout path retrieves `holder_states` at line 472 but never calls `_record_visible_holder_states()`, so `observed_holder_states` misses any chart errors or empty states from this timeout. The final diagnostic at line 854 will therefore use `holder_states` directly instead of the per-chart progressive state that `_record_visible_holder_states` builds. Add the missing call alongside the already-invoked `ChartHolderDiagnostics`.
-
Review Details
-
Files reviewed - 6 · Commit Range:
652f61e..652f61e- superset/utils/report_execution.py
- superset/utils/screenshot_utils.py
- superset/utils/webdriver.py
- tests/unit_tests/utils/test_report_execution.py
- tests/unit_tests/utils/test_screenshot_utils.py
- tests/unit_tests/utils/webdriver_test.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| assert warning_call.args[9] == [{"chartId": "42", "state": "nothing_mounted"}] | ||
| assert warning_call.args[9] == 1 # unready holders | ||
| assert warning_call.args[16] == [{"chartId": "42", "state": "nothing_mounted"}] | ||
| assert warning_call.args[17] == [{"chartId": "42", "state": "nothing_mounted"}] |
There was a problem hiding this comment.
Array access warning_call.args[17] will raise IndexError at runtime because the new logger.warning call in webdriver.py only passes 17 arguments (indices 0–16).
Code Review Run #6153b4
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Not a bug: call.args includes the format string at index 0, so a logger.warning with 17 format arguments has indices 1–17 and args[17] is the final states payload. These exact assertions run green in this PR's suite (and have on two other branches carrying the same test).
There was a problem hiding this comment.
The suggestion to avoid this type of review comment is noted. Based on the provided context, the assertion that warning_call.args[17] is out-of-bounds appears to be a false positive, as the test suite confirms the argument index is valid for the current implementation.
| assert failure_args[9] == diagnostics | ||
| assert failure_args[10] == diagnostics | ||
| assert failure_args[16] == diagnostics | ||
| assert failure_args[17] == diagnostics |
There was a problem hiding this comment.
Array access failure_args[17] will raise IndexError at runtime because the logger.warning call only has 17 arguments (indices 0–16).
Code Review Run #6153b4
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Same as the sibling comment: call.args[0] is the format string, so 17 format args occupy indices 1–17 and failure_args[17] is in bounds. The test passes in this PR's CI.
There was a problem hiding this comment.
The suggestion is based on an incorrect assumption about the number of arguments passed to the logger. Since the format string is the first argument, the subsequent 17 arguments are correctly accessed at indices 1 through 17, making the array access valid in this context.
tests/unit_tests/utils/webdriver_test.py
failure_args = mock_logger.warning.call_args.args
assert failure_args[16] == diagnostics
assert failure_args[17] == diagnostics
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42816 +/- ##
==========================================
- Coverage 65.73% 65.72% -0.02%
==========================================
Files 2843 2843
Lines 162670 162723 +53
Branches 37255 37267 +12
==========================================
+ Hits 106932 106945 +13
- Misses 53646 53685 +39
- Partials 2092 2093 +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:
|
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Review of the chart-holder diagnostics change. Posting the one blocking item as an apply-able suggestion, plus the two should-fix items inline. The design is sound and the security model is intact — the new lines add only integer counts, a boolean, and a constant policy string; url and the raw states list were already logged on the base terminal paths, and every format string is a literal with %-args. A few Low nits (semantic_success vs its docstring on virtualized holders, the final report_readiness_ready data-source switch, per-tile/thumbnail log levels, and brittle positional-index test asserts) are left out here — glad to add them if useful.
| else: | ||
| tile_elapsed = time.monotonic() - tile_wait_start | ||
| if report_execution_context: | ||
| holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS) |
There was a problem hiding this comment.
🔴 High — Unguarded per-tile page.evaluate can fail an otherwise-successful capture
This runs on the readiness-succeeded branch with no local guard — the enclosing try only catches PlaywrightTimeout. A generic error here (Playwright "execution context was destroyed" on a navigation race, or a render-process crash on a large dashboard — exactly the case the tiled path exists to serve) propagates to except Exception at line 927, where readiness_timeout is False, so it logs "Tiled screenshot failed" and return None — discarding the tiles already captured and failing a report that would otherwise deliver (allow_partial_fallback=False for reports). The twin evaluate at line 842 is wrapped for exactly this reason (# diagnostics must not discard valid tiles); this one isn't.
| holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS) | |
| try: | |
| holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS) | |
| if not isinstance(holder_states, list): | |
| holder_states = [] | |
| except Exception: # noqa: BLE001 # diagnostics must not discard valid tiles | |
| logger.warning( | |
| "Unable to collect per-tile chart-holder diagnostics%s", | |
| context_suffix, | |
| exc_info=True, | |
| ) | |
| holder_states = [] |
There was a problem hiding this comment.
Confirmed and fixed in 52f16c59bd — this was the best catch of the review. The per-tile evaluate now carries the same guard as the final block, and test_per_tile_diagnostics_failure_does_not_discard_capture pins the regression: the diagnostics evaluate raising a non-timeout error ("Execution context was destroyed") on all three tiles still yields a successful combined capture with a warning per tile.
| ) | ||
| elapsed, remaining = _deadline_values() | ||
| logger.info( | ||
| "report_readiness_tile url=%s expected_holders=%s " |
There was a problem hiding this comment.
🟡 Medium — The enriched fields (the point of this PR) are effectively untested
Every test that drives an enriched log line uses an all-zero-terminal scenario (a single waiting_on_database/nothing_mounted holder), so the rendered/empty/error/virtualized/semantic_success field→slot mapping across the ~9 logger calls is never asserted with non-zero values — swap error_holders and rendered_holders in any emitter and the suite still passes. The cross-tile aggregation, the report_semantic_status branch, and the Selenium enriched lines have zero assertions; notably, no test would have caught the High above. Add one log-path test per emitter driving a mixed set (e.g. 2 rendered + 1 empty + 1 error) that asserts the formatted substrings, using the warning_args[0] % warning_args[1:] pattern already adopted in test_screenshot_utils.py.
There was a problem hiding this comment.
Fair — added in 52f16c59bd: test_tile_line_reports_mixed_holder_states drives 2 rendered + 1 empty + 1 error + 1 unready through the per-tile emitter and asserts every field substring in the rendered message (a swap between any two counters now fails), plus test_final_semantic_status_fires_exactly_once_for_error_holders covers the semantic-status branch. I stopped short of one-per-emitter across all ~9 call sites — the counters all come from the same ChartHolderDiagnostics.from_holder_states, which has its own mixed-state unit tests, so the residual risk is per-emitter arg-ordering; the two highest-traffic emitters (tile + final) are now pinned and the rest share the pattern. Happy to extend to the Selenium pair if you want it.
| f"{remaining:.2f}" if remaining is not None else None, | ||
| context_suffix, | ||
| ) | ||
| if diagnostics.error_holders: |
There was a problem hiding this comment.
🟡 Medium — report_semantic_status WARNING is duplicative and noisy for a delivered condition
This if diagnostics.error_holders: fires the WARNING per-tile here and again in the final block (line 885), plus the webdriver paths — so one persistent error chart spanning 3 tiles emits ~4 WARNINGs per run, multiplied across scheduled runs, for an artifact the policy still delivers. Operators alerting on WARNING from this module get paged for customer-side chart errors, which can mask real faults. Consider emitting the semantic-status line once per capture (final only), and possibly at INFO given delivery still succeeds.
There was a problem hiding this comment.
Agreed and changed in 52f16c59bd, with one nuance: rather than demoting to INFO, the per-tile WARNING is removed entirely (the report_readiness_tile INFO line already carries error_holders per tile, so it was pure duplication) and the final-block WARNING stays a WARNING. Rationale for keeping the final one warm: deliver_terminal_errors_with_warning is the policy — the deliverable went out with broken content, and the single WARNING per capture is the operator-visible half of that contract. One error chart now produces exactly one WARNING per run instead of N+1; test_final_semantic_status_fires_exactly_once_for_error_holders pins it.
…e per capture Review fixes: - Wrap the per-tile diagnostics evaluate on the readiness-succeeded path (same guard as the final block: diagnostics must not discard valid tiles). A navigation race or renderer crash during the diagnostics call no longer fails a capture whose tiles are already good. - Emit report_semantic_status once per capture (final block only); the per-tile report_readiness_tile INFO lines already carry error_holders, and one error chart spanning N tiles was producing N+1 WARNINGs. - Mixed-state tests pin the field-to-slot mapping of the enriched lines (2 rendered + 1 empty + 1 error + 1 unready), the once-per-capture semantic status, and the diagnostics-failure regression. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #61dd56Actionable 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 |
2 participants
SUMMARY
Follow-up to #42624, logging-only. Enriches the report readiness/capture log lines with a structured per-state chart-holder breakdown, proven out in production while diagnosing blank-report incidents (the same investigations that produced #42624).
What's added:
ChartHolderDiagnostics(report_execution.py): structured counts separating capture readiness from semantic success —rendered/empty/error/virtualized/unreadyper capture, withTERMINAL_CHART_HOLDER_STATESand an explicitCHART_HOLDER_SEMANTIC_POLICY = deliver_terminal_errors_with_warning.report_readiness_poll/_ready/_terminallines on both the Playwright and Selenium paths: the existing mounted/ready counts gain the full per-state breakdown andsemantic_success.report_readiness_tile: per-tile readiness diagnostics during tiled capture. Each holder's strongest observed terminal state is aggregated across tiles, so the final ready line reports true whole-dashboard counts even though virtualization means any single viewport only sees a fraction of the charts.report_semantic_status: a WARNING emitted whenever capture readiness is satisfied but the artifact contains terminal chart errors — delivery is not semantic completeness, and this makes that distinction operator-visible and alertable, per tile and per capture.Example of what the tile lines look like in production (52-chart dashboard, 6 tiles — note
virtualized_holdersshowing why per-viewport counts can't be summed naively):No behavior changes: readiness predicates, budgets, and delivery are untouched; this only changes what gets logged. Log call style stays lazy
%-formatting throughout.TESTING INSTRUCTIONS
ChartHolderDiagnosticscounts (terminal-error separation, unready counting, zero-holder non-success) and the enriched line formats on the poll/ready/terminal/tile paths.tests/unit_tests/utils/ + commands/report/ + tasks/scope: 1345 passing; ruff and mypy clean.ADDITIONAL INFORMATION