You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "withnale (via GitHub)" <gi...@apache.org> on 2025/02/27 13:29:49 UTC
[PR] fix(import): Ensure import exceptions are logged [superset]
withnale opened a new pull request, #32410:
URL: https://github.com/apache/superset/pull/32410
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
At present, if there is an import error occurs there is very little information to work with. If the import is being performed in the UI then the following error will appear...
<img width="742" alt="Screenshot 2025-02-27 at 13 24 37" src="https://github.com/user-attachments/assets/57975e01-cb14-4a7c-9e06-ecb3ef82f8f8" />
In the logs, only the following error will be generated:
```text
superset.commands.exceptions.CommandInvalidError: Error importing dashboard
```
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32410:
URL: https://github.com/apache/superset/pull/32410#discussion_r1973599933
##########
superset/commands/importers/v1/__init__.py:
##########
@@ -104,6 +107,8 @@ def validate(self) -> None: # noqa: F811
self._prevent_overwrite_existing_model(exceptions)
if exceptions:
+ for ex in exceptions:
+ logger.warning("Import Error: %s",ex)
Review Comment:
### Incorrect log level for critical errors <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using warning level for import validation errors that lead to command failure
###### Why this matters
Warning level suggests the error is not severe, but these validation errors cause command failure and should be treated as errors
###### Suggested change ∙ *Feature Preview*
Use error level logging instead:
```python
logger.error("Import validation error: %s", ex)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a836a3de-1efd-48b1-a03a-f73e46e0d256?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:c3d1c140-5af6-47f2-afd2-d8d7d43204d2 -->
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #32410:
URL: https://github.com/apache/superset/pull/32410#discussion_r1975322961
##########
superset/commands/importers/v1/__init__.py:
##########
@@ -104,6 +107,8 @@ def validate(self) -> None: # noqa: F811
self._prevent_overwrite_existing_model(exceptions)
if exceptions:
+ for ex in exceptions:
+ logger.warning("Import Error: %s",ex)
Review Comment:
This is still under `error` label so Korbit is correct, imo.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "withnale (via GitHub)" <gi...@apache.org>.
withnale commented on code in PR #32410:
URL: https://github.com/apache/superset/pull/32410#discussion_r1973817461
##########
superset/commands/importers/v1/__init__.py:
##########
@@ -104,6 +107,8 @@ def validate(self) -> None: # noqa: F811
self._prevent_overwrite_existing_model(exceptions)
if exceptions:
+ for ex in exceptions:
+ logger.warning("Import Error: %s",ex)
Review Comment:
These are not critical errors in terms of the superset app itself.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on PR #32410:
URL: https://github.com/apache/superset/pull/32410#issuecomment-2690554085
Thanks @withnale for raising PR following your issue! I left 2 reviews
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #32410:
URL: https://github.com/apache/superset/pull/32410#discussion_r1975362462
##########
superset/commands/importers/v1/__init__.py:
##########
@@ -104,6 +107,8 @@ def validate(self) -> None: # noqa: F811
self._prevent_overwrite_existing_model(exceptions)
if exceptions:
+ for ex in exceptions:
+ logger.warning("Import Error: %s", ex)
raise CommandInvalidError(
Review Comment:
L112 already raised the exceptions and surely logged by the logger. You're not seeing the exceptions?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #32410:
URL: https://github.com/apache/superset/pull/32410#issuecomment-2687969643
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #32410:
URL: https://github.com/apache/superset/pull/32410
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "withnale (via GitHub)" <gi...@apache.org>.
withnale commented on code in PR #32410:
URL: https://github.com/apache/superset/pull/32410#discussion_r1991502798
##########
superset/commands/importers/v1/__init__.py:
##########
@@ -104,6 +107,8 @@ def validate(self) -> None: # noqa: F811
self._prevent_overwrite_existing_model(exceptions)
if exceptions:
+ for ex in exceptions:
+ logger.warning("Import Error: %s", ex)
raise CommandInvalidError(
Review Comment:
Exceptions is a list. The existing code path will just add an exception to the log that looks like this....
```
2025-03-12 13:18:35,465:WARNING:superset.views.error_handling:CommandException
Traceback (most recent call last):
File "/Users/testuser/.venv/lib/python3.11/site-packages/flask/app.py", line 1484, in full_dispatch_request
rv = self.dispatch_request()
^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/testuser/.venv/lib/python3.11/site-packages/flask/app.py", line 1469, in dispatch_request
return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/testuser/.venv/lib/python3.11/site-packages/flask_appbuilder/security/decorators.py", line 95, in wraps
return f(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/testuser/superset/views/base_api.py", line 119, in wraps
duration, response = time_function(f, self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/testuser/superset/utils/core.py", line 1364, in time_function
response = func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/Users/testuser/superset/utils/log.py", line 303, in wrapper
value = f(*args, **kwargs)
^^^^^^^^^^^^^^^^^^
File "/Users/testuser/superset/views/base_api.py", line 106, in wraps
return f(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/testuser/superset/dashboards/api.py", line 1408, in import_
command.run()
File "/Users/testuser/superset/commands/dashboard/importers/dispatcher.py", line 57, in run
command.run()
File "/Users/testuser/superset/utils/decorators.py", line 266, in wrapped
return on_error(ex)
^^^^^^^^^^^^
File "/Users/testuser/superset/utils/decorators.py", line 236, in on_error
raise ex
File "/Users/testuser/superset/utils/decorators.py", line 259, in wrapped
result = func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/Users/testuser/superset/commands/importers/v1/__init__.py", line 73, in run
self.validate()
File "/Users/testuser/superset/commands/importers/v1/__init__.py", line 107, in validate
raise CommandInvalidError(
superset.commands.exceptions.CommandInvalidError: Error importing dashboard
```
This log message tells me there was a problem importing the dashboard(s) but gives me no context as to what the problems were. The exceptions list contains all the individual errors associated with each resource that failed validation but these are just discarded without the attached 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #32410:
URL: https://github.com/apache/superset/pull/32410#issuecomment-2688771770
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/32410?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `50.00000%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 65.56%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`61f3477`)](https://app.codecov.io/gh/apache/superset/commit/61f3477437a188fb8ec2d24168c72016da9935cc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1500 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/32410?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/importers/v1/\_\_init\_\_.py](https://app.codecov.io/gh/apache/superset/pull/32410?src=pr&el=tree&filepath=superset%2Fcommands%2Fimporters%2Fv1%2F__init__.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL19faW5pdF9fLnB5) | 50.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/32410?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #32410 +/- ##
==========================================
+ Coverage 60.48% 65.56% +5.08%
==========================================
Files 1931 546 -1385
Lines 76236 39095 -37141
Branches 8568 0 -8568
==========================================
- Hits 46114 25634 -20480
+ Misses 28017 13461 -14556
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/32410/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/32410/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.52% <50.00%> (-0.64%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/32410/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/32410/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.05% <50.00%> (-0.75%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/32410/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.56% <50.00%> (+2.08%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/32410/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.11% <50.00%> (+3.48%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/32410?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] fix(import): Ensure import exceptions are logged [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #32410:
URL: https://github.com/apache/superset/pull/32410#discussion_r1975362462
##########
superset/commands/importers/v1/__init__.py:
##########
@@ -104,6 +107,8 @@ def validate(self) -> None: # noqa: F811
self._prevent_overwrite_existing_model(exceptions)
if exceptions:
+ for ex in exceptions:
+ logger.warning("Import Error: %s", ex)
raise CommandInvalidError(
Review Comment:
This line already raised the exceptions and surely logged by the logger. You're not seeing the exceptions?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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