Improve visibility of error message during schema retrieval #5879
Conversation
…issues with schema retrieval
redash/tasks/general.py
Outdated
| except Exception: | ||
| return {"error": {"code": 2, "message": "Error retrieving schema."}} | ||
| except Exception as e: | ||
| return {"error": {"code": 2, "message": f"Error retrieving schema: {e}"}} |
There was a problem hiding this comment.
How about we split it into a separate field (details?). This way we can better format it in the UI.
There was a problem hiding this comment.
Oh yes, this is a good idea. Let me adjust it here. Thank you for reviewing!
| return | ||
|
|
||
| logger.error(error) | ||
| raise Exception(f"Error during query execution. Reason: {error}") |
There was a problem hiding this comment.
I wonder if the Error during query execution prefix really needed or it might be confusing? (if the error happens when trying to connect, for example)
There was a problem hiding this comment.
Well, it used to be "Failed getting schema." which covered only specific query to retrieve a schema. However, now this method is being used to handle errors for the run_query method, which could run different types of queries. so I figured for readability it would be nice to specify the action which led to the error.
For the db connect issue it will still make sense, because technically the error happened during the query operation. However, I'm sure in case of db disconnect we should handle it in a deeper layer and not care during each individual query run.
Let me know what you think.
There was a problem hiding this comment.
it also aligns with the vision to split message and details so we can report properly the error to the user.
I imagine there could be other types of errors like validation, pre\post checks and so on, that is why this prefix would be useful here.
| return | ||
|
|
||
| logger.error(error) | ||
| raise Exception(f"Error during query execution. Reason: {error}") |
There was a problem hiding this comment.
Well, it used to be "Failed getting schema." which covered only specific query to retrieve a schema. However, now this method is being used to handle errors for the run_query method, which could run different types of queries. so I figured for readability it would be nice to specify the action which led to the error.
For the db connect issue it will still make sense, because technically the error happened during the query operation. However, I'm sure in case of db disconnect we should handle it in a deeper layer and not care during each individual query run.
Let me know what you think.
| return | ||
|
|
||
| logger.error(error) | ||
| raise Exception(f"Error during query execution. Reason: {error}") |
There was a problem hiding this comment.
it also aligns with the vision to split message and details so we can report properly the error to the user.
I imagine there could be other types of errors like validation, pre\post checks and so on, that is why this prefix would be useful here.
|
👍 thanks. |
…h#5879) * handle query execution error in one place. increase ability to debug issues with schema retrieval * split message and details for error reporting Co-authored-by: Dmitriy Apollonin <dmitriy.apollonin@aspireiq.com>
What type of PR is this?
Description
Right now it is impossible to debug the error during schema update. There is not logs in worker logs as well as response from the server is useless in chrome dev tools:
With the current change we:
Pass the real error message back to the task response:
Reduce code duplication by consolidating error handling in one place for future improvements.
improve bigQuery error handling to parse json for 404 errors as well as for 400th.
How is this tested?
I tested this behavior by reproducing the error during schema update operation. With this new change I can clearly see the issue in dev tools as well as in logs.
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)