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

EQL: Error responses do not include caused_by fields #63855

Closed
rylnd opened this issue Oct 17, 2020 · 6 comments
Closed

EQL: Error responses do not include caused_by fields #63855

rylnd opened this issue Oct 17, 2020 · 6 comments
Labels
:Analytics/EQL EQL querying >bug Team:QL (Deprecated) Meta label for query languages team

Comments

@rylnd
Copy link

rylnd commented Oct 17, 2020

Elasticsearch version (bin/elasticsearch --version): 7.10BC2

When error responses are returned from EQL searches, they lack the error.caused_by.type and error.caused_by.reason fields that appear to be standard across elasticsearch error responses.

In practice, this means that the default response error presentation in kibana does not provide useful EQL error information to the user, despite it being present elsewhere in the response.

Example EQL error response:
DevTools_-_d4949281e4954bb089dbb8f7ac5b2af1_europe-west1_gcp_cloud_es_io_9243_app_security_detections_rules_id_33fbcc07-372b-4c49-9b4c-1385437302ec_sourcerer__default______timerange__global__linkTo___timeline__timerange__from__272020-10-16T

Example elasticsearch error response (taken from elastic/kibana#70352):

@rylnd rylnd added >bug :Analytics/EQL EQL querying Team:QL (Deprecated) Meta label for query languages team needs:triage Requires assignment of a team area label labels Oct 17, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/EQL)

@costin
Copy link
Member

costin commented Oct 17, 2020

EQL exceptions are Elasticsearch exceptions (both semantically and in terms of hierarchy) however that doesn't mean they should contain a caused_by. That is used when there's an underlying exception - in your example the date_histogram failed (top exception) because there was a parsing exception related to the timezone.
The verification exception in EQL doesn't have an underlying exception - it is the root of the problem and there's no wrapping of it. You could argue that the style is different however that's on purpose as we're trying to report as many failures as possible for a given query inside the same exception.

@rylnd
Copy link
Author

rylnd commented Oct 19, 2020

@costin thanks for the response, that's great information! It sounds like the logic in kibana could be more correct, then.

I wasn't able to find any current documentation on the structure of these error responses, however I tracked down what looks to be the original PR for standardizing errors, which then lead to this subsequent PR in kibana.

Just a few quick clarifications, then, to assist in creating the kibana issue:

  • Is there documentation for the error response fields and their meaning?
  • Are error.reason and error.type always present?
    • They seem to always represent the most useful error message, but I'm cautious since that first kibana PR instead focused on error.root_cause
  • If not, is there an error field that will always be present, or an algorithm by which we can find the best information for the user?

@astefan
Copy link
Contributor

astefan commented Oct 20, 2020

@rylnd I don't think the response message format is documented, or at least I couldn't find this info. Maybe a question better asked to the wider Elasticsearch team. I see a tangential mentioning in the "Common options" docs page: https://www.elastic.co/guide/en/elasticsearch/reference/current/common-options.html#common-options-error-options.

Regarding the structure of the error message, an ElasticsearchException (which we use as base class for our eql exceptions) can output a detailed error message or a trimmed down one, depending on http.detailed_errors.enabled parameter setting (code here).

Looking at the code in ElasticsearchException:

  • if detailed is true (the default), then the error message has an error object in the final JSON structure with potentially multiple root_causes as an array of JSON objects. Each root_cause exception should have a type and a reason.
  • if detailed is false then just the error element is present with the name of the exception

@gwbrown gwbrown removed the needs:triage Requires assignment of a team area label label Oct 22, 2020
@rylnd
Copy link
Author

rylnd commented Oct 27, 2020

Thanks for the help here! I've opened elastic/kibana#81857 to address this on the kibana side 👍

@rylnd rylnd closed this as completed Oct 27, 2020
@astefan
Copy link
Contributor

astefan commented Oct 28, 2020

@rylnd heads up, I'm about to merge in 7.10 the following backport #64267 that might affect the error type displayed in some scenarios. The original issue is here #63529.
The PR itself contains some tests that show the way the error message will look like after the merge is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/EQL EQL querying >bug Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Development

No branches or pull requests

5 participants