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

[dev.icinga.com #12654] Exceptions on CLI should NOT show exception details #2546

Closed
icinga-migration opened this issue Sep 6, 2016 · 4 comments
Labels
area/cli Affects the command line (icingacli) bug Something isn't working

Comments

@icinga-migration
Copy link

This issue has been migrated from Redmine: https://dev.icinga.com/issues/12654

Created by tgelf on 2016-09-06 08:12:35 +00:00

Assignee: (none)
Status: New
Target Version: Backlog
Last Update: 2016-10-04 11:51:35 +00:00 (in Redmine)


Exceptions on CLI should NOT show exception details (neither class nor file or line) unless -trace is passed. Especially not when the exception got triggered intentionally by issuing $this>fail() in a command action. Currently the filename is often longer than the real error message, when your output is wrapped or shortened (e.g. default system/journalctl output) you can not even read what really happened.

Reasoning: CLI is for admins, they need readable error messages. Who wants to see a stack trace can add --trace.

@icinga-migration
Copy link
Author

Updated by jmeyer on 2016-09-06 09:13:33 +00:00

No. --trace has nothing to do with this and won't help anyone if the error is not that easily reproducible.

What is actually required here, is a more advanced error handling for the CLI. I'm with you about handled errors, these should focus on being easy to read. But unhandled exceptions must include the file and line at the very least.

What I'd like to see is the same what we currently have for forms:

  • Command::error()
  • Command::warning()
  • Command::info()

These will emit easy to read and possibly colored messages.

Command::fail() on the other hand should preferably not adjusted in any way and be handled like any other exception. For me it's a convenience method for throwing a new exception, nothing else. But that's open for discussion I guess. Whatever will happen with Command::fail(), we must ensure that all modules using this method are properly adjusted so that the new error handling is used instead.

@icinga-migration
Copy link
Author

Updated by tgelf on 2016-09-06 10:14:51 +00:00

Meaningful exception messages are mostly unique or at least rarely distributed, so a quick grep should in most cases lead you to the desired target. If the message makes no sense it's a bug in the throwing class.

I do not remember to have been interested in this information even once, and I write LOTS of CLI commands in Web 2 modules. And where the fail() shortcut is in use it is just absurd, as the provided information never makes any sense:

ERROR: Icinga\Exception\IcingaException in /usr/local/icingaweb2/library/Icinga/Cli/Command.php:141 with message: This didn't work well

Sure, this could be fixed in fail(), but I'm not interested in this information at all. Never. More practical example, cronjob fails because of DB outage. A sysadmin wants to read...

ERROR: Connection to MySQL on 192.0.2.10:3306 failed

...when something went wrong with the DB. And not something like this:

ERROR: Icinga\Exception\WhateverFancyException in /usr/local/icingaweb2/module/monitoring/library/Monitoring/Backend/Ido/IdoBackend.php:1233 with message: Connection to MySQL on 192.0.2.10:3306 failed

To better understand your reasoning: could you please show me one of our commands where full exception class name, filename and line make any sense?

Regarding your other examples: that's what we use the Logger for. Sure, there could also be shortcuts - but that's completely unrelated to this issue.

Cheers,
Thomas

@icinga-migration
Copy link
Author

Updated by jmeyer on 2016-09-06 11:58:00 +00:00

Ah well, yes, you're right. The Logger class is for beautiful error messages. Then scratch that. ;)

The point of my previous comment was to clarify that I want to see the small stacktrace regardless whether --trace is passed or not, if an unhandled exception occurred.

To be clear: The error handling in class Icinga\Cli\Loader was effectively masking every exception, whether handled or not. This must not be introduced again when fixing Command::fail().

@icinga-migration
Copy link
Author

Updated by jmeyer on 2016-10-04 11:51:35 +00:00

  • Category set to CLI
  • Target Version set to Backlog

Well, I had a discussion a few weeks ago with Thomas but forgot what we "agreed" upon. However, our two standpoints were as follows:

  • The output on the CLI must focus on the error message itself and must not get lost in case a message limit is in effect (e.g. syslog)
  • Unhandled exceptions must not get lost as they may not that easily reproducible (e.g. An import/export which is running a few hours/days and fails near the end)

A solution that considers both standpoints is still to be defined.

@icinga-migration icinga-migration added bug Something isn't working area/cli Affects the command line (icingacli) labels Jan 17, 2017
@icinga-migration icinga-migration added this to the Backlog milestone Jan 17, 2017
@lippserd lippserd removed this from the Backlog milestone Apr 11, 2018
@lippserd lippserd closed this as completed May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Affects the command line (icingacli) bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants