Skip to content

fix(reports): anchor tiled budget clock at overall screenshot start - #42661

Closed
eschutho wants to merge 1 commit into
masterfrom
fix-tiled-budget-clock-anchor
Closed

fix(reports): anchor tiled budget clock at overall screenshot start#42661
eschutho wants to merge 1 commit into
masterfrom
fix-tiled-budget-clock-anchor

Conversation

@eschutho

Copy link
Copy Markdown
Member

SUMMARY

Follow-up to #42118, implementing @rebenitez1802's non-blocking review suggestion left at merge time (#42118 (comment) — "tiled budget clock resets instead of accounting for pre-capture time").

The tiled wait budget is derived from the running Celery task's own time limit, but elapsed time was measured from a clock started inside take_tiled_screenshot() — so page.goto (bounded 60s), the headstart sleep, element.wait_for, and dimension probing all ran before the clock started. The tile loop effectively got a fresh full budget on top of task time already spent. In the common case the 20%/300s cleanup margin absorbs the overhang (and a soft-limit overrun still surfaces as a clean SoftTimeLimitExceeded), but on hard-limit-only tasks the uncounted pre-capture time plus a full budget of tiled waits can still crest the hard limit — the SIGKILL #42118 exists to prevent.

The non-tiled readiness wait already avoids this: #42427 threads screenshot_started_at from the top of get_screenshot into _wait_for_charts_ready and subtracts already-elapsed time. This PR applies exactly the same pattern to the tiled call — the three coordinated changes from the review suggestion:

  1. take_tiled_screenshot() accepts screenshot_started_at: float | None = None and anchors the budget clock on it (falls back to "now" when omitted, preserving the existing API for any other caller);
  2. WebDriverPlaywright.get_screenshot() passes its existing screenshot_started_at into the tiled call;
  3. both capture paths now share one clock.

TESTING INSTRUCTIONS

pytest -q tests/unit_tests/utils/   # 732 passed
ruff check / ruff format --check    # clean

New tests: with screenshot_started_at provided, 900s of pre-capture elapsed time reduces the first tile's capped wait to the true remaining budget (100s of a 1000s budget, not the full load_wait); with it omitted, the local-clock default behaves exactly as before. The existing tiled call-site assertion now pins the new argument being forwarded.

ADDITIONAL INFORMATION

Follow-up to #42118, implementing the non-blocking review suggestion
left at merge time: the tiled wait budget is derived from the running
Celery task's own time limit, but elapsed time was measured from a
clock started inside take_tiled_screenshot() -- so navigation
(page.goto, bounded 60s), the headstart sleep, element waits, and
dimension probing all ran before the clock started, effectively
granting the tile loop a fresh full budget on top of task time already
spent. On hard-limit-only tasks that overhang can still crest the task
limit -- the SIGKILL #42118 exists to prevent. The non-tiled readiness
wait already avoids this by threading screenshot_started_at from the
top of get_screenshot (#42427); this applies the same pattern to the
tiled call:

- take_tiled_screenshot() accepts screenshot_started_at (optional,
  defaults to "now" for backward compatibility) and anchors the budget
  clock on it;
- WebDriverPlaywright.get_screenshot() passes its existing
  screenshot_started_at into the tiled call, putting both capture paths
  on one clock.

Tests: pre-capture elapsed time reduces the first tile's capped wait
when the anchor is provided; the local-clock default is unchanged when
it is omitted; the existing tiled call-site assertion now pins the new
argument.

Co-Authored-By: Claude <noreply@anthropic.com>
@dosubot dosubot Bot added the alert-reports Namespace | Anything related to the Alert & Reports feature label Jul 31, 2026
@bito-code-review

bito-code-review Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #32a320

Actionable Suggestions - 0
Review Details
  • Files reviewed - 4 · Commit Range: 9536a28..9536a28
    • superset/utils/screenshot_utils.py
    • superset/utils/webdriver.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 Bito Logo

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.42%. Comparing base (6929d03) to head (9536a28).

Files with missing lines Patch % Lines
superset/utils/screenshot_utils.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #42661   +/-   ##
=======================================
  Coverage   65.42%   65.42%           
=======================================
  Files        2810     2810           
  Lines      159422   159422           
  Branches    36382    36382           
=======================================
  Hits       104309   104309           
  Misses      53070    53070           
  Partials     2043     2043           
Flag Coverage Δ
hive 38.07% <0.00%> (ø)
mysql 57.79% <0.00%> (ø)
postgres 57.84% <0.00%> (ø)
presto 39.96% <0.00%> (ø)
python 59.22% <0.00%> (ø)
sqlite 57.46% <0.00%> (ø)
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@eschutho

eschutho commented Aug 1, 2026

Copy link
Copy Markdown
Member Author

Folding this into #42624 rather than maintaining it standalone: #42624 restructures the exact code this PR patches (the tiled budget clock), and its report-path deadline already starts at task start — superseding this anchor for scheduled reports. The residual non-report case (thumbnails routed to tiling counting pre-capture time) is now implemented on #42624's branch with the same tests, credited to the original review suggestion on #42118. Branch stays for reference.

eschutho added a commit to fitzee/superset that referenced this pull request Aug 1, 2026
…hot start

Folds open PR apache#42661 into this branch (its standalone form patched code this
branch restructures): take_tiled_screenshot() accepts the caller's
screenshot_started_at so navigation/headstart/element-wait time counts
against the non-report task budget, matching the clock _wait_for_charts_ready
already uses. Report captures are unaffected -- their deadline starts at
task start, which supersedes the anchor. Falls back to "now" when omitted.

Co-Authored-By: Claude <noreply@anthropic.com>
@eschutho eschutho closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alert-reports Namespace | Anything related to the Alert & Reports feature preset-io size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant