From 96b24d1b4a1616ffefc1e47023c9e86f0abba075 Mon Sep 17 00:00:00 2001 From: Karen X Date: Wed, 8 Oct 2025 20:32:41 +0000 Subject: [PATCH] [GRPC] Return detailed error for GRPC error response Signed-off-by: Karen X --- CHANGELOG.md | 1 + .../SearchRequestActionListener.java | 2 +- .../grpc/services/DocumentServiceImpl.java | 2 +- .../grpc/services/SearchServiceImpl.java | 2 +- .../transport/grpc/util/GrpcErrorHandler.java | 81 +++++++-- .../grpc/util/GrpcErrorHandlerTests.java | 165 +++++++++++++----- 6 files changed, 189 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d087c7e8d3371..f050227c82d64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Harden the circuit breaker and failure handle logic in query result consumer ([#19396](https://github.com/opensearch-project/OpenSearch/pull/19396)) - Add streaming cardinality aggregator ([#19484](https://github.com/opensearch-project/OpenSearch/pull/19484)) - Disable request cache for streaming aggregation queries ([#19520](https://github.com/opensearch-project/OpenSearch/pull/19520)) +- Return full error for GRPC error response ([#19568](https://github.com/opensearch-project/OpenSearch/pull/19568)) ### Changed - Refactor `if-else` chains to use `Java 17 pattern matching switch expressions`(([#18965](https://github.com/opensearch-project/OpenSearch/pull/18965)) diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java index 7712d85a3a217..ec20b15bde263 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java @@ -54,7 +54,7 @@ public void onResponse(SearchResponse response) { @Override public void onFailure(Exception e) { - logger.error("SearchRequestActionListener failed to process search request: " + e.getMessage()); + logger.debug("SearchRequestActionListener failed to process search request: " + e.getMessage()); StatusRuntimeException grpcError = GrpcErrorHandler.convertToGrpcError(e); responseObserver.onError(grpcError); } diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/DocumentServiceImpl.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/DocumentServiceImpl.java index 873bad5e69e4f..e1348bc78961f 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/DocumentServiceImpl.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/DocumentServiceImpl.java @@ -48,7 +48,7 @@ public void bulk(org.opensearch.protobufs.BulkRequest request, StreamObserver INVALID_ARGUMENT via RestToGrpcStatusConverter assertEquals(Status.INVALID_ARGUMENT.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("[Test exception]")); + // Uses ExceptionsHelper.summaryMessage() format + XContent details + assertTrue(result.getMessage().contains("OpenSearchException[Test exception]")); + assertTrue(result.getMessage().contains("details=")); + assertTrue(result.getMessage().contains("\"type\":\"exception\"")); + assertTrue(result.getMessage().contains("\"reason\":\"Test exception\"")); + assertTrue(result.getMessage().contains("\"status\":400")); } public void testIllegalArgumentExceptionConversion() { @@ -50,11 +56,10 @@ public void testIllegalArgumentExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // IllegalArgumentException -> INVALID_ARGUMENT via direct gRPC mapping assertEquals(Status.INVALID_ARGUMENT.getCode(), result.getStatus().getCode()); - // Now includes full exception information for debugging (preserves original responseObserver.onError(e) behavior) - assertTrue(result.getMessage().contains("Invalid parameter")); - assertTrue(result.getMessage().contains("IllegalArgumentException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging + assertTrue(result.getMessage().contains("java.lang.IllegalArgumentException: Invalid parameter")); } public void testInputCoercionExceptionConversion() { @@ -62,10 +67,11 @@ public void testInputCoercionExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // InputCoercionException -> INVALID_ARGUMENT via direct gRPC mapping assertEquals(Status.INVALID_ARGUMENT.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("Cannot coerce string to number")); + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging assertTrue(result.getMessage().contains("InputCoercionException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + assertTrue(result.getMessage().contains("Cannot coerce string to number")); } public void testJsonParseExceptionConversion() { @@ -73,10 +79,11 @@ public void testJsonParseExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // JsonParseException -> INVALID_ARGUMENT via direct gRPC mapping assertEquals(Status.INVALID_ARGUMENT.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("Unexpected character")); + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging assertTrue(result.getMessage().contains("JsonParseException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + assertTrue(result.getMessage().contains("Unexpected character")); } public void testOpenSearchRejectedExecutionExceptionConversion() { @@ -84,21 +91,11 @@ public void testOpenSearchRejectedExecutionExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // OpenSearchRejectedExecutionException -> RESOURCE_EXHAUSTED via direct gRPC mapping assertEquals(Status.RESOURCE_EXHAUSTED.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("Thread pool full")); + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging assertTrue(result.getMessage().contains("OpenSearchRejectedExecutionException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator - } - - public void testNotXContentExceptionConversion() { - NotXContentException exception = new NotXContentException("Content is not XContent"); - - StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); - - assertEquals(Status.INVALID_ARGUMENT.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("Content is not XContent")); - assertTrue(result.getMessage().contains("NotXContentException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + assertTrue(result.getMessage().contains("Thread pool full")); } public void testIllegalStateExceptionConversion() { @@ -106,10 +103,10 @@ public void testIllegalStateExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // IllegalStateException -> FAILED_PRECONDITION via direct gRPC mapping assertEquals(Status.FAILED_PRECONDITION.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("Invalid state")); - assertTrue(result.getMessage().contains("IllegalStateException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging + assertTrue(result.getMessage().contains("java.lang.IllegalStateException: Invalid state")); } public void testSecurityExceptionConversion() { @@ -117,10 +114,10 @@ public void testSecurityExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // SecurityException -> PERMISSION_DENIED via direct gRPC mapping assertEquals(Status.PERMISSION_DENIED.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("Access denied")); - assertTrue(result.getMessage().contains("SecurityException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging + assertTrue(result.getMessage().contains("java.lang.SecurityException: Access denied")); } public void testTimeoutExceptionConversion() { @@ -128,10 +125,10 @@ public void testTimeoutExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // TimeoutException -> DEADLINE_EXCEEDED via direct gRPC mapping assertEquals(Status.DEADLINE_EXCEEDED.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("Operation timed out")); - assertTrue(result.getMessage().contains("TimeoutException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging + assertTrue(result.getMessage().contains("java.util.concurrent.TimeoutException: Operation timed out")); } public void testInterruptedExceptionConversion() { @@ -139,9 +136,10 @@ public void testInterruptedExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // InterruptedException -> CANCELLED via direct gRPC mapping assertEquals(Status.CANCELLED.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("InterruptedException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging + assertTrue(result.getMessage().contains("java.lang.InterruptedException")); } public void testIOExceptionConversion() { @@ -149,10 +147,10 @@ public void testIOExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // IOException -> INTERNAL via direct gRPC mapping assertEquals(Status.INTERNAL.getCode(), result.getStatus().getCode()); - assertTrue(result.getMessage().contains("I/O error")); - assertTrue(result.getMessage().contains("IOException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging + assertTrue(result.getMessage().contains("java.io.IOException: I/O error")); } public void testUnknownExceptionConversion() { @@ -160,11 +158,10 @@ public void testUnknownExceptionConversion() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // RuntimeException -> INTERNAL via fallback (unknown exception type) assertEquals(Status.INTERNAL.getCode(), result.getStatus().getCode()); - // Now includes full exception information for debugging - assertTrue(result.getMessage().contains("Unknown error")); - assertTrue(result.getMessage().contains("RuntimeException")); - assertTrue(result.getMessage().contains("at ")); // Stack trace indicator + // Uses ExceptionsHelper.stackTrace() - includes full stack trace for debugging + assertTrue(result.getMessage().contains("java.lang.RuntimeException: Unknown error")); } public void testOpenSearchExceptionWithNullMessage() { @@ -177,8 +174,11 @@ public RestStatus status() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + // NOT_FOUND -> NOT_FOUND via RestToGrpcStatusConverter assertEquals(Status.NOT_FOUND.getCode(), result.getStatus().getCode()); + // Uses ExceptionsHelper.summaryMessage() format + XContent details assertTrue(result.getMessage().contains("OpenSearchException[null]")); + assertTrue(result.getMessage().contains("details=")); } public void testCircuitBreakingExceptionInCleanMessage() { @@ -186,9 +186,11 @@ public void testCircuitBreakingExceptionInCleanMessage() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); - assertEquals(Status.RESOURCE_EXHAUSTED.getCode(), result.getStatus().getCode()); // CircuitBreakingException -> TOO_MANY_REQUESTS -> - // RESOURCE_EXHAUSTED + // CircuitBreakingException extends OpenSearchException with TOO_MANY_REQUESTS -> RESOURCE_EXHAUSTED + assertEquals(Status.RESOURCE_EXHAUSTED.getCode(), result.getStatus().getCode()); + // Uses ExceptionsHelper.summaryMessage() format + XContent details for OpenSearchException assertTrue(result.getMessage().contains("CircuitBreakingException[Memory circuit breaker]")); + assertTrue(result.getMessage().contains("details=")); } public void testSearchPhaseExecutionExceptionInCleanMessage() { @@ -200,8 +202,81 @@ public void testSearchPhaseExecutionExceptionInCleanMessage() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); - // SearchPhaseExecutionException with empty shardFailures -> SERVICE_UNAVAILABLE -> UNAVAILABLE + // SearchPhaseExecutionException extends OpenSearchException with SERVICE_UNAVAILABLE -> UNAVAILABLE assertEquals(Status.UNAVAILABLE.getCode(), result.getStatus().getCode()); + // Uses ExceptionsHelper.summaryMessage() format + XContent details for OpenSearchException assertTrue(result.getMessage().contains("SearchPhaseExecutionException[Search failed]")); + assertTrue(result.getMessage().contains("details=")); + // Should include phase information in XContent + assertTrue(result.getMessage().contains("\"phase\":\"query\"")); + } + + public void testXContentMetadataExtractionSuccess() { + // Test that XContent metadata extraction works for OpenSearchException + OpenSearchException exception = new OpenSearchException("Test with metadata") { + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } + }; + exception.addMetadata("opensearch.test_key", "test_value"); + + StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + + // Should include metadata in JSON details + assertTrue(result.getMessage().contains("details=")); + assertTrue(result.getMessage().contains("\"test_key\":\"test_value\"")); + assertTrue(result.getMessage().contains("\"status\":400")); + } + + public void testXContentMetadataExtractionFailure() { + // Test graceful handling when XContent extraction fails + OpenSearchException exception = new OpenSearchException("Test exception") { + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } + + @Override + public void metadataToXContent( + org.opensearch.core.xcontent.XContentBuilder builder, + org.opensearch.core.xcontent.ToXContent.Params params + ) throws java.io.IOException { + // Simulate XContent failure + throw new IOException("XContent failure"); + } + }; + + StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception); + + // Should fall back to base description when XContent extraction fails + assertEquals(Status.INVALID_ARGUMENT.getCode(), result.getStatus().getCode()); + assertTrue(result.getMessage().contains("OpenSearchException[Test exception]")); + // Should not contain details when extraction fails + assertFalse(result.getMessage().contains("details=")); + } + + public void testRootCauseAnalysis() { + // Test that root_cause analysis is included like HTTP responses + OpenSearchException rootCause = new OpenSearchException("Root cause") { + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } + }; + + OpenSearchException wrappedException = new OpenSearchException("Wrapper exception", rootCause) { + @Override + public RestStatus status() { + return RestStatus.INTERNAL_SERVER_ERROR; + } + }; + + StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(wrappedException); + + // Should include root_cause array like HTTP responses + assertTrue(result.getMessage().contains("root_cause")); + assertTrue(result.getMessage().contains("Root cause")); + assertTrue(result.getMessage().contains("Wrapper exception")); } }