fix error order flake

Signed-off-by: Adrian Cole <adrian@tetrate.io>
openzipkin · Feb 7, 2024 · c9bced2 · c9bced2
1 parent 0d0c99e
commit c9bced2
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 8 deletions.
diff --git a/...n-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java b/...n-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2015-2023 The OpenZipkin Authors
+ * Copyright 2015-2024 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -45,6 +45,7 @@
 import static zipkin2.Call.propagateIfFatal;
 import static zipkin2.elasticsearch.ElasticsearchVersion.V7_0;
 import static zipkin2.elasticsearch.internal.JsonSerializers.OBJECT_MAPPER;
+import static zipkin2.elasticsearch.internal.client.HttpCall.maybeRootCauseReason;
 
 // See https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html
 // exposed to re-use for testing writes of dependency links
@@ -60,7 +61,7 @@ public final class BulkCallBuilder {
         // only throw when we know it is an error
         if (!root.at("/errors").booleanValue() && !root.at("/error").isObject()) return null;
 
-        String message = root.findPath("reason").textValue();
+        String message = maybeRootCauseReason(root);
         if (message == null) message = contentString.get();
         Number status = root.findPath("status").numberValue();
         if (status != null && status.intValue() == 429) {

diff --git a/...n-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java b/...n-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2015-2023 The OpenZipkin Authors
+ * Copyright 2015-2024 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -233,7 +233,7 @@ V parseResponse(AggregatedHttpResponse response, BodyConverter<V> bodyConverter)
         String message = null;
         try {
           JsonNode root = OBJECT_MAPPER.readTree(parser);
-          message = root.findPath("reason").textValue();
+          message = maybeRootCauseReason(root);
           if (message == null) message = root.at("/Message").textValue();
         } catch (RuntimeException | IOException possiblyParseException) {
           // EmptyCatch ignored
@@ -252,4 +252,15 @@ V parseResponse(AggregatedHttpResponse response, BodyConverter<V> bodyConverter)
       return bodyConverter.convert(parser, content::toStringUtf8);
     }
   }
+
+  public static String maybeRootCauseReason(JsonNode root) {
+    // Prefer the root cause to an arbitrary reason.
+    String message;
+    if (!root.findPath("root_cause").isMissingNode()) {
+      message = root.findPath("root_cause").findPath("reason").textValue();
+    } else {
+      message = root.findPath("reason").textValue();
+    }
+    return message;
+  }
 }
diff --git a/...orage/elasticsearch/src/test/java/zipkin2/elasticsearch/internal/BulkCallBuilderTest.java b/...orage/elasticsearch/src/test/java/zipkin2/elasticsearch/internal/BulkCallBuilderTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2015-2023 The OpenZipkin Authors
+ * Copyright 2015-2024 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -32,13 +32,58 @@ class BulkCallBuilderTest {
         "rejected execution of org.elasticsearch.transport.TransportService$7@7ec1ea93 on EsThreadPoolExecutor[bulk, queue capacity = 200, org.elasticsearch.common.util.concurrent.EsThreadPoolExecutor@621571ba[Running, pool size = 4, active threads = 4, queued tasks = 200, completed tasks = 3838534]]");
   }
 
-  @Test void throwsRuntimeExceptionAsReasonWhenPresent() {
-    String response =
-      "{\"error\":{\"root_cause\":[{\"type\":\"illegal_argument_exception\",\"reason\":\"Fielddata is disabled on text fields by default. Set fielddata=true on [spanName] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead.\"}],\"type\":\"search_phase_execution_exception\",\"reason\":\"all shards failed\",\"phase\":\"query\",\"grouped\":true,\"failed_shards\":[{\"shard\":0,\"index\":\"zipkin-2017-05-14\",\"node\":\"IqceAwZnSvyv0V0xALkEnQ\",\"reason\":{\"type\":\"illegal_argument_exception\",\"reason\":\"Fielddata is disabled on text fields by default. Set fielddata=true on [spanName] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead.\"}}]},\"status\":400}";
+  @Test void throwsRuntimeExceptionAsRootCauseReasonWhenPresent() {
+    String response = """
+      {
+        "error": {
+          "root_cause": [
+            {
+              "type": "illegal_argument_exception",
+              "reason": "Fielddata is disabled on text fields by default. Set fielddata=true on [spanName] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead."
+            }
+          ],
+          "type": "search_phase_execution_exception",
+          "reason": "all shards failed",
+          "phase": "query",
+          "grouped": true,
+          "failed_shards": [
+            {
+              "shard": 0,
+              "index": "zipkin-2017-05-14",
+              "node": "IqceAwZnSvyv0V0xALkEnQ",
+              "reason": {
+                "type": "illegal_argument_exception",
+                "reason": "Fielddata is disabled on text fields by default. Set fielddata=true on [spanName] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead."
+              }
+            }
+          ]
+        },
+        "status": 400
+      }
+      """;
 
     assertThatThrownBy(
       () -> CHECK_FOR_ERRORS.convert(JSON_FACTORY.createParser(response), () -> response))
       .isInstanceOf(RuntimeException.class)
       .hasMessage("Fielddata is disabled on text fields by default. Set fielddata=true on [spanName] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead.");
   }
+
+  /** Tests lack of a root cause won't crash */
+  @Test void throwsRuntimeExceptionAsReasonWhenPresent() {
+    String response = """
+      {
+        "error": {
+          "type": "search_phase_execution_exception",
+          "reason": "all shards failed",
+          "phase": "query"
+        },
+        "status": 400
+      }
+      """;
+
+    assertThatThrownBy(
+      () -> CHECK_FOR_ERRORS.convert(JSON_FACTORY.createParser(response), () -> response))
+      .isInstanceOf(RuntimeException.class)
+      .hasMessage("all shards failed");
+  }
 }