rebenitez1802 commented on code in PR #42816:
URL: https://github.com/apache/superset/pull/42816#discussion_r3729340769
##########
superset/utils/screenshot_utils.py:
##########
@@ -610,6 +661,60 @@ def _raise_if_budget_exhausted() -> None:
raise
else:
tile_elapsed = time.monotonic() - tile_wait_start
+ if report_execution_context:
+ holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ holder_states
+ )
+ _record_visible_holder_states(
+ holder_states,
+ tile_number=i + 1,
+ )
+ elapsed, remaining = _deadline_values()
+ logger.info(
+ "report_readiness_tile url=%s expected_holders=%s "
Review 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`.
##########
superset/utils/screenshot_utils.py:
##########
@@ -610,6 +661,60 @@ def _raise_if_budget_exhausted() -> None:
raise
else:
tile_elapsed = time.monotonic() - tile_wait_start
+ if report_execution_context:
+ holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
Review 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.
```suggestion
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 = []
```
##########
superset/utils/screenshot_utils.py:
##########
@@ -610,6 +661,60 @@ def _raise_if_budget_exhausted() -> None:
raise
else:
tile_elapsed = time.monotonic() - tile_wait_start
+ if report_execution_context:
+ holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ holder_states
+ )
+ _record_visible_holder_states(
+ holder_states,
+ tile_number=i + 1,
+ )
+ elapsed, remaining = _deadline_values()
+ logger.info(
+ "report_readiness_tile url=%s expected_holders=%s "
+ "mounted_holders=%s ready_holders=%s
rendered_holders=%s "
+ "empty_holders=%s error_holders=%s "
+ "virtualized_holders=%s unready_holders=%s "
+ "semantic_success=%s semantic_policy=%s tile=%s/%s "
+ "tile_elapsed_seconds=%.2f elapsed_seconds=%.2f "
+ "remaining_seconds=%s%s",
+ url,
+ report_execution_context.expected_chart_count,
+ diagnostics.mounted_holders,
+ diagnostics.ready_holders,
+ diagnostics.rendered_holders,
+ diagnostics.empty_holders,
+ diagnostics.error_holders,
+ diagnostics.virtualized_holders,
+ diagnostics.unready_holders,
+ diagnostics.semantic_success,
+ CHART_HOLDER_SEMANTIC_POLICY,
+ i + 1,
+ num_tiles,
+ tile_elapsed,
+ elapsed,
+ f"{remaining:.2f}" if remaining is not None else None,
+ context_suffix,
+ )
+ if diagnostics.error_holders:
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
