From c9bced231cc40df67773edc07fde4bd5e18ac5a3 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Wed, 7 Feb 2024 17:56:07 +0700 Subject: [PATCH] fix error order flake Signed-off-by: Adrian Cole --- .../internal/BulkCallBuilder.java | 5 +- .../internal/client/HttpCall.java | 15 +++++- .../internal/BulkCallBuilderTest.java | 53 +++++++++++++++++-- 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java b/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java index 148c2019eba..b6e37996fa7 100644 --- a/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java +++ b/zipkin-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/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java b/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java index 6f9dd13d128..629a2763c11 100644 --- a/zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java +++ b/zipkin-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 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 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/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/internal/BulkCallBuilderTest.java b/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/internal/BulkCallBuilderTest.java index 6996cec416b..32c4a400ac0 100644 --- a/zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/internal/BulkCallBuilderTest.java +++ b/zipkin-storage/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"); + } }