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

[zhaorui2022]

SUMMARY

As mentioned in #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

After

Case 2

Before

After

TESTING INSTRUCTIONS

Tested locally with some broken dashboards. Screenshots attached.

ADDITIONAL INFORMATION

• Has associated issue: Alert/Report screenshots showing "Unexpected error" even though it shows no error on server side #21653
• Required feature flags: SCREENSHOT_REPLACE_UNEXPECTED_ERRORS

[codecov]

Codecov Report

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

❗ 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.

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

[mayurnewase]

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.

[zhaorui2022]

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.

[dpgaspar]

Thank you for the contribution. This looks good, please address the comments and rebase

[dpgaspar]

If this is the default no need to set it

[dpgaspar]

please set the config back to it's expected state
app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = False

[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!

[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!
Updated

[rusackas]

Looks to me like comments have been addressed and we're all set here! I'll merge shortly if there's no further reason from @dpgaspar not to do so!