[Lens] Leverage original http request error

[flash1293]

Fixes #77360

This PR makes sure the underlying http request error is propagated to the expression error handler and uses the underlying ES information to show a more meaningful error message:

Full list of changes:

• src/plugins/expressions/common/util/create_error.ts If an error is thrown in a sub expression (as it is the case for Lens), the original error was not propagated correctly but discarded because an ExpressionValueError['error'] is not a real JS Error. This makes sure to always keep the underlying real error object even if it's wrapped multiple times.
• src/plugins/expressions/public/react_expression_renderer.tsx: Only the error message was passed to the expression renderer react component callback - this PR adds a second parameter for the whole error
• src/plugins/expressions/public/types/index.ts: Add original to the type of this error - not sure about the type structure here, but it definitely ends up being defined at that level as well
• In Lens: Add a helper for pulling the interesting pieces out of the original error and use it in the workspace panel and in the embeddable.

Testing

• Create a Lens dashboard based on an index
• Create a user without read rights on that index
• Access the dashboard logged in as that user
• Error messages should be helpful

[dej611]

I think you could leverage some knowledge from TS here and just do

...
: err.original instanceof Error ? err.original : undefined

[flash1293]

This code is necessary because err could be a string based on the type, so we have to do some generic checks first, otherwise typescript will complain about accessing original on a union type that might not have the property

[lizozom]

I think this code looks good (haven't tested, but it makes sense).

I do wonder if we could find a uniform way to display errors on panels in both Dashboard and Lens (see EmbeddableErrorLabel)

[wylieconlon]

This is working well for the permissions use case, but the main comment I'm leaving is that I want to validate the error response handling a little bit more. It's very difficult to find documentation of all the error cases, but I have to assume that there are people at the company who have dealt with this before.

[wylieconlon]

The logic you're using here looks incomplete, so I tried to break it. There appear to be different error responses for different types of failures, and I think you've only tested permission failures. I don't feel comfortable with the sort of one-off error response handling that you're doing here unless we can validate it against some kind of reference error handling implementation. Maybe the ES Clients team can take a look at this?

For example, sometimes there is a caused_by.caused_by response from Elasticsearch when Elasticsearch fails to validate the request itself, such as when you use a terms agg on a text field. For example, try running this request in the dev tools:

GET kibana_sample_data_logs/_search
{
  "size": 0,
  "aggs": {
    "t": {
      "terms": {
        "field": "host"
      }
    }
  }
}

Another example of a response that you're missing a case for is when the script is failing. This can happen fairly often with an example like metricbeat:

GET metricbeat-*/_search
{
  "size": 0,
  "aggs": {
    "d": {
      "max": {
        "script": """doc['system.network.in.bytes'].value"""
      }
    }
  }
}

[flash1293]

I used the same logic as this piece of code from core:

kibana/src/core/public/notifications/toasts/error_toast.tsx

Line 71 in 24d29a3

if (isRequestError(error)) {

I totally see your point, but I don’t think we have a “handleAllCommonEsErrors” helper (yet). What do you think about adding nice handling for the cases you mention above (great findings btw), and if none of these apply falling back to the default behavior of unknown errors (which is showing the message of the thrown exception and also what’s happening on master right now) - at least for 7.10. We should continue to investigate to find a holistic solution and ideally roll it out everywhere (Discover, Visualize etc - all of them produce really bad error messages depending on the case)

I’m also more than happy to loop in someone knowledgeable on the topic to improve the PR beyond what’s mentioned above (do you know who would be a good fit?), my only ask is to not increase scope too much for an improvement of the current state of 7.10 (that’s also why I didn’t touch core code itself to refactor out a generic helper, it felt like opening a can of worms too big for the scope of the intended fix).

[flash1293]

@wylieconlon I changed the lookup to recursively check the caused_bys in the Elasticsearch error message. This is how some error messages look like with this fix:

(Missing permissions to access index and aggregating on a text field (with updated index pattern)

(Error in painless script)

(Aggregating on text field without the index pattern knowing about it)

Those are far from perfect, but IMHO they get the point across and are at least "searchable" (Google will show up help for the individual problems)

[wylieconlon]

Did not test again, but the error screenshots you showed look much better than before

[wylieconlon]

Should we be skipping all the intermediate types and reasons? This recursion makes sense, but are there cases where it's losing important information?

[flash1293]

I see the point - for the known cases the root exception is the relevant one and I made this experience very often in Java in general (lot's of re-thrown exceptions, but the actual cause is in the very last one). I guess when we solve this cleanly there should be an option to expand the error to show all the details. I will open an issue for this.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 0b2aaa4

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
lens	563	564	+1

async chunks size

id	before	after	diff
lens	1.0MB	1.0MB	+1.8KB

page load bundle size

id	before	after	diff
expressions	198.4KB	198.5KB	+96.0B

History

• 💚 Build #81070 succeeded a8cf08f
• 💔 Build #81061 failed 521e932
• 💔 Build #81057 failed b7d33a7
• 💚 Build #80443 succeeded 20710c8
• 💔 Build #80433 failed a0b1958

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[kibanamachine]

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.