feat: include RequestID in requests and errors - #4263
Conversation
Summary of ChangesHello @olavloite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the observability and debugging experience for the Spanner client library by integrating a comprehensive Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to automate the handling of RequestID for Spanner requests. The changes move the responsibility of creating, propagating, and incrementing request IDs from the application logic into a new gRPC interceptor layer. This simplifies the higher-level code, reduces boilerplate, and makes the request ID handling more robust and centralized.
The key changes include:
- Introducing
RequestIdCreatorImplfor generating request IDs. - Adding
RequestIdInterceptorto inject the request ID into gRPC headers and increment attempt numbers on retries. - Modifying
SpannerErrorInterceptorto propagate the request ID in error trailers, allowing exceptions to automatically capture it. - Removing manual request ID management from numerous classes, including
SpannerExceptionsubclasses,AbstractReadContext,TransactionRunnerImpl, andSessionImpl. - Adding a comprehensive new test class,
RequestIdMockServerTest, which thoroughly validates the new mechanism.
The code quality is high, and the changes are consistent and logical. This is an excellent improvement to the client library's architecture.
|
|
||
| if (operationName == null) { | ||
| GrpcCallContext context = newCallContext(null, instanceName, initialRequest, method); | ||
| GrpcCallContext context = newAdminCallContext(instanceName, initialRequest, method); |
There was a problem hiding this comment.
This isn't directly related, but a small cleanup to prevent each of these methods to invoke newCallContext(null, ...), and instead use a more descriptive method.
| @Override | ||
| public long executePartitionedUpdate(Statement stmt, UpdateOption... options) { | ||
| return createMultiplexedSessionTransaction(/* singleUse= */ true) | ||
| return createMultiplexedSessionTransaction(/* singleUse= */ false) |
There was a problem hiding this comment.
PartitionedDML is not a single-use transaction. It invokes the BeginTransaction RPC and the ExecuteStreamingSql RPC. These two calls should use the same channel.
| defaultInterceptorList.add( | ||
| new LoggingInterceptor(Logger.getLogger(GapicSpannerRpc.class.getName()), Level.FINER)); | ||
| defaultInterceptorList.add(new HeaderInterceptor(new SpannerRpcMetrics(openTelemetry))); | ||
| defaultInterceptorList.add(new RequestIdInterceptor()); |
There was a problem hiding this comment.
This adds a generic interceptor that adds the RequestID header from a CallOption.
| return; | ||
| } | ||
| try { | ||
| if (headers.containsKey(XGoogSpannerRequestId.REQUEST_ID_HEADER_KEY)) { |
There was a problem hiding this comment.
This adds the original RequestID in the error if Spanner returns a non-OK status code for an RPC. This ensures that all errors that originate from Spanner contain a RequestID, and those that do not originate from Spanner do not contain a RequestID.
This change removes most of the manual handling of RequestIDs, and instead lets the
SpannerRpcimplementation and a couple of interceptors handle RequestIDs.