[ML] Report cause when datafeed extraction encounters error #66167

dimitris-athanasiou · 2020-12-10T13:37:25Z

When a datafeed encounters errors extracting data, often
the error is an instance of SearchPhaseExecutionException.
In that case the top level error message is Partial shards failure
which is not very informative.

This commit refactors a transform util method from
ExceptionRootCauseFinder, which unwraps exceptions with special
handling for SearchPhaseExecutionException, and makes use of
it from datafeed ProblemTracker in order to provide a more
useful error message.

When a datafeed encounters errors extracting data, often the error is an instance of `SearchPhaseExecutionException`. In that case the top level error message is `Partial shards failure` which is not very informative. This commit refactors a transform util method from `ExceptionRootCauseFinder`, which unwraps exceptions with special handling for `SearchPhaseExecutionException`, and makes use of it from datafeed `ProblemTracker` in order to provide a more useful error message.

elasticmachine · 2020-12-10T13:37:28Z

Pinging @elastic/ml-core (:ml)

benwtrent

I have some minor concerns. Overall, this greatly improves things

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJob.java

benwtrent · 2020-12-10T13:59:35Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/utils/ExceptionsHelper.java

+            for (ShardSearchFailure shardFailure : searchPhaseException.shardFailures()) {
+                Throwable unwrappedShardFailure = org.elasticsearch.ExceptionsHelper.unwrapCause(shardFailure.getCause());
+
+                if (unwrappedShardFailure instanceof ElasticsearchException) {
+                    return unwrappedShardFailure;
+                }
+            }


Anyway to add the rest of the shard failures as suppressed?

Trying something like that can make the message unreadable. Note we're already logging the SearchPhaseExecutionException.

I think there are 2 scenarios:

the search fails because of some user error (e.g. time field doesn't exist in some index). In this scenario there will be a single cause and we'll bubble it up.

the search fails due to the cluster being unstable. Here, we will have many shard failures. I think in this scenario we'd have to look at the logs anyhow to debug what's happening.

Having said that, I'm open to suggestions if you see another way.

I haven't explored this for search, but for indexing I use a similar method that tries to get the most important error. That means you could loop through the shard failures and get the most problematic one, e.g. IllegalArgumentException > RuntimeException, because the IAE is irrecoverable, the RE might just be a 429 (too manu requests).

Having that said: "it depends" on what you are aiming for with this

…feed/DatafeedJob.java Co-authored-by: Benjamin Trent <[email protected]>

hendrikmuhs · 2020-12-10T14:25:58Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/utils/ExceptionsHelper.java

+     * @param t raw Throwable
+     * @return unwrapped throwable if possible
+     */
    public static Throwable unwrapCause(Throwable t) {


I think this is a bad name as it collides with org.elasticsearch.ExceptionsHelper.unwrapCause

Not saying getRootCauseException is a good one, maybe something that indicates the intent: unwrapping search responses, e.g. unwrapSearchException or findSearchExceptionRootCause.

I think this method should not become a general exception handling method, but only for handling search errors. Otherwise the chances are high that code sharing lead to undesired bugs.

For that reason I think it would be good to move it out of the ml subpackage (despite this is just a logical but not technically limiting) into a more common area.

Transform should use this without indirection.

It is basically an unwrapCause that also handles SearchPhaseExecutionException. The problem with SearchPhaseExecutionException is that it doesn't implement ElasticsearchWrapperException. Let's discuss this further offline.

OK, I have refactored the code to keep a method named findSearchExceptionRootCause instead of replacing unwrapCause.

hendrikmuhs · 2020-12-10T14:28:27Z

...plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/utils/ExceptionsHelper.java

+     * @see org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper#unwrapCause(Throwable)
+     */
+    public static Throwable unwrapCause(Throwable t) {
+        return org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper.unwrapCause(t);


see my 1st comment, this makes no sense to me

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJob.java

dimitris-athanasiou · 2020-12-14T15:38:00Z

@elasticmachine update branch

…unters-error

hendrikmuhs

LGTM

…stic#66167) When a datafeed encounters errors extracting data, often the error is an instance of `SearchPhaseExecutionException`. In that case the top level error message is `Partial shards failure` which is not very informative. This commit refactors a transform util method from `ExceptionRootCauseFinder`, which unwraps exceptions with special handling for `SearchPhaseExecutionException`, and makes use of it from datafeed `ProblemTracker` in order to provide a more useful error message. Backport of elastic#66167

) (#66277) When a datafeed encounters errors extracting data, often the error is an instance of `SearchPhaseExecutionException`. In that case the top level error message is `Partial shards failure` which is not very informative. This commit refactors a transform util method from `ExceptionRootCauseFinder`, which unwraps exceptions with special handling for `SearchPhaseExecutionException`, and makes use of it from datafeed `ProblemTracker` in order to provide a more useful error message. Backport of #66167

* elastic/master: (33 commits) Add searchable snapshot cache folder to NodeEnvironment (elastic#66297) [DOCS] Add dynamic runtime fields to docs (elastic#66194) Add HDFS searchable snapshot integration (elastic#66185) Support canceling cross-clusters search requests (elastic#66206) Mute testCacheSurviveRestart (elastic#66289) Fix cat tasks api params in spec and handler (elastic#66272) Snapshot of a searchable snapshot should be empty (elastic#66162) [ML] DFA _explain API should not fail when none field is included (elastic#66281) Add action to decommission legacy monitoring cluster alerts (elastic#64373) move rollup_index param out of RollupActionConfig (elastic#66139) Improve FieldFetcher retrieval of fields (elastic#66160) Remove unsed fields in `RestAnalyzeAction` (elastic#66215) Simplify searchable snapshot CacheKey (elastic#66263) Autoscaling remove feature flags (elastic#65973) Improve searchable snapshot mount time (elastic#66198) [ML] Report cause when datafeed extraction encounters error (elastic#66167) Remove suggest reference in some API specs (elastic#66180) Fix warning when installing a plugin for different ESversion (elastic#66146) [ML] make `xpack.ml.max_ml_node_size` and `xpack.ml.use_auto_machine_memory_percent` dynamically settable (elastic#66132) [DOCS] Add `require_alias` to Bulk API (elastic#66259) ...

przemekwitek · 2020-12-15T08:17:24Z

...plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/utils/ExceptionsHelper.java

+     * @see org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper#findSearchExceptionRootCause(Throwable)
+     */
+    public static Throwable findSearchExceptionRootCause(Throwable t) {
+        return org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper.findSearchExceptionRootCause(t);


Is this cross-package dependency ok?
I'd consider introducing ExceptionsHelper class under org.elasticsearch.xpack.core.common instead.

We discussed this with Hendrik. It's not great but harmless otherwise. Adding another ExceptionsHelper class is marginally worse. There is much to improve in our exceptions handling framework. I am going to raise an issue that once fixed should make this method unnecessary.

dimitris-athanasiou added >non-issue :ml Machine learning v8.0.0 v7.11.0 labels Dec 10, 2020

dimitris-athanasiou added >bug and removed >non-issue labels Dec 10, 2020

benwtrent approved these changes Dec 10, 2020

View reviewed changes

dimitris-athanasiou and others added 3 commits December 10, 2020 16:02

Update x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/data…

a28233a

…feed/DatafeedJob.java Co-authored-by: Benjamin Trent <[email protected]>

Update x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/data…

0df275d

…feed/DatafeedJob.java Co-authored-by: Benjamin Trent <[email protected]>

Add missing import

944d19c

hendrikmuhs reviewed Dec 10, 2020

View reviewed changes

Address review comments

3894a49

elasticmachine and others added 2 commits December 14, 2020 07:38

Merge branch 'master' into report-cause-when-datafeed-extraction-enco…

d2ed151

…unters-error

Fix failing tests

33c8a78

hendrikmuhs approved these changes Dec 14, 2020

View reviewed changes

dimitris-athanasiou merged commit f8bda89 into elastic:master Dec 14, 2020

dimitris-athanasiou deleted the report-cause-when-datafeed-extraction-encounters-error branch December 14, 2020 17:24

dimitris-athanasiou mentioned this pull request Dec 14, 2020

[7.x][ML] Report cause when datafeed extraction encounters error (#66… #66277

Merged

przemekwitek reviewed Dec 15, 2020

View reviewed changes

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

[ML] Report cause when datafeed extraction encounters error #66167

[ML] Report cause when datafeed extraction encounters error #66167

Uh oh!

Conversation

dimitris-athanasiou commented Dec 10, 2020

Uh oh!

elasticmachine commented Dec 10, 2020

Uh oh!

benwtrent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dimitris-athanasiou commented Dec 14, 2020

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hendrikmuhs Dec 10, 2020 •

edited

Loading