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) ```
elastic · Aug 21, 2015 · d96e863 · d96e863
1 parent ded4429
commit d96e863
Show file tree

Hide file tree

Showing 7 changed files with 32 additions and 53 deletions.
diff --git 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
@@ -25,6 +25,8 @@
 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,28 +117,34 @@ public RestStatus status() {
         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);
             builder.field("root_cause");
             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 @@ private static XContentBuilder convert(RestChannel channel, RestStatus status, T
         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
@@ -57,7 +57,7 @@
 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
@@ -526,7 +526,7 @@ private String toXContent(ToXContent x) {
         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
@@ -125,7 +125,7 @@ public void simpleAdd4() throws Exception {
     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
@@ -32,6 +32,7 @@
 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 void testThatErrorTraceWorksByDefault() throws Exception {
                 .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
@@ -105,8 +105,8 @@ public void testErrorTrace() throws Exception {
         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 @@ private static class DetailedExceptionRestChannel extends RestChannel {
 
         DetailedExceptionRestChannel(RestRequest request) {
             super(request, true);
-            request.params().put(ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "true");
         }
 
         @Override