fix: Emit a warning message rather than an exception on query failure

[willbarrett]

SUMMARY

Exception messages are frequently bubbled up to error aggregators like Sentry, Airbrake, and the like from web applications. As queries can fail for a wide variety of reasons, most of which are beyond the ability of Superset to catch and fix before execution, I'd like to downgrade the messaging around analytical query failures to the warning level. This will keep the events in log streams but largely remove them from error aggregators.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

• Trigger an error in an analytical query
• Ensure that it is logged at the "warning" level rather than the "exception" level.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

Reviewers

@craig-rueda @rusackas @john-bodley

[robdiciuccio]

LGTM

[john-bodley]

cc @etr2460 for context.

[craig-rueda]

An issue I see with this is the potential swallowing of useful information. Does it make sense to set exc_info=True here? In this way we would still get stack traces, and instrumentation tools would ignore as the threshold would be WARN

[mistercrunch]

I think with exception you get the stack trace in the logs which can be very useful. We use this ~79 times in the codebase

20:51 $ git grep "\.exception(" | wc -l
      79

[etr2460]

No strong opinions here one way or another, I can see both sides of the argument. I lean slightly on the side of agreeing with Will as these exceptions are generally going to be coming from the db engine and aren't really something we can resolve on the Superset side

[willbarrett]

I think Craig's suggestion is the correct balance. I'll set exec_info=True to capture the stacktraces. To @mistercrunch 's point, many of those .exception calls make a lot of sense, but this one is not actionable. We can tune the log levels without loosing the stacktraces.

[codecov-commenter]

Codecov Report

Merging #9811 into master will increase coverage by 6.71%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9811      +/-   ##
==========================================
+ Coverage   64.31%   71.02%   +6.71%     
==========================================
  Files         536      583      +47     
  Lines       29109    30615    +1506     
  Branches     2806     3162     +356     
==========================================
+ Hits        18721    21745    +3024     
+ Misses      10208     8758    -1450     
+ Partials      180      112      -68     

Flag	Coverage Δ
#cypress	53.59% <ø> (-0.05%)	⬇️
#javascript	59.29% <ø> (?)
#python	71.27% <100.00%> (+0.24%)	⬆️

Impacted Files	Coverage Δ
superset/connectors/sqla/models.py	88.60% <100.00%> (+0.03%)	⬆️
superset-frontend/src/setup/setupPlugins.ts	12.90% <0.00%> (-87.10%)	⬇️
...rset-frontend/src/setup/setupErrorMessagesExtra.ts	50.00% <0.00%> (-50.00%)	⬇️
superset-frontend/src/setup/setupErrorMessages.ts	66.66% <0.00%> (-33.34%)	⬇️
superset-frontend/src/setup/setupApp.ts	25.00% <0.00%> (-3.58%)	⬇️
...rontend/src/visualizations/FilterBox/FilterBox.jsx	74.16% <0.00%> (-0.41%)	⬇️
superset/errors.py	100.00% <0.00%> (ø)
superset/viz_sip38.py	0.00% <0.00%> (ø)
superset/exceptions.py	100.00% <0.00%> (ø)
superset-frontend/src/featureFlags.ts	100.00% <0.00%> (ø)
... and 233 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 3cc5400...a315090. Read the comment docs.