You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/01/10 16:44:45 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #8947:
[WiP] [thumbnails] thumbnails for dashboards and charts
dpgaspar opened a new pull request #8947: [WiP] [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947
### CATEGORY
Choose one
- [ ] Bug Fix
- [X] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Piking up from @mistercrunch work on #7716
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [X] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r406283900
##########
File path: superset/charts/api.py
##########
@@ -340,3 +350,58 @@ def bulk_delete(self, **kwargs) -> Response: # pylint: disable=arguments-differ
return self.response_403()
except ChartBulkDeleteFailedError as e:
return self.response_422(message=str(e))
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
+ description: Chart thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = self.datamodel.get(pk, self._base_filters)
+ if not chart:
+ return self.response_404()
+ if kwargs["rison"].get("force", False):
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the chart screenshot using the current user and cache if set
+ screenshot = ChartScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If not screenshot then send request to compute thumb to celery
+ if not screenshot:
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if chart.digest != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return self.response(304, message="Digest differs")
Review comment:
It's now 302 has we talked
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391622755
##########
File path: superset/views/chart/api.py
##########
@@ -173,3 +189,62 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi):
"owners": ("first_name", "asc"),
}
filter_rel_fields_field = {"owners": "first_name", "dashboards": "dashboard_title"}
+
+ def __init__(self, *args, **kwargs):
+ if is_feature_enabled("THUMBNAILS"):
+ self.include_route_methods = self.include_route_methods | {"thumbnail"}
+ super().__init__(*args, **kwargs)
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
+ description: Chart thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = self.datamodel.get(pk, self._base_filters)
+ if not chart:
+ return self.response_404()
+ if kwargs["rison"].get("force", False):
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the chart screenshot using the current user and cache if set
+ screenshot = ChartScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If not screenshot then send request to compute thumb to celery
+ if not screenshot:
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if chart.unique_value != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return Response(
Review comment:
Returning 304
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391056670
##########
File path: superset/views/chart/api.py
##########
@@ -173,3 +189,62 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi):
"owners": ("first_name", "asc"),
}
filter_rel_fields_field = {"owners": "first_name", "dashboards": "dashboard_title"}
+
+ def __init__(self, *args, **kwargs):
+ if is_feature_enabled("THUMBNAILS"):
+ self.include_route_methods = self.include_route_methods | {"thumbnail"}
+ super().__init__(*args, **kwargs)
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
+ description: Chart thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = self.datamodel.get(pk, self._base_filters)
+ if not chart:
+ return self.response_404()
+ if kwargs["rison"].get("force", False):
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the chart screenshot using the current user and cache if set
+ screenshot = ChartScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If not screenshot then send request to compute thumb to celery
+ if not screenshot:
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if chart.unique_value != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return Response(
Review comment:
We are circumventing browser cache with the `unique_value` "trick", HTTP GET will send to the frontend the current `unique_value` for each dashboard and chart, and the frontend will use that to actually make the HTTP GET thumb request. Do you think setting browser cache could be a better replacement for this logic? or could add value?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-576632036
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=h1) Report
> Merging [#8947](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/8c80aeefbd7f0812ce8f8bad3ac15f66a0377637?src=pr&el=desc) will **decrease** coverage by `0.22%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8947 +/- ##
==========================================
- Coverage 59.15% 58.92% -0.23%
==========================================
Files 367 372 +5
Lines 11686 11999 +313
Branches 2866 2940 +74
==========================================
+ Hits 6913 7071 +158
- Misses 4594 4750 +156
+ Partials 179 178 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/components/Checkbox.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL0NoZWNrYm94LmpzeA==) | | |
| [.../assets/src/dashboard/reducers/dashboardFilters.js](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkRmlsdGVycy5qcw==) | | |
| [...ets/src/dashboard/components/dnd/DragDroppable.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9kbmQvRHJhZ0Ryb3BwYWJsZS5qc3g=) | | |
| [...assets/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL0xpc3RWaWV3L1RhYmxlQ29sbGVjdGlvbi50c3g=) | | |
| [superset/assets/src/components/EditableTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL0VkaXRhYmxlVGl0bGUuanN4) | | |
| [...src/explore/components/controls/VizTypeControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvY29udHJvbHMvVml6VHlwZUNvbnRyb2wuanN4) | | |
| [...t/assets/src/components/InfoTooltipWithTrigger.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL0luZm9Ub29sdGlwV2l0aFRyaWdnZXIuanN4) | | |
| [.../src/dashboard/components/UndoRedoKeylisteners.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9VbmRvUmVkb0tleWxpc3RlbmVycy5qc3g=) | | |
| [...ts/src/explore/components/ExploreActionButtons.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvRXhwbG9yZUFjdGlvbkJ1dHRvbnMuanN4) | | |
| [.../src/dashboard/components/FilterTooltipWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9GaWx0ZXJUb29sdGlwV3JhcHBlci5qc3g=) | | |
| ... and [729 more](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=footer). Last update [8c80aee...e246e8c](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405146876
##########
File path: superset/charts/api.py
##########
@@ -39,9 +41,12 @@
ChartPostSchema,
ChartPutSchema,
get_delete_ids_schema,
+ thumbnail_query_schema,
)
from superset.constants import RouteMethod
from superset.models.slice import Slice
+from superset.tasks.thumbnails import cache_chart_thumbnail
+from superset.utils.selenium import ChartScreenshot
Review comment:
I wonder if the ChartScreenshot class should be part of a package that isn't named after the underlying implementation? I can see a future where we want to remove Selenium from the stack - other languages have far better tools for this system, and I hope some of them will be ported soon. It would be nice to be able to replace the underlying implementation without touching files from other modules.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391051254
##########
File path: superset/models/dashboard.py
##########
@@ -184,6 +185,22 @@ def dashboard_link(self) -> Markup:
title = escape(self.dashboard_title or "<empty>")
return Markup(f'<a href="{self.url}">{title}</a>')
+ @property
+ def unique_value(self) -> str:
Review comment:
No problem, but, this is actually an MD5 Exa, `unique_value` is aiming for a generic unique value digest. Maybe `unique_digest` or just `digest`, any ideas?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8947:
[WiP] [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8947: [WiP] [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-576632036
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=h1) Report
> Merging [#8947](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c1700dc8e4743cb4baf53d7bb53a1e30c1ad6883?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `60%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8947 +/- ##
==========================================
+ Coverage 59.15% 59.15% +<.01%
==========================================
Files 367 367
Lines 11682 11686 +4
Branches 2864 2866 +2
==========================================
+ Hits 6910 6913 +3
- Misses 4593 4594 +1
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset/assets/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9TcWxFZGl0b3IuanN4) | `51.94% <100%> (+0.31%)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `54.21% <33.33%> (-0.11%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=footer). Last update [c1700dc...8c80aee](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405155549
##########
File path: tests/thumbnails_tests.py
##########
@@ -0,0 +1,229 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# from superset import db
+# from superset.models.dashboard import Dashboard
+import subprocess
+import urllib.request
+from unittest import skipUnless
+from unittest.mock import patch
+
+from flask_testing import LiveServerTestCase
+from sqlalchemy.sql import func
+
+from superset import db, is_feature_enabled, security_manager, thumbnail_cache
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.utils.selenium import (
+ ChartScreenshot,
+ DashboardScreenshot,
+ get_auth_cookies,
+)
+from tests.test_app import app
+
+from .base_tests import SupersetTestCase
+
+
+class CeleryStartMixin:
+ @classmethod
+ def setUpClass(cls):
+ with app.app_context():
+ from werkzeug.contrib.cache import RedisCache
+
+ class CeleryConfig(object):
+ BROKER_URL = "redis://localhost"
+ CELERY_IMPORTS = ("superset.tasks.thumbnails",)
+ CONCURRENCY = 1
+
+ app.config["CELERY_CONFIG"] = CeleryConfig
+
+ def init_thumbnail_cache(app) -> RedisCache:
+ return RedisCache(
+ host="localhost",
+ key_prefix="superset_thumbnails_",
+ default_timeout=10000,
+ )
+
+ app.config["THUMBNAIL_CACHE_CONFIG"] = init_thumbnail_cache
+
+ base_dir = app.config["BASE_DIR"]
+ worker_command = base_dir + "/bin/superset worker -w 2"
+ subprocess.Popen(
+ worker_command,
+ shell=True,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE,
+ )
+
+ @classmethod
+ def tearDownClass(cls):
+ subprocess.call(
+ "ps auxww | grep 'celeryd' | awk '{print $2}' | xargs kill -9", shell=True
+ )
+ subprocess.call(
+ "ps auxww | grep 'superset worker' | awk '{print $2}' | xargs kill -9",
+ shell=True,
+ )
+
+
+class ThumbnailsSeleniumLive(CeleryStartMixin, LiveServerTestCase):
+ def create_app(self):
+ return app
+
+ def url_open_auth(self, username: str, url: str):
+ admin_user = security_manager.find_user(username=username)
+ cookies = {}
+ for cookie in get_auth_cookies(admin_user):
+ cookies["session"] = cookie
+
+ opener = urllib.request.build_opener()
+ opener.addheaders.append(("Cookie", f"session={cookies['session']}"))
+ return opener.open(f"{self.get_server_url()}/{url}")
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_async_dashboard_screenshot(self):
+ """
+ Thumbnails: Simple get async dashboard screenshot
+ """
+ dashboard = db.session.query(Dashboard).all()[0]
+ with patch("superset.dashboards.api.DashboardRestApi.get") as mock_get:
+ response = self.url_open_auth(
+ "admin",
+ f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/",
+ )
+ self.assertEqual(response.getcode(), 202)
+
+
+class ThumbnailsTests(CeleryStartMixin, SupersetTestCase):
+
+ mock_image = b"bytes mock image"
+
+ def test_dashboard_thumbnail_disabled(self):
+ """
+ Thumbnails: Dashboard thumbnail disabled
+ """
+ if is_feature_enabled("THUMBNAILS"):
+ return
+ dashboard = db.session.query(Dashboard).all()[0]
+ self.login(username="admin")
+ uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 404)
+
+ def test_chart_thumbnail_disabled(self):
+ """
+ Thumbnails: Chart thumbnail disabled
+ """
+ if is_feature_enabled("THUMBNAILS"):
+ return
+ chart = db.session.query(Slice).all()[0]
+ self.login(username="admin")
+ uri = f"api/v1/chart/{chart}/thumbnail/{chart.digest}/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 404)
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_async_dashboard_screenshot(self):
+ """
+ Thumbnails: Simple get async dashboard screenshot
+ """
+ dashboard = db.session.query(Dashboard).all()[0]
+ self.login(username="admin")
+ uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/"
+ with patch(
+ "superset.tasks.thumbnails.cache_dashboard_thumbnail.delay"
+ ) as mock_task:
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 202)
+ mock_task.assert_called_with(dashboard.id, force=True)
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_async_dashboard_notfound(self):
+ """
+ Thumbnails: Simple get async dashboard not found
+ """
+ max_id = db.session.query(func.max(Dashboard.id)).scalar()
+ self.login(username="admin")
+ uri = f"api/v1/dashboard/{max_id + 1}/thumbnail/1234/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 404)
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_async_dashboard_not_allowed(self):
+ """
+ Thumbnails: Simple get async dashboard not allowed
+ """
+ dashboard = db.session.query(Dashboard).all()[0]
+ self.login(username="gamma")
+ uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 404)
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_async_chart_screenshot(self):
+ """
+ Thumbnails: Simple get async chart screenshot
+ """
+ chart = db.session.query(Slice).all()[0]
+ self.login(username="admin")
+ uri = f"api/v1/chart/{chart.id}/thumbnail/{chart.digest}/"
+ with patch(
+ "superset.tasks.thumbnails.cache_chart_thumbnail.delay"
+ ) as mock_task:
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 202)
+ mock_task.assert_called_with(chart.id, force=True)
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_async_chart_notfound(self):
+ """
+ Thumbnails: Simple get async chart not found
+ """
+ max_id = db.session.query(func.max(Slice.id)).scalar()
+ self.login(username="admin")
+ uri = f"api/v1/chart/{max_id + 1}/thumbnail/1234/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 404)
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_cached_dashboard_screenshot(self):
+ """
+ Thumbnails: Simple get cached dashboard screenshot
+ """
+ dashboard = db.session.query(Dashboard).all()[0]
+ # Cache a test "image"
+ screenshot = DashboardScreenshot(model_id=dashboard.id)
+ thumbnail_cache.set(screenshot.cache_key, self.mock_image)
+ self.login(username="admin")
+ uri = f"api/v1/dashboard/{dashboard.id}/thumbnail/{dashboard.digest}/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ self.assertEqual(rv.data, self.mock_image)
+
+ @skipUnless((is_feature_enabled("THUMBNAILS")), "Thumbnails feature")
+ def test_get_cached_chart_screenshot(self):
Review comment:
I think we're missing test cases for the 304 response
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r406283745
##########
File path: superset/charts/api.py
##########
@@ -340,3 +350,58 @@ def bulk_delete(self, **kwargs) -> Response: # pylint: disable=arguments-differ
return self.response_403()
except ChartBulkDeleteFailedError as e:
return self.response_422(message=str(e))
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
Review comment:
It's now 302 has we talked
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r390490421
##########
File path: superset/models/slice.py
##########
@@ -321,3 +343,8 @@ def set_related_perm(mapper, connection, target):
sqla.event.listen(Slice, "after_insert", ChartUpdater.after_insert)
sqla.event.listen(Slice, "after_update", ChartUpdater.after_update)
sqla.event.listen(Slice, "after_delete", ChartUpdater.after_delete)
+
+# events for updating tags
+if is_feature_enabled("THUMBNAILS_SQLA_LISTENERS"):
Review comment:
Is there a way to drag this out of the model? I see this as debt that will need to be paid down in the future, as models should themselves be as lean as possible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-600128497
@dpgaspar this needs a rebase, I can review+test once the conflicts are resolved
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar merged pull request #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r390489408
##########
File path: superset/models/dashboard.py
##########
@@ -184,6 +185,22 @@ def dashboard_link(self) -> Markup:
title = escape(self.dashboard_title or "<empty>")
return Markup(f'<a href="{self.url}">{title}</a>')
+ @property
+ def unique_value(self) -> str:
Review comment:
Instead of `unique_value`, maybe call this something like `sha_sum`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391078032
##########
File path: superset/views/chart/api.py
##########
@@ -173,3 +189,62 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi):
"owners": ("first_name", "asc"),
}
filter_rel_fields_field = {"owners": "first_name", "dashboards": "dashboard_title"}
+
+ def __init__(self, *args, **kwargs):
+ if is_feature_enabled("THUMBNAILS"):
+ self.include_route_methods = self.include_route_methods | {"thumbnail"}
+ super().__init__(*args, **kwargs)
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
+ description: Chart thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = self.datamodel.get(pk, self._base_filters)
+ if not chart:
+ return self.response_404()
+ if kwargs["rison"].get("force", False):
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the chart screenshot using the current user and cache if set
+ screenshot = ChartScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If not screenshot then send request to compute thumb to celery
+ if not screenshot:
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if chart.unique_value != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return Response(
Review comment:
Things like this can be pushed up to the reverse proxy (nginx) layer also...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405159406
##########
File path: superset/models/dashboard.py
##########
@@ -184,6 +185,22 @@ def dashboard_link(self) -> Markup:
title = escape(self.dashboard_title or "<empty>")
return Markup(f'<a href="{self.url}">{title}</a>')
+ @property
+ def digest(self) -> str:
+ """
+ Returns a MD5 HEX digest that makes this dashboard unique
+ """
+ unique_string = f"{self.position_json}.{self.css}.{self.json_metadata}"
+ return utils.md5_hex(unique_string)
+
+ @property
+ def thumbnail_url(self) -> str:
+ """
+ Returns a thumbnail URL with a HEX digest. We want to avoid browser cache
+ if the dashboard has changed
+ """
+ return f"/api/v1/dashboard/{self.id}/thumbnail/{self.digest}/"
Review comment:
I agree, is there a way to shape responses via Marshmallow?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405148718
##########
File path: superset/charts/api.py
##########
@@ -340,3 +350,58 @@ def bulk_delete(self, **kwargs) -> Response: # pylint: disable=arguments-differ
return self.response_403()
except ChartBulkDeleteFailedError as e:
return self.response_422(message=str(e))
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
Review comment:
Missing 202 and 304 documentation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r406284898
##########
File path: tests/superset_test_config_thumbnails.py
##########
@@ -0,0 +1,77 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from copy import copy
+
+from flask import Flask
+from werkzeug.contrib.cache import RedisCache
+
+from superset.config import * # type: ignore
+
+AUTH_USER_REGISTRATION_ROLE = "alpha"
+SQLALCHEMY_DATABASE_URI = "sqlite:///" + os.path.join(DATA_DIR, "unittests.db")
+DEBUG = True
+SUPERSET_WEBSERVER_PORT = 8081
+
+# Allowing SQLALCHEMY_DATABASE_URI to be defined as an env var for
+# continuous integration
+if "SUPERSET__SQLALCHEMY_DATABASE_URI" in os.environ:
+ SQLALCHEMY_DATABASE_URI = os.environ["SUPERSET__SQLALCHEMY_DATABASE_URI"]
+
+SQL_SELECT_AS_CTA = True
+SQL_MAX_ROW = 666
Review comment:
copy paste from a previous joke on tests config ¯\_(ツ)_/¯
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on issue #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-600329589
Thanks @villebro, done it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405150565
##########
File path: superset/dashboards/api.py
##########
@@ -389,3 +401,80 @@ def export(self, **kwargs):
"Content-Disposition"
]
return resp
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @safe
+ @rison(thumbnail_query_schema)
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Dashboard thumbnail
+ ---
+ get:
+ description: >-
+ Compute async or get already computed dashboard thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ name: digest
+ description: A hex digest that makes this dashboard unique
+ schema:
+ type: string
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ force:
+ type: boolean
+ default: false
+ responses:
+ 200:
+ description: Dashboard thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 202:
+ description: Thumbnail does not exist on cache, fired async to compute
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ dashboard = self.datamodel.get(pk, self._base_filters)
+ if not dashboard:
+ return self.response_404()
+ # If force, request a screenshot from the workers
+ if kwargs["rison"].get("force", False):
+ cache_dashboard_thumbnail.delay(dashboard.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the dashboard screenshot using the current user and cache if set
+ screenshot = DashboardScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If the screenshot does not exist, request one from the workers
+ if not screenshot:
+ cache_dashboard_thumbnail.delay(dashboard.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if dashboard.digest != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return self.response(304, message="Digest differs")
Review comment:
Same comment here - I would assume 304 would indicate the digests match so there's no point in re-sending.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-576632036
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=h1) Report
> Merging [#8947](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/5e6662ab12e4b5d3c392e8cc1bfd37afc5b20b63&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8947 +/- ##
=======================================
Coverage 58.96% 58.96%
=======================================
Files 374 374
Lines 12137 12137
Branches 2992 2992
=======================================
Hits 7156 7156
Misses 4802 4802
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=footer). Last update [5e6662a...acf675d](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391075957
##########
File path: superset/models/dashboard.py
##########
@@ -184,6 +185,22 @@ def dashboard_link(self) -> Markup:
title = escape(self.dashboard_title or "<empty>")
return Markup(f'<a href="{self.url}">{title}</a>')
+ @property
+ def unique_value(self) -> str:
Review comment:
I'm good with `digest`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r394640226
##########
File path: docs/installation.rst
##########
@@ -647,6 +647,74 @@ section in `config.py`:
This will cache all the charts in the top 5 most popular dashboards every hour.
For other strategies, check the `superset/tasks/cache.py` file.
+Caching Thumbnails
+------------------
+
+This is an optional feature that can be turned on by activating it's feature flag on config:
+
+.. code-block:: python
+
+ FEATURE_FLAGS = {
+ "THUMBNAILS": True,
+ "THUMBNAILS_SQLA_LISTENERS": True,
+ }
+
+
+For this feature you will need a cache system and celery workers. All thumbnails are store on cache and are processed
+asynchronously by the workers.
+
+An example config could be:
+
+.. code-block:: python
+
+ from flask import Flask
+ from werkzeug.contrib.cache import RedisCache
+
+ ...
+
+ class CeleryConfig(object):
+ BROKER_URL = "redis://localhost:6379/0"
+ CELERY_IMPORTS = ("superset.sql_lab", "superset.tasks", "superset.tasks.thumbnails")
+ CELERY_RESULT_BACKEND = "redis://localhost:6379/0"
+ CELERYD_PREFETCH_MULTIPLIER = 10
+ CELERY_ACKS_LATE = True
+
+
+ CELERY_CONFIG = CeleryConfig
+
+ def init_thumbnail_cache(app: Flask) -> RedisCache:
Review comment:
Let's have the example be more realistic - can we use an S3 cache in the example? Redis isn't super appropriate for storing images.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-597279097
@dpgaspar am I understanding this correctly, that the image thumbnails would be stored in Redis cache? If so, Redis really is the wrong piece of tech for this. Let's have a chat about this once you're online tomorrow morning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r390489159
##########
File path: superset/models/slice.py
##########
@@ -163,6 +164,21 @@ def data(self) -> Dict[str, Any]:
"changed_on": self.changed_on.isoformat(),
}
+ @property
+ def unique_value(self) -> str:
Review comment:
Instead of `unique_value`, maybe call this something like `sha_sum`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405150115
##########
File path: superset/dashboards/api.py
##########
@@ -41,8 +43,11 @@
DashboardPutSchema,
get_delete_ids_schema,
get_export_ids_schema,
+ thumbnail_query_schema,
)
from superset.models.dashboard import Dashboard
+from superset.tasks.thumbnails import cache_dashboard_thumbnail
+from superset.utils.selenium import DashboardScreenshot
Review comment:
Same comment here - could we move DashboardScreenshot to a module that isn't named after the specific schema?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r406216224
##########
File path: superset/charts/api.py
##########
@@ -39,9 +41,12 @@
ChartPostSchema,
ChartPutSchema,
get_delete_ids_schema,
+ thumbnail_query_schema,
)
from superset.constants import RouteMethod
from superset.models.slice import Slice
+from superset.tasks.thumbnails import cache_chart_thumbnail
+from superset.utils.selenium import ChartScreenshot
Review comment:
Totally agree
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391076137
##########
File path: superset/models/slice.py
##########
@@ -321,3 +343,8 @@ def set_related_perm(mapper, connection, target):
sqla.event.listen(Slice, "after_insert", ChartUpdater.after_insert)
sqla.event.listen(Slice, "after_update", ChartUpdater.after_update)
sqla.event.listen(Slice, "after_delete", ChartUpdater.after_delete)
+
+# events for updating tags
+if is_feature_enabled("THUMBNAILS_SQLA_LISTENERS"):
Review comment:
😆
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on issue #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-597383316
@willbarrett there's an S3 implementation of the flask-cache (really Werkzeug) abstraction we're using.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r390498816
##########
File path: superset/views/chart/api.py
##########
@@ -173,3 +189,62 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi):
"owners": ("first_name", "asc"),
}
filter_rel_fields_field = {"owners": "first_name", "dashboards": "dashboard_title"}
+
+ def __init__(self, *args, **kwargs):
+ if is_feature_enabled("THUMBNAILS"):
+ self.include_route_methods = self.include_route_methods | {"thumbnail"}
+ super().__init__(*args, **kwargs)
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
+ description: Chart thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = self.datamodel.get(pk, self._base_filters)
+ if not chart:
+ return self.response_404()
+ if kwargs["rison"].get("force", False):
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the chart screenshot using the current user and cache if set
+ screenshot = ChartScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If not screenshot then send request to compute thumb to celery
+ if not screenshot:
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if chart.unique_value != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return Response(
Review comment:
Do we want to add any browser cache headers here? The thing should be able to be cached almost indefinitely, as it would need to be recomputed on update.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405150714
##########
File path: superset/dashboards/api.py
##########
@@ -389,3 +401,80 @@ def export(self, **kwargs):
"Content-Disposition"
]
return resp
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @safe
+ @rison(thumbnail_query_schema)
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Dashboard thumbnail
+ ---
+ get:
+ description: >-
+ Compute async or get already computed dashboard thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ name: digest
+ description: A hex digest that makes this dashboard unique
+ schema:
+ type: string
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ force:
+ type: boolean
+ default: false
+ responses:
+ 200:
+ description: Dashboard thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 202:
+ description: Thumbnail does not exist on cache, fired async to compute
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ 401:
Review comment:
Missing 304 response
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8947:
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-576632036
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=h1) Report
> Merging [#8947](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/e44571c0221fc6f6431e54fc4814419a1724e556&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8947 +/- ##
=======================================
Coverage 58.57% 58.57%
=======================================
Files 386 386
Lines 12287 12287
Branches 3025 3025
=======================================
Hits 7197 7197
Misses 4906 4906
Partials 184 184
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=footer). Last update [e44571c...e44571c](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r406284074
##########
File path: superset/dashboards/api.py
##########
@@ -389,3 +401,80 @@ def export(self, **kwargs):
"Content-Disposition"
]
return resp
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @safe
+ @rison(thumbnail_query_schema)
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Dashboard thumbnail
+ ---
+ get:
+ description: >-
+ Compute async or get already computed dashboard thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ name: digest
+ description: A hex digest that makes this dashboard unique
+ schema:
+ type: string
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ force:
+ type: boolean
+ default: false
+ responses:
+ 200:
+ description: Dashboard thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 202:
+ description: Thumbnail does not exist on cache, fired async to compute
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ dashboard = self.datamodel.get(pk, self._base_filters)
+ if not dashboard:
+ return self.response_404()
+ # If force, request a screenshot from the workers
+ if kwargs["rison"].get("force", False):
+ cache_dashboard_thumbnail.delay(dashboard.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the dashboard screenshot using the current user and cache if set
+ screenshot = DashboardScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If the screenshot does not exist, request one from the workers
+ if not screenshot:
+ cache_dashboard_thumbnail.delay(dashboard.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if dashboard.digest != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return self.response(304, message="Digest differs")
Review comment:
It's now 302 has we talked
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391077356
##########
File path: superset/views/chart/api.py
##########
@@ -173,3 +189,62 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi):
"owners": ("first_name", "asc"),
}
filter_rel_fields_field = {"owners": "first_name", "dashboards": "dashboard_title"}
+
+ def __init__(self, *args, **kwargs):
+ if is_feature_enabled("THUMBNAILS"):
+ self.include_route_methods = self.include_route_methods | {"thumbnail"}
+ super().__init__(*args, **kwargs)
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
+ description: Chart thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = self.datamodel.get(pk, self._base_filters)
+ if not chart:
+ return self.response_404()
+ if kwargs["rison"].get("force", False):
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the chart screenshot using the current user and cache if set
+ screenshot = ChartScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If not screenshot then send request to compute thumb to celery
+ if not screenshot:
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if chart.unique_value != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return Response(
Review comment:
It's always better to return a 304 :). The digest will change when the underlying thumbnail does, I'm assuming. Therefore the frontend will bust through anything that's cached on update.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #8947: [WiP]
[thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8947: [WiP] [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#issuecomment-576632036
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=h1) Report
> Merging [#8947](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/b19f17bfd06a69314e6b61ed172451b1f5966bd8?src=pr&el=desc) will **increase** coverage by `0.17%`.
> The diff coverage is `67.79%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8947 +/- ##
==========================================
+ Coverage 58.98% 59.16% +0.17%
==========================================
Files 359 367 +8
Lines 11336 11679 +343
Branches 2788 2862 +74
==========================================
+ Hits 6687 6910 +223
- Misses 4471 4590 +119
- Partials 178 179 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `55.69% <ø> (-4.08%)` | :arrow_down: |
| [superset/assets/src/welcome/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy93ZWxjb21lL0FwcC5qc3g=) | `0% <ø> (ø)` | :arrow_up: |
| [superset/assets/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvaW5kZXguanM=) | `100% <ø> (ø)` | :arrow_up: |
| [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
| [...t/assets/src/dashboard/reducers/dashboardLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkTGF5b3V0Lmpz) | `93% <0%> (-1.9%)` | :arrow_down: |
| [.../src/explore/components/AdhocMetricEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3Zlci5qc3g=) | `62.35% <100%> (ø)` | :arrow_up: |
| [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3ZlclRpdGxlLmpzeA==) | `70% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [...src/dashboard/util/getFilterConfigsFromFormdata.js](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvdXRpbC9nZXRGaWx0ZXJDb25maWdzRnJvbUZvcm1kYXRhLmpz) | `88.46% <100%> (ø)` | :arrow_up: |
| [...src/dashboard/components/HeaderActionsDropdown.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9IZWFkZXJBY3Rpb25zRHJvcGRvd24uanN4) | `69.56% <100%> (ø)` | :arrow_up: |
| ... and [24 more](https://codecov.io/gh/apache/incubator-superset/pull/8947/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=footer). Last update [b19f17b...b470e80](https://codecov.io/gh/apache/incubator-superset/pull/8947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r394699307
##########
File path: docs/installation.rst
##########
@@ -647,6 +647,74 @@ section in `config.py`:
This will cache all the charts in the top 5 most popular dashboards every hour.
For other strategies, check the `superset/tasks/cache.py` file.
+Caching Thumbnails
+------------------
+
+This is an optional feature that can be turned on by activating it's feature flag on config:
+
+.. code-block:: python
+
+ FEATURE_FLAGS = {
+ "THUMBNAILS": True,
+ "THUMBNAILS_SQLA_LISTENERS": True,
+ }
+
+
+For this feature you will need a cache system and celery workers. All thumbnails are store on cache and are processed
+asynchronously by the workers.
+
+An example config could be:
+
+.. code-block:: python
+
+ from flask import Flask
+ from werkzeug.contrib.cache import RedisCache
+
+ ...
+
+ class CeleryConfig(object):
+ BROKER_URL = "redis://localhost:6379/0"
+ CELERY_IMPORTS = ("superset.sql_lab", "superset.tasks", "superset.tasks.thumbnails")
+ CELERY_RESULT_BACKEND = "redis://localhost:6379/0"
+ CELERYD_PREFETCH_MULTIPLIER = 10
+ CELERY_ACKS_LATE = True
+
+
+ CELERY_CONFIG = CeleryConfig
+
+ def init_thumbnail_cache(app: Flask) -> RedisCache:
Review comment:
Sure, will do that
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r391051837
##########
File path: superset/models/slice.py
##########
@@ -321,3 +343,8 @@ def set_related_perm(mapper, connection, target):
sqla.event.listen(Slice, "after_insert", ChartUpdater.after_insert)
sqla.event.listen(Slice, "after_update", ChartUpdater.after_update)
sqla.event.listen(Slice, "after_delete", ChartUpdater.after_delete)
+
+# events for updating tags
+if is_feature_enabled("THUMBNAILS_SQLA_LISTENERS"):
Review comment:
Agree, yes. Promise to clean this debt when refactoring Chart's API to comply with SIP-35
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405149180
##########
File path: superset/charts/api.py
##########
@@ -340,3 +350,58 @@ def bulk_delete(self, **kwargs) -> Response: # pylint: disable=arguments-differ
return self.response_403()
except ChartBulkDeleteFailedError as e:
return self.response_422(message=str(e))
+
+ @expose("/<pk>/thumbnail/<digest>/", methods=["GET"])
+ @protect()
+ @rison(thumbnail_query_schema)
+ @safe
+ def thumbnail(self, pk, digest, **kwargs): # pylint: disable=invalid-name
+ """Get Chart thumbnail
+ ---
+ get:
+ description: Compute or get already computed chart thumbnail from cache
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: path
+ schema:
+ type: string
+ name: sha
+ responses:
+ 200:
+ description: Chart thumbnail image
+ content:
+ image/*:
+ schema:
+ type: string
+ format: binary
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = self.datamodel.get(pk, self._base_filters)
+ if not chart:
+ return self.response_404()
+ if kwargs["rison"].get("force", False):
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # fetch the chart screenshot using the current user and cache if set
+ screenshot = ChartScreenshot(pk).get_from_cache(cache=thumbnail_cache)
+ # If not screenshot then send request to compute thumb to celery
+ if not screenshot:
+ cache_chart_thumbnail.delay(chart.id, force=True)
+ return self.response(202, message="OK Async")
+ # If digests
+ if chart.digest != digest:
+ logger.info("Requested thumbnail digest differs from actual digest")
+ return self.response(304, message="Digest differs")
Review comment:
Is 304 the right response code? I would assume we would return 304 when the digests match rather than when the digests differ.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r396446865
##########
File path: docs/installation.rst
##########
@@ -647,6 +647,74 @@ section in `config.py`:
This will cache all the charts in the top 5 most popular dashboards every hour.
For other strategies, check the `superset/tasks/cache.py` file.
+Caching Thumbnails
+------------------
+
+This is an optional feature that can be turned on by activating it's feature flag on config:
+
+.. code-block:: python
+
+ FEATURE_FLAGS = {
+ "THUMBNAILS": True,
+ "THUMBNAILS_SQLA_LISTENERS": True,
+ }
+
+
+For this feature you will need a cache system and celery workers. All thumbnails are store on cache and are processed
+asynchronously by the workers.
+
+An example config could be:
+
+.. code-block:: python
+
+ from flask import Flask
+ from werkzeug.contrib.cache import RedisCache
+
+ ...
+
+ class CeleryConfig(object):
+ BROKER_URL = "redis://localhost:6379/0"
+ CELERY_IMPORTS = ("superset.sql_lab", "superset.tasks", "superset.tasks.thumbnails")
+ CELERY_RESULT_BACKEND = "redis://localhost:6379/0"
+ CELERYD_PREFETCH_MULTIPLIER = 10
+ CELERY_ACKS_LATE = True
+
+
+ CELERY_CONFIG = CeleryConfig
+
+ def init_thumbnail_cache(app: Flask) -> RedisCache:
Review comment:
updated
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405151345
##########
File path: superset/models/dashboard.py
##########
@@ -184,6 +185,22 @@ def dashboard_link(self) -> Markup:
title = escape(self.dashboard_title or "<empty>")
return Markup(f'<a href="{self.url}">{title}</a>')
+ @property
+ def digest(self) -> str:
+ """
+ Returns a MD5 HEX digest that makes this dashboard unique
+ """
+ unique_string = f"{self.position_json}.{self.css}.{self.json_metadata}"
+ return utils.md5_hex(unique_string)
+
+ @property
+ def thumbnail_url(self) -> str:
+ """
+ Returns a thumbnail URL with a HEX digest. We want to avoid browser cache
+ if the dashboard has changed
+ """
+ return f"/api/v1/dashboard/{self.id}/thumbnail/{self.digest}/"
Review comment:
Should this live in the model? It seems strange to compute the URL for the resource in the model. This seems like a concern for higher up in the application.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #8947: [thumbnails] thumbnails for dashboards and charts
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #8947: [thumbnails] thumbnails for dashboards and charts
URL: https://github.com/apache/incubator-superset/pull/8947#discussion_r405154915
##########
File path: tests/superset_test_config_thumbnails.py
##########
@@ -0,0 +1,77 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from copy import copy
+
+from flask import Flask
+from werkzeug.contrib.cache import RedisCache
+
+from superset.config import * # type: ignore
+
+AUTH_USER_REGISTRATION_ROLE = "alpha"
+SQLALCHEMY_DATABASE_URI = "sqlite:///" + os.path.join(DATA_DIR, "unittests.db")
+DEBUG = True
+SUPERSET_WEBSERVER_PORT = 8081
+
+# Allowing SQLALCHEMY_DATABASE_URI to be defined as an env var for
+# continuous integration
+if "SUPERSET__SQLALCHEMY_DATABASE_URI" in os.environ:
+ SQLALCHEMY_DATABASE_URI = os.environ["SUPERSET__SQLALCHEMY_DATABASE_URI"]
+
+SQL_SELECT_AS_CTA = True
+SQL_MAX_ROW = 666
Review comment:
😈
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org