Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: bypass cache on screenshots for alerts #17695

Merged
merged 4 commits into from
Dec 22, 2021

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Dec 9, 2021

SUMMARY

When creating a screenshot for an alert we should bypass the cache. This PR changes the alert jobs to pass force=true when fetching a screenshot, and also implements the logic to carry the force parameter through the stack.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

I added unit tests to the backend and frontend.

To test locally:

  1. Set up an alert that triggers (eg, SELECT COUNT(*) FROM bart_lines and >0).
  2. Check that the celery worker takes a screenshot with ?force=true (you can see it in the logs).
  3. Click the link, the image should open fullscreen and bypassing the cache.

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

@betodealmeida betodealmeida marked this pull request as ready for review December 10, 2021 02:38
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 10, 2021
@codecov
Copy link

codecov bot commented Dec 10, 2021

Codecov Report

Merging #17695 (2db7633) into master (8c25f2f) will decrease coverage by 0.90%.
The diff coverage is 54.25%.

❗ Current head 2db7633 differs from pull request most recent head ff741e9. Consider uploading reports for the commit ff741e9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17695      +/-   ##
==========================================
- Coverage   68.86%   67.95%   -0.91%     
==========================================
  Files        1598     1653      +55     
  Lines       65297    66373    +1076     
  Branches     6952     7120     +168     
==========================================
+ Hits        44966    45103     +137     
- Misses      18446    19373     +927     
- Partials     1885     1897      +12     
Flag Coverage Δ
hive ?
mysql 82.19% <96.51%> (?)
postgres 82.20% <96.51%> (+0.05%) ⬆️
python 82.29% <96.51%> (-0.17%) ⬇️
sqlite 81.88% <96.51%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
...et-ui-chart-controls/src/shared-controls/index.tsx 38.53% <0.00%> (-0.36%) ⬇️
...d/packages/superset-ui-chart-controls/src/types.ts 91.30% <ø> (ø)
...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts 80.00% <ø> (ø)
...set-chart-deckgl/src/AnimatableDeckGLContainer.jsx 0.00% <ø> (ø)
...et-chart-deckgl/src/CategoricalDeckGLContainer.jsx 0.00% <0.00%> (ø)
...legacy-preset-chart-deckgl/src/DeckGLContainer.jsx 0.00% <0.00%> (ø)
...ins/legacy-preset-chart-deckgl/src/Multi/Multi.jsx 0.00% <0.00%> (ø)
...gacy-preset-chart-deckgl/src/Multi/controlPanel.js 0.00% <ø> (ø)
...gins/legacy-preset-chart-deckgl/src/Multi/index.js 66.66% <ø> (ø)
...gins/legacy-preset-chart-deckgl/src/TooltipRow.jsx 0.00% <ø> (ø)
... and 153 more

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 8c25f2f...ff741e9. Read the comment docs.

@betodealmeida betodealmeida changed the title feat: bypass cache on screenshots for alerts (WIP) feat: bypass cache on screenshots for alerts Dec 10, 2021
@eschutho
Copy link
Member

@betodealmeida does this use a different cache key for reports or by running an alert/report that busts the cache, does it update the explore view as well?

@betodealmeida
Copy link
Member Author

@betodealmeida does this use a different cache key for reports or by running an alert/report that busts the cache, does it update the explore view as well?

It's the same cache, so it should update the explore view as well.

@@ -45,12 +45,27 @@ test('Get url when endpointType:standalone', () => {
expect(
getExploreLongUrl(
params.formData,
params.endpointType,
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was wrong.

@graceguo-supercat
Copy link

graceguo-supercat commented Dec 16, 2021

@betodealmeida Thanks for the PR. I have 1 question: this change only affect Alert but not Report, right?
Thinking about if a user setup email report for a large dashboard, and with 1 min as frequency, it might impact backend query engine.

@betodealmeida
Copy link
Member Author

@betodealmeida Thanks for the PR. I have 1 question, this change only impact Alert but not Report, right? Thinking about if a user setup email report for a large dashboard, and with 1 min as frequency, it might impact backend query engine.

Right, this is just for alerts. For reports I have another PR that makes it configurable, with the default being to use the cache.

@graceguo-supercat
Copy link

@betodealmeida If alert triggers a dashboard, will every chart in the dashboard have force refresh?

@betodealmeida
Copy link
Member Author

@betodealmeida If alert triggers a dashboard, will every chart in the dashboard have force refresh?

That's a great question... right now I'm adding this just for charts, not dashboards. Maybe for dashboards we should also make it configurable (default off), since the impact can be much bigger?

@graceguo-supercat
Copy link

Maybe for dashboards we should also make it configurable (default off), since the impact can be much bigger?

agree. Thanks!

@betodealmeida betodealmeida merged commit b7707e8 into apache:master Dec 22, 2021
shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* feat: bypass cache on screenshots for alerts

* Update existing tests

* Add backend test

* Add frontend test
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* feat: bypass cache on screenshots for alerts

* Update existing tests

* Add backend test

* Add frontend test
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants