aminghadersohi commented on code in PR #43784:
URL: https://github.com/apache/superset/pull/43784#discussion_r3920793554
##########
superset/utils/screenshot_utils.py:
##########
@@ -805,28 +898,179 @@ def _raise_if_budget_exhausted() -> None:
"height": clip_height,
}
+ holder_count_failed = False
+ try:
+ holder_counts = page.evaluate(
+ CONTENTFUL_CHART_HOLDERS_IN_CLIP_JS,
+ {"top": clip_y, "bottom": clip_y + clip_height},
+ )
+ if not (
+ isinstance(holder_counts, dict)
+ and isinstance(holder_counts.get("total"), int)
+ and isinstance(holder_counts.get("contentful"), int)
+ ):
+ raise ValueError("Unexpected chart-holder count result")
+ total_chart_holders = holder_counts["total"]
+ contentful_chart_holders = holder_counts["contentful"]
+ except Exception: # noqa: BLE001
+ logger.warning(
+ "Unable to count chart holders intersecting tile %s/%s%s",
+ i + 1,
+ num_tiles,
+ context_suffix,
+ exc_info=True,
+ )
+ holder_count_failed = True
+ total_chart_holders = 0
+ contentful_chart_holders = 0
+
+ if (
+ total_chart_holders == 0
+ and report_execution_context
+ and report_execution_context.expected_chart_count
+ ):
+ logger.warning(
+ "report_capture_no_chart_holders tile=%s/%s "
+ "expected_holders=%s holder_count_failed=%s%s",
+ i + 1,
+ num_tiles,
+ report_execution_context.expected_chart_count,
+ holder_count_failed,
+ context_suffix,
+ )
+
# Take screenshot with clipping to capture only this tile's content
- capture_timeout = (
- _timeout_seconds(
- "screenshot_capture",
- reserve_seconds=(
- report_execution_context.post_capture_reserve_seconds
- if report_execution_context
- else 0.0
- ),
+ tile_screenshot: bytes | None = None
+ for capture_attempt in range(1,
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS + 1):
+ capture_timeout = (
+ _timeout_seconds(
+ "screenshot_capture",
+
requested_seconds=TILED_SCREENSHOT_CAPTURE_TIMEOUT_SECONDS,
+ reserve_seconds=(
+
report_execution_context.post_capture_reserve_seconds
+ if report_execution_context
+ else 0.0
+ ),
+ )
+ if report_execution_context or task_budget is not None
+ else None
)
- if report_execution_context or task_budget is not None
- else None
- )
- tile_screenshot = page.screenshot(
- type="png",
- clip=clip,
- **(
- {"timeout": capture_timeout * 1000}
- if capture_timeout is not None
- else {}
- ),
- )
+ capture_started_at = time.monotonic()
+ try:
+ candidate = page.screenshot(
+ type="png",
+ clip=clip,
+ **(
+ {"timeout": capture_timeout * 1000}
+ if capture_timeout is not None
+ else {}
+ ),
+ )
+ except PlaywrightTimeout as ex:
+ capture_elapsed = time.monotonic() - capture_started_at
+ logger.warning(
+ "report_capture_tile_timeout tile=%s/%s attempt=%s/%s "
+ "capture_elapsed_seconds=%.2f%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS,
+ capture_elapsed,
+ context_suffix,
+ )
+ if capture_attempt ==
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS:
+ raise ScreenshotCaptureTimeoutError(
+ f"Chromium timed out capturing tile {i +
1}/{num_tiles} "
+ f"after {capture_attempt} attempts"
+ ) from ex
+ else:
+ capture_elapsed = time.monotonic() - capture_started_at
+ is_uniform, dominant_ratio =
is_screenshot_nearly_uniform(candidate)
+ is_blank = is_uniform and (
+ contentful_chart_holders > 0 or holder_count_failed
+ )
+ logger.debug(
+ "Captured tile %s/%s attempt %s/%s in %.2fs "
+ "(contentful_chart_holders=%s
dominant_pixel_ratio=%.5f)%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS,
+ capture_elapsed,
+ contentful_chart_holders,
+ dominant_ratio,
+ context_suffix,
+ )
+ if not is_blank:
+ tile_screenshot = candidate
+ break
+ blank_tile_retries += 1
+ logger.warning(
+ "report_capture_blank_tile tile=%s/%s attempt=%s/%s "
+ "capture_elapsed_seconds=%.2f
contentful_chart_holders=%s "
+ "dominant_pixel_ratio=%.5f%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS,
+ capture_elapsed,
+ contentful_chart_holders,
+ dominant_ratio,
+ context_suffix,
+ )
+ if capture_attempt ==
TILED_SCREENSHOT_MAX_CAPTURE_ATTEMPTS:
+ tile_screenshot = candidate
+ logger.warning(
+ "report_capture_uniform_tile_retained tile=%s/%s "
+ "attempts=%s contentful_chart_holders=%s "
+ "dominant_pixel_ratio=%.5f%s",
+ i + 1,
+ num_tiles,
+ capture_attempt,
+ contentful_chart_holders,
+ dominant_ratio,
+ context_suffix,
+ )
+ break
+
+ _raise_if_budget_exhausted()
+ try:
+ page.bring_to_front()
+ page.evaluate(
+ """() => {
+ window.scrollBy(0, 1);
+ window.scrollBy(0, -1);
+ window.__supersetRepaintComplete = false;
+ requestAnimationFrame(() =>
requestAnimationFrame(() => {
+ window.__supersetRepaintComplete = true;
+ }));
+ }"""
+ )
+ repaint_timeout = _timeout_seconds(
+ "screenshot_repaint",
+ requested_seconds=5.0,
+ reserve_seconds=(
+
report_execution_context.post_capture_reserve_seconds
+ if report_execution_context
+ else 0.0
+ ),
+ )
+ page.wait_for_function(
+ "() => window.__supersetRepaintComplete === true",
+ timeout=repaint_timeout * 1000,
+ )
+ except Exception: # noqa: BLE001
Review Comment:
**NIT, non-blocking.** `_timeout_seconds("screenshot_repaint", ...)` sits
inside this `try`, and it can raise `ReportExecutionBudgetExceededError` /
`TiledScreenshotBudgetExceededError` — which `except Exception` then swallows
and mislabels as `report_capture_repaint_timeout`.
`_raise_if_budget_exhausted()` on the line above only catches raw exhaustion
(`remaining <= 0`). Reserve-adjusted exhaustion still gets through: with ~20s
left and `post_capture_reserve_seconds=60`, the guard passes and
`_timeout_seconds` raises inside the `try`.
**It self-corrects**, which is why this is only a nit — the repaint block
runs only when another loop iteration will follow (every `capture_attempt ==
MAX` path either `break`s or raises), and the next iteration opens with
`_timeout_seconds("screenshot_capture", ...)` *outside* any `try`, so the same
budget error propagates one iteration later. Nothing is lost.
The cost is diagnostic: a budget exhaustion is logged as a repaint failure,
which is the wrong thing to look at when triaging a timed-out report. Hoisting
the `_timeout_seconds` call above the `try`, or re-raising the two budget
types, would keep the log honest:
```python
except (ReportExecutionBudgetExceededError,
TiledScreenshotBudgetExceededError):
raise
except Exception: # noqa: BLE001
logger.warning("report_capture_repaint_timeout ...")
```
--
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]
