chore(bigquery): Add extra logging for BigQuery exceptions so we can have better insight on exceptions #22024

Antonio-RiveroMartnez · 2022-11-03T12:37:49Z

SUMMARY

Some BigQuery exceptions are not being logged with enough information, so we need to add some extra method that parses the original exception and logs the correct message. We also add some unit tests to the new method.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

TESTING INSTRUCTIONS

Connect Locally to a BigQuery DB
Run an invalid SQL statement so you get a BigQuery exception
Check you are logging the correct message out of the original exception

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-11-03T14:13:38Z

Codecov Report

Merging #22024 (afa7200) into master (aa48cae) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #22024   +/-   ##
=======================================
  Coverage   67.07%   67.07%           
=======================================
  Files        1815     1815           
  Lines       69575    69581    +6     
  Branches     7486     7486           
=======================================
+ Hits        46665    46673    +8     
+ Misses      20974    20972    -2     
  Partials     1936     1936

Flag	Coverage Δ
hive	`52.83% <60.00%> (+<0.01%)`	⬆️
mysql	`78.39% <60.00%> (-0.01%)`	⬇️
postgres	`78.46% <60.00%> (-0.01%)`	⬇️
presto	`52.73% <40.00%> (-0.01%)`	⬇️
python	`81.57% <100.00%> (+<0.01%)`	⬆️
sqlite	`76.92% <60.00%> (-0.01%)`	⬇️
unit	`51.21% <80.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset/db_engine_specs/base.py	`89.21% <100.00%> (+0.05%)`	⬆️
superset/db_engine_specs/bigquery.py	`82.58% <100.00%> (+0.53%)`	⬆️
superset/db_engine_specs/druid.py	`83.33% <0.00%> (+2.38%)`	⬆️

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

superset/db_engine_specs/base.py

superset/db_engine_specs/bigquery.py

superset/db_engine_specs/base.py

superset/utils/core.py

eschutho

look at inheritance.. come back to this

- Add new method to parse unknown exception in BigQuery engines. This method is going to shed some light so we can show correct messages later using our regex

- Use new EngineType enum instead of raw string - Add better exception handling for new parse_error_exception method in case anything fails during the parsing - Add test to back the exception handling

- parse_error_exception returns an exception with the parsed message, because of that we can skip the compare with Bigquery and instead just make use of the existing logger upstream (sql_lab)

hughhhh · 2022-11-07T21:44:26Z

tests/unit_tests/db_engine_specs/test_bigquery.py

+    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |    .    |
+
+
+    bigquery error: 400 Table \"case_detail_all_suites\" must be qualified with a dataset (e.g. dataset.table).


make all the test work with this example

Updated 👍

- Update our method and tests so bigquery error: portion doesn't appear in the message

eschutho · 2022-11-09T00:23:09Z

superset/db_engine_specs/bigquery.py

+        try:
+            return Exception(
+                str(exception)  # pylint: disable=use-maxsplit-arg
+                .rsplit("\n")[0]


you can also use .splitlines()[0] here

eschutho · 2022-11-09T00:24:31Z

superset/db_engine_specs/bigquery.py

+                .rsplit(":")[1]
+                .strip()
+            )
+        except Exception:  # pylint: disable=broad-except


I tried to get this to raise an exception and wasn't able to. I believe as long as the return value is a string, you should be able to perform all of the operations and don't need to put it in a try catch.

Hmm if for some unknown reason the Exception has no : for example, accessing the [1] would cause a: IndexError: list index out of range. Just adding the try/catch there in case anything like that happens with the message we get.

Oh, ok I tried it with no new line. That makes sense.

- Use splitlines instead and avoid ignoring pylint rules

eschutho

This looks great @Antonio-RiveroMartnez!

pull-request-size bot added the size/M label Nov 3, 2022

hughhhh requested changes Nov 3, 2022

View reviewed changes

superset/db_engine_specs/base.py Outdated Show resolved Hide resolved

superset/db_engine_specs/bigquery.py Outdated Show resolved Hide resolved

Antonio-RiveroMartnez force-pushed the bug-60610 branch 2 times, most recently from 577d90f to 547a6d6 Compare November 3, 2022 17:31

Antonio-RiveroMartnez closed this Nov 4, 2022

Antonio-RiveroMartnez reopened this Nov 4, 2022

Antonio-RiveroMartnez closed this Nov 4, 2022

Antonio-RiveroMartnez reopened this Nov 4, 2022

hughhhh reviewed Nov 7, 2022

View reviewed changes

superset/db_engine_specs/base.py Outdated Show resolved Hide resolved

hughhhh reviewed Nov 7, 2022

View reviewed changes

superset/utils/core.py Outdated Show resolved Hide resolved

eschutho requested changes Nov 7, 2022

View reviewed changes

Antonio-RiveroMartnez added 3 commits November 7, 2022 15:16

BigQuery:

eb32133

- Add new method to parse unknown exception in BigQuery engines. This method is going to shed some light so we can show correct messages later using our regex

BigQuery:

fcf0bf8

- Use new EngineType enum instead of raw string - Add better exception handling for new parse_error_exception method in case anything fails during the parsing - Add test to back the exception handling

BigQuery:

7cadba9

- parse_error_exception returns an exception with the parsed message, because of that we can skip the compare with Bigquery and instead just make use of the existing logger upstream (sql_lab)

Antonio-RiveroMartnez force-pushed the bug-60610 branch from 547a6d6 to 7cadba9 Compare November 7, 2022 19:27

hughhhh reviewed Nov 7, 2022

View reviewed changes

Antonio-RiveroMartnez force-pushed the bug-60610 branch from d1b09a0 to 33d6dd1 Compare November 7, 2022 22:22

BigQuery:

5c8bf8e

- Update our method and tests so bigquery error: portion doesn't appear in the message

Antonio-RiveroMartnez force-pushed the bug-60610 branch from 33d6dd1 to 5c8bf8e Compare November 7, 2022 22:27

Antonio-RiveroMartnez closed this Nov 7, 2022

Antonio-RiveroMartnez reopened this Nov 7, 2022

hughhhh approved these changes Nov 8, 2022

View reviewed changes

hughhhh requested a review from eschutho November 8, 2022 22:20

eschutho reviewed Nov 9, 2022

View reviewed changes

BigQuery:

afa7200

- Use splitlines instead and avoid ignoring pylint rules

eschutho approved these changes Nov 10, 2022

View reviewed changes

hughhhh merged commit 95b4c7b into apache:master Nov 10, 2022

Antonio-RiveroMartnez mentioned this pull request Dec 6, 2022

fix(bigquery): Properly display errors for BigQuery DBs #22349

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(bigquery): Add extra logging for BigQuery exceptions so we can have better insight on exceptions #22024

chore(bigquery): Add extra logging for BigQuery exceptions so we can have better insight on exceptions #22024

Antonio-RiveroMartnez commented Nov 3, 2022

codecov bot commented Nov 3, 2022 •

edited

Loading

eschutho left a comment

hughhhh Nov 7, 2022

Antonio-RiveroMartnez Nov 7, 2022

eschutho Nov 9, 2022

eschutho Nov 9, 2022

Antonio-RiveroMartnez Nov 9, 2022

eschutho Nov 10, 2022

eschutho left a comment

		\| . \| . \| . \| . \| . \| . \| . \| . \| . \| . \| . \| . \| . \| . \| . \|


		bigquery error: 400 Table \"case_detail_all_suites\" must be qualified with a dataset (e.g. dataset.table).

chore(bigquery): Add extra logging for BigQuery exceptions so we can have better insight on exceptions #22024

chore(bigquery): Add extra logging for BigQuery exceptions so we can have better insight on exceptions #22024

Conversation

Antonio-RiveroMartnez commented Nov 3, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Nov 3, 2022 • edited Loading

Codecov Report

eschutho left a comment

Choose a reason for hiding this comment

hughhhh Nov 7, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Nov 7, 2022

Choose a reason for hiding this comment

eschutho Nov 9, 2022

Choose a reason for hiding this comment

eschutho Nov 9, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Nov 9, 2022

Choose a reason for hiding this comment

eschutho Nov 10, 2022

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2022 •

edited

Loading