Simplify error reporting in default case
Closed this issue · 16 comments
When the CLI fails we get output that looks like:
[stratis-cli]$ PYTHONPATH=./src ./bin/stratis pool create fubar /dev/sde
Execution failure caused by:
Rejected send message, 2 matched rules; type="method_call", sender=":1.855" (uid=1000 pid=21926 comm="python3 ./bin/stratis pool create fubar /dev/sde " label="unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023") interface="org.storage.stratis1.Manager" member="CreatePool" error name="(unset)" requested_reply="0" destination=":1.852" (uid=0 pid=21858 comm="./target/x86_64-unknown-linux-gnu/debug/stratisd -" label="unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023")
which in turn caused:
Error while invoking method "CreatePool" belonging to interface "org.storage.stratis1.Manager" with arguments (fubar, dbus.Struct((dbus.Boolean(True), dbus.UInt16(0)), signature=dbus.Signature('bq')), 0, dbus.Array([dbus.String('/dev/sde')], signature=dbus.Signature('s')))
Most likely you have insufficient permissions for the action you specified.
Should we consider making the output a little less verbose in the default case?
ref. #165
Another example:
PYTHONPATH=./src ./bin/stratis pool create fubar /dev/sde
Execution failure caused by:
Could not get owner of name 'org.storage.stratis1': no such name
which in turn caused:
The name org.storage.stratis1 was not provided by any .service files
Most likely the Stratis daemon, stratisd, is not running.
These are two examples of the general format. The last line, starting with "Most likely..." is a best guess based on the exception chain, and may not always exist.
We can simplify error reporting in the default case by just printing "Something went wrong." whenever we catch an exception. That doesn't seem to me like an improvement over what we currently have though, because it's less informative than what we currently have. So, what would be an improvement over what we currently have, or at least simpler but not much less helpful?
How about if last error and the "guess" (if present) to be the only thing output unless some flag is given. (This could be our friend --propagate or a new one)
I also suggest a different template for the guess. We should avoid "you", and I think "may" may flow a little better than "most likely". e.g.:
The Stratis daemon, stratisd, may not be running.
This command may require additional privileges. Try as root?
There may be an error at line....
There are three issues now.
-
The use of the
--propagateflag. There is a very real distinction between what the program does when executed by the interpreter and what the interpreter does all on its own. The purpose of--propagateis to allow the exception to propagate and to let the interpreter handle it in whatever way the interpreter is set up to handle it. That should continue to be the case for strong theoretical and practical reasons. I won't go into the theoretical reasons unless requested; one practical reason is that at least 16 of our existing tests check the exception raised. Make--propagatedo something other than it does and those 16 tests will fail. If fixed up, they will not be as useful as they are now; if removed they will be quite useless. There are certainly more practical reasons, and I can discuss those if requested. -
The wording of the interpretation sentences. I think this is a separate issue and mostly cultural. It probably deserves a separate GitHub issue.
-
The default format of the error messages. This is easy to alter; all the code is localized to
_error_reporting.py. But we should have a clear idea of the goal and expected outcome of the change before writing the code to make the alterations. We used to just print out the most-recently raised exception and we changed that to the current set-up because that was unsatisfactory: #124. If we flip-flop back to what we had previously, we won't have accomplished that much. Now we have interpretation strings and the exception chain we get with--propagateis far more helpful than it used to be as a result of a lot of subsequent work, but those are all just nice side-effects which won't change that much what the regular user would see with the default output.
Split 2 off into #169.
IMO this is less of a flip flop and more of a refinement. We want the full error chain available if desired, but we don't want it displayed by default. Maybe display it with a --show-all-errors option or something? (Sounds like my suggestion to overload --propagate is not the way to go, ok.)
All I'm saying regarding my point (3) is that we should have a clearly stated plan and expectation of the result if that plan is executed before any coding is done. What is the "last error"? The tail of the exception chain or its head? Why pick that one? If the goal is to be less verbose, we could just as well pick the one with the shortest error message, whatever it happened to be. Superficially that seems like a bad idea, but with a little more consideration it might become clear that it's probably just as good as any other choice.
We want to try to present the user with one error message, the one that is the most relevant, useful, and actionable from their perspective. How can we best achieve this?
That is the question.
My answer is print the last entry in the chain and the interpretation if available. Lets start with doing that, and we can change it if we come up with something better.
That would look like this in the example:
[stratis-cli]$ PYTHONPATH=./src ./bin/stratis pool create fubar /dev/sde
Execution failure caused by:
Rejected send message, 2 matched rules; type="method_call", sender=":1.855" (uid=1000 pid=21926 comm="python3 ./bin/stratis pool create fubar /dev/sde " label="unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023") interface="org.storage.stratis1.Manager" member="CreatePool" error name="(unset)" requested_reply="0" destination=":1.852" (uid=0 pid=21858 comm="./target/x86_64-unknown-linux-gnu/debug/stratisd -" label="unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023")
Most likely you have insufficient permissions for the action you specified.
assuming I understand what you mean by "last". As far as I am concerned, what the code does right now is just as good as that.
No, I meant the other last -- the "topmost" error.
I just want a message that says "you don't have permissions, you need to run as root", without a bunch of programmerish gobbeldygook. Unless --with--gobbeldygook option is given.
Ok, so the issue seems to be that aside from the cases where there's an explanation message, all the error messages in the chain are not that helpful (which is why @mulkieran added the Explanation message in the first place.)
So, if we don't have an Explanation, then let's print a generic error message like: "Unexplained error. Re-run command with '--propagate' flag for full error information".
This is admittedly not that helpful to the user, but we should be trying to add friendly Explanations for common errors so that this is rare.
Let's table this until we get some feedback from our non-developer users. So far, there have been a bunch of bugs reported, and nobody has complained about the excessively verbose error messages. I propose moving it off Stratis 1.1.
ok.
I'm going to close this and wait until external users have specific complaints.
Another alternative that might be reasonable: if an explanation exists, just show that, and if not propagate the exception. That would force us to confront propagated exceptions as bugs, which could be healthy.