Remove ExceptionHelper.detailedMessage by pugnascotia · Pull Request #45878 · elastic/elasticsearch

pugnascotia · 2019-08-22T20:21:57Z

Closes #19069.

I'm unsure of my approach here, so more opinions would be welcome. Mostly I replaced calls to ExceptionHelper.detailedMessage with e.getMessage(), which is all well and good but does remove some level of detail. There were a couple of cases where I replaced the method call with a throw, in the hope that code elsewhere will catch it and do the right thing.

Closes elastic#19069.

elasticmachine · 2019-08-22T20:21:59Z

Pinging @elastic/es-core-infra

rjernst · 2019-08-22T22:37:34Z

I don't think changing these uses to getMessage() solves the problem outlined in #19069. We often wrap exceptions, creating deep chains. By changing to getMessage() we omit even more information than detailedMessage is emitting. The problem is we need the actual stack trace. detailedMessage tries to summarize an exception into a single string that omits stack frames from each cause, summarizing as just the exception class + message for each cause. For something like a NullPointerException we lose all information that makes the exception debuggable (the location where the NPE occurred). I think a better substitute would be ExceptionHelper.stackTrace?

pugnascotia · 2019-08-23T11:27:03Z

server/src/main/java/org/elasticsearch/cluster/routing/allocation/FailedShard.java

-        return "failed shard, shard " + routingEntry + ", message [" + message + "], failure [" +
-                   ExceptionsHelper.detailedMessage(failure) + "], markAsStale [" + markAsStale + "]";
+        return "failed shard, shard " + routingEntry + ", message [" + message + "], markAsStale [" + markAsStale + "], failure ["
+            + ExceptionsHelper.stackTrace(failure) + "]";


I changed the order of fields here so that the stack trace comes last.

pugnascotia · 2019-08-23T11:27:21Z

server/src/main/java/org/elasticsearch/cluster/action/shard/ShardStateAction.java

+            components.add("markAsStale [" + markAsStale + "]");
            if (failure != null) {
-                components.add("failure [" + ExceptionsHelper.detailedMessage(failure) + "]");
+                components.add("failure [" + ExceptionsHelper.stackTrace(failure) + "]");


Also changed the order of elements here, so the stack trace is last.

rjernst

Thanks, this looks better to me. I left a couple last questions, and would also appreicate @jasontedor's opinion on whether using a full stack trace is always appropriate (IMO compared to the useless/reduced info from detailedmessage stacktrace is better).

rjernst · 2019-08-24T21:35:57Z

server/src/main/java/org/elasticsearch/ElasticsearchException.java

            builder.append(' ');
        }
-        return builder.append(ExceptionsHelper.detailedMessage(this).trim()).toString();
+        return builder.append(super.toString().trim()).toString();


Won't we lose all stack information by just using the exception tostring?

Only if whatever's calling ElasticsearchException.toString() isn't also printing the stack trace. The problem with calling ElasticsearchException.stackTrace() here is that it calls .toString() on the exception, so you end up blowing the stack. detailedMessage() didn't do this because it essentially reimplemented Throwable.toString(). Open to ideas here.

Ok, I see now why you are using toString(). I've fine with matching the behavior of Exception.

rjernst · 2019-08-24T21:37:53Z

...r/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java

            return Strings.toString(builder);
        } catch (Exception e) {
-            return "{ \"error\" : \"" + ExceptionsHelper.detailedMessage(e) + "\"}";
+            throw new RuntimeException(e);


Do we have a test for this failure case so we know the response is not already too far written to throw/serialize an exception like this?

We do have integration tests in SearchSuggestIT with failure cases, but they don't look related to a failure to serialize content. I'd be happy to add a new test - any ideas how to provoke a failure when serialising a DirectCandidateGeneratorBuilder to XContent?

I'm not exactly sure why we have this toString() implemented as json. But upon looking at this again, I think leaving the test coverage as is will be ok. Mimicking a failure would be quite difficult since the json builder is in memory and not mockable.

rjernst

LGTM once CI passes

pugnascotia · 2019-08-29T10:37:06Z

server/src/test/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponseTests.java

-                            assertThat(actualFailure.reason(), equalTo(expectedFailure.reason()));
+                            // Serialising and deserialising an exception seems to remove the "java.base/" part from the stack trace,
+                            // so these string replacements account for this.
+                            assertThat(actualFailure.reason().replace("java.base/", ""), equalTo(expectedFailure.reason().replace("java.base/", "")));


I made this change (and again in 1 other class) to get the test to pass, but I don't like it. I'd appreciate other suggestions.

…ptionhelper-detailedmessage

rjernst · 2019-08-30T17:59:18Z

server/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java

@@ -368,7 +368,7 @@ private void internalRecoverFromStore(IndexShard indexShard) throws IndexShardRe
                        files = Arrays.toString(store.directory().listAll());
                    } catch (Exception inner) {
                        inner.addSuppressed(e);


Seems odd that we are adding the original exception suppressed here. I dont' think it is necessary since the proper exception cause will be given in IndexShardRecoveryException below.

Agreed, I've removed this line.

rjernst · 2019-08-30T18:32:09Z

server/src/test/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponseTests.java

+                            // Serialising and deserialising an exception seems to remove the "java.base/" part from the stack trace,
+                            // so these string replacements account for this.
+                            assertThat(
+                                actualFailure.reason().replace("java.base/", ""),


Can we check for a portion of the failure instead of an exact stack trace?

Yes, I've reworked both instances of this.

A message in RestoreInProgressAllocationDecider included details about allocation decisions. If an allocation couldn't happen due to an exception, this resulted in a stack trace in the middle of the message. Move the details to the end of the message, which makes it easier to handle in tests.

Instead of relying on substring matching, parse the response JSON and check fields explicitly. This uses Jackson, which has been added to the build dependencies for :qa:smoke-test-http.

This required adding a getting to `VerificationFailure` so that the `cause` field could be accessed and added as suppressed exceptions elsewhere.

…ptionhelper-detailedmessage

pugnascotia · 2019-09-04T15:19:46Z

@elasticsearch run elasticsearch-ci/1

…ptionhelper-detailedmessage

pugnascotia · 2019-09-11T15:07:45Z

@rjernst I've merged in master again, would you mind taking another look?

rjernst

LGTM, two last nits

rjernst · 2019-09-11T15:44:59Z

qa/smoke-test-http/build.gradle

 apply plugin: 'elasticsearch.test-with-dependencies'

 dependencies {
+    testCompile("com.fasterxml.jackson.core:jackson-databind:2.8.11")


nit: we normally do this without the parens so it maintains the "dsl" look

rjernst · 2019-09-11T15:46:10Z

server/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java

+            client().prepareSearch().setQuery(builder).get();
+            shardFailures = searchResponse.getShardFailures();
+            assertThat("Expected shard failures, got none", shardFailures, not(emptyArray()));
+        } catch (SearchPhaseExecutionException e) {


we should use expectThrows here

My understanding of this code is that it doesn't always throw an exception, so expectThrows isn't appropriate. It was largely taken from ElasticsearchAssertions#assertFailures(SearchRequestBuilder, RestStatus, Matcher<String>), which for $REASONS didn't work for this specific test with the exception helper changes.

Sorry I misread the assert as a fail.

pugnascotia · 2019-09-12T13:25:58Z

@elasticmachine test this please

pugnascotia · 2019-09-13T19:36:25Z

@elasticmachine run elasticsearch-ci/1

pugnascotia · 2019-09-13T20:22:10Z

@elasticmachine run elasticsearch-ci/bwc

pugnascotia · 2019-09-16T09:20:38Z

@elasticmachine run elasticsearch-ci/bwc

pugnascotia · 2019-09-17T14:20:27Z

@elasticmachine update branch

pugnascotia · 2019-09-17T14:36:20Z

@elasticmachine run elasticsearch-ci/2

Remove ExceptionHelper.detailedMessage

ef97457

Closes elastic#19069.

pugnascotia added :Core/Infra/Core Core issues without another label v8.0.0 labels Aug 22, 2019

pugnascotia requested a review from rjernst August 22, 2019 20:21

Use ExceptionsHelper.stackTrace instead of Exception.getMessage

d1fab04

pugnascotia commented Aug 23, 2019

View reviewed changes

rjernst reviewed Aug 24, 2019

View reviewed changes

rjernst approved these changes Aug 27, 2019

View reviewed changes

pugnascotia added 5 commits August 27, 2019 16:47

Fix checkstyle error

628c3ba

Unit test fixes

7f23f12

Yet more fixes

2ef4bed

More test fixes

72c9873

Another raft of fixes

c8bf19d

pugnascotia commented Aug 29, 2019

View reviewed changes

pugnascotia added 3 commits August 29, 2019 12:32

Checkstyle again

f9189f9

Checkstyle

fe455db

Merge remote-tracking branch 'upstream/master' into 19069-remove-exce…

db6446e

…ptionhelper-detailedmessage

rjernst reviewed Aug 30, 2019

View reviewed changes

pugnascotia added 8 commits September 3, 2019 12:50

Lots of test fixes

52ae66f

Checkstyle

1f989b6

Checkstyle

da86b61

Rewrite DetailedErrorsEnabledIT to parse responses

422b007

Instead of relying on substring matching, parse the response JSON and check fields explicitly. This uses Jackson, which has been added to the build dependencies for :qa:smoke-test-http.

Integration test fixes

2255c67

This required adding a getting to `VerificationFailure` so that the `cause` field could be accessed and added as suppressed exceptions elsewhere.

Checkstyle

bc48e2e

Merge remote-tracking branch 'upstream/master' into 19069-remove-exce…

905a5b9

…ptionhelper-detailedmessage

Merge remote-tracking branch 'upstream/master' into 19069-remove-exce…

b97f27f

…ptionhelper-detailedmessage

rjernst approved these changes Sep 11, 2019

View reviewed changes

Fixes from review

a5cc8f2

Merge branch 'master' into 19069-remove-exceptionhelper-detailedmessage

70bb7b6

pugnascotia merged commit ff9e8c6 into elastic:master Sep 17, 2019

pugnascotia deleted the 19069-remove-exceptionhelper-detailedmessage branch September 17, 2019 17:30

hendrikmuhs mentioned this pull request Jan 23, 2020

TransformTaskFailedStateIT.testForceStopFailedTransform failure #51360

Closed

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

droberts195 mentioned this pull request Nov 16, 2023

Improve transform error handling #102240

Merged

Conversation

pugnascotia commented Aug 22, 2019

Uh oh!

elasticmachine commented Aug 22, 2019

Uh oh!

rjernst commented Aug 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst 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!

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!

rjernst 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!

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!

pugnascotia commented Sep 4, 2019

Uh oh!

pugnascotia commented Sep 11, 2019

Uh oh!

rjernst 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pugnascotia commented Sep 12, 2019

Uh oh!

pugnascotia commented Sep 13, 2019

Uh oh!

pugnascotia commented Sep 13, 2019

Uh oh!

pugnascotia commented Sep 16, 2019

Uh oh!

pugnascotia commented Sep 17, 2019

Uh oh!

pugnascotia commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants