You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/08/07 07:31:04 UTC

[GitHub] [incubator-superset] villebro opened a new pull request #10548: fix: handle query exceptions gracefully

villebro opened a new pull request #10548:
URL: https://github.com/apache/incubator-superset/pull/10548


   ### SUMMARY
   Currently expected exceptions in visualization queries are not correctly caught, leading to a large number of unnecessary false positives on monitoring systems. This catches typical exceptions (e.g. `TemplateError`) and reraises them as `QueryObjectValidationError` (extends `SupersetException`) with more context, which are later caught and sent to the frontend without exception logging in the backend.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   ![image](https://user-images.githubusercontent.com/33317356/89620818-dc3f8380-d898-11ea-84a3-c6b076f34cb1.png)
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   Local testing with both legacy and new charts + ensure that no exceptions are logged
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] 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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10548: fix: handle query exceptions gracefully

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10548:
URL: https://github.com/apache/incubator-superset/pull/10548#issuecomment-670395341


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=h1) Report
   > Merging [#10548](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/363abfa1a50a92ef5f2f2eac1efe507e2ba8f509&el=desc) will **decrease** coverage by `4.44%`.
   > The diff coverage is `24.07%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10548/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10548      +/-   ##
   ==========================================
   - Coverage   63.75%   59.30%   -4.45%     
   ==========================================
     Files         767      767              
     Lines       36269    36299      +30     
     Branches     3426     3426              
   ==========================================
   - Hits        23122    21528    -1594     
   - Misses      13033    14578    +1545     
   - Partials      114      193      +79     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `59.89% <ø> (ø)` | |
   | #python | `58.96% <24.07%> (-0.41%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `57.52% <12.50%> (-0.39%)` | :arrow_down: |
   | [superset/common/query\_context.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHQucHk=) | `83.75% <20.00%> (-1.51%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.04% <30.00%> (-2.24%)` | :arrow_down: |
   | [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.32% <33.33%> (-0.23%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupColors.js](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [155 more](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=footer). Last update [363abfa...ad03c1e](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #10548: fix: handle query exceptions gracefully

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #10548:
URL: https://github.com/apache/incubator-superset/pull/10548#discussion_r466935536



##########
File path: superset/connectors/sqla/models.py
##########
@@ -1193,13 +1234,13 @@ def fetch_metadata(self, commit: bool = True) -> None:
         """Fetches the metadata for the table and merges it in"""
         try:
             table_ = self.get_sqla_table_object()
-        except Exception as ex:
-            logger.exception(ex)
-            raise Exception(
+        except Exception:

Review comment:
       `SQLAlchemyError` would be nice, but riskier




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10548: fix: handle query exceptions gracefully

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10548:
URL: https://github.com/apache/incubator-superset/pull/10548#issuecomment-670395341


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=h1) Report
   > Merging [#10548](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/363abfa1a50a92ef5f2f2eac1efe507e2ba8f509&el=desc) will **decrease** coverage by `4.41%`.
   > The diff coverage is `24.07%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10548/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10548      +/-   ##
   ==========================================
   - Coverage   63.75%   59.33%   -4.42%     
   ==========================================
     Files         767      767              
     Lines       36269    36301      +32     
     Branches     3426     3426              
   ==========================================
   - Hits        23122    21540    -1582     
   - Misses      13033    14568    +1535     
   - Partials      114      193      +79     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `59.89% <ø> (ø)` | |
   | #python | `59.01% <24.07%> (-0.37%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `57.52% <12.50%> (-0.39%)` | :arrow_down: |
   | [superset/common/query\_context.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHQucHk=) | `83.75% <20.00%> (-1.51%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.61% <30.00%> (-1.67%)` | :arrow_down: |
   | [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.32% <33.33%> (-0.23%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupColors.js](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [149 more](https://codecov.io/gh/apache/incubator-superset/pull/10548/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=footer). Last update [363abfa...ad03c1e](https://codecov.io/gh/apache/incubator-superset/pull/10548?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro commented on a change in pull request #10548: fix: handle query exceptions gracefully

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10548:
URL: https://github.com/apache/incubator-superset/pull/10548#discussion_r466938337



##########
File path: superset/connectors/sqla/models.py
##########
@@ -1193,13 +1234,13 @@ def fetch_metadata(self, commit: bool = True) -> None:
         """Fetches the metadata for the table and merges it in"""
         try:
             table_ = self.get_sqla_table_object()
-        except Exception as ex:
-            logger.exception(ex)
-            raise Exception(
+        except Exception:

Review comment:
       Good point. Having been burned by weird exceptions from SQLA I'm slightly hesitant to do this, but at least we'd catch the clean SQLA exceptions with it and probably avoid the majority of exceptions. And we can always change it to `Exception` later if needed. I'll make the change.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] villebro merged pull request #10548: fix: handle query exceptions gracefully

Posted by GitBox <gi...@apache.org>.
villebro merged pull request #10548:
URL: https://github.com/apache/incubator-superset/pull/10548


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org