Repository navigation
feat(storage): add rpc.client.call.duration metric for gRPC calls - #14743
nidhiii-27 wants to merge 13 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.
… feat/storage-rpc-call-duration [Generated-by: AI]
…torage-rpc-call-duration
[Generated-by: AI]
…torage-rpc-call-duration [Generated-by: AI]
| if (callRecorded.compareAndSet(false, true)) { | ||
| 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.
For ReadObject the retrier returns as soon as the first response arrives (GapicUnbufferedReadableByteChannel completes open in onResponseImpl), so the attempt is recorded before this onClose write and the first read attempt still falls back to cloudpath. Can we also set the path from onHeaders (peer attrs are available by then) so streaming attempts pick up the real one?
| if (metricsCtx != null) { | ||
| metricsCtx.setNetworkPath(networkPath); | ||
| } | ||
| String statusCode = |
There was a problem hiding this comment.
status and its code can't be null here per the Listener contract, so this can just be status.getCode().name() and the @Nullables can go.
| } | ||
|
|
||
| @Test | ||
| public void interceptCall_withStorageMetricsContext_propagatesContextAndFeedsNetworkPath() |
There was a problem hiding this comment.
Can we also assert the duration point here has network.path=directpath? Right now only the attempts side is checked.
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.