REST error readability #3303
Comments
|
When I have this kind of error, I'm always opening my ElasticSearch logs. Errors are way more complete and usefull in there! Look at what I get for an undesirable I have the full stack trace, and the exception line 38:
And my query is readable, as I sent it. I agree that errors returned by the API could be improved 👍 |
|
I think the right long-term answer here (for query parse failures at least) is to change our query parsing logic so that when it encounters an error it can somehow annotate the point in the original JSON with the exact problem in the same way search highlighting shows where search terms are found in text or an IDE might highlight a compilation error. This is clearly an ambitious goal so marking this issue as "high hanging fruit" in order to explore what can be done there. |
|
In the case of /tmp/broken.js:5
var broken code here;
^^^^
SyntaxError: Unexpected identifierThis however is presentation logic which should not be the role of the API layer; however if it provided such granular information that allowed me to produce a view like the above then I'd be pretty happy. The general issue is that the error messages don't appear to be machine readable nor human readable. In the gist example above, the message I am scanning for is: The rest of the message is pretty much just noise and contains a bunch of characters which make machine parsing this section very difficult. Also any attempt to parse error messages in such a way would be very bad practise as they are bound to change over time. Since we're talking {
"error": "I exist for backward compatibility",
"status": 500,
"message": "I am a human readable short description of the error",
"class": "I am the Class of the error",
"lineNumber": "I am very useful",
"columnNumber": "as am I",
"input": "I am the original input as related to the line and column numbers"
}I guess the discussion should really be about how external errors are modelled, as they are currently not structured at all? |
|
@polyfractal I saw you wrote a 'pretty error' parser for inquisitor, do you have any suggestions? ref: https://github.com/polyfractal/elasticsearch-inquisitor#pretty-errors |
|
@missinglink Not much useful help, unfortunately. The parser in Inquisitor is very naive. It recognizes that ES exceptions tend to be large blocks of exceptions surrounded by brackets ( It's a good "80%" solution, but fails for a variety of exceptions that don't follow the same pattern. And it definitely doesn't produce machine-readable output, just something that humans can digest (since the most "interior" exception is usually the one you want, that's what Inquisitor shows as the main error) |
|
This is really important for Kibana as well, right now we have a rather difficult time informing the user what went wrong. +1 to the modeling it on the javascript error object. Error types/codes might be useful as well. |
|
What if the "parse error" is just that the query is too long? It might not be a good idea to parrot the entire thing back out to the caller. Also, note how silly it is that each of your shards gives you the same exact error. Constructing that string in memory (again, consider the case of an overlong query) might kill your node. If you OOM trying to build an error message, then you're going to receive a very unhelpful response. Here is a thought: why is it necessary for the response to contain the request? Shouldn't the requester already know what its own request looks like? It seems reasonable to me that if the response is just "parse error starting at character ###, unexpected XYZ", then the requester can reasonably be expected to piece that together into a pretty display if it cares to. |
This generally isn't a problem, since ES places a limit on the size of the request body via the So unless the limit has been set unreasonably high, or the search threadpool has been increased to allow an unreasonable number of queries to execute simultaneously, I think it is unlikely you would get an OOM in this particular part of the code. Note: if the query would have caused an OOM when writing the parse error exception, it probably would have triggered an exception upstream in the networking stack first :)
It's possible, although not common, for shards to return different errors (e.g. forgot to install a plugin on one node, different mappings if searching cross-index, corruption, etc).
I agree with your sentiment, but you'd be surprised (or disappointed) at how many production environments don't log their own queries. Many systems have no idea what's going through them and rely on the ES logs for retrospective analysis. I think removing the queries from the exceptions would make them even more irritating, since many people don't log their own queries client-side. |
|
I have actually seen OOM's like this. The problem is not the raw amount of data, but rather the fact that it all gets concatenated together into one contiguous string. Also, again, each shard reports the same thing, with the original query embedded in it. So, it tries to build up a string of length roughly (number of shards) x (length of query). That can be much longer than any individual request is allowed to be. I am indeed surprised to learn that many clients apparently don't know what query they're getting a response for. I didn't think about anything like that when I made my suggestion. Still, how about making it somehow configurable, whether the original request has to be part of an error message? The realistic set of clients should be supported, but I also think that reasonable clients should have a way to operate reasonably. If it's configurable, then everyone can have what they need. |
|
Any additional thoughts on the |
|
poke |
|
yeah, I second this request. I just got one of these errors. |
|
@s1monw is keen to overhaul the parser framework and so this issue is likely to be addressed as part of that activity. Currently all data nodes parse the same JSON query and generally report the same parse issues. In Simon's proposal only the receiving node performs parsing/validation and for valid queries sends out non-JSON representations of the query to data nodes for execution. This is a big change and so improved error management will be addressed as part of that refactoring. |
|
+1 to what @markharwood said. It's not helpful to add half baked solutions IMO we should get these exceptions when we parse the query and then we can render all the required things. Today we do that too late (on the executing node) and we are moving query parsing to the coordinating node (REST node) so we can throw the right exception and render more infos. I like to do these things right even if it takes much longer sorry for the delay. |
|
Important to consider this isn't limited to query parsing. This goes much deeper than the node that generates the error. Simply moving validation to the coordinating node won't fix the premise of this ticket, which is that that REST errors are neither human nor machine readable. For example, invalid scripts will return a concatenated string leaving us with no way to tell the user what went wrong. In this it would be great to get something like (could certainly be improved upon): Or if a shard fails due to corruption, all we can tell the user is that the shard failed, we have no idea why, and neither do they. Here I'd love to have: |
This is a first cut aiming at progress rather than perfection for elastic#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: ``` GET localhost:9200/foo1/_search/?q=~3&pretty=true { "error" : { "type" : "index_missing_exception", "reason" : "[foo1] missing" }, "status" : 404 } ``` or a search phase exception: ``` GET localhost:9200/_search?q=~3&pretty=true { "error" : { "type" : "search_phase_execution_exception", "reason" : "all shards failed", "phase" : "query", "failed_shards" : [ { "shard" : 0, "index" : "foo", "node" : "0-75uocCQhyX4clLGbE6OQ", "reason" : { "type" : "search_parse_exception", "reason" : "Failed to parse source [{\"query\":{\"query_string\":{\"query\":\"~3\",\"lowercase_expanded_terms\":true,\"analyze_wildcard\":false}}}]", "caused_by" : { "type" : "query_parsing_exception", "reason" : "[foo] Failed to parse query [~3]", "caused_by" : { "type" : "parse_exception", "reason" : "Cannot parse '~3': Encountered \" <FUZZY_SLOP> \"~3 \"\" at line 1, column 0.\nWas expecting one of:\n <NOT> ...\n \"+\" ...\n \"-\" ...\n <BAREOPER> ...\n \"(\" ...\n \"*\" ...\n <QUOTED> ...\n <TERM> ...\n <PREFIXTERM> ...\n <WILDTERM> ...\n <REGEXPTERM> ...\n \"[\" ...\n \"{\" ...\n <NUMBER> ...\n <TERM> ...\n \"*\" ...\n ", "caused_by" : { "type" : "parse_exception", "reason" : "Encountered \" <FUZZY_SLOP> \"~3 \"\" at line 1, column 0.\nWas expecting one of:\n <NOT> ...\n \"+\" ...\n \"-\" ...\n <BAREOPER> ...\n \"(\" ...\n \"*\" ...\n <QUOTED> ...\n <TERM> ...\n <PREFIXTERM> ...\n <WILDTERM> ...\n <REGEXPTERM> ...\n \"[\" ...\n \"{\" ...\n <NUMBER> ...\n <TERM> ...\n \"*\" ...\n " } } } } }] }, "status" : 400 } ```
Extend SearchParseException and QueryParsingException to report position information in query JSON where errors were found. All query DSL parser classes that throw these exception types now pass the underlying position information (line and column number) at the point the error was found. Closes #3303
Errors returned by the REST API are not easily human readable due to the way the data is stored in the
errorproperty.Eg. https://gist.github.com/missinglink/e3cb9b127ae00e8c561c
You can 'prettify' the results, but it is still un-readable.
Eg. https://gist.github.com/pecke01/5956684
Note: (this specific error was caused by invalid syntax, the outer curly brackets were missing)
This issue makes it difficult for beginners to understand syntax errors and for advanced users to debug quickly when they make silly errors.
Ideally usage of
?pretty=1would return "developer friendly" messages.Any thoughts / suggestions? Maybe there is a tool which may help?
The text was updated successfully, but these errors were encountered: