From d96e8634ac45c8144b942a7b12ce1a01b66301fa Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 19 Aug 2015 15:40:22 +0200 Subject: [PATCH] Suppress rest exceptions by default and log them instead Today we are very verbose when rendering exceptions on the rest layer. Yet, this isn't necessarily very easy to read and way too much infromation most of the time. This change suppresses the stacktrace rendering by default but instead adds a `rest.suppressed` logger that logs the suppressed stacktrace or rather the entire exception on the node that renderes the exception The log message looks like this: ``` [2015-08-19 16:21:58,427][INFO ][rest.suppressed ] /test/_search/ Params: {index=test} [test] IndexNotFoundException[no such index] at org.elasticsearch.cluster.metadata.IndexNameExpressionResolver$WildcardExpressionResolver.resolve(IndexNameExpressionResolver.java:551) ``` --- .../elasticsearch/ElasticsearchException.java | 4 +- .../elasticsearch/rest/BytesRestResponse.java | 57 +++++-------------- .../org/elasticsearch/ESExceptionTests.java | 2 +- .../ExceptionSerializationTests.java | 2 +- .../search/MultiSearchRequestTests.java | 2 +- .../DetailedErrorsEnabledIT.java | 13 ++++- .../rest/BytesRestResponseTests.java | 5 +- 7 files changed, 32 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/ElasticsearchException.java b/core/src/main/java/org/elasticsearch/ElasticsearchException.java index a4def64bf08..4f478057fa0 100644 --- a/core/src/main/java/org/elasticsearch/ElasticsearchException.java +++ b/core/src/main/java/org/elasticsearch/ElasticsearchException.java @@ -45,8 +45,8 @@ public class ElasticsearchException extends RuntimeException implements ToXConte public static final String REST_EXCEPTION_SKIP_CAUSE = "rest.exception.cause.skip"; public static final String REST_EXCEPTION_SKIP_STACK_TRACE = "rest.exception.stacktrace.skip"; - private static final boolean REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT = false; - private static final boolean REST_EXCEPTION_SKIP_CAUSE_DEFAULT = false; + public static final boolean REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT = true; + public static final boolean REST_EXCEPTION_SKIP_CAUSE_DEFAULT = false; private static final String INDEX_HEADER_KEY = "es.index"; private static final String SHARD_HEADER_KEY = "es.shard"; private static final String RESOURCE_HEADER_TYPE_KEY = "es.resource.type"; diff --git a/core/src/main/java/org/elasticsearch/rest/BytesRestResponse.java b/core/src/main/java/org/elasticsearch/rest/BytesRestResponse.java index b64c9c58732..fc944f49460 100644 --- a/core/src/main/java/org/elasticsearch/rest/BytesRestResponse.java +++ b/core/src/main/java/org/elasticsearch/rest/BytesRestResponse.java @@ -25,6 +25,8 @@ import org.elasticsearch.bootstrap.Elasticsearch; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.logging.ESLogger; +import org.elasticsearch.common.logging.ESLoggerFactory; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -115,11 +117,20 @@ public class BytesRestResponse extends RestResponse { return this.status; } + private static final ESLogger SUPPRESSED_ERROR_LOGGER = ESLoggerFactory.getLogger("rest.suppressed"); + private static XContentBuilder convert(RestChannel channel, RestStatus status, Throwable t) throws IOException { XContentBuilder builder = channel.newErrorBuilder().startObject(); if (t == null) { builder.field("error", "unknown"); } else if (channel.detailedErrorsEnabled()) { + final ToXContent.Params params; + if (channel.request().paramAsBoolean("error_trace", !ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT)) { + params = new ToXContent.DelegatingMapParams(Collections.singletonMap(ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "false"), channel.request()); + } else { + SUPPRESSED_ERROR_LOGGER.info("{} Params: {}", t, channel.request().path(), channel.request().params()); + params = channel.request(); + } builder.field("error"); builder.startObject(); final ElasticsearchException[] rootCauses = ElasticsearchException.guessRootCauses(t); @@ -127,16 +138,13 @@ public class BytesRestResponse extends RestResponse { builder.startArray(); for (ElasticsearchException rootCause : rootCauses){ builder.startObject(); - rootCause.toXContent(builder, new ToXContent.DelegatingMapParams(Collections.singletonMap(ElasticsearchException.REST_EXCEPTION_SKIP_CAUSE, "true"), channel.request())); + rootCause.toXContent(builder, new ToXContent.DelegatingMapParams(Collections.singletonMap(ElasticsearchException.REST_EXCEPTION_SKIP_CAUSE, "true"), params)); builder.endObject(); } builder.endArray(); - ElasticsearchException.toXContent(builder, channel.request(), t); + ElasticsearchException.toXContent(builder, params, t); builder.endObject(); - if (channel.request().paramAsBoolean("error_trace", false)) { - buildErrorTrace(t, builder); - } } else { builder.field("error", simpleMessage(t)); } @@ -145,45 +153,6 @@ public class BytesRestResponse extends RestResponse { return builder; } - - private static void buildErrorTrace(Throwable t, XContentBuilder builder) throws IOException { - builder.startObject("error_trace"); - boolean first = true; - int counter = 0; - while (t != null) { - // bail if there are more than 10 levels, becomes useless really... - if (counter++ > 10) { - break; - } - if (!first) { - builder.startObject("cause"); - } - buildThrowable(t, builder); - if (!first) { - builder.endObject(); - } - t = t.getCause(); - first = false; - } - builder.endObject(); - } - - private static void buildThrowable(Throwable t, XContentBuilder builder) throws IOException { - builder.field("message", t.getMessage()); - for (StackTraceElement stElement : t.getStackTrace()) { - builder.startObject("at") - .field("class", stElement.getClassName()) - .field("method", stElement.getMethodName()); - if (stElement.getFileName() != null) { - builder.field("file", stElement.getFileName()); - } - if (stElement.getLineNumber() >= 0) { - builder.field("line", stElement.getLineNumber()); - } - builder.endObject(); - } - } - /* * Builds a simple error string from the message of the first ElasticsearchException */ diff --git a/core/src/test/java/org/elasticsearch/ESExceptionTests.java b/core/src/test/java/org/elasticsearch/ESExceptionTests.java index 6be1b7ccae2..dea127a6450 100644 --- a/core/src/test/java/org/elasticsearch/ESExceptionTests.java +++ b/core/src/test/java/org/elasticsearch/ESExceptionTests.java @@ -57,7 +57,7 @@ import java.util.Collections; import static org.hamcrest.Matchers.equalTo; public class ESExceptionTests extends ESTestCase { - private static final ToXContent.Params PARAMS = new ToXContent.MapParams(Collections.singletonMap(ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "true")); + private static final ToXContent.Params PARAMS = ToXContent.EMPTY_PARAMS; @Test public void testStatus() { diff --git a/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java b/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java index 889901e549a..c8a042fe1fc 100644 --- a/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java +++ b/core/src/test/java/org/elasticsearch/ExceptionSerializationTests.java @@ -526,7 +526,7 @@ public class ExceptionSerializationTests extends ESTestCase { try { XContentBuilder builder = XContentFactory.jsonBuilder(); builder.startObject(); - x.toXContent(builder, new ToXContent.MapParams(Collections.singletonMap(ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "true"))); + x.toXContent(builder, ToXContent.EMPTY_PARAMS); builder.endObject(); return builder.string(); } catch (IOException e) { diff --git a/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java b/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java index cad56790631..d2533eaf2da 100644 --- a/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java +++ b/core/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java @@ -125,7 +125,7 @@ public class MultiSearchRequestTests extends ESTestCase { public void testResponseErrorToXContent() throws IOException { MultiSearchResponse response = new MultiSearchResponse(new MultiSearchResponse.Item[]{new MultiSearchResponse.Item(null, new IllegalStateException("foobar")), new MultiSearchResponse.Item(null, new IllegalStateException("baaaaaazzzz"))}); XContentBuilder builder = XContentFactory.jsonBuilder(); - response.toXContent(builder, new ToXContent.MapParams(Collections.singletonMap(ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "true"))); + response.toXContent(builder, ToXContent.EMPTY_PARAMS); assertEquals("\"responses\"[{\"error\":{\"root_cause\":[{\"type\":\"illegal_state_exception\",\"reason\":\"foobar\"}],\"type\":\"illegal_state_exception\",\"reason\":\"foobar\"}},{\"error\":{\"root_cause\":[{\"type\":\"illegal_state_exception\",\"reason\":\"baaaaaazzzz\"}],\"type\":\"illegal_state_exception\",\"reason\":\"baaaaaazzzz\"}}]", builder.string()); } diff --git a/core/src/test/java/org/elasticsearch/options/detailederrors/DetailedErrorsEnabledIT.java b/core/src/test/java/org/elasticsearch/options/detailederrors/DetailedErrorsEnabledIT.java index af3e57075c5..050d88c2f39 100644 --- a/core/src/test/java/org/elasticsearch/options/detailederrors/DetailedErrorsEnabledIT.java +++ b/core/src/test/java/org/elasticsearch/options/detailederrors/DetailedErrorsEnabledIT.java @@ -32,6 +32,7 @@ import org.elasticsearch.test.rest.client.http.HttpResponse; import org.junit.Test; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; /** * Tests that by default the error_trace parameter can be used to show stacktraces @@ -59,6 +60,16 @@ public class DetailedErrorsEnabledIT extends ESIntegTestCase { .execute(); assertThat(response.getHeaders().get("Content-Type"), containsString("application/json")); - assertThat(response.getBody(), containsString("\"error_trace\":{\"message\":\"Validation Failed")); + assertThat(response.getBody(), containsString("\"stack_trace\":\"[Validation Failed: 1: index / indices is missing;]; nested: ActionRequestValidationException[Validation Failed: 1:")); + + // Make the HTTP request + response = new HttpRequestBuilder(HttpClients.createDefault()) + .httpTransport(internalCluster().getDataNodeInstance(HttpServerTransport.class)) + .path("/") + .method(HttpDeleteWithEntity.METHOD_NAME) + .execute(); + + assertThat(response.getHeaders().get("Content-Type"), containsString("application/json")); + assertThat(response.getBody(), not(containsString("\"stack_trace\":\"[Validation Failed: 1: index / indices is missing;]; nested: ActionRequestValidationException[Validation Failed: 1:"))); } } diff --git a/core/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java b/core/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java index 20a20581c6d..e379ebbd32f 100644 --- a/core/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java +++ b/core/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java @@ -105,8 +105,8 @@ public class BytesRestResponseTests extends ESTestCase { BytesRestResponse response = new BytesRestResponse(channel, t); String text = response.content().toUtf8(); assertThat(text, containsString("\"type\":\"throwable\",\"reason\":\"an error occurred reading data\"")); - assertThat(text, containsString("{\"type\":\"file_not_found_exception\",\"reason\":\"/foo/bar\"}")); - assertThat(text, containsString("\"error_trace\":{\"message\":\"an error occurred reading data\"")); + assertThat(text, containsString("{\"type\":\"file_not_found_exception\"")); + assertThat(text, containsString("\"stack_trace\":\"[an error occurred reading data]")); } public void testGuessRootCause() throws IOException { @@ -176,7 +176,6 @@ public class BytesRestResponseTests extends ESTestCase { DetailedExceptionRestChannel(RestRequest request) { super(request, true); - request.params().put(ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "true"); } @Override