Render REST errors in a structural way #10643
Conversation
|
Few minutes of fiddling feedback. Definitely an improvement! We can dig into the objects until we find a I don't know much of the code but I'm guessing De-duping the If we look at a sort failure, we have a much better scenario. The message will be the same every index and every shard, so we de-dupe them and tell the user Figuring out which From these two examples I would probably pick the "deepest" In the query example, we'd say we still understand Ideally we could do this on the Elasticsearch side and float the "simple" error to the top somehow, but I don't know that my rule devised from 2 examples is going to apply globally 😄 |
|
@rashidkpc thanks a lot for your feedback, I tried another interation for this including some possible solutions to your problems: De-duping the reason I added an optional deduplication for the shard failures that only returns on of the shard failures if there are multiple like this: I added a best effort human readable error on the top level that basically uses all the unique lowest level Elasticsearch created exceptions (the first level we control) which looks like this: Those are all best effort and simple heuristics but I think it's an improvement over what we have today though. |
| } | ||
|
|
||
| protected static String getExceptionName(Throwable ex) { | ||
| return Strings.toUnderscoreCase(ex.getClass().getSimpleName()); |
There was a problem hiding this comment.
can we remove an "elasticsearch_" prefix if it exists, I think it will be cleaner? down the road, we can also remove the specific ElasticsearchIllegalArgumentException and such, it was added historically to get the correct status code, but now we also identify IlleglaArgumentException and return the correct status code, so the need for those became irrelevant.
|
LGTM except for the minor comment on the |
|
w00t |
|
@kimchy @clintongormley @rashidkpc I think we are ready here any more comments on the API? Changes I made are this:
GET /_search/asf?q=::&pretty=true
{
"error" : {
"root_cause" : [ {
"type" : "illegal_argument_exception",
"reason" : "No feature for name [asf]"
} ],
"type" : "illegal_argument_exception",
"reason" : "No feature for name [asf]"
},
"status" : 400
}
GET /_search?q=::&pretty=true
{
"error" : {
"root_cause" : [ {
"type" : "search_phase_execution_exception",
"reason" : "all shards failed"
} ],
"type" : "search_phase_execution_exception",
"reason" : "all shards failed",
"phase" : "query",
"grouped" : true,
"failed_shards" : [ {
"shard" : 0,
"index" : "foo",
"node" : "SvSibBJsTGKcgfyIDchoyA",
"reason" : {
"type" : "search_parse_exception",
"reason" : "Failed to parse source [{\"query\":{\"query_string\":{\"query\":\"::\",\"lowercase_expanded_terms\":true,\"analyze_wildcard\":false}}}]",
"caused_by" : {
"type" : "query_parsing_exception",
"reason" : "Failed to parse query [::]",
"index" : "foo",
"caused_by" : {
"type" : "parse_exception",
"reason" : "Cannot parse '::': Encountered \" \":\" \": \"\" at line 1, column 0.\n.. ",
"caused_by" : {
"type" : "parse_exception",
"reason" : "Encountered \" \":\" \": \"\" at line 1, column 0.\n..."
}
}
}
}
} ]
},
"status" : 400
}I think we are ready here... we can now improve stuff as we go... |
|
@s1monw In your second example in #10643 (comment), surely the An improvement in the second example (and not necessarily as part of this PR) might be to mark a particular exception in the tree as the one to use for the root cause, eg it may make more sense to stop at |
I guess it would make more sense to just dont' render the verbose part if verbose=false? |
yeah that is right! I fixed it, it now looks like this: GET /_search?q=::&pretty=true
{
"error" : {
"root_cause" : [ {
"type" : "query_parsing_exception",
"reason" : "Failed to parse query [:::]",
"index" : "foo"
} ],
"type" : "search_phase_execution_exception",
"reason" : "all shards failed",
"phase" : "query",
"grouped" : true,
"failed_shards" : [ {
"shard" : 0,
"index" : "foo",
"node" : "CdPVI-Y-QHyBRctNFXVmSA",
"reason" : {
"type" : "search_parse_exception",
"reason" : "Failed to parse source [{\"query\":{\"query_string\":{\"query\":\":::\",\"lowercase_expanded_terms\":true,\"analyze_wildcard\":false}}}]",
"caused_by" : {
"type" : "query_parsing_exception",
"reason" : "Failed to parse query [:::]",
"index" : "foo",
"caused_by" : {
"type" : "parse_exception",
"reason" : "Cannot parse ':::': Encountered \" \":\" \": \"\" at line 1, column 0.\nWas expecting one...",
"caused_by" : {
"type" : "parse_exception",
"reason" : "Encountered \" \":\" \": \"\" at line 1, column 0.\nWas expecting one... "
}
}
}
}
} ]
},
"status" : 400
} |
|
unless anybody objects I'd push this tomorrow morning CEST |
|
+1 |
This commit adds support for structural errors / failures / exceptions on the elasticsearch REST layer. Exceptions are rendering with at least a `type` and a `reason` corresponding to the exception name and the message. Some expcetions like the ones associated with an index or a shard will have additional information about the index the exception was triggered on or the shard respectivly. Each rendered response will also contain a list of root causes which is a list of distinct shard level errors returned for the request. Root causes are the lowest level elasticsearch exception found per shard response and are intended to be displayed to the user to indicate the soruce of the exception. Shard level response are by-default grouped by their type and reason to reduce the amount of duplicates retunred. Yet, the same exception retunred from different indices will not be grouped. Closes elastic#3303
s1monw
force-pushed
the
better_error_msg
branch
from
af1f123
to
15d58d9
Compare
This is a first cut aiming at progress rather than perfection for #3303
It renders errors on the REST layer as JSON to allow more insight into the error message ie
the query with a missing index renders like this:
or a search phase exception: