fix(thumbnail cache): Enabling force parameter on screenshot/thumbnail cache #31757

fisjac · 2025-01-08T21:40:29Z

SUMMARY

Both the /api/v1/chart/{{id}}/cache_screenshot/ and /api/v1/dashboard/{{id}}/cache_dashboard_screenshot/ endpoints enable generating and retrieve a screenshot from the respective resource.

The endpoints accept sending force=true in the query parameters, but currently this parameter does nothing. It should instead invalidate the previous image and generate a new one.

This PR introduces a fix as well as some improvements to the functioning of the thumbnail cache, such that the status of the screenshot can be tracked by the relevant api endpoints as well.

Rather than storing plain bytes objects in the cache, this PR introduces a new class ScreenshotCachePayoad which can store the image bytes as well as timestamp and status values. Status values include
Pending: screenshot is awaiting being processed
Computing: screenshot job has been picked up by the worker and is processing
Updated: screenshot has been updated
Error: an error occured while the webdriver was processing the screenshot task.

While in a Computing or Updated state, requests to regenerate the screenshot will be blocked unless the force parameter is set to True

Additional update_status and updated_at fields have been added to 202 response payload for endpoints as follows:

{
  "cache_key": "ef13dd170eda0581d74a5d1c84983778",
  "dashboard_url": "http://superset_app:8088/superset/dashboard/15/",
  "image_url": "http://superset_app:8088/api/v1/dashboard/15/thumbnail/ef13dd170eda0581d74a5d1c84983778/",
  "update_status": "Pending",
  "updated_at": "2025/01/16-14:58:34"
}

How to reproduce the bug
Create a chart.
Perform a modification in the data.
Send a GET request to /api/v1/chart/{{id}}/cache_screenshot/?q=(force:!t).
Access the URL returned in the image_url value.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

tests have been added in the tests/unit_tests/utils/screenshot_tests.py

For manual testing

ping the /api/v1/chart/<pk>/cache_screenshot/ endpoint. Ensure that a 202 response is received.
refresh periodically until status changes to "Computing." Then again to "Updated"
once updated, ensure the screenshot url returns the proper screenshot.
refresh again, and ensure the status has not changed from "Updated"
ping the endpoint again with force parameter /api/v1/chart/cache_screenshot/<pk>/?q=(force:!t) to ensure that the status changes to "Pending"

Repeat the steps for the POST /api/v1/dashboard/<pk>/cache_dashboard_screenshot/

access the GET /api/v1/chart/<pk>/thumbnail/<digest> endpoint to ensure that a 202 response is received.
access again once the status changes to "Updated" ensure that the endpoint results in a 200 response with the screenshot.
repeat with GET /api/v1/dashboard/<pk>/thumbnail/<digest>

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags: THUMBNAILS, ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS
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

korbit-ai · 2025-01-08T21:40:33Z

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

codecov · 2025-01-08T22:28:24Z

Codecov Report

Attention: Patch coverage is 84.14634% with 26 lines in your changes missing coverage. Please review.

Project coverage is 83.45%. Comparing base (b12f515) to head (feba297).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/charts/api.py	50.00%	12 Missing ⚠️
superset/utils/screenshots.py	91.46%	7 Missing ⚠️
superset/utils/webdriver.py	12.50%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #31757       +/-   ##
===========================================
+ Coverage        0   83.45%   +83.45%     
===========================================
  Files           0      544      +544     
  Lines           0    38998    +38998     
===========================================
+ Hits            0    32547    +32547     
- Misses          0     6451     +6451

Flag	Coverage Δ
hive	`48.48% <27.43%> (?)`
mysql	`75.87% <61.58%> (?)`
postgres	`75.94% <61.58%> (?)`
presto	`53.02% <27.43%> (?)`
python	`83.45% <84.14%> (?)`
sqlite	`75.46% <61.58%> (?)`
unit	`60.91% <54.26%> (?)`

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

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

adding CacheScreenshotPayload state machine

kgabryje · 2025-01-30T17:05:10Z

/korbit-review

korbit-ai

I've completed my review and didn't find any issues.

Files scanned

File Path	Reviewed
superset/tasks/thumbnails.py	✅
superset/models/slice.py	✅
superset/dashboards/schemas.py	✅
superset/utils/screenshots.py	✅
superset/utils/webdriver.py	✅
superset/charts/api.py	✅
superset/charts/schemas.py	✅
superset/dashboards/api.py	✅
superset/config.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings

Setting Value

Review Schedule Automatic excluding drafts

Max Issue Count 10

Automatic PR Descriptions ❌

Issue Categories

Category Enabled

Naming ✅

Database Operations ✅

Documentation ✅

Logging ✅

Error Handling ✅

Systems and Environment ✅

Objects and Data Structures ✅

Readability and Maintainability ✅

Asynchronous Processing ✅

Design Patterns ✅

Third-Party Libraries ✅

Performance ✅

Security ✅

Functionality ✅

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ [email protected]

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

villebro

First pass comments

superset/charts/api.py

superset/charts/schemas.py

superset/config.py

villebro · 2025-01-30T18:48:26Z

superset/models/slice.py

-        current_user=get_current_user(),
-        chart_id=target.id,
-        force=True,
+        current_user=get_current_user(), chart_id=target.id, force=True


Hmm, I'd maybe be ok with forcefully invalidating thumbs on chart/dashboard change, but recaching it? I'd propose adding a config flag THUMBNAIL_ON_CHANGE: "invalidate" | "recalculate" | None for admins to be able to specify if they want to just ignore changes, invalidate or recache every time the object changes.

discussed on slack - out of scope

superset/utils/screenshots.py

villebro

Second pass comments. I'd propose breaking the status check into a method of CachePayload to avoid duplication of the logic. Something like .should_trigger_task() that returns bool based on status, error TTL etc.

villebro · 2025-01-31T17:07:35Z

superset/utils/screenshots.py

+class ScreenshotCachePayload:
+    def __init__(self, image: bytes | None = None):
+        self._image = image
+        self._timestamp = datetime.now().strftime("%Y/%m/%d-%H:%M:%S")


Hmm, I'd prefer not to use a custom format here. Let's just use isoformat()

Suggested change

self._timestamp = datetime.now().strftime("%Y/%m/%d-%H:%M:%S")

self._timestamp = datetime.now().isoformat()

villebro · 2025-01-31T17:08:00Z

superset/utils/screenshots.py

+            self.status = StatusValues.UPDATED
+
+    def update_timestamp(self) -> None:
+        self._timestamp = datetime.now().strftime("%Y/%m/%d-%H:%M:%S")


same

Suggested change

self._timestamp = datetime.now().strftime("%Y/%m/%d-%H:%M:%S")

self._timestamp = datetime.now().isoformat()

villebro · 2025-01-31T17:11:36Z

superset/charts/api.py

+        error_cache_ttl = config["THUMBNAIL_ERROR_CACHE_TTL"]
+        error_cache_expired = (
+            datetime.now()
+            - datetime.strptime(cache_payload.get_timestamp(), "%Y/%m/%d-%H:%M:%S")


same

Suggested change

- datetime.strptime(cache_payload.get_timestamp(), "%Y/%m/%d-%H:%M:%S")

- datetime.fromisoformat(cache_payload.get_timestamp())

villebro · 2025-01-31T17:12:37Z

superset/charts/api.py

            202:
-              description: Chart async result
+              description: Chart async task created


Sorry about the back and forth here, but this seems more appropriate

Suggested change

description: Chart async task created

description: Chart screenshot task created

superset/charts/api.py

superset/dashboards/api.py

villebro

A few last non-blocking comments. Great to see this land before 5.0 cut, this will save considerable compute where enabled!

villebro · 2025-01-31T17:51:09Z

superset/utils/screenshots.py

+        def is_error_cache_ttl_expired() -> bool:
+            error_cache_ttl = app.config["THUMBNAIL_ERROR_CACHE_TTL"]
+            return (
+                datetime.now() - datetime.fromisoformat(self.get_timestamp())
+            ).total_seconds() > error_cache_ttl


Is there some reason we're nesting this? Couldn't it just be a regular method?

I figured that we don't use it anywhere else and we only need to check it inside of should_trigger_task method. I can make it a regular method or a private one instead of nested if you think it's better

I'd prefer a separate method, as redefining it each time will incur some extra overhead and makes the main function slightly less readable.

villebro · 2025-01-31T17:52:51Z

superset/charts/api.py

+                task_status=cache_payload.get_status(),
+            )
+
+        if cache_payload.should_trigger_task() or force:


very very nit, but flipping this around will likely save some CPU. To DRY it even more, we could maybe even consider making force an optional parameter in should_trigger_task that defaults to False (not highly opinionated here)

Suggested change

if cache_payload.should_trigger_task() or force:

if force or cache_payload.should_trigger_task():

villebro · 2025-01-31T17:53:07Z

superset/dashboards/api.py


-        def trigger_celery() -> WerkzeugResponse:
+        if cache_payload.should_trigger_task() or force:


same

Suggested change

if cache_payload.should_trigger_task() or force:

if force or cache_payload.should_trigger_task():

github-actions bot added risk:db-migration PRs that require a DB migration api Related to the REST API doc Namespace | Anything related to documentation plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages labels Jan 8, 2025

fisjac force-pushed the force-cache-screenshot branch from f05f59a to 5ac969c Compare January 8, 2025 22:22

pull-request-size bot added the size/L label Jan 8, 2025

github-actions bot removed risk:db-migration PRs that require a DB migration doc Namespace | Anything related to documentation plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages labels Jan 8, 2025

rusackas added the review:draft label Jan 9, 2025

pull-request-size bot added size/XL and removed size/L labels Jan 15, 2025

fisjac marked this pull request as ready for review January 16, 2025 15:07

fisjac force-pushed the force-cache-screenshot branch from 22f419d to 3112c16 Compare January 16, 2025 15:16

sadpandajoe removed the review:draft label Jan 17, 2025

sadpandajoe requested a review from geido January 17, 2025 19:31

fisjac added 3 commits January 30, 2025 17:54

enabling force parameter on screenshot endpoints

1b4ca40

adding CacheScreenshotPayload state machine

test fixes

7bae9d4

test-fix

896b029

kgabryje force-pushed the force-cache-screenshot branch from a63c5ec to 896b029 Compare January 30, 2025 16:54

korbit-ai bot reviewed Jan 30, 2025

View reviewed changes

villebro reviewed Jan 30, 2025

View reviewed changes

kgabryje added 2 commits January 31, 2025 17:36

Implement error cache ttl check

7dc75c7

Move error ttl logic to cache class

33a3742

villebro reviewed Jan 31, 2025

View reviewed changes

kgabryje added 2 commits January 31, 2025 18:38

address comments

b4f001d

change comment

684d733

villebro approved these changes Jan 31, 2025

View reviewed changes

more drying

feba297

kgabryje merged commit 7db0589 into apache:master Jan 31, 2025
44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(thumbnail cache): Enabling force parameter on screenshot/thumbnail cache #31757

fix(thumbnail cache): Enabling force parameter on screenshot/thumbnail cache #31757

fisjac commented Jan 8, 2025 •

edited

Loading

korbit-ai bot commented Jan 8, 2025

codecov bot commented Jan 8, 2025 •

edited

Loading

kgabryje commented Jan 30, 2025

korbit-ai bot left a comment •

edited

Loading

villebro left a comment

villebro Jan 30, 2025

kgabryje Jan 31, 2025

villebro left a comment

villebro Jan 31, 2025

villebro Jan 31, 2025

villebro Jan 31, 2025

villebro Jan 31, 2025

villebro left a comment

villebro Jan 31, 2025

kgabryje Jan 31, 2025

villebro Jan 31, 2025 •

edited

Loading

villebro Jan 31, 2025

villebro Jan 31, 2025

Setting	Value
Review Schedule	Automatic excluding drafts
Max Issue Count	10
Automatic PR Descriptions	❌

Category	Enabled
Naming	✅
Database Operations	✅
Documentation	✅
Logging	✅
Error Handling	✅
Systems and Environment	✅
Objects and Data Structures	✅
Readability and Maintainability	✅
Asynchronous Processing	✅
Design Patterns	✅
Third-Party Libraries	✅
Performance	✅
Security	✅
Functionality	✅

	self._timestamp = datetime.now().strftime("%Y/%m/%d-%H:%M:%S")
	self._timestamp = datetime.now().isoformat()

	- datetime.strptime(cache_payload.get_timestamp(), "%Y/%m/%d-%H:%M:%S")
	- datetime.fromisoformat(cache_payload.get_timestamp())

	description: Chart async task created
	description: Chart screenshot task created

	if cache_payload.should_trigger_task() or force:
	if force or cache_payload.should_trigger_task():


		def trigger_celery() -> WerkzeugResponse:
		if cache_payload.should_trigger_task() or force:

fix(thumbnail cache): Enabling force parameter on screenshot/thumbnail cache #31757

fix(thumbnail cache): Enabling force parameter on screenshot/thumbnail cache #31757

Conversation

fisjac commented Jan 8, 2025 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

korbit-ai bot commented Jan 8, 2025

codecov bot commented Jan 8, 2025 • edited Loading

Codecov Report

kgabryje commented Jan 30, 2025

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Interacting with Korbit

Customizing Korbit

Current Korbit Configuration

Feedback and Support

Looking to add Korbit to your team? Get started with a free 2 week trial here

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisjac commented Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 8, 2025 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading

villebro Jan 31, 2025 •

edited

Loading