Skip to content

feat(storage): add rpc.client.call.duration metric for gRPC calls - #14743

Draft
nidhiii-27 wants to merge 3 commits into
feat/storage-grpc-veneer-operation-metricsfrom
feat/storage-rpc-call-duration
Draft

nidhiii-27 wants to merge 3 commits into
feat/storage-grpc-veneer-operation-metricsfrom
feat/storage-rpc-call-duration

Conversation

@nidhiii-27

Copy link
Copy Markdown
Contributor

Instruments gRPC storage calls to record rpc.client.call.duration OpenTelemetry histogram metrics.

  • Adds StorageGrpcMetricsInterceptor to measure per-RPC duration and attach rpc.system.name, rpc.method, rpc.response.status_code, network.path, server.address, and error.type attributes.
  • Wires StorageGrpcMetricsInterceptor into GrpcStorageOptions and initializes rpc.client.call.duration in StorageClientMetrics.
  • Adds unit tests in StorageGrpcMetricsInterceptorTest and StorageClientMetricsTest.

Stacked on #14729.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant