Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recommended way of handling errors. #1793

Closed
michaelbudnik opened this issue Feb 2, 2016 · 7 comments
Closed

Recommended way of handling errors. #1793

michaelbudnik opened this issue Feb 2, 2016 · 7 comments

Comments

@michaelbudnik
Copy link
Contributor

What is the recommended way of handling errors with 2.x?
IsValid and ServerError? What about IApiCallDetails?
Thanks

@gmarz
Copy link
Contributor

gmarz commented Feb 2, 2016

It depends on whether you have exceptions enabled or not (via ConnectionSettings.ThrowExceptions().

First, let's summarize the differences quickly:

ServerError: The actual structured error response from Elasticsearch. In other words, the request made it to the server but something went wrong in Elasticsearch (query parsing exception, non-existent index, etc...)

OriginalException: The actual underlying CLR exception, whether an Elasticsearch error occurred or a connection error (the server could not be reached, request timed out), etc.

If you're on .NET 4.5/4.6, then HttpConnection uses HttpWebRequest which throws on all bad status codes, so if an ES error occurs, both ServerError and OriginalException will be populated.

For .NET Core, HttpConnection uses HttpClient which behaves differently. A 404 considered a valid status code. So in most cases either ServerError will be populated on Elasticsearch errors, or OriginalException if the server couldn't be reached.

With exceptions turned off (default)...

You use the IsValid property to check if anything went wrong, either an Elasticsearch error or a connection error. You can then inspect either ServerError or OriginalException on the response.
IsValid also provides a layer of abstraction over some API calls. For instance, on a bulk request if some items fail, ES will return a successful response, but IsValid will be false hinting that you need to check which operations failed by inspecting ItemsWithErrors on IBulkResponse.

With exceptions turned on...

Three types of exceptions will bubble out:

ElasticsearchClientException: These are known exceptions, either an exception that occurred in the request pipeline (such as max retries or timeout reached, bad authentication, etc...) or Elasticsearch itself returned an error (could not parse the request, bad query, missing field, etc...). If it is an Elasticsearch error, the ServerError property on the response will contain the the actual error that was returned. The inner exception will always contain the root causing exception.

UnexpectedElasticsearchClientException: These are unknown exceptions, for instance a response from Elasticsearch not properly deserialized. These are usually bugs and should be reported. This excpetion also inherits from ElasticsearchClientException so an additional catch block isn't necessary, but can be helpful in distinguishing between the two.

Development time exceptions: These are CLR exceptions like ArgumentException, NullArgumentException etc... that are thrown when an API in the client is misused. These should not be handled as you want to know about them during development.

We've been contemplating (internally) creating a single type that encapsulates and simplifies all of this information, and makes it easier to error check on the response. Additionally, we want to add DebugInformation on the response that will pretty print all of the error information. I'll open a new issue for this.

@michaelbudnik
Copy link
Contributor Author

Thanks for the explanation =)

@godefroi
Copy link

godefroi commented Feb 3, 2016

When exceptions are not enabled, and the server returns an error 400, ElasticsearchResponse<T>.Success is false, .ServerError is null, and the only information that .OriginalException gives you is that the server returned a 400. Elasticsearch sent back a bunch of useful information, but it's totally unavailable to the client. The serializer is never asked to deserialize anything (which is probably an improvement over ES.NET 1.x, where the serializer was asked to deserialize an internal OneToOneServerException type that looked nothing like the response JSON).

When exceptions are enabled, the situation is no better. There's no way (that I can find) to access any of the useful information about what REALLY went wrong. Knowing that there was an error 400 is all well and good, but especially in development scenarios, it's really important to know what went wrong. Now that we don't have tracing either, we can't even look at those responses.

@Mpdreamz
Copy link
Member

Mpdreamz commented Feb 3, 2016

@godefroi mind sharing what call you are doing, sounds like a bug?

related: this is exactly why we are adding this: #1797 before 2.0 final so we can easily see whats going on 😄

Trace will also make a reentry before the final release.

@godefroi
Copy link

godefroi commented Feb 3, 2016

My call looks exactly like this:
var resp = m_esclient.Index<IndexResponse>(m_idxname, m_idxtype, documentId, documentBody, p => version.HasValue ? p.Version(version.Value).VersionType(VersionType.External) : p);

IndexResponse is my own object, and has all the necessary properties to successfully deserialize the error that comes back. When the call returns, resp.Success is false, and resp.ServerError is null. Looking through the ES.NET code, I can see that it should be populating resp.ServerError, but that is not happening.

HttpConnection.HandleException() definitely tries to save the response stream from the HttpWebException, and ResponseBuilder.SetBody() should be using the stream to set the .ServerError, but it's not happening for some reason.

@godefroi
Copy link

godefroi commented Feb 3, 2016

My situation could be a red herring. I finally broke down and busted out Wireshark, and I found that I was not, in fact, getting any error information back from ES. It could be due to #1799, or it could be due to #1800.

@gmarz
Copy link
Contributor

gmarz commented Feb 4, 2016

Closing as #199 and #1800 have been addressed. Feel free to reopen @godefroi if you're still running into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants