[nidhiii-27]

Instruments read operations across gRPC and HTTP transports to record gcp.storage.client.ttfb (time to first byte) OpenTelemetry histogram metrics.

• Records time from RPC start to first response message on ReadObject and BidiReadObject in StorageGrpcMetricsInterceptor.
• Records time from request execution to response headers on media download requests in StorageHttpMetricsInterceptor.
• Initializes gcp.storage.client.ttfb in StorageClientMetrics and adds unit tests in StorageGrpcMetricsInterceptorTest, StorageHttpMetricsInterceptorTest, and StorageClientMetricsTest.

Stacked on #14744.

[gemini-code-assist]

Code Review

This pull request adds Time to First Byte (TTFB) metrics tracking for both gRPC and HTTP storage clients, along with corresponding unit tests. The feedback highlights a potential NullPointerException in StorageGrpcMetricsInterceptor due to a missing null check in onClose when rpcClientCallDuration is null, and suggests restricting HTTP TTFB recording to 2xx status codes to prevent misleading metrics on 3xx responses.

[gemini-code-assist]

Relaxing this guard to allow the interceptor to run when metrics.getRpcClientCallDuration() is null (but metrics.getTtfb() is non-null) will cause a NullPointerException in onClose because onClose lacks a null check for rpcClientCallDuration.

In StorageHttpMetricsInterceptor.java, a null check was correctly added around the recording of httpClientRequestDuration (line 90). A similar null check must be added in onClose in this file.

Please update onClose to check for null before recording:

@Override
public void onClose(Status status, Metadata trailers) {
  try {
    if (callRecorded.compareAndSet(false, true) && metrics.getRpcClientCallDuration() != null) {
      // ... recording logic ...
    }
  } finally {
    super.onClose(status, trailers);
  }
}

[gemini-code-assist]

The gcp.storage.client.ttfb metric measures the time to first byte of the actual media content. Recording TTFB for 3xx status codes (such as 304 Not Modified or redirects) is misleading because no media content is returned in those responses. Restricting this to successful 2xx status codes ensures the metric accurately reflects actual media download performance.

Suggested change

	if (metrics.getTtfb() != null && statusCode < 400 && isMediaDownloadRequest(request)) {
	if (metrics.getTtfb() != null && statusCode >= 200 && statusCode < 300 && isMediaDownloadRequest(request)) {

[gemini-code-assist]

Code Review

This pull request introduces Time to First Byte (TTFB) metrics tracking for both gRPC and HTTP Google Cloud Storage clients, along with comprehensive unit tests. The feedback highlights a potential NullPointerException in the gRPC interceptor if call duration metrics are disabled while TTFB is enabled, and suggests a refactoring to safely guard against this. Additionally, an optimization is suggested for the HTTP interceptor to avoid registering response interceptors when no metrics need to be recorded.

[gemini-code-assist]

If metrics.getRpcClientCallDuration() is null but metrics.getTtfb() is non-null, the interceptor will proceed. However, the onClose method (which is not shown in this diff but is part of the intercepted call) will attempt to record the call duration using metrics.getRpcClientCallDuration().record(...) without a null check, leading to a NullPointerException.

Additionally, if the method is not a read method (e.g., WriteObject) and metrics.getRpcClientCallDuration() is null, we still intercept the call even though we will not record any metrics for it.

We can solve both issues by extracting the method check earlier and defining recordDuration and recordTtfb flags. You should also wrap the duration recording in onClose with if (recordDuration).

    if (metrics == null) {
      return next.newCall(method, callOptions);
    }

    String shortMethod = extractShortMethodName(method.getFullMethodName());
    boolean isReadMethod = "ReadObject".equals(shortMethod) || "BidiReadObject".equals(shortMethod);

    boolean recordDuration = metrics.getRpcClientCallDuration() != null;
    boolean recordTtfb = isReadMethod && metrics.getTtfb() != null;

    if (!recordDuration && !recordTtfb) {
      return next.newCall(method, callOptions);
    }

[gemini-code-assist]

We can optimize this guard by checking if we actually need to record any metrics for this specific request before registering the response interceptor. If metrics.getHttpClientRequestDuration() is null and the request is not a media download request, we can return early and avoid the overhead of intercepting the response.

Using effectively final recordDuration and recordTtfb flags also allows us to simplify the checks inside the response interceptor.

Suggested change

	if (metrics == null
	|| (metrics.getHttpClientRequestDuration() == null && metrics.getTtfb() == null)) {
	return;
	}
	if (metrics == null) {
	return;
	}
	boolean recordDuration = metrics.getHttpClientRequestDuration() != null;
	boolean recordTtfb = metrics.getTtfb() != null && isMediaDownloadRequest(request);
	if (!recordDuration && !recordTtfb) {
	return;
	}