superset icon indicating copy to clipboard operation
superset copied to clipboard

fix(report): Capture unexpected errors in report screenshots. Fixes #21653

Open zhaorui2022 opened this issue 3 years ago • 3 comments

SUMMARY

As mentioned in https://github.com/apache/superset/issues/21653, there are cases that users receive a screenshot with "Unexpected errors" and "See More" links and there is no error on the server side. It might be caused by transient errors and the error is already gone after opening the dashboard again. In this case, there is no way to know what exactly happened and why we got the errors. Adding an option to log those errors to help debug issues and improve systems, and also reveal the errors to users to understand what happens.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Case 1

Before

screenshot_before_1

After

screenshot_after_1

Case 2

Before

screenshot_before_2

After

screenshot_after_2

TESTING INSTRUCTIONS

Tested locally with some broken dashboards. Screenshots attached.

ADDITIONAL INFORMATION

  • [x] Has associated issue: #21653
  • [x] Required feature flags: SCREENSHOT_REPLACE_UNEXPECTED_ERRORS

zhaorui2022 avatar Oct 06 '22 19:10 zhaorui2022

Codecov Report

Merging #21724 (d41cb66) into master (d41cb66) will not change coverage. The diff coverage is n/a.

:exclamation: Current head d41cb66 differs from pull request most recent head 2c1e87f. Consider uploading reports for the commit 2c1e87f to get more accurate results

@@           Coverage Diff           @@
##           master   #21724   +/-   ##
=======================================
  Coverage   66.76%   66.76%           
=======================================
  Files        1847     1847           
  Lines       70562    70562           
  Branches     7742     7742           
=======================================
  Hits        47110    47110           
  Misses      21451    21451           
  Partials     2001     2001           
Flag Coverage Δ
mysql 77.95% <0.00%> (ø)
postgres 78.02% <0.00%> (ø)
presto 52.42% <0.00%> (ø)
python 81.02% <0.00%> (ø)
sqlite 76.48% <0.00%> (ø)
unit 50.91% <0.00%> (ø)

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

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Oct 06 '22 23:10 codecov[bot]

good stuff, have 2 questions.

  1. in case of error modal, should we send the alert to owner and not to the receivers?
  2. I think we should add 1 test case when there is error modal.

mayurnewase avatar Oct 08 '22 19:10 mayurnewase

good stuff, have 2 questions.

1. in case of error modal, should we send the alert to owner and not to the receivers?
    I think two approach could both work. The reason I prefer sending it to receivers now is that even if some of the charts are broken in the screenshot, there might be other charts in the same screenshot that are valuable to the receivers.
2. I think we should add 1 test case when there is error modal.
       I was thinking about that but didn't figure out a way to mock an error. It might due to my experience with any front end related code. Will try to investigate more and see if I can add a test case where it shows an error modal.

zhaorui2022 avatar Oct 11 '22 23:10 zhaorui2022

Hi @zhaorui2022 - just following up to see if you can address comments and resolve the merge conflicts. Would love to get this across the finish line! Holler if we can help! Thanks for the contribution!

rusackas avatar Dec 02 '22 21:12 rusackas

Hi @zhaorui2022 - just following up to see if you can address comments and resolve the merge conflicts. Would love to get this across the finish line! Holler if we can help! Thanks for the contribution! Updated

zhaorui2022 avatar Dec 08 '22 21:12 zhaorui2022