[GRPC] Add handling for the grpc.detailed_errors.enabled setting and global gRPC error_trace parameter #19644
Conversation
…andling Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
…setting for gRPC Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
github-actions
bot
added
enhancement
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
|
❌ Gradle check result for 702f7d9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19644 +/- ##
============================================
+ Coverage 73.27% 73.29% +0.01%
- Complexity 71553 71565 +12
============================================
Files 5789 5790 +1
Lines 327144 327187 +43
Branches 47156 47161 +5
============================================
+ Hits 239715 239798 +83
+ Misses 68235 68180 -55
- Partials 19194 19209 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
|
❌ Gradle check result for 9b42790: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Looks like a failure due to a flaky test, more here: #14312 |
|
Hi folks, this PR is ready for review, could someone please take a look? Thanks a lot! |
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java
Outdated
Show resolved
Hide resolved
The CI can be rerun by closing/re-opening the PR, or pushing an empty commit. (Or by resolving conflicts which creates a merge commit) |
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
…ror-handling-setting
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
…ror-handling-setting
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
|
❌ Gradle check result for f6d8f9a: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
karenyrx
left a comment
karenyrx
left a comment
There was a problem hiding this comment.
Side question: Do we also eventually plan for support for REST_EXCEPTION_SKIP_STACK_TRACE as well?
...ain/java/org/opensearch/transport/grpc/proto/response/exceptions/ResponseHandlingParams.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/transport/grpc/proto/response/exceptions/ResponseHandlingParams.java
Outdated
Show resolved
Hide resolved
...s/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/SearchServiceImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/transport/grpc/proto/response/exceptions/ResponseHandlingParams.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/transport/grpc/proto/response/exceptions/ResponseHandlingParams.java
Outdated
Show resolved
Hide resolved
@karenyrx, good question. What would be use cases here and what an implementation might look like? |
|
❕ Gradle check result for 9cd0575: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
|
❌ Gradle check result for 7863268: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
fdesu
force-pushed
the
issue/19639-grpc-error-handling-setting
branch
from
d3d661e to
06adbf2
Compare
|
❌ Gradle check result for 06adbf2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for ba7fae3: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
…ror-handling-setting Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
fdesu
force-pushed
the
issue/19639-grpc-error-handling-setting
branch
from
7c4c483 to
8767699
Compare
|
The issue with DCO was a bit deeper down the line. It's solved now. |
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
fdesu
force-pushed
the
issue/19639-grpc-error-handling-setting
branch
from
4ba8567 to
a5a64e3
Compare
WalkthroughThis PR implements handling for the global gRPC error_trace parameter by introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…ror-handling-setting Signed-off-by: Sergei Ustimenko <fdesu@proton.me>
fdesu
force-pushed
the
issue/19639-grpc-error-handling-setting
branch
from
5ceb0ee to
2907384
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
48-50: Minor typo: Missing space in error message.There's a missing space before "cannot" in the error message.
default -> throw new UnsupportedOperationException( - "Unsupported ShardOperationFailedException " + exception.getClass().getName() + "cannot be converted to proto." + "Unsupported ShardOperationFailedException " + exception.getClass().getName() + " cannot be converted to proto." );
🧹 Nitpick comments (12)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/GrpcPluginTests.java (1)
147-147: Consider adding an explicit assertion for the new setting.The count was updated to 14 to account for
SETTING_GRPC_DETAILED_ERRORS_ENABLED, but unlike other settings, there's no explicitassertTruecheck verifying it's included. This could mask issues if the setting is accidentally removed later.assertTrue("SETTING_GRPC_KEEPALIVE_TIMEOUT should be included", settings.contains(SETTING_GRPC_KEEPALIVE_TIMEOUT)); + assertTrue("SETTING_GRPC_DETAILED_ERRORS_ENABLED should be included", settings.contains(Netty4GrpcServerTransport.SETTING_GRPC_DETAILED_ERRORS_ENABLED)); // Verify the number of settings assertEquals("Should return 14 settings", 14, settings.size());modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkResponseProtoUtils.java (1)
38-56: LGTM - GlobalParams correctly threaded through bulk response conversion.The
paramsargument is properly propagated toBulkItemResponseProtoUtils.toProto(), enabling consistent error_trace handling for each bulk item's error cause.Minor cleanup: Consider removing the commented-out debug statement on line 39.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtilsTests.java (1)
22-23: LGTM - Tests updated to match new API signature.The test cases correctly use
GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSEfor the updatedtoProto()signature.Consider adding a test case with
GLOBAL_PARAMS_WITH_ERROR_TRACE_TRUEfor thetestToProtoWithFailuresscenario to verify that stack traces are included in the shard failure reasons when error_trace is enabled.Also applies to: 31-31, 60-60, 92-92
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListenerTests.java (1)
37-37: Consider using TestFixtures constants for consistency.The listener is initialized with
GlobalParams.newBuilder().build(). For consistency with other tests and clarity, consider usingTestFixtures.GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE(which is equivalent sinceerrorTracedefaults tofalse).+import static org.opensearch.transport.grpc.TestFixtures.GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE; ... - listener = new SearchRequestActionListener(responseObserver, GlobalParams.newBuilder().build()); + listener = new SearchRequestActionListener(responseObserver, GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE);modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
14-24: Add private constructor to complete utility class pattern.The class is correctly marked
final, but a private constructor should be added to prevent instantiation, following the utility class pattern used throughout this codebase.public final class TestFixtures { + private TestFixtures() { + // Utility class, no instances + } + public static final GlobalParams GLOBAL_PARAMS_WITH_ERROR_TRACE_TRUE = GlobalParams.newBuilder().setErrorTrace(true).build();modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java (1)
39-43: Consider adding null-check forparamsparameter.The constructor stores
paramswithout validation, but bothonResponseandonFailurepaths use it. If a caller passesnull, this would cause an NPE whenSearchResponseProtoUtils.toProtoorGrpcErrorHandler.convertToGrpcErroris invoked.public SearchRequestActionListener(StreamObserver<org.opensearch.protobufs.SearchResponse> responseObserver, GlobalParams params) { super(); + if (params == null) { + throw new IllegalArgumentException("params cannot be null"); + } this.responseObserver = responseObserver; this.params = params; }modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtils.java (1)
52-60: Missing@paramdocumentation forparams.The Javadoc for the private
toProtomethod is missing the@param paramsentry to match the updated method signature./** * Converts a ReplicationResponse.ShardInfo.Failure Java object to a protobuf ShardFailure. * Similar to {@link ReplicationResponse.ShardInfo.Failure#toXContent(XContentBuilder, ToXContent.Params)} * * @param failure The shard failure to convert to protobuf format + * @param params The global gRPC request parameters * @return The protobuf representation of the shard failure * @throws IOException If there's an error during conversion */ private static ShardFailure toProto(ReplicationResponse.ShardInfo.Failure failure, GlobalParams params) throws IOException {modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcParamsHandlerTests.java (1)
35-43: Simplify the validation pass test.Using try-catch with
fail()is verbose. SincevalidateStackTraceDetailsConfigurationdoesn't return a value, simply calling it without catching exceptions is sufficient—the test framework will fail if an unexpected exception is thrown.public void testValidationPassesWhenDetailedErrorsDisabledAndClientDoesNotRequestStackTrace() { GrpcParamsHandler.initialize(settingsWithGivenStackTraceConfig(false)); - - try { - GrpcParamsHandler.validateStackTraceDetailsConfiguration(GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE); - } catch (Exception e) { - fail("Validation should pass without exceptions when stack traces are not requested."); - } + // Should not throw - test framework will fail if exception is thrown + GrpcParamsHandler.validateStackTraceDetailsConfiguration(GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE); }modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/services/SearchServiceImplTests.java (2)
44-55: Redundant@Afterreset when@Beforealready initializes.Both
@Beforeand@AftercallGrpcParamsHandler.initialize(settingsWithGivenStackTraceConfig(true)). Since@Beforeruns before each test, the@Afterreset is unnecessary unless there's a concern about test class cleanup affecting other test classes sharing static state.If the intent is to ensure clean state for other test classes, consider adding a clarifying comment. Otherwise, the
@Aftermethod can be removed since@Beforealready handles initialization.
117-130: Misleading variable name in passing test case.The variable
serviceThatFailsToProvideErrorInfois used in both tests, but its name only accurately describes the first test's scenario. In this test, validation passes and the service operates normally.public void testErrorTraceConfigValidationPassesWhenServerSettingIsDisabledAndRequestSkipsStackTrace() { // Setup request and the service, server setting is off and request skips a stack trace SearchRequest request = createTestSearchRequest().toBuilder() .setGlobalParams(org.opensearch.protobufs.GlobalParams.newBuilder().setErrorTrace(false)) .build(); GrpcParamsHandler.initialize(settingsWithGivenStackTraceConfig(false)); - SearchServiceImpl serviceThatFailsToProvideErrorInfo = new SearchServiceImpl(client, queryUtils); + SearchServiceImpl serviceWithErrorTracingDisabled = new SearchServiceImpl(client, queryUtils); // Call search method - serviceThatFailsToProvideErrorInfo.search(request, responseObserver); + serviceWithErrorTracingDisabled.search(request, responseObserver); // Verify that client.search was called verify(client).search(any(org.opensearch.action.search.SearchRequest.class), any()); }modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java (1)
52-57: Incomplete Javadoc forparamsparameter.The
@param paramsdocumentation is incomplete—it should describe what the parameter controls (e.g., "The global gRPC request parameters used to control error detail level").* @param e The exception to convert - * @param params + * @param params The global gRPC request parameters controlling error detail level (e.g., stack trace inclusion) * @return StatusRuntimeException with appropriate gRPC status and enhanced error detailsmodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java (1)
449-455: Consider adding a two-argument test for null exception handling.This test uses the single-argument overload
convertToGrpcError(null), while all other tests use the two-argument form. For completeness, consider adding a test that verifies null exception handling with the two-argument form as well, ensuring consistent behavior regardless of which overload is used.public void testNullExceptionConversionWithGlobalParams() { StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(null, GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE); assertEquals(Status.INTERNAL.getCode(), result.getStatus().getCode()); assertEquals("INTERNAL: Unexpected null exception", result.getMessage()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
CHANGELOG.md(1 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcPlugin.java(6 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/Netty4GrpcServerTransport.java(1 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/BulkRequestActionListener.java(4 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java(2 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkItemResponseProtoUtils.java(3 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkResponseProtoUtils.java(3 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/DocWriteResponseProtoUtils.java(3 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtils.java(3 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java(8 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java(6 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ReplicationResponseShardInfoFailureProtoUtils.java(3 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java(2 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java(2 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/SnapshotShardFailureProtoUtils.java(2 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ProtoActionsProtoUtils.java(3 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtils.java(4 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java(4 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/DocumentServiceImpl.java(2 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/SearchServiceImpl.java(2 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java(8 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/GrpcPluginTests.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/listeners/BulkRequestActionListenerTests.java(2 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListenerTests.java(3 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/BulkResponseProtoUtilsTests.java(4 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkItemResponseProtoUtilsTests.java(8 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/common/DocWriteResponseProtoUtilsTests.java(5 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtilsTests.java(3 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java(11 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java(6 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java(6 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtilsTests.java(8 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/services/SearchServiceImplTests.java(5 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/services/document/DocumentServiceImplTests.java(5 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java(16 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcParamsHandlerTests.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (23)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java (2)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
ShardOperationFailedExceptionProtoUtils(25-53)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkResponseProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkItemResponseProtoUtils.java (1)
BulkItemResponseProtoUtils(31-98)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/DocWriteResponseProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtils.java (1)
ShardInfoProtoUtils(23-70)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/SnapshotShardFailureProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
OpenSearchExceptionProtoUtils(50-273)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/BulkResponseProtoUtilsTests.java (2)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkResponseProtoUtils.java (1)
BulkResponseProtoUtils(23-80)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/listeners/BulkRequestActionListenerTests.java (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkItemResponseProtoUtilsTests.java (2)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkItemResponseProtoUtils.java (1)
BulkItemResponseProtoUtils(31-98)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ProtoActionsProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java (1)
ShardStatisticsProtoUtils(27-69)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/services/document/DocumentServiceImplTests.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java (1)
GrpcParamsHandler(30-82)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java (3)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java (1)
GrpcParamsHandler(30-82)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/RestToGrpcStatusConverter.java (1)
RestToGrpcStatusConverter(18-152)libs/core/src/main/java/org/opensearch/ExceptionsHelper.java (1)
ExceptionsHelper(76-476)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcPlugin.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java (1)
GrpcParamsHandler(30-82)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/Netty4GrpcServerTransport.java (1)
Netty4GrpcServerTransport(64-501)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/SearchServiceImpl.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java (1)
GrpcParamsHandler(30-82)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java (1)
GrpcErrorHandler(36-225)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/services/SearchServiceImplTests.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java (1)
GrpcParamsHandler(30-82)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtils.java (1)
SearchResponseProtoUtils(25-208)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java (1)
GrpcErrorHandler(36-225)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ReplicationResponseShardInfoFailureProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
OpenSearchExceptionProtoUtils(50-273)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtilsTests.java (2)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtils.java (1)
SearchResponseProtoUtils(25-208)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/Netty4GrpcServerTransport.java (1)
Netty4GrpcServerTransport(64-501)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
OpenSearchExceptionProtoUtils(50-273)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
ShardOperationFailedExceptionProtoUtils(25-53)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcParamsHandlerTests.java (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java (1)
GrpcParamsHandler(30-82)libs/core/src/main/java/org/opensearch/ExceptionsHelper.java (1)
ExceptionsHelper(76-476)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtilsTests.java (2)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtils.java (1)
ShardInfoProtoUtils(23-70)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (2)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/TestFixtures.java (1)
TestFixtures(14-24)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
OpenSearchExceptionProtoUtils(50-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: Analyze (java)
🔇 Additional comments (61)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkItemResponseProtoUtilsTests.java (2)
33-34: LGTM!The static import for
GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSEfromTestFixturesprovides a clean way to supply the requiredGlobalParamsparameter across all test cases.
52-55: Consistent update to the new method signature.All
toProtocalls are correctly updated to passGLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE, aligning with the production code's new signature that requiresGlobalParamsfor error trace control.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java (2)
24-24: LGTM!Static import added consistently with other test files in this PR.
136-144: Single-argumenttoProtooverload exists and the test is valid.The test code compiles successfully as shown in the final state, confirming the single-argument
toProto(ShardOperationFailedException)overload is present. No action required.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/document/common/DocWriteResponseProtoUtilsTests.java (1)
20-21: LGTM!Import and test updates are consistent with the pattern established across other test files in this PR.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/BulkResponseProtoUtilsTests.java (1)
25-26: LGTM!Consistent with the GlobalParams propagation pattern across all updated test files.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/DocWriteResponseProtoUtils.java (3)
13-13: LGTM!Import added for the new
GlobalParamsparameter type.
35-39: Proper Javadoc and signature update for GlobalParams threading.The method signature correctly includes the
GlobalParamsparameter with appropriate documentation. This enables error trace control to propagate through the proto conversion chain.
61-61: Correct propagation of GlobalParams to downstream conversion.
GlobalParamsis properly passed toShardInfoProtoUtils.toProto, which (per the relevant code snippet) uses it when generating error causes viaOpenSearchExceptionProtoUtils.generateThrowableProto. This ensures consistent error trace behavior for shard failure information.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/Netty4GrpcServerTransport.java (1)
190-200: Well-structured setting definition with clear documentation.The new
SETTING_GRPC_DETAILED_ERRORS_ENABLEDsetting follows the established pattern for gRPC transport settings. The Javadoc clearly explains the interaction between this server-side setting and the per-requesterror_traceparameter, which matches the PR's stated objectives for parity with HTTP behavior. Verify that this setting is consumed inGrpcParamsHandleror the relevant error handling implementation.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/GrpcPlugin.java (3)
189-189: LGTM - GrpcParamsHandler initialization properly wired.The initialization call correctly precedes transport and service creation, ensuring the handler is configured before any gRPC request processing occurs.
241-241: Consistent initialization in secure transport path.Matches the non-secure path initialization pattern, ensuring both transport types respect the
grpc.detailed_errors.enabledsetting.
291-292: Setting correctly exposed via getSettings().The new
SETTING_GRPC_DETAILED_ERRORS_ENABLEDis properly registered, allowing OpenSearch to recognize and validate the setting at node startup.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/listeners/BulkRequestActionListenerTests.java (1)
26-26: LGTM - Listener test properly updated for new constructor signature.The test correctly passes
GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSEto the listener constructor, aligning with the production code changes.Also applies to: 41-41
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ReplicationResponseShardInfoFailureProtoUtils.java (1)
36-48: LGTM - GlobalParams correctly propagated for error reason generation.The
paramsargument is properly passed togenerateThrowableProto(), ensuring stack trace inclusion respects theerror_tracesetting when generating the shard failure reason.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java (1)
36-44: LGTM - GlobalParams correctly propagated for search failure reason generation.The
paramsargument is properly passed togenerateThrowableProto(), ensuring consistent error_trace handling for shard search failures.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtilsTests.java (1)
24-24: LGTM - Consistent GlobalParams propagation in tests.The test updates correctly align with the new
toProto(SearchResponse, GlobalParams)API signature. All tests consistently useGLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE.Consider adding a test case with
GLOBAL_PARAMS_WITH_ERROR_TRACE_TRUEto verify behavior when error traces are enabled, particularly for scenarios involving shard failures where theGlobalParamswould affect error serialization.Also applies to: 45-48
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/DocumentServiceImpl.java (1)
46-56: LGTM - Proper GlobalParams validation and propagation.The implementation correctly:
- Validates stack trace configuration early (fail-fast pattern)
- Propagates
GlobalParamsto both the listener and error handler- Handles validation failures through the existing exception path
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
39-44: LGTM - Consistent GlobalParams propagation through exception handling.The
GlobalParamsparameter is correctly propagated to all specialized exception proto utilities through the switch expression.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListenerTests.java (1)
62-71: LGTM - Test correctly validates failure handling.The test properly verifies that
onFailuretriggersonErrorwith aStatusRuntimeException. The refactoring to use the class-levelresponseObserveris cleaner.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ProtoActionsProtoUtils.java (1)
43-54: LGTM - Clean GlobalParams propagation.The
GlobalParamsparameter is correctly added to the method signature and propagated toShardStatisticsProtoUtils.getShardStats, enabling error trace control for shard failure serialization.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtils.java (1)
40-44: LGTM - Proper GlobalParams threading through search response conversion.The
GlobalParamsis correctly propagated through the conversion chain toProtoActionsProtoUtils.buildBroadcastShardsHeader, enabling proper error trace control for shard failures in search responses.Also applies to: 88-97
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/services/SearchServiceImpl.java (1)
60-75: LGTM - Consistent implementation with proper error handling.The implementation correctly:
- Validates stack trace configuration early via
GrpcParamsHandler.validateStackTraceDetailsConfiguration- Propagates
GlobalParamsto the listener and error handler- Handles both
RuntimeExceptionandIOExceptionappropriatelyThis aligns well with
DocumentServiceImpland the broaderGlobalParamspropagation pattern.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/SnapshotShardFailureProtoUtils.java (1)
36-49: Null-safety improvements and GlobalParams propagation look good.The addition of null checks for
index()andnodeId()(lines 38-45) is a good defensive practice. The method signature correctly propagatesGlobalParamsto control error detail level, aligning with the broader PR changes.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/bulk/BulkItemResponseProtoUtils.java (1)
48-70: LGTM! GlobalParams propagation is correctly implemented.The
GlobalParamsparameter is properly passed to bothDocWriteResponseProtoUtils.toProtofor success cases andOpenSearchExceptionProtoUtils.generateThrowableProtofor failure cases, ensuring consistent error detail control throughout the response conversion pipeline.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/OpenSearchExceptionProtoUtilsTests.java (3)
58-70: Good test coverage for summary mode.This test correctly verifies that when
error_trace=false, the resultingErrorCausedoes not include stack trace, suppressed exceptions, or cause information. This validates the core behavior of the new GlobalParams-driven detail control.
95-116: Thorough nested exception testing for summary mode.This test verifies that the summary mode correctly propagates through nested exceptions—both the outer exception and its cause should lack stack traces when
error_trace=false, while still preserving the cause chain structure (type, reason). Good coverage of the recursive behavior.
205-217: Good symmetry withtestInnerToProtoWithBasicException.This new test provides the counterpart to the existing basic exception test, verifying that
generateThrowableProtocorrectly omits stack traces whenerror_trace=false.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/services/document/DocumentServiceImplTests.java (4)
44-54: Good test setup with proper cleanup, though redundant.The
@Aftermethod ensures test isolation by resettingGrpcParamsHandlerstate, which is important since it uses staticAtomicBoolean. However, both@Beforeand@Afterset the same value (true). Consider if the teardown is strictly necessary given it duplicates the setup.
81-92: Good test for server-side error trace rejection.This test correctly validates that when
grpc.detailed_errors.enabled=falseand a request haserrorTrace=true, the service rejects the request with aStatusRuntimeException. This aligns with the PR objective of failing fast when traces are requested but disabled.
94-107: Good test for allowed requests when server setting is disabled.This test verifies that requests with
errorTrace=falseproceed normally even when the server setting is disabled. The test correctly rebuilds the request with modifiedGlobalParamsand verifies thatclient.bulkis invoked.
117-121: Default request now includeserrorTrace(true).The helper method now sets
errorTrace=trueby default, which ensures existing tests exercise the stack-trace-enabled path. This is a sensible default for test coverage.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java (1)
46-67: LGTM! Consistent GlobalParams propagation.The
GlobalParamsparameter is correctly added to the method signature and propagated toShardOperationFailedExceptionProtoUtils.toProtofor shard failure conversion. This maintains consistency with the broader PR pattern of threadingGlobalParamsthrough all proto conversion utilities.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/SearchRequestActionListener.java (1)
45-64: LGTM!The
GlobalParamsis correctly propagated through both the success path (toProto) and error paths (convertToGrpcError), ensuring consistent error trace behavior across all response scenarios.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/document/common/ShardInfoProtoUtils.java (1)
38-50: LGTM!The
GlobalParamspropagation through the shard failure conversion chain ensures that error trace settings are respected when generating the failure reason proto.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcParamsHandlerTests.java (1)
18-54: Good test coverage for the critical validation paths.The tests cover the key scenarios: validation failure when server disables traces but client requests them, and successful validation when client doesn't request traces. The
isDetailedStackTraceRequestedtest verifies the basic parameter extraction.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcParamsHandler.java (2)
30-46: LGTM! Thread-safe initialization with proper static state management.The use of
AtomicBooleanensures thread-safe access to thedetailedErrorsEnabledflag. The default value oftrueprovides a safe starting state, and theinitializemethod correctly reads from the gRPC-specific setting.
76-80: Validation logic correctly implements fail-fast behavior.The check
detailedErrorsEnabled.get() == false && globalRequestParams.getErrorTrace()precisely enforces the requirement: reject requests that ask for error traces when the server has disabled them, aligning with the HTTP-sidehttp.detailed_errors.enabledbehavior.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/services/SearchServiceImplTests.java (1)
132-138: Good default for test helper.Setting
ErrorTrace(true)as the default increateTestSearchRequestensures existing tests exercise the primary use case, while tests needing different behavior can override viatoBuilder()as shown intestErrorTraceConfigValidationPassesWhenServerSettingIsDisabledAndRequestSkipsStackTrace.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/listeners/BulkRequestActionListener.java (2)
30-42: LGTM! Clean threading of GlobalParams through the listener.The
GlobalParamsfield is properly stored and the constructor cleanly accepts and assigns the parameter. The Javadoc accurately describes the purpose.
51-75: Consistent error handling with GlobalParams.Both the success path (with potential conversion failure) and the failure path properly pass
paramstoGrpcErrorHandler.convertToGrpcError, ensuring consistent stack trace behavior based on request parameters.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java (3)
21-22: Appropriate use of test fixture for consistent test behavior.Using
GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSEensures tests verify the non-stack-trace path consistently.
25-126: Test coverage appropriately updated for new API.All exception type test cases (DefaultShardOperationFailedException, AddBlockShardResult.Failure, IndicesShardStoresResponse.Failure, CloseIndexResponse.ShardResult.Failure, and null nodeId scenario) correctly pass
GLOBAL_PARAMS_WITH_ERROR_TRACE_FALSEto the updatedtoProtomethod.
128-135: MissingGlobalParamsparameter in null test case.This test calls
toProto(null)with a single argument, but the updated API signature istoProto(DefaultShardOperationFailedException, GlobalParams). Either this test should be updated to passGLOBAL_PARAMS_WITH_ERROR_TRACE_FALSE, or there's a separate overload for null handling that needs verification.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (4)
20-21: Appropriate imports for GlobalParams support.The imports for
GlobalParamsandGrpcParamsHandlerare correctly added to support the new param-driven error detail control.
66-81: Correct propagation ofGlobalParamsthrough exception unwrapping.Both the unwrapped cause path (
generateThrowableProto(ex, params)) and the direct conversion path (innerToProto(..., params)) properly receive theGlobalParamsparameter.
96-104: Proper delegation withGlobalParamsin both exception branches.The method correctly passes
paramsto bothtoProto(for OpenSearchException) andinnerToProto(for generic Throwable), maintaining consistent error detail behavior.
121-190: Well-implemented param-driven stack trace control.The
innerToProtomethod correctly:
- Propagates
paramsto recursivegenerateThrowableProtocalls for cause (line 166) and suppressed exceptions (line 185)- Uses
GrpcParamsHandler.isDetailedStackTraceRequested(params)to conditionally include stack traces (lines 177-179)This ensures consistent error detail behavior throughout the exception hierarchy.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java (5)
39-41: Clear constant for stack trace inclusion.The
INCLUDE_STACK_TRACESconstant correctly setsREST_EXCEPTION_SKIP_STACK_TRACEto"false", which means stack traces will be included when this parameter is used. The naming aligns with the intent.
62-136: Consistent error detail handling across all exception types.All exception branches properly use
getErrorDetailsForConfig(exception, shouldIncludeDetailedStackTrace)to determine error detail level, with appropriate special handling forOpenSearchException(delegated tohandleOpenSearchException) and null (static message).
148-159: Proper threading of stack trace flag to XContent metadata enhancement.The
handleOpenSearchExceptionmethod correctly passesshouldIncludeDetailedStackTracetoenhanceDescriptionWithXContentMetadata, ensuring consistent error detail behavior for OpenSearchExceptions.
184-190: Correct separation of request-level and server-level error detail controls.The implementation properly distinguishes between:
shouldIncludeDetailedStackTrace(request parameter) → controlsINCLUDE_STACK_TRACESvsEMPTY_PARAMSfor XContent serializationGrpcParamsHandler.isDetailedErrorsEnabled()(server setting) → passed togenerateFailureXContentThis aligns with the PR objective of providing both per-request and server-side control over error details.
215-223: Clean helper for conditional error detail selection.The
getErrorDetailsForConfigmethod clearly encapsulates the decision between full stack trace and simple message based on the request parameter. This avoids code duplication across all exception branches.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java (4)
16-17: Appropriate import for GlobalParams.The
GlobalParamsimport is correctly added to support the param-driven error detail control.
40-60: Consistent GlobalParams propagation through all exception type branches.The
toProtomethod correctly passesparamsto all specializedinnerToProtooverloads and toparentInnerToProtofor the default case. The null case appropriately returns an empty builder without needing params.
71-118: AllinnerToProtooverloads correctly thread GlobalParams.Each specialized
innerToProtomethod forAddBlockShardResult.Failure,IndicesShardStoresResponse.Failure, andCloseIndexResponse.ShardResult.Failureproperly acceptsGlobalParamsand passes it toparentInnerToProto. Javadocs are appropriately updated.
129-142: Critical integration point: GlobalParams flows to reason generation.The
parentInnerToProtomethod correctly passesparamstoOpenSearchExceptionProtoUtils.generateThrowableProtowhen building the failure reason. This ensures the error detail level (stack traces) is controlled by the request parameters throughout the entire exception conversion chain.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java (4)
40-48: LGTM: Setup and teardown ensure consistent test state.Both methods initialize with
trueto ensure each test starts with a known state and resets after completion, preventing state leakage between tests.
96-118: Missing test case: server-side disabled witherror_trace=trueshould fail fast.Per the PR objectives, when
grpc.detailed_errors.enabledis false and a request includeserror_trace=true, the server should "fail fast with an error indicating traces are disabled." This test only covers the case where both server-side config and request parameter are false.Consider adding a test for the fail-fast behavior:
public void testOpenSearchExceptionConversionWhenDetailedErrorsAreDisabledButClientRequestsThem() { GrpcParamsHandler.initialize(settingsWithGivenStackTraceConfig(false)); OpenSearchException exception = new OpenSearchException("Test exception") { @Override public RestStatus status() { return RestStatus.BAD_REQUEST; } }; StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(exception, GLOBAL_PARAMS_WITH_ERROR_TRACE_TRUE); // Should fail fast with INVALID_ARGUMENT indicating traces are disabled assertEquals(Status.INVALID_ARGUMENT.getCode(), result.getStatus().getCode()); assertTrue(result.getMessage().contains("error traces in responses are disabled")); }
120-331: LGTM: Comprehensive test coverage for error_trace behavior across exception types.The paired tests (with and without error trace) for each exception type provide good coverage of the new two-argument API. The assertions verify both the gRPC status code mapping and the expected message content based on the error_trace parameter.
333-447: LGTM: Edge cases are well covered.Good coverage of edge cases including null messages, metadata extraction, XContent failures, and root cause analysis. The graceful fallback behavior when XContent extraction fails (line 422:
assertFalse(result.getMessage().contains("details="))) is particularly important for robustness.
| - Add handling for the global gRPC error_trace parameter ([#19644](https://github.com/opensearch-project/OpenSearch/pull/19644)) | ||
| - Handle deleted documents for filter rewrite subaggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643)) | ||
| - Handle deleted documents for filter rewrite sub-aggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643)) |
There was a problem hiding this comment.
Incomplete changelog entry and duplicate entry detected.
Line 18 describes the PR but only mentions the error_trace parameter, omitting the grpc.detailed_errors.enabled setting that was introduced according to the PR objectives. Additionally, lines 19 and 20 are duplicate entries—both reference PR #19643 with nearly identical descriptions but inconsistent terminology ("subaggregation" vs. "sub-aggregation").
Apply this diff to expand the gRPC entry and remove the duplicate:
- Add handling for the global gRPC error_trace parameter ([#19644](https://github.com/opensearch-project/OpenSearch/pull/19644))
- Handle deleted documents for filter rewrite subaggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643))
- Handle deleted documents for filter rewrite sub-aggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643))
+ Add handling for the global gRPC error_trace parameter and grpc.detailed_errors.enabled setting ([#19644](https://github.com/opensearch-project/OpenSearch/pull/19644))
+ Handle deleted documents for filter rewrite sub-aggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Add handling for the global gRPC error_trace parameter ([#19644](https://github.com/opensearch-project/OpenSearch/pull/19644)) | |
| - Handle deleted documents for filter rewrite subaggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643)) | |
| - Handle deleted documents for filter rewrite sub-aggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643)) | |
| - Add handling for the global gRPC error_trace parameter and grpc.detailed_errors.enabled setting ([#19644](https://github.com/opensearch-project/OpenSearch/pull/19644)) | |
| - Handle deleted documents for filter rewrite sub-aggregation optimization ([#19643](https://github.com/opensearch-project/OpenSearch/pull/19643)) |
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 18 to 20, the gRPC changelog entry is incomplete
and there is a duplicate entry for PR #19643; update the gRPC line to mention
both the global gRPC error_trace parameter and the new
grpc.detailed_errors.enabled setting (briefly describe their effect), remove one
of the duplicate lines for PR #19643, and normalize the remaining PR #19643
entry to use consistent wording (e.g., "sub-aggregation") so the three bullets
read: the expanded gRPC item and a single, consistently-worded filter rewrite
sub-aggregation optimization line.
| shardFailure.setNode(exception.nodeId()); | ||
| } | ||
| shardFailure.setStatus(exception.status().name()); | ||
| shardFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause(), params)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find SnapshotShardFailure class and getCause() method
find . -type f -name "SnapshotShardFailure.java" | head -5Repository: opensearch-project/OpenSearch
Length of output: 202
🏁 Script executed:
# Search for getCause() in SnapshotShardFailure or related classes
rg -n "class SnapshotShardFailure" --type java -A 20Repository: opensearch-project/OpenSearch
Length of output: 202
🏁 Script executed:
# Search for generateThrowableProto implementation
rg -n "def generateThrowableProto|public.*generateThrowableProto" --type java -B 2 -A 10Repository: opensearch-project/OpenSearch
Length of output: 202
🏁 Script executed:
# Search for ExceptionsHelper.unwrapCause implementation
rg -n "unwrapCause" --type java -B 2 -A 10Repository: opensearch-project/OpenSearch
Length of output: 202
🌐 Web query:
OpenSearch ExceptionsHelper unwrapCause null handling
💡 Result:
Short answer: ExceptionsHelper.unwrapCause(t) returns the deepest (unwrapped) throwable it can find — and if there is no deeper cause it returns the original throwable. In other words, passing a null or a throwable with no cause yields the input (null stays null; a throwable with no cause is returned as-is). This is the behavior used in Elasticsearch / OpenSearch code that calls ExceptionsHelper.unwrapCause to find the “real” cause before mapping to status/handling. [1][2]
Sources
- Elasticsearch/OpenSearch usage and description of unwrapCause/why it’s used. [1]
- Analysis/implementation notes showing the unwrapping loop and behavior when no deeper cause is found. [2]
🌐 Web query:
OpenSearch OpenSearchExceptionProtoUtils generateThrowableProto null safety
💡 Result:
Short answer: guard against null before calling generateThrowableProto (or change the util to accept/handle null). OpenSearch utilities don’t always null-check throwables—so either check t != null and supply a default, or modify generateThrowableProto to return an empty/placeholder proto when passed null.
Suggested options (Java):
- Call-site null check
- Before calling:
if (t == null) { /* build placeholder proto or skip */ } else { OpenSearchExceptionProtoUtils.generateThrowableProto(builder, t); }
- Small defensive wrapper
- Wrap the utility so nulls are handled centrally:
public static void safeGenerateThrowableProto(Builder b, @nullable Throwable t) throws IOException {
if (t == null) {
b.setType("null").setMessage("no throwable");
return;
}
OpenSearchExceptionProtoUtils.generateThrowableProto(b, t);
}
- Patch the util (if you maintain the code)
- Edit generateThrowableProto to accept @nullable and early-return or populate a safe placeholder when t == null.
Rationale: OpenSearch exception XContent/proto helpers (see EngineException generateThrowableXContent for analogous behavior) are not guaranteed to accept null Throwables; many community reports show NPEs if exception data is missing, so defensive checks are required. [1][2]
References:
- EngineException.generateThrowableXContent (OpenSearch source reference for exception serialization helpers). [1]
- Community reports of NullPointerExceptions related to exception/aggregation serialization in OpenSearch logs/forums. [2]
If you want, tell me which OpenSearch version and the exact stack trace or code and I’ll give a concrete patch or wrapper snippet.
🌐 Web query:
SnapshotShardFailure getCause OpenSearch repository
💡 Result:
Use the Snapshot Status API (or the Java client SnapshotShardFailure model). The snapshot status response contains a failures array of SnapshotShardFailure entries; each failure exposes the human-readable failure text as the "reason" (the SnapshotShardFailure model has a reason field). Call GET /_snapshot// or GET /_snapshot/_status and inspect failures[].reason (or use the Java client SnapshotShardFailure.reason getter). [1][2]
Sources:
- OpenSearch Java client snapshot types (SnapshotShardFailure in API docs). [1]
- OpenSearch docs / error-log explaining snapshot failures and where the specific reason appears in the response. [2]
Potential NPE if exception.getCause() returns null.
The generateThrowableProto method does not safely handle null throwables. While ExceptionsHelper.unwrapCause() itself handles null gracefully, the proto generation utility requires defensive null-checking at call sites.
Add a null check before passing the cause:
- shardFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause(), params));
+ if (exception.getCause() != null) {
+ shardFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause(), params));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| shardFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause(), params)); | |
| if (exception.getCause() != null) { | |
| shardFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause(), params)); | |
| } |
🤖 Prompt for AI Agents
In
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/SnapshotShardFailureProtoUtils.java
around line 47, the call
shardFailure.setReason(OpenSearchExceptionProtoUtils.generateThrowableProto(exception.getCause(),
params)); can NPE if exception.getCause() is null; add a defensive null check
and only call generateThrowableProto if cause != null, otherwise setReason(null)
or skip setting it, ensuring you use the unwrapped cause
(ExceptionsHelper.unwrapCause(exception)) if required and pass that to
generateThrowableProto when non-null.
| * | ||
| * @param response The SearchResponse to convert | ||
| * @param searchResponseProtoBuilder The builder to populate with the SearchResponse data | ||
| * @param params |
There was a problem hiding this comment.
Incomplete Javadoc for params parameter.
The @param params lacks a description. It should match the documentation style used elsewhere.
- * @param params
+ * @param params The global gRPC request parameters📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @param params | |
| * @param params The global gRPC request parameters |
🤖 Prompt for AI Agents
In
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseProtoUtils.java
around line 52, the Javadoc contains an incomplete @param params tag; update the
Javadoc to include a brief description of what params represents (e.g., explain
that it is the serialization/deserialization parameters or context used when
converting SearchResponse to/from proto), following the same phrasing and style
as other @param tags in this class (concise, present-tense description), and
ensure the tag is formatted and aligned consistently with surrounding Javadoc
entries.
|
❌ Gradle check result for 2907384: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
2 participants
Description
This PR introduces:
grpc.detailed_errors.enabled(much like the similar http flag.)globalParams#errorTracerequest parameterThe
grpc.detailed_errors.enabledhas the same effect as its http counterparthttp.detailed_errors.enabled:error_trace=trueand the setting turned offerror_trace=falseerror_trace=trueSome of the test evidences are listed in this comment.
Related Issues
Resolves #19639
Check List
[ ] Public documentation issue/PR created, if applicable.Please note, that I haven't yet created the public documentation issue/PR for this setting as first I wanted to give this PR a go, get more feedback and then create the corresponding issue/PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
grpc.detailed_errors.enabled) to control whether detailed error information is included in gRPC error responses.✏️ Tip: You can customize this high-level summary in your review settings.