Wrap errors so that details are more visible

[olivere]

Use e.g. github.com/pkg/errors to wrap certain errors and make them more visible.

The goal is to make the underlying reason for certain errors more transparent, e.g. elastic.ErrNoClient.

[eticzon]

@olivere if you don't mind, I'd like to get cracking on this one next. What do you think should the strategy here be? Should we do it in phases (i.e. per ES endpoint) or one big bang?

Also we might want to consider drafting a list of "underlying reasons" or "friendly messages" for certain error classes. I'm happy to draft one up for your review.

[olivere]

@eticzon Thanks for your helping hand.

It'd be awesome if we could start with those elastic.ErrNoClient errors. That's probably the no. 1 issue that people have issues with. If we could provide a useful error message for each of the underlying problem for each one of those case, that'd help a lot (just search for "No ElasticSearch Node Available" in the issues).

Right now, elastic is swalling most of the underlying errors. It could probably be as simple as wrapping the underlying error via github.com/pkg/errors.

[eticzon]

Thanks for the quick reply. 👍 Ok I'll start with those elastic.ErrNoClient ones first.

[olivere]

I also added the help appreciated label so you can quickly decide what'd be interesting for you (or anyone willing to spend some time to support this little library of ours).

[eticzon]

+1! Thanks for that.

[eticzon]

I've opened #518 to get the ball rolling.

Let me know if this shallow errors.Wrap is good for now and if you think we need to add more tests to check that we're correctly adding detail to every single error scenario.

[olivere]

I like the error messages. Not too chatty and to the point. 👍

However, the wrapping could become a problem in that existing clients can no longer compare err == elastic.ErrNoClient. I mean, they can, but it won't be the same as before. This test will e.g. fail now:

func TestClientHealthcheckStartupTimeout(t *testing.T) {
	_, err := elastic.NewClient(
		elastic.SetURL("http://localhost:9299"),
		elastic.SetHealthcheckTimeoutStartup(5*time.Second),
	)
	if err != elastic.ErrNoClient {
		t.Fatal(err)
	}
}

... because it will directly compare the error with the elastic.ErrNoClient. As it's wrapped now, it will not be equal to elastic.ErrNoClient while before it was!

I personally don't think it's too critical as this typically only happens at startup and clients should typically check if there's an error at all, not the specific type... But maybe we should be more explicit about the change.

I think we have two options:

1. Ask implementors to import github.com/pkg/errors and switch to errors.Cause(err) == elastic.ErrNoClient to replace the above code.
2. Rename elastic.ErrNoClient and choose a different error, just to ensure that a compile time error will be raised if users rely on that, e.g. in an error check. This at list gives implementors a hint that they need to change something, otherwise something might break without warning (option 1 doesn't give this hint, and things will break silently).

Regardless of 1 or 2, we should probably provide a helper function like elastic.IsConnErr(err error) that allows implementors to check for what used to be elastic.ErrNoClient, but without forcing users to import github.com/pkg/errors. That way we keep the freedom to switch to a different "error wrapper" in the future without affecting Elastic users.

What is your opinion about this?

[eticzon]

Thanks for the feedback @olivere. 👍

Actually, to make the tests pass I already did item 1 (like what you've mentioned) in the PR:

func TestClientHealthcheckStartupTimeout(t *testing.T) {
    start := time.Now()
    _, err := NewClient(SetURL("http://localhost:9299"), SetHealthcheckTimeoutStartup(5*time.Second))
    duration := time.Now().Sub(start)
    if errors.Cause(err) != ErrNoClient {
        t.Fatal(err)
    }
    if duration < 5*time.Second {
        t.Fatalf("expected a timeout in more than 5 seconds; got: %v", duration)
    }
}

Out of the two options, I would prefer item 1. IMHO, item 2 will result to the same thing even if we rename the error (unless you're also planning on introducing other error types). Maybe there's something I'm missing here?

I do agree that we should have the helper function. That way the tests won't also be coupled with the github/pkg/errors package.

Also that goes in line with what is said in this blog:

https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully
(Refer to the definition of the IsTemporary() helper function).

[eticzon]

I've updated #518 to add the helper func. Let me know if you're ok with this change. Thanks.

[olivere]

Sorry for the lack of feedback. I've been out for a few days...

I think the helper is good. And as far as I see there's no need to import github.com/pkg/errors for clients if they're using that helper, no?

I'll give it another 2-3 days and hopefully find the time to merge over the weekend. Thanks again for your helping hand!

[eticzon]

Not a problem at all.

I think the helper is good. And as far as I see there's no need to import github.com/pkg/errors for clients if they're using that helper, no?

We've decoupled client_test.go but we'll still need to import the library for the client though. We could just write our own implementation of errors.Wrap and errors.Cause. It should be straightforward enough. WDYT?

[olivere]

I think importing github.com/pkg/errors from within gopkg.in/olivere/elastic.v5 is okay.

What I'd dislike is that users of gopkg.in/olivere/elastic.v5 would need to import github.com/pkg/errors, just for checking for a connection error. And as far as I see, they don't have to, because they could simply use elastic.IsConnErr(...).

[olivere]

I just merged the changes. Thanks for your support!

[eticzon]

You're welcome @olivere!