Repository navigation
feat(storage): add rpc.client.call.duration metric for gRPC calls - #14743
nidhiii-27 wants to merge 10 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces StorageGrpcMetricsInterceptor, a new gRPC client interceptor designed to record call metrics such as RPC duration, and integrates it into GrpcStorageOptions. It also adds comprehensive unit tests to verify its behavior. The review feedback is highly constructive, focusing on preventing a potential NullPointerException when combining interceptors, simplifying redundant server address resolution, and optimizing performance on the hot path by precomputing the DirectPath endpoint status in the constructor rather than performing string operations on every RPC completion.
There was a problem hiding this comment.
Code Review
This pull request introduces StorageGrpcMetricsInterceptor to record gRPC client call metrics (such as duration, status codes, and network paths) and integrates it into GrpcStorageOptions. It also adds comprehensive unit tests to verify the interceptor's behavior. The review feedback highlights two key improvement opportunities: adding a null check for grpcInterceptorProvider to prevent a potential NullPointerException when combining providers, and simplifying the redundant logic used to resolve the serverAddress.
…Interceptor - Precompute defaultServerAddress and isDirectPathEndpoint in StorageGrpcMetricsInterceptor constructor - Guard grpcInterceptorProvider and its getInterceptors() against null in GrpcStorageOptions - Add defensive null checks and cause-aware error mapping in StorageGrpcMetricsInterceptor.onClose - Use JSpecify @nullable and expand unit test coverage in StorageGrpcMetricsInterceptorTest [Generated-by: AI]
…ls.extractServerAddress Merge updated feat/storage-grpc-veneer-operation-metrics base branch and update StorageGrpcMetricsInterceptor to call OpenTelemetryBootstrappingUtils.extractServerAddress. [Generated-by: AI]
[Generated-by: AI]
[Generated-by: AI]
[Generated-by: AI]
…rage-rpc-call-duration
…Interceptor - Use full gRPC method name (`method.getFullMethodName()`) for `rpc.method` and remove `extractShortMethodName` - Capture fallback `Context.current()` in `interceptCall` on the caller thread - Remove `isDirectPathEndpoint` and return `NETWORK_PATH_UNKNOWN` when `remoteAddr` is absent - Restore `/18` and `/42` CIDR byte-mask check in `isDirectPathIp` [Generated-by: AI]
… mapping Update recordAttemptError_specificExceptions_mappedToCanonicalTypes to assert STATUS_DEADLINE_EXCEEDED (1) and STATUS_TIMEOUT (1) following the StorageErrorTypeMapper update. [Generated-by: AI]
cpriti-os
left a comment
cpriti-os
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier comments. Mostly looks good, one real issue with the IPv6 mask plus a few cleanups inline.
| && b[2] == 0x48 | ||
| && b[3] == 0x60 | ||
| && b[4] == (byte) 0x80 | ||
| && (b[5] & 0xFC) == 0x40; |
There was a problem hiding this comment.
The /42 only covers the top 2 bits of byte 5, so this should be (b[5] & 0xC0) == 0x40; right now 2001:4860:8044::1 is inside the range but gets classified as cloudpath and the test asserts that. Can we flip that expectation and use 2001:4860:807f::1 (in) / 2001:4860:8080::1 (out) as the boundary?
| @Override | ||
| public void onClose(@Nullable Status status, @Nullable Metadata trailers) { | ||
| try { | ||
| if (callRecorded.compareAndSet(false, true) |
There was a problem hiding this comment.
Is this re-check needed? The histogram is a final field already checked in interceptCall (and again inside recordRpcCallDuration), and the metrics == null / empty method name guards look unreachable too.
| } | ||
| } | ||
|
|
||
| static void recordRpcCallDuration( |
There was a problem hiding this comment.
This doesn't use any context state, so should it live on StorageClientMetrics? That would let it start from baseAttributes (which already has rpc.system.name and server.address) and drop the defaultEndpoint/authority fallback plumbing in the interceptor.
| && metrics.getRpcClientCallDuration() != null) { | ||
| double durationSeconds = | ||
| Math.max(0.0, (clock.nanoTime() - startTimeNanos) / 1_000_000_000.0); | ||
| String networkPath = detectNetworkPath(getAttributes()); |
There was a problem hiding this comment.
Since the interceptor now knows the peer path, will a later PR feed it back into StorageMetricsContext so attempts/errors stop defaulting to cloudpath? Just want to confirm that's planned.
| StorageOptions.grpc() | ||
| .setProjectId("test-project") | ||
| .setCredentials(NoCredentials.getInstance()) | ||
| .setEnableOtelMetrics(true) |
There was a problem hiding this comment.
Can we also cover the metrics-disabled case here, asserting the user's provider is passed through untouched and no StorageGrpcMetricsInterceptor gets added?
… feat/storage-rpc-call-duration [Generated-by: AI]
Instruments gRPC storage calls to record
rpc.client.call.durationOpenTelemetry histogram metrics.StorageGrpcMetricsInterceptorto measure per-RPC duration and attachrpc.system.name,rpc.method,rpc.response.status_code,network.path,server.address, anderror.typeattributes.StorageGrpcMetricsInterceptorintoGrpcStorageOptionsand initializesrpc.client.call.durationinStorageClientMetrics.StorageGrpcMetricsInterceptorTestandStorageClientMetricsTest.Stacked on #14729.