-
Notifications
You must be signed in to change notification settings - Fork 7
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
Reduce exception info for chadmin output #235
Conversation
Test fail is related to #231 |
logging.disable_stdout_logger() | ||
logging.exception("Command '{}' failed with error:", cmd.name) | ||
logging.print_last_exception(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For another logging.exception(
calls the behaviour will be as before? Should we log only the latest exc inside logging.exception
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging.exception
behaviour is not changed except for backtrace
option which cuts unnecessary part of last stack trace:
File "/usr/bin/chadmin", line 8, in <module>
sys.exit(main())
File "/opt/yandex/clickhouse-tools/lib/python3.6/site-packages/ch_tools/chadmin/chadmin_cli.py", line 152, in main
cli.main()
File "/opt/yandex/clickhouse-tools/lib/python3.6/site-packages/click/core.py", line 1053, in main
rv = self.invoke(ctx)
File "/opt/yandex/clickhouse-tools/lib/python3.6/site-packages/click/core.py", line 1659, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/opt/yandex/clickhouse-tools/lib/python3.6/site-packages/click/core.py", line 1659, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/opt/yandex/clickhouse-tools/lib/python3.6/site-packages/click/core.py", line 1395, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/opt/yandex/clickhouse-tools/lib/python3.6/site-packages/click/core.py", line 754, in invoke
return __callback(*args, **kwargs)
File "/opt/yandex/clickhouse-tools/lib/python3.6/site-packages/click/decorators.py", line 26, in new_func
logging.exception
still logs all exceptions in case they are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey, so we don't want to enable it in all cases?
Also, maybe move it to separate method in logging or put it in the logging.exception
with option(eg logging.exception(...., show_all_exceptions)
) ? Think it will much more beautiful:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way to tell if we will need all exceptions or just one, that's why I think we should always log all of them. logging.exception
is called only once per command, logs will not be flooded with exceptions anyway.
Or did you mean to put logging.print_last_exception(e)
inside logging.exception
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or did you mean to put logging.print_last_exception(e) inside logging.exception?
Yes
@kirillgarbar I made small refactoring, could you check and approve if everything is fine |
Only last exception without diagnose is printed, but full exception is logged.
Logged example:
Printed example: