Unwrap ErrorPolykeyRemote when reporting errors
#107
Comments
|
Only the commands that are performing remote calls should unwrap it. This is because they know the reason why, the reason is due to the remote side failing. However there's a caveat. This is what it looks like when there's a remote side error: You can see here that it's a bit verbose firstly with alot of metadata. This is being discussed in ENG-119 or #17. Additionally the error formatting isn't properly being done recursively. There's another cause after the first cause, and that's just JSON now. Now the main problem here is how do we properly communicate that the error is coming from the remote side, and it's not an error of the local side. This is where the command that's receiving this error needs to catch and interrogate it. I can imagine that if we can reduce verbosity, it would look like: Yet I still don't like this. There's an old issue I cannot find anymore that talked flipping this chain order. Because the error that I do care about is actually the last error. If the errors are really long, it obviously favours the last message, but if they are short, it's clearer to write down the actual error, and the wrapper. But then there's also the fact that one has to be discerning about the level of errors that's relevant. Only the command understands the context. Which is to say, it knows that ErrorSecretsSecretUndefined is what really matters. In which case, it should drop irrelevant information assuming low verbosity error reporting and say: This would be an interesting way of reporting it because it's not exactly how the structure of the error objects themselves are setup. You have to invert the chain to report it. It could be confusing if later we increase error verbosity and we end up reversing it again back to the normal object chain in order to show all the error object properties properly. It also wouldn't match the JSON version of errors too. Finally the command also has enough context to to specifically understand that Additionally we may drop descriptions in favour of messages if the message is sufficient. We may keep the description in case the message doesn't exist. This makes the description a sort of "default message". Thus giving us: Now why do we keep the I still don't quite like this. I think instead the "code" in this case being the unique error class name, should actually be on the same line. All of our errors should be easily explained in one line (or can they…?) The While the inverted chain may be better in just understanding, the fact that it ends up being flipped when it is actually verbose I think is a bad trade off. |
I do prefer the dash. |
|
So that would mean we don't technically unwrap it, because we do still need to indicate whether it is a local error or a remote error. Inversion is too confusing when the high verbosity output will be in the regular format. So the only thing really here is verbosity itself, and changing it so that the error class name is at the very end of the message. This assumes all error messages can be explained in one short line, and that messages override descriptions when reported, so that descriptions is acting as the default error message. What if the error message is far longer, and has to take up multiple lines? |
|
I personally really like the python way of handling errors, and I believe it would be easier to read for non-technical users. Something like this might work: I just wrote this test script which raises two errors. Here, not much nesting or indentation is seen, but all the information is provided to the reader. Of course, we might not have the details like the file name or line number, but that can be replaced with the error that was called, etc. For example, this is how I currently do error displaying in #255 [aryanj@matrix-34xx]$ pk secrets ls newvault:/doesntexist
ErrorPolykeyCLIFileRead: Failed to read from filesystem: Failed to read directory: /doesntexistI did this by catching the thrown remote error, and throwing the unwrapped error with the same details without any additional metadata. I will remove this before merging the PR, but this made it much easier to read and understand what is going on and what went wrong during development. |
|
I'm very aware of how python does their errors. Make sure you read my post to the end! Our cause chain is a more streamlined version of python traces. |
|
My favourite is this atm. But read what I wrote earlier as it explains that iteration. |
|
Technically pythons report is actually the inverted version of ours. In a way it's doing a stack trace which is inverted. The object error chain goes outside in, while the stack trace goes inside out. |
|
This is kinda related to MatrixAI/js-errors#21, so I might take over this too. How do we want to deal with this? Is there a format or standard that we have decided upon?
This changes the way we currently format errors. I've made some comments on how we can update the formatting of errors too. MatrixAI/js-errors#21 (comment) |
|
Can you read my comment and summarise the tradeoffs and preferred style? |
Error Reporting Formats: Summary and Analysis1. Verbose Nested FormatExample: Pros:
Cons:
2. Simplified Chain FormatExample: Pros:
Cons:
3. Inverted Chain FormatExample: Pros:
Cons:
4. Reduced, Context-Aware FormatExample: Pros:
Cons:
5. Flat One-Line FormatExample: Pros:
Cons:
6. Python-Style Traceback FormatExample: Pros:
Cons:
7. Minimalist, Human-Friendly FormatExample: Pros:
Cons:
Recommendations
Updated Error Reporting: Conciseness for End Users vs. Verbosity for LogsThe suggestion aligns with creating a dual-layered error reporting system:
Comparison of Proposed Error Formats1. Current Verbose Nested Format Example: Pros:
Cons:
2. Concise User-Facing Format Example: Pros:
Cons:
3. Enhanced Concise Format with Cause Example: Pros:
Cons:
4. Detailed Error Chain for Power Users Example: Pros:
Cons:
5. Dual-Layered Approach User-Facing Error: Log-Facing Error: Pros:
Cons:
Updated Recommendations
Final Example OutputUser-Facing (Low Verbosity): User-Facing (Moderate Verbosity): Log-Facing (High Verbosity): This approach ensures readability, usability, and traceability across diverse user roles and scenarios. |
|
I still think that the errors on the front-facing CLI shouldn't have too much information, and concise one-liners are the way to go here to inform the user. These messages can be printed to the logs in full verbosity like we currently see. |
|
There are 2 slightly conflicting goals here. One is a useful error message to the end user so they know how to CORRECT their "usage" of the program. Another is a useful error message for developers to understand how to CORRECT the behaviour of the program. This means, when we report errors on the CLI, we really want a system that can do both, because when an error occurs, it could occur for both reasons, it's hard to know. And when the user needs to submit a but report, it's important for them to be able to copy the error output and provide that information. In order to achieve this, we need to distinguish "usage" errors vs non-usage errors. We actually have this already in the sysexit codes. However those are defined on an exception basis, whereas what is considered a user error and what is considered a behaviour error sometimes depends on the command itself. So we can have a "base" distinction on the sysexit, and then a more derivative distinction occur be dispatched on the command handler itself. What this means is that at the UI level, usage errors should be simple and to the point. But behaviour errors need to be explicit and detailed. |
|
I can push up a quick fix which strips the |
Specification
When an RPC call fails it throws
ErrorPolykeyRemotewith the true error as the cause. This is very noisy when printed by the CLI command and is frankly confusing at times. When reporting these errors we want to unwrap theErrorPolykeyRemoteand just report the actual cause of the error.Additional context
--verbosityflags #17Tasks
ErrorPolykeyRemoteerror and just report the cause when reporting the RPC errors.The text was updated successfully, but these errors were encountered: