Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

presto error messages are hard to read #1529

Closed
cldellow opened this issue Jan 16, 2017 · 7 comments
Closed

presto error messages are hard to read #1529

cldellow opened this issue Jan 16, 2017 · 7 comments

Comments

@cldellow
Copy link
Contributor

Issue Summary

Errors for the presto query runner produce a hard-to-read stringified JSON blob, e.g.:

presto-error-old

It'd probably be useful to just have the embedded message field instead of the entire thing -- the JSON escaping and and Presto stack trace are rarely relevant to the user.

Steps to Reproduce

  1. Run a query that will fail, e.g. 'SELECT foo FROM bar WHERE baz = 1'

Technical details:

  • Redash Version: 0.12.0+b2449
@cldellow
Copy link
Contributor Author

Note, an easy "fix" is to patch query_runner/presto.py and add:

if type(error) is dict and 'failureInfo' in error and 'message' in error['failureInfo']:
    error = error['failureInfo']['message']

to pull our the error message.

Then you get error messages like:

presto-error-new

That feels kinda hacky though -- maybe there's a reason the fuller blob is being sent? If not, I can put together a PR that makes this change.

@arikfr
Copy link
Member

arikfr commented Jan 19, 2017

This definitely looks nicer! But I agree that the implementation feels hacky. The snippet you posted on what line in preto.py does it belong?

@cldellow
Copy link
Contributor Author

@arikfr
Copy link
Member

arikfr commented Jan 19, 2017

Maybe to make it less hacky we catch a more specific exception class there rather than just Exception?

@aslotnick
Copy link
Contributor

@arikfr I think this PR should address this issue - anything I should change? I've provided a default and caught the specific exception (DatabaseError) but the overall effect is the same as what @cldellow has proposed.

@arikfr
Copy link
Member

arikfr commented Feb 15, 2017

Thanks @aslotnick ! Merged.

@arikfr arikfr closed this as completed Feb 15, 2017
@aslotnick
Copy link
Contributor

Thank you, @arikfr ! Happy to finally contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants