[Alerting] Show more user friendly ES error message when executor fails #96254

chrisronline · 2021-04-05T20:15:37Z

Resolves to #95516

This PR attempts to identify the root cause of an ES exception and display that in the error message. Unfortunately, it does not appear as if error formats are not normalized across all ES APIs so we need to look in a few places for the correct root cause.

The logic in this PR is borrowed from work by the ES UI team -> https://github.com/elastic/kibana/blob/master/src/plugins/es_ui_shared/__packages_do_not_import__/errors/es_error_parser.ts. I decided to copy/modify as we aren't using all of the logic and we might need to change it over time for our own purposes.

Here is the difference in the server log and UI:

When a runtime field changes type to something incompatible with the existing query

server log [10:02:56.549] [error][alerting][alerting][plugins][plugins] Executing Alert "d7913b90-9633-11eb-ac94-471ab4f6ffdf" has resulted in Error: search_phase_execution_exception, caused by: "cannot implicitly cast def [org.elasticsearch.index.fielddata.ScriptDocValues.Dates] to long"

When the provided query is invalid

server log [10:05:08.594] [error][alerting][alerting][plugins][plugins] Executing Alert "d7913b90-9633-11eb-ac94-471ab4f6ffdf" has resulted in Error: x_content_parse_exception, caused by: "unknown query [range2] did you mean [range]?,[1:119] unknown field [range2]"

We do have a little more information available so please feel free to suggest how the messaging could be better and what other information we should show.

elasticmachine · 2021-04-06T14:39:53Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

chrisronline · 2021-04-07T15:10:47Z

@elasticmachine merge upstream

ymao1 · 2021-04-07T20:09:11Z

I created an index threshold rule with a date type runtime field as the timestamp and then changed the type of the runtime field to keyword and I am just seeing

server    log   [15:58:48.385] [warning][plugins][stackAlerts] indexThreshold timeSeriesQuery: callCluster error: search_phase_execution_exception
server    log   [15:59:06.397] [warning][plugins][stackAlerts] indexThreshold timeSeriesQuery: callCluster error: search_phase_execution_exception
server    log   [15:59:24.408] [warning][plugins][stackAlerts] indexThreshold timeSeriesQuery: callCluster error: search_phase_execution_exception
server    log   [15:59:43.484] [warning][plugins][stackAlerts] indexThreshold timeSeriesQuery: callCluster error: search_phase_execution_exception

in the logs. It looks like the timeSeries query called by the index threshold rule executor does its own try catch logic for the ES query to handle and log errors without actually bubbling up the error to the framework, so you may need to update the error logging in there as well.

chrisronline · 2021-04-08T13:30:05Z

@ymao1 Thank you! I'll look into that

…parser

pmuellr · 2021-04-08T16:01:27Z

I created an index threshold rule with a date type runtime field as the timestamp and then changed the type of the runtime field to keyword and I am just seeing

For some reason I'm thinking there is a built-in restriction in using a runtime field as the timestamp field in index patterns. That would obviously result in a table scan for every query that uses date ranges, which many of the alerts use, which is why I think it's disallowed.

So - it might be possible/better to special case this in alerting as well - if we can determine the field used for the date is actually a runtime field, we could potentially disallow that usage when validating the alert params during create/edit. And after the alert is created/edited, and the user later changes the field to a runtime field, we could detect this situation and provide an explicit error message, instead of trying to infer the error state from the elasticsearch response.

chrisronline · 2021-04-08T17:57:53Z

@ymao1 All fixed up and ready for another review!

…parser

chrisronline · 2021-04-12T14:17:18Z

@elasticmachine merge upstream

ymao1

LGTM! Just some comments about additional tests and a question about deduping the reason string.

ymao1 · 2021-04-13T12:37:50Z

x-pack/plugins/alerting/server/lib/errors/es_error_parser.test.ts

+import { getEsErrorMessage } from './es_error_parser';
+
+describe('ES error parser', () => {
+  test('should return all the cause of the error', () => {


Can we add a test for when there are nested caused_by reasons?

ymao1 · 2021-04-13T12:38:48Z

x-pack/plugins/alerting/server/lib/errors/es_error_parser.ts

+    return getEsCause(obj.caused_by, updated);
+  }
+
+  if (obj.failed_shards && obj.failed_shards.length) {


Can we add a test for this case? When there is no caused_by but there is an array of failed_shards

ymao1 · 2021-04-13T12:40:08Z

x-pack/plugins/alerting/server/lib/errors/es_error_parser.ts

+    }
+
+    // Recursively find all the "caused by" reasons
+    return getEsCause(obj.caused_by, updated);


nit: It looks like sometimes the recursively nested caused_by reasons can be pretty redundant. Could we possibly dedupe them?

YulNaumenko

LGTM

chrisronline · 2021-04-14T13:00:10Z

Thanks all. I'm finishing some last minute item and will get back to this PR later this week.

…parser

kibanamachine · 2021-04-15T14:14:30Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #118735 succeeded 94e000f0e610b167b636010f6ca04d6349b4b559
💚 Build #118496 succeeded 8ef71a0
💔 Build #118364 failed 1c25705
💔 Build #118345 failed c86091c
💚 Build #117882 succeeded 7d22fa7

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

cc @chrisronline

…ls (elastic#96254) * WIP for ES error parser * Fix tests * Ensure the error shows up in the UI too * wip * Handle multiple types here * Fix tests * PR feedback Co-authored-by: Kibana Machine <[email protected]>

…ls (#96254) (#97259) * WIP for ES error parser * Fix tests * Ensure the error shows up in the UI too * wip * Handle multiple types here * Fix tests * PR feedback Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

WIP for ES error parser

5ac1897

chrisronline self-assigned this Apr 5, 2021

chrisronline added 2 commits April 5, 2021 23:35

Fix tests

1064edd

Ensure the error shows up in the UI too

204b0ea

chrisronline marked this pull request as ready for review April 6, 2021 14:39

chrisronline requested a review from a team as a code owner April 6, 2021 14:39

chrisronline added Feature:Alerting review Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.13.0 v8.0.0 labels Apr 6, 2021

chrisronline added the release_note:skip Skip the PR/issue when compiling release notes label Apr 6, 2021

Merge branch 'master' into alerting/es_error_parser

7d22fa7

Merge remote-tracking branch 'elastic/master' into alerting/es_error_…

899410c

…parser

chrisronline added 3 commits April 8, 2021 12:59

wip

f9a8aea

Handle multiple types here

b935426

Merge in the WIP branch

c86091c

chrisronline added 2 commits April 8, 2021 14:28

Fix tests

1c25705

Merge remote-tracking branch 'elastic/master' into alerting/es_error_…

8ef71a0

…parser

ymao1 approved these changes Apr 13, 2021

View reviewed changes

ymao1 linked an issue Apr 13, 2021 that may be closed by this pull request

[Alerting] Log more descriptive error messages when runtime field mappings are updated to be incompatible with original query #95516

Closed

YulNaumenko approved these changes Apr 13, 2021

View reviewed changes

chrisronline added 2 commits April 15, 2021 07:06

Merge remote-tracking branch 'elastic/master' into alerting/es_error_…

b019ef9

…parser

PR feedback

0320100

chrisronline force-pushed the alerting/es_error_parser branch from 94e000f to 0320100 Compare April 15, 2021 12:05

chrisronline merged commit 9a76d34 into elastic:master Apr 15, 2021

chrisronline deleted the alerting/es_error_parser branch April 15, 2021 14:17

chrisronline mentioned this pull request Apr 15, 2021

[7.x] [Alerting] Show more user friendly ES error message when executor fails (#96254) #97259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting] Show more user friendly ES error message when executor fails #96254

[Alerting] Show more user friendly ES error message when executor fails #96254

chrisronline commented Apr 5, 2021 •

edited

Loading

elasticmachine commented Apr 6, 2021

chrisronline commented Apr 7, 2021

ymao1 commented Apr 7, 2021

chrisronline commented Apr 8, 2021

pmuellr commented Apr 8, 2021

chrisronline commented Apr 8, 2021

chrisronline commented Apr 12, 2021

ymao1 left a comment

ymao1 Apr 13, 2021

ymao1 Apr 13, 2021

ymao1 Apr 13, 2021

YulNaumenko left a comment

chrisronline commented Apr 14, 2021

kibanamachine commented Apr 15, 2021

[Alerting] Show more user friendly ES error message when executor fails #96254

[Alerting] Show more user friendly ES error message when executor fails #96254

Conversation

chrisronline commented Apr 5, 2021 • edited Loading

When a runtime field changes type to something incompatible with the existing query

When the provided query is invalid

elasticmachine commented Apr 6, 2021

chrisronline commented Apr 7, 2021

ymao1 commented Apr 7, 2021

chrisronline commented Apr 8, 2021

pmuellr commented Apr 8, 2021

chrisronline commented Apr 8, 2021

chrisronline commented Apr 12, 2021

ymao1 left a comment

Choose a reason for hiding this comment

ymao1 Apr 13, 2021

Choose a reason for hiding this comment

ymao1 Apr 13, 2021

Choose a reason for hiding this comment

ymao1 Apr 13, 2021

Choose a reason for hiding this comment

YulNaumenko left a comment

Choose a reason for hiding this comment

chrisronline commented Apr 14, 2021

kibanamachine commented Apr 15, 2021

💚 Build Succeeded

Metrics [docs]

History

chrisronline commented Apr 5, 2021 •

edited

Loading