chore: handle errors in wsproxy server for cli using buildinfo #11584
Conversation
| // traceError is a helper function that aides developers debugging failed cli | ||
| // commands. When we pretty print errors, we lose the context in which they came. | ||
| // This function adds the context back. Unfortunately there is no easy way to get | ||
| // the prefix to: "error string: %w", so we do a bit of string manipulation. | ||
| // | ||
| //nolint:errorlint | ||
| func traceError(err error) string { | ||
| if uw, ok := err.(interface{ Unwrap() error }); ok { | ||
| a, b := err.Error(), uw.Unwrap().Error() | ||
| c := strings.TrimSuffix(a, b) | ||
| return c | ||
| } | ||
| return err.Error() | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the biggest thing I added, and it only prints in verbose mode.
Essentially when we wrap some errors, we lose the context because the pretty printer doesn't know what to do with them.
An example is:
return fmt.Errorf("fetch buildinfo: %w", err)
The underlying err is an SDK error, so the context fetch buildinfo: is lost.
This code aims to put that back in somewhere that is reasonable, so if you run -v in your command, that info returns.
I am debating just always printing this, because I can't image it gets too unwieldly.
There was a problem hiding this comment.
I decided to always print this. Without it, it is really hard to debug where some api calls fail.
| @@ -65,14 +70,28 @@ func (RootCmd) errorExample() *clibase.Cmd { | |||
| // A multi-error | |||
| { | |||
| Use: "multi-error", | |||
| Handler: func(inv *clibase.Invocation) error { | |||
There was a problem hiding this comment.
should we hide this command? There's no reason a user would ever need to use this.
There was a problem hiding this comment.
It is hidden up the stack:
Line 12 in 9656c7a
|
In this stack:
This stack of pull requests is managed by Graphite. Learn more about stacking. |
We can. We can make it a followup PR. I don't have the bandwidth atm to add that |
|
@Emyrk could you open an issue so that we can track and not forget? |
See for yourself. I don't think encoding this into a static unit test would be very helpful. It would just cost more time to fix when you change things. Maybe a golden-file type test would be appropriate here at some point.
Before
After
We have to show the html because we have no clue what the response body is. This error is not perfect, but it is a lot better.