Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Dashboard): Sync/Async Dashboard Screenshot Generation #30755

Merged
merged 6 commits into from
Nov 1, 2024

Conversation

geido
Copy link
Member

@geido geido commented Oct 30, 2024

SUMMARY

PR #29187 introduced a new endpoint for generating dashboard screenshots and PR #29272 made this endpoint mandatory for the frontend. However, Celery should be an optional dependency in Superset and the endpoint is dependent on Celery to work. This PR makes Celery optional for generating dashboard screenshots by:

  • Reintroducing the previous download screenshot functionality:
    This update enables dashboards to be downloaded directly from the browser, without using the dedicated API endpoint. This approach has known CORS limitations but will work for most cases where Celery isn't configured.

  • Adding two new feature flags:

    • ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS:
      Controls whether the API endpoint that caches and retrieves dashboard screenshots via a webdriver is enabled, allowing this functionality to be decoupled from the UI. This flag is set to False by default.

    • ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT:
      Controls whether the UI’s download functionality uses webdriver-based screenshot generation. This flag is also False by default and it is dependent on ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS.

Fixes #30645

TESTING INSTRUCTIONS

  1. Enter a Dashboard
  2. Download a PDF or Image screenshot via the Dashboard menu
  3. The screenshot should download normally

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS and ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@github-actions github-actions bot added the api Related to the REST API label Oct 30, 2024
@dosubot dosubot bot added change:backend Requires changing the backend dashboard:export Related to exporting dashboards labels Oct 30, 2024
@geido geido force-pushed the geido/fix/dash-screenshot-celery-optional branch 2 times, most recently from 4c123c5 to 48c681d Compare October 30, 2024 16:00
@apache apache deleted a comment from github-actions bot Oct 30, 2024
@geido geido force-pushed the geido/fix/dash-screenshot-celery-optional branch from 48c681d to c9bab36 Compare October 30, 2024 16:08
@geido geido marked this pull request as draft October 30, 2024 16:47
@geido
Copy link
Member Author

geido commented Oct 30, 2024

Moving this to draft as we might change the approach here, bring back the previous frontend-generated screenshots while having the feature flag to enable Selenium generated screenshots optionally.

@sadpandajoe sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Oct 30, 2024
@geido geido force-pushed the geido/fix/dash-screenshot-celery-optional branch from c9bab36 to fe5017c Compare October 31, 2024 11:55
@geido geido marked this pull request as ready for review October 31, 2024 11:56
@dosubot dosubot bot added change:frontend Requires changing the frontend infra:caching Infra setup and configuration related to caching labels Oct 31, 2024
@michael-s-molina
Copy link
Member

Thanks for the updated version of the PR @geido. Do you think we need ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS? I'm ok with the endpoint being exposed all the time. We have the same characteristic for other endpoints where the feature might not be enabled client-side. Maybe only ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT is sufficient?

@geido
Copy link
Member Author

geido commented Oct 31, 2024

Thanks for the updated version of the PR @geido. Do you think we need ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS? I'm ok with the endpoint being exposed all the time. We have the same characteristic for other endpoints where the feature might not be enabled client-side. Maybe only ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT is sufficient?

This is to be consistent with the thumbnails endpoint that are also behind feature flag. In general, I think it is good to be intentional about enabling these endpoints as they expose performance-heavy functionality.

@geido geido force-pushed the geido/fix/dash-screenshot-celery-optional branch from fe5017c to e9816f7 Compare October 31, 2024 13:06
@geido
Copy link
Member Author

geido commented Oct 31, 2024

/testenv up

Copy link
Contributor

@geido Ephemeral environment spinning up at http://35.90.38.171:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the fix @geido!

Copy link
Contributor

@Vitor-Avila Vitor-Avila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@villebro
Copy link
Member

Can we have a link to the PR that caused this regression? Just to make it easier to discover and understand how we can avoid these types of things in the future..

@michael-s-molina michael-s-molina force-pushed the geido/fix/dash-screenshot-celery-optional branch from e620ac7 to 4b79ed0 Compare October 31, 2024 18:35
@michael-s-molina
Copy link
Member

michael-s-molina commented Oct 31, 2024

Ok, I think the exception for the rgbcolor license resolved CI. @villebro we need an owner approval 🙏🏼

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving license override

@geido
Copy link
Member Author

geido commented Nov 1, 2024

Can we have a link to the PR that caused this regression? Just to make it easier to discover and understand how we can avoid these types of things in the future..

Description updated

@geido geido merged commit 3e29777 into master Nov 1, 2024
36 checks passed
Copy link
Contributor

github-actions bot commented Nov 1, 2024

Ephemeral environment shutdown and build artifacts deleted.

sadpandajoe pushed a commit that referenced this pull request Nov 1, 2024
…t Cache (#30755)

Co-authored-by: Michael S. Molina <[email protected]>
Co-authored-by: Michael S. Molina <[email protected]>
(cherry picked from commit 3e29777)
@geido geido changed the title fix(Dashboard): Sync/Async Dashboard Screenshot Generation and Default Cache fix(Dashboard): Sync/Async Dashboard Screenshot Generation Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API change:backend Requires changing the backend change:frontend Requires changing the frontend dashboard:export Related to exporting dashboards dependencies:npm github_actions Pull requests that update GitHub Actions code infra:caching Infra setup and configuration related to caching packages size/XL v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[4.1.0.rc3] Error to download PDF or Image
6 participants