Improve error reporting in controller#1017
Conversation
|
@kaccardi to review |
| t, err := ds.getTenant(id) | ||
| if err != nil || t == nil { | ||
| glog.V(2).Info(err, " unable to get tenant: ", id) | ||
| return nil, err |
There was a problem hiding this comment.
Yes intentionally base. As ds.getTenant() wraps and we can't add any useful context over that one.
| ts, err := ds.getTenants() | ||
|
|
||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Yes, as we have nothing useful to add over the message in ds.getTenants()
There was a problem hiding this comment.
Ok makes sense. ds.getTenants() either does something that can't fail or calls ds.db.getTenants(), where the latter would do an errors.Wrap().
| if err == nil { | ||
| err = errors.Wrapf(err, "error releasing IP for instance (%v)", i.ID) | ||
| } | ||
| } |
There was a problem hiding this comment.
Are the conditional wraps here required? I understood from https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully examples that you could just wrap nil and the right thing happened.
And is keeping the glog useful? I understood the idiom with errors.Wrap() to be one where you no longer do that.
There was a problem hiding this comment.
Good point. I checked the code and Wrap(nil, "My message") returns nil.
There was a problem hiding this comment.
In this case I want to continue to do further cleanups and not return an error immediately. So therefore i'm logging so that both potentially errors will be seen even if only the first one is returned.
Hence why I save the error result into tmpError and then update err with the one that will be actually returned.
There was a problem hiding this comment.
I was thinking in this function you could unconditionally do a series of errors.Wrapf(err, "text") and you'd end up with the cumulative set of wrapped errors if multiple, or the single one if just one, or none if none, and then return which ever it is in the single err at the end.
There was a problem hiding this comment.
I think that would be weird as you normally do that for a call chain, not multiple errors in the same function.
There was a problem hiding this comment.
The officially expected usage paradigm is unclear. But from reading the documentation and watching the video, I see the wrap function as adding information to a stack of events where the stack is multiple errors. Whether those are from one function or a series of functions doesn't seem different to me in the abstract.
| if tmpErr := ds.db.addFrameStat(i); tmpErr != nil { | ||
| if err != nil { | ||
| err = errors.Wrapf(tmpErr, "error adding stats to database") | ||
| } |
There was a problem hiding this comment.
Another one where I thought you could just wrap unconditionally.
There was a problem hiding this comment.
Here I want to continue around the loop even if any one errors out. The code is not consistent on this but i'm simply trying to adhere to the previous behaviour as closely as possible. A later cleanup might decide it's fine to abort halfway through the loop.
There is a bug here though, it should say if err == nil not err != nil.
Where we are able to return an error, wrap the error so that there is some useful context available for debugging. Where we are not able to return an error (or on paths with multiple potential errors) ensure that the errors that do occur are consistently logged. Signed-off-by: Rob Bradford <robert.bradford@intel.com>
The prototype returns an error but there is no way for this code to generate an error so modify the prototype and users not to generate an error. Fixes: ciao-project#1008 Signed-off-by: Rob Bradford <robert.bradford@intel.com>
2954d22 to
0bc450a
Compare
kaccardi
left a comment
There was a problem hiding this comment.
LGTM, thanks for the cleanup!
Where we are able to return an error, wrap the error so that there is some useful context available for debugging.
Where we are not able to return an error (or on paths with multiple potential errors) ensure that the errors that do occur are logged in a consistent manner.
Also modify the prototype of GetStorageAttachments not to return an error as it cannot do so.