RFC: Better Error Messages

[kevina]

This is a start towards giving better error messages. There is a lot still that needs to be done.

[kevina]

Notes

(1) Each package had its own error code for when a Cid could not be found. All I see this doing is creating unnecessary complexity, so I created a new errs package and created a new error ErrCidNotFound and used that instead. This does change the error message but in my view "CID not found" is a better messages than "merkeldag: not found"

(2) In order to have any hope of giving useful error messages we stop using the equivalent of error codes and instead start returning more structured error objects that include at least the Cid or Path in question that caused the error. There are many ways to do this and almost any way will break code that tests errors for equality which needs to be carefully audited. What I choose to do was introduce the concept of a wrapped error that can be "Unwrapped" to the basic error which can then be tested for equality.

[kevina]

Also, the current API for GetBlocks doesn't leave any room returning why an individual block could not be retrieved as it just returns a channel with the blocks that can be fetched. I am not sure how to fix this, we could (1) return an channel to a interface and the either return a block or an error wrapped with the Cid as to why that block could not be retrieved, (2) return a struct with the fields {cid, block, err} that will populate either block or err, (3) return a separate error channel or (4) communicate this via out of band means, although honestly this feels like a hack to me.

[kevina]

@whyrusleeping @Stebalien (others) could I get some feedback here on if I am even heading down the right path. As I see it, as we start to separate components from go-ipfs it is going to become harder and harder to make the necessary API changes required for better error messages.

[whyrusleeping]

@kevina some thoughts:

• instead of normalizing all the 'not found' error types to the same type, maybe have one of: An IsErrNotFound(err) function that we can call at the top level, or an interface for 'not found' type errors so we can do the IsErrNotFound check with the type system.
• We should probably look into using https://github.com/pkg/errors for errors. It gives us some nice functions to use, and it also operates on interfaces we can add to any of our errors. e.g. your RetrievalError could implement the causer interface from that package
• The big thing we want out of this refactor is making things less confusing to users. So having the ability to give more clear descriptions of whats gone wrong and what the user should do is important here.

[whyrusleeping]

cc @Stebalien @keks @lgierth @hsanjuan @Kubuxu @ForrestWeston @vyzo @magik6k @dignifiedquire as this will be defining standards that we will want to apply across all of our go codebases

[kevina]

Instead of normalizing all the 'not found' error types to the same type, maybe have one of: An IsErrNotFound(err) function that we can call at the top level, or an interface for 'not found' type errors so we can do the IsErrNotFound check with the type system.

The reason I wanted to do this is because I see three different error types for the same exact error with out adding an useful information. There is a lot of complicated logic that translates between the different types that having a single ErrCidNotFound type simplifies.

The big thing we want out of this refactor is making things less confusing to users. So having the ability to give more clear descriptions of whats gone wrong and what the user should do is important here.

And that is my goal, but before we probably need more structured error types as right now there is no way to pass up what CID could not be found, just an very unhelpful "not found" error.

[kevina]

Instead of normalizing all the 'not found' error types to the same type, maybe have one of: An IsErrNotFound(err) function that we can call at the top level

That might be useful in its own right, but as I mentioned in the previous problem doesn't help elimiate the complicated logic for translating between the three NotFound error types.

an interface for 'not found' type errors so we can do the IsErrNotFound check with the type system

That actually what the RetrievalError is for.

[kevina]

We should probably look into using https://github.com/pkg/errors for errors. It gives us some nice functions to use, and it also operates on interfaces we can add to any of our errors. e.g. your RetrievalError could implement the causer interface from that package

How standard is this package? It is doing a very similar thing that I had in mind, but unless it is standard I think a similar style of errors but specialized for IPFS would be better as that package is fairly simple and it would easy to replicate its functionality.

[whyrusleeping]

How standard is this package?

Pretty standard, dave cheney recommends it in his blog post and i've seen quite a few other groups using it.

[Kubuxu]

quite a lot of projects are using it so it had become quite a common occurrence and people are used to it (https://godoc.org/github.com/pkg/errors?importers)

It also has lightweight stack-tracing whenever you call Wrap. (It records return pointer from the stack, which is fast, and when you want to display the error it transforms them into function name and line if you ask it for it).

The last functionality would be very useful for us. I've spent many hours chasing errors around and this would solve it in minutes.

[dignifiedquire]

👍 on using pkg/errors, I was actually reading the mentioned blog post some time ago when trying to figure out what better ways there are in the community to give context to errors, and this seems so far the closest thing to a standard out there.

[kevina]

I read that blog post and pretty much agree with all those points. Since https://github.com/pkg/errors is widely used we can make use of it.

Given that post I propose we unexport all Sentinel errors, this will give us a handle how much we rely on them. By renaming the errors this will also catch and test for equality in which we can decide what to do with. @whyrusleeping (others) agree?

[whyrusleeping]

@kevina I think I tenatively agree with unexporting sentinel errors. Though maybe we should be taking this in stages, Doing that all at once would create a lot of churn and lead to a difficult to review PR

[Kubuxu]

and also constant conflicts. Refactor style PRs should ideally be incremental and be open for few days to avoid conflicts.

[whyrusleeping]

@kevina any update?

[kevina]

@whyrusleeping this P.R. needs to be closed and redone now that the block service is factored out and the key changes where in the blockstore. In addition the ErrNotFound in the merkledag package was moved to go-ipld-format for some reason.

[dignifiedquire]

@kevina can we close this PR for now, given your last comment?

[kevina]

Closed, but please don't delete the branch and let it get archived.