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

Causes of XContentParseException should be included in search for root_cause #30261

Closed
droberts195 opened this issue Apr 30, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@droberts195
Copy link
Contributor

commented Apr 30, 2018

Following #29373 parse exceptions are now XContentParseException rather than ParsingException. This has a major effect on the determination of the root_cause of an exception that is found during parsing due to the definition of root_cause.

The root_cause is determined by starting at the outermost exception and recursing through the causes until an exception is found that is not an ElasticsearchException. Then:

  • If the outermost exception was not an ElasticsearchException then it is the root_cause
  • Otherwise the most deeply nested ElasticsearchException is the root_cause

ParsingException extends ElasticsearchException whereas XContentParseException does not.

This means that if a validation error occurs during parsing then following #29373 the most general parsing exception is reported as the root cause. Prior to #29373 the most specific parsing exception, i.e. most likely the one detailing the validation error, was considered the root_cause.

As a concrete example, this is a validation error after #29373:

{
    "error": {
        "root_cause": [{
            "type": "x_content_parse_exception",
            "reason": "[1:144] [job_details] failed to parse field [analysis_limits]"
        }],
        "type": "x_content_parse_exception",
        "reason": "[1:144] [job_details] failed to parse field [analysis_limits]",
        "caused_by": {
            "type": "x_content_parse_exception",
            "reason": "Failed to build [analysis_limits] after last required field arrived",
            "caused_by": {
                "type": "status_exception",
                "reason": "categorization_examples_limit cannot be less than 0. Value = -1"
            }
        }
    },
    "status": 400
}

And this is the equivalent error before #29373:

{
  "error": {
    "root_cause": [
      {
        "type": "status_exception",
        "reason": "categorization_examples_limit cannot be less than 0. Value = -1"
      }
    ],
    "type": "parsing_exception",
    "reason": "[job_details] failed to parse field [analysis_limits]",
    "line": 11,
    "col": 3,
    "caused_by": {
      "type": "parsing_exception",
      "reason": "Failed to build [analysis_limits] after last required field arrived",
      "caused_by": {
        "type": "status_exception",
        "reason": "categorization_examples_limit cannot be less than 0. Value = -1"
      }
    }
  },
  "status": 400
}

If all you have to go on is the root_cause then [1:144] [job_details] failed to parse field [analysis_limits] is nowhere near as useful as categorization_examples_limit cannot be less than 0. Value = -1. The red error bars that Kibana shows when an error occurs only show the root_cause, so Kibana users suffer from this.

I think the solution that will keep the root_cause functionality as it was previously would be to consider both ElasticsearchException and XContentParseException when recursing through causes.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2018

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2018

This seems like a legit thing to me. @dakrone do you want this or should I have a look?

@sophiec20

This comment has been minimized.

Copy link

commented Apr 30, 2018

This is a regression with respect to user experience. It's the sort of issue that results in more support calls which, once raised, will be difficult to diagnose. What are the chances of a fix for 6.3?

@jasontedor

This comment has been minimized.

Copy link
Member

commented Apr 30, 2018

@nik9000 Would you pick this one up please?

@jasontedor

This comment has been minimized.

Copy link
Member

commented Apr 30, 2018

This is a regression with respect to user experience. It's the sort of issue that results in more support calls which, once raised, will be difficult to diagnose. What are the chances of a fix for 6.3?

We will assess and let you know.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2018

@nik9000 Would you pick this one up please?

Sure. I'll start right now.

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Apr 30, 2018

Core: Pick inner most parse exception as root cause
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that elastic#29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes elastic#30261
@jasontedor

This comment has been minimized.

Copy link
Member

commented Apr 30, 2018

This is a regression with respect to user experience. It's the sort of issue that results in more support calls which, once raised, will be difficult to diagnose. What are the chances of a fix for 6.3?

We will assess and let you know.

We will get this fixed in 6.3.0.

nik9000 added a commit that referenced this issue May 1, 2018

Core: Pick inner most parse exception as root cause (#30270)
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that #29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes #30261

nik9000 added a commit that referenced this issue May 1, 2018

Core: Pick inner most parse exception as root cause (#30270)
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that #29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes #30261

nik9000 added a commit that referenced this issue May 1, 2018

Core: Pick inner most parse exception as root cause (#30270)
Just like `ElasticsearchException`, the inner most
`XContentParseException` tends to contain the root cause of the
exception and show be show to the user in the `root_cause` field.

The effectively undoes most of the changes that #29373 made to the
`root_cause` for parsing exceptions. The `type` field still changes from
`parse_exception` to `x_content_parse_exception`, but this seems like a
fairly safe change.

`ElasticsearchWrapperException` *looks* tempting to implement this but
the behavior isn't quite right. `ElasticsearchWrapperExceptions` are
entirely unwrapped until the cause no longer
`implements ElasticsearchWrapperException` but `XContentParseException`
should be unwrapped until its cause is no longer an
`XContentParseException` but no further. In other words,
`ElasticsearchWrapperException` are unwrapped one step too far.

Closes #30261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.