feat: Allow users to bust cache in report dashboard + alerts charts + alert dashboards #18795

hughhhh · 2022-02-17T20:20:46Z

SUMMARY

Added the setting to allow users to bypass caching for both dashboard alerts, charts alerts, + dashboard reports. This will always default to false. Additionally add the functionally to allow the chart component to read the params and force refresh whenever the url param force=true is in the url.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

bypass_cache.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

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

codecov · 2022-02-17T20:29:05Z

Codecov Report

Merging #18795 (3cbdfb4) into master (3298551) will increase coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head 3cbdfb4 differs from pull request most recent head fd235bf. Consider uploading reports for the commit fd235bf to get more accurate results

@@           Coverage Diff           @@
##           master   #18795   +/-   ##
=======================================
  Coverage   66.52%   66.52%           
=======================================
  Files        1641     1641           
  Lines       63475    63476    +1     
  Branches     6443     6444    +1     
=======================================
+ Hits        42226    42227    +1     
- Misses      19584    19585    +1     
+ Partials     1665     1664    -1

Flag	Coverage Δ
javascript	`51.28% <66.66%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset/reports/commands/execute.py	`91.51% <ø> (ø)`
superset-frontend/src/chart/Chart.jsx	`53.33% <50.00%> (-0.12%)`	⬇️
...frontend/src/views/CRUD/alert/AlertReportModal.tsx	`52.38% <100.00%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3298551...fd235bf. Read the comment docs.

chonyy · 2022-02-23T02:54:13Z

Hi @hughhhh , we recently encountered this issue and found this PR. I saw that the option you are adding is called “Ignore cache when generating screenshot”. I’m wondering whether this also works when I’m sending csv or text. We found the content is cached when we are sending a chart as text. Thanks 🙏

…-add-bypass-cache-dash

hughhhh · 2022-02-25T18:44:43Z

Hi @hughhhh , we recently encountered this issue and found this PR. I saw that the option you are adding is called “Ignore cache when generating screenshot”. I’m wondering whether this also works when I’m sending csv or text. We found the content is cached when we are sending a chart as text. Thanks 🙏

That is prolly a seperate problem i'm currently only focusing on adding the checkboxes, and also adding the configuration in the client to setup the network calls for forcing all charts to pull from the source.

File another ticket and tag me in it and I can take a look

AAfghahi · 2022-02-25T18:52:09Z

superset-frontend/src/chart/Chart.jsx

@@ -29,6 +29,8 @@ import ErrorBoundary from 'src/components/ErrorBoundary';
 import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils';
 import ChartRenderer from './ChartRenderer';
 import { ChartErrorMessage } from './ChartErrorMessage';
+import { URL_PARAMS } from 'src/constants';


fix these import issues

@hughhhh I fixed these real quick so that I could use your branch for testing locally.

AAfghahi · 2022-02-25T18:52:40Z

superset-frontend/src/chart/Chart.jsx

@@ -157,7 +159,7 @@ class Chart extends React.PureComponent {
      // Load saved chart with a GET request
      this.props.actions.getSavedChart(
        this.props.formData,
-        this.props.force,
+        this.props.force || getUrlParam(URL_PARAMS.force), // allow override via url params force=true


what does URL_PARAMS.force do?

URL_PARAMS is an enum that represents the key for url param we want to pull. In this case it is force

eschutho

Looks awesome.. if you have time to add a front end test, that would be great, but not blocking.

AAfghahi · 2022-02-25T18:54:34Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

              </>
            )}
+            <StyledCheckbox


should this have a tooltip that explains what it is? Or is it self-explanatory

it's not in the spec, personally think it is fine since we are explicitly putting text right next to the checkbox

jinghua-qa · 2022-02-25T18:58:49Z

I am still in the process of test, but i found an issue that in the alert and report configuration page, after one notification method is added, the "+ Add notification method " link is gone and user could not add another notification method, is this expected? I think we can add another notification method

This is what i expected:

chonyy · 2022-02-27T07:43:08Z

Hi @hughhhh , we recently encountered this issue and found this PR. I saw that the option you are adding is called “Ignore cache when generating screenshot”. I’m wondering whether this also works when I’m sending csv or text. We found the content is cached when we are sending a chart as text. Thanks 🙏

That is prolly a seperate problem i'm currently only focusing on adding the checkboxes, and also adding the configuration in the client to setup the network calls for forcing all charts to pull from the source.

File another ticket and tag me in it and I can take a look

Hi @hughhhh , thanks for your quick reply!

However, I don't quite get why it's a separate problem. I assume that no matter it's a screenshot or text or csv, they are all using the same method to pull the chart right? Therefore, that's why I think if I have an option to disable cache when alerting, then the content is pulled directly from the source (real-time). No matter it's a screenshot / CSV / text. Not sure what I'm missing here. Thanks 🙏

This is the issue I'm facing

betodealmeida · 2022-02-28T21:02:48Z

However, I don't quite get why it's a separate problem. I assume that no matter it's a screenshot or text or csv, they are all using the same method to pull the chart right? Therefore, that's why I think if I have an option to disable cache when alerting, then the content is pulled directly from the source (real-time). No matter it's a screenshot / CSV / text. Not sure what I'm missing here. Thanks 🙏

@chonyy unfortunately the assumption is incorrect, we use different endpoints depending on the format, and some of them even have separate caches. It's something we want to improve eventually.

hughhhh · 2022-03-01T19:43:08Z

/testenv up

github-actions · 2022-03-01T19:44:54Z

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

betodealmeida · 2022-03-01T23:15:52Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

    const data: any = {
      ...currentAlert,
      type: isReport ? 'Report' : 'Alert',
-      force_screenshot: shouldEnableForceScreenshot || forceScreenshot,
+      force_screenshot: !isReport || forceScreenshot, // alerts (!isReport) should always bypass cache


Wait... for alerts with dashboards we don't want to always bypass the cache.

Why not? Is it because of the perf issue that may arise?

superset/reports/commands/execute.py

jinghua-qa · 2022-03-04T17:00:14Z

I have tested bust cache option in alert+dashboard, report+dashbaord, and report+chart, working as expected. Alert+chart always have bust cache, and no bust cache option for Alert+chart. LGTM!

jinghua-qa

LGTM from QA testing

already got 3 and address beto's concerns

github-actions · 2022-03-04T20:31:01Z

Ephemeral environment shutdown and build artifacts deleted.

… alert dashboards (#18795) * wip * add force cahce bypass option to alerts * remove default for alerts to bypass cache * save for now * save for now * fix * commenting out for now * fix linting * remove link * add back force id test * add frontend test * address (cherry picked from commit 8c52fe3)

wip

5537e2e

pull-request-size bot added the size/S label Feb 17, 2022

add force cahce bypass option to alerts

8e41881

hughhhh force-pushed the hugh-add-bypass-cache-dash branch from 9ee0013 to 8e41881 Compare February 22, 2022 22:50

pull-request-size bot added size/M and removed size/S labels Feb 22, 2022

remove default for alerts to bypass cache

a8aa035

eschutho added the need:qa-review Requires QA review label Feb 23, 2022

save for now

6ba541a

pull-request-size bot added size/L and removed size/M labels Feb 25, 2022

hughhhh force-pushed the hugh-add-bypass-cache-dash branch from 02ff786 to 7868634 Compare February 25, 2022 18:27

save for now

843f1bc

hughhhh force-pushed the hugh-add-bypass-cache-dash branch from 7868634 to 843f1bc Compare February 25, 2022 18:33

Merge branch 'master' of https://github.com/apache/superset into hugh…

d46ee31

…-add-bypass-cache-dash

pull-request-size bot added size/M and removed size/L labels Feb 25, 2022

fix

54f8a49

AAfghahi requested changes Feb 25, 2022

View reviewed changes

eschutho approved these changes Feb 25, 2022

View reviewed changes

AAfghahi reviewed Feb 25, 2022

View reviewed changes

commenting out for now

8138b0d

hughhhh force-pushed the hugh-add-bypass-cache-dash branch from d9d6d65 to 8138b0d Compare February 28, 2022 18:33

fix linting

c4bc334

remove link

5c92769

hughhhh force-pushed the hugh-add-bypass-cache-dash branch from 6a35537 to 5c92769 Compare March 1, 2022 02:03

add back force id test

9c0fdf3

pull-request-size bot added size/L and removed size/M labels Mar 1, 2022

add frontend test

3678612

hughhhh force-pushed the hugh-add-bypass-cache-dash branch from b6e088e to 3678612 Compare March 1, 2022 04:03

AAfghahi approved these changes Mar 1, 2022

View reviewed changes

hughhhh requested a review from betodealmeida March 1, 2022 22:20

betodealmeida previously requested changes Mar 1, 2022

View reviewed changes

address

ac22e03

jinghua-qa approved these changes Mar 4, 2022

View reviewed changes

fix merge conflicts

fd235bf

hughhhh merged commit 8c52fe3 into master Mar 4, 2022

chonyy mentioned this pull request Mar 13, 2022

[Alert] Avoid caching charts when sending as text #19134

Closed

villebro added lts-v1 and removed need:qa-review Requires QA review labels Mar 29, 2022

mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

mistercrunch deleted the hugh-add-bypass-cache-dash branch March 26, 2024 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow users to bust cache in report dashboard + alerts charts + alert dashboards #18795

feat: Allow users to bust cache in report dashboard + alerts charts + alert dashboards #18795

hughhhh commented Feb 17, 2022 •

edited

Loading

codecov bot commented Feb 17, 2022 •

edited

Loading

chonyy commented Feb 23, 2022

hughhhh commented Feb 25, 2022

AAfghahi Feb 25, 2022

eschutho Feb 25, 2022

AAfghahi Feb 25, 2022

hughhhh Feb 28, 2022

eschutho left a comment

AAfghahi Feb 25, 2022

hughhhh Feb 28, 2022

jinghua-qa commented Feb 25, 2022

chonyy commented Feb 27, 2022 •

edited

Loading

betodealmeida commented Feb 28, 2022

hughhhh commented Mar 1, 2022

github-actions bot commented Mar 1, 2022

betodealmeida Mar 1, 2022

hughhhh Mar 2, 2022

jinghua-qa commented Mar 4, 2022

jinghua-qa left a comment

github-actions bot commented Mar 4, 2022

feat: Allow users to bust cache in report dashboard + alerts charts + alert dashboards #18795

feat: Allow users to bust cache in report dashboard + alerts charts + alert dashboards #18795

Conversation

hughhhh commented Feb 17, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Feb 17, 2022 • edited Loading

Codecov Report

chonyy commented Feb 23, 2022

hughhhh commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinghua-qa commented Feb 25, 2022

chonyy commented Feb 27, 2022 • edited Loading

betodealmeida commented Feb 28, 2022

hughhhh commented Mar 1, 2022

github-actions bot commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinghua-qa commented Mar 4, 2022

jinghua-qa left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 4, 2022

hughhhh commented Feb 17, 2022 •

edited

Loading

codecov bot commented Feb 17, 2022 •

edited

Loading

chonyy commented Feb 27, 2022 •

edited

Loading