From b55633f97c161edab4c732eb926d1e6f80308210 Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Thu, 22 Oct 2026 07:48:31 +0000 Subject: [PATCH 1/2] feat: add gcp.storage.client.ttfb metric for read operations --- .../StorageGrpcMetricsInterceptor.java | 27 +++- .../StorageHttpMetricsInterceptor.java | 83 +++++++++---- .../StorageGrpcMetricsInterceptorTest.java | 87 +++++++++++++ .../StorageHttpMetricsInterceptorTest.java | 115 ++++++++++++++++++ 4 files changed, 285 insertions(+), 27 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java index a09d6c37b06e..5b8f9cca783e 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java @@ -53,11 +53,13 @@ final class StorageGrpcMetricsInterceptor implements ClientInterceptor { @Override public ClientCall interceptCall( MethodDescriptor method, CallOptions callOptions, Channel next) { - if (metrics == null || metrics.getRpcClientCallDuration() == null) { + if (metrics == null + || (metrics.getRpcClientCallDuration() == null && metrics.getTtfb() == null)) { return next.newCall(method, callOptions); } String shortMethod = extractShortMethodName(method.getFullMethodName()); + boolean isReadMethod = "ReadObject".equals(shortMethod) || "BidiReadObject".equals(shortMethod); String initialServerAddress = StorageOptions.extractServerAddress( defaultEndpoint != null ? defaultEndpoint : next.authority()); @@ -74,9 +76,32 @@ public ClientCall interceptCall( public void start(Listener responseListener, Metadata headers) { long startTimeNanos = clock.nanoTime(); AtomicBoolean callRecorded = new AtomicBoolean(false); + AtomicBoolean ttfbRecorded = new AtomicBoolean(false); super.start( new SimpleForwardingClientCallListener(responseListener) { + @Override + public void onMessage(RespT message) { + if (isReadMethod + && metrics.getTtfb() != null + && ttfbRecorded.compareAndSet(false, true)) { + double ttfbSeconds = + Math.max(0.0, (clock.nanoTime() - startTimeNanos) / 1_000_000_000.0); + String networkPath = detectNetworkPath(getAttributes(), defaultEndpoint); + AttributesBuilder ttfbAttrs = + metrics + .clientAttributesBuilder() + .put(StorageClientMetrics.KEY_RPC_SYSTEM_NAME, "grpc") + .put(StorageClientMetrics.KEY_RPC_METHOD, shortMethod) + .put(StorageClientMetrics.KEY_NETWORK_PATH, networkPath) + .put( + StorageClientMetrics.KEY_SERVER_ADDRESS, + serverAddress != null ? serverAddress : ""); + metrics.getTtfb().record(ttfbSeconds, ttfbAttrs.build(), otelContext); + } + super.onMessage(message); + } + @Override public void onClose(Status status, Metadata trailers) { try { diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java index 9c50c5d7448c..e838ed47edb8 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java @@ -55,7 +55,8 @@ public void initialize(HttpRequest request) throws java.io.IOException { if (delegate != null) { delegate.initialize(request); } - if (metrics == null || metrics.getHttpClientRequestDuration() == null) { + if (metrics == null + || (metrics.getHttpClientRequestDuration() == null && metrics.getTtfb() == null)) { return; } @@ -80,37 +81,54 @@ public void initialize(HttpRequest request) throws java.io.IOException { if (existingResp != null) { existingResp.interceptResponse(response); } - if (recorded.compareAndSet(false, true) - && metrics.getHttpClientRequestDuration() != null) { + if (recorded.compareAndSet(false, true)) { double durationSeconds = Math.max(0.0, (clock.nanoTime() - startTimeNanos.get()) / 1_000_000_000.0); int statusCode = response.getStatusCode(); - String method = - request.getRequestMethod() != null ? request.getRequestMethod() : "UNKNOWN"; - String urlTemplate = extractUrlTemplate(request.getUrl()); String serverAddress = resolveServerAddress(request.getUrl(), defaultServerAddress); - AttributesBuilder attrBuilder = - metrics - .clientAttributesBuilder() - .put(StorageClientMetrics.KEY_HTTP_REQUEST_METHOD, method) - .put(StorageClientMetrics.KEY_HTTP_RESPONSE_STATUS_CODE, (long) statusCode) - .put( - StorageClientMetrics.KEY_NETWORK_PATH, - StorageClientMetrics.NETWORK_PATH_CLOUDPATH) - .put(StorageClientMetrics.KEY_URL_TEMPLATE, urlTemplate) - .put( - StorageClientMetrics.KEY_SERVER_ADDRESS, - serverAddress != null ? serverAddress : "") - .put( - StorageClientMetrics.KEY_ERROR_TYPE, - statusCode >= 400 - ? StorageErrorTypeMapper.mapHttpResponseCode(statusCode) - : StorageErrorTypeMapper.STATUS_OK); + if (metrics.getHttpClientRequestDuration() != null) { + String method = + request.getRequestMethod() != null ? request.getRequestMethod() : "UNKNOWN"; + String urlTemplate = extractUrlTemplate(request.getUrl()); - metrics - .getHttpClientRequestDuration() - .record(durationSeconds, attrBuilder.build(), Context.current()); + AttributesBuilder attrBuilder = + metrics + .clientAttributesBuilder() + .put(StorageClientMetrics.KEY_HTTP_REQUEST_METHOD, method) + .put(StorageClientMetrics.KEY_HTTP_RESPONSE_STATUS_CODE, (long) statusCode) + .put( + StorageClientMetrics.KEY_NETWORK_PATH, + StorageClientMetrics.NETWORK_PATH_CLOUDPATH) + .put(StorageClientMetrics.KEY_URL_TEMPLATE, urlTemplate) + .put( + StorageClientMetrics.KEY_SERVER_ADDRESS, + serverAddress != null ? serverAddress : "") + .put( + StorageClientMetrics.KEY_ERROR_TYPE, + statusCode >= 400 + ? StorageErrorTypeMapper.mapHttpResponseCode(statusCode) + : StorageErrorTypeMapper.STATUS_OK); + + metrics + .getHttpClientRequestDuration() + .record(durationSeconds, attrBuilder.build(), Context.current()); + } + + if (metrics.getTtfb() != null && statusCode < 400 && isMediaDownloadRequest(request)) { + AttributesBuilder ttfbAttrs = + metrics + .clientAttributesBuilder() + .put(StorageClientMetrics.KEY_RPC_SYSTEM_NAME, "http") + .put(StorageClientMetrics.KEY_RPC_METHOD, "ReadObject") + .put( + StorageClientMetrics.KEY_NETWORK_PATH, + StorageClientMetrics.NETWORK_PATH_CLOUDPATH) + .put( + StorageClientMetrics.KEY_SERVER_ADDRESS, + serverAddress != null ? serverAddress : ""); + metrics.getTtfb().record(durationSeconds, ttfbAttrs.build(), Context.current()); + } } }); @@ -223,4 +241,17 @@ static String extractUrlTemplate(@Nullable GenericUrl url) { String[] parts = trimmed.split("/", 2); return parts.length == 1 ? "/{bucket}" : "/{bucket}/{object}"; } + + static boolean isMediaDownloadRequest(@Nullable HttpRequest request) { + if (request == null) { + return false; + } + if (!"GET".equalsIgnoreCase(request.getRequestMethod()) || request.getUrl() == null) { + return false; + } + GenericUrl url = request.getUrl(); + String path = url.getRawPath(); + return (path != null && path.startsWith("/download/storage/v1/")) + || "media".equals(url.getFirst("alt")); + } } diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java index 6bc17c0df9da..e96ee13539d8 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java @@ -50,6 +50,14 @@ public final class StorageGrpcMetricsInterceptorTest { .setResponseMarshaller(new NoopMarshaller()) .build(); + private static final MethodDescriptor WRITE_OBJECT_METHOD = + MethodDescriptor.newBuilder() + .setType(MethodDescriptor.MethodType.CLIENT_STREAMING) + .setFullMethodName("google.storage.v2.Storage/WriteObject") + .setRequestMarshaller(new NoopMarshaller()) + .setResponseMarshaller(new NoopMarshaller()) + .build(); + private InMemoryMetricReader reader; private SdkMeterProvider meterProvider; private FakeClock clock; @@ -215,6 +223,79 @@ public void interceptCall_callFails_recordsErrorType() throws Exception { .isEqualTo("storage.googleapis.com"); } + @Test + public void interceptCall_readObject_recordsTtfbOnFirstMessage() throws Exception { + StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); + StorageGrpcMetricsInterceptor interceptor = + new StorageGrpcMetricsInterceptor(metrics, clock, "storage.googleapis.com"); + + TestClientCall testCall = new TestClientCall<>(io.grpc.Attributes.EMPTY); + TestChannel testChannel = new TestChannel(testCall, "storage.googleapis.com"); + + ClientCall call = + interceptor.interceptCall(READ_OBJECT_METHOD, CallOptions.DEFAULT, testChannel); + call.start(new TestListener<>(), new Metadata()); + + clock.advanceNanos(200_000_000L); // 0.2s + testCall.deliverMessage(null); // First response message: triggers TTFB + + clock.advanceNanos(300_000_000L); // 0.3s more (total 0.5s) + testCall.deliverMessage(null); // Second message: should NOT record TTFB again + testCall.close(Status.OK, new Metadata()); + + Collection metricData = reader.collectAllMetrics(); + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + + assertThat(ttfbData).isNotNull(); + assertThat(ttfbData.getHistogramData().getPoints()).hasSize(1); + HistogramPointData ttfbPoint = ttfbData.getHistogramData().getPoints().iterator().next(); + assertThat(ttfbPoint.getSum()).isWithin(1e-6).of(0.2); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_RPC_SYSTEM_NAME)) + .isEqualTo("grpc"); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_RPC_METHOD)) + .isEqualTo("ReadObject"); + + MetricData durationData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_RPC_CLIENT_CALL_DURATION)) + .findFirst() + .orElse(null); + assertThat(durationData).isNotNull(); + HistogramPointData durationPoint = + durationData.getHistogramData().getPoints().iterator().next(); + assertThat(durationPoint.getSum()).isWithin(1e-6).of(0.5); + } + + @Test + public void interceptCall_nonReadMethod_doesNotRecordTtfb() throws Exception { + StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); + StorageGrpcMetricsInterceptor interceptor = + new StorageGrpcMetricsInterceptor(metrics, clock, "storage.googleapis.com"); + + TestClientCall testCall = new TestClientCall<>(io.grpc.Attributes.EMPTY); + TestChannel testChannel = new TestChannel(testCall, "storage.googleapis.com"); + + ClientCall call = + interceptor.interceptCall(WRITE_OBJECT_METHOD, CallOptions.DEFAULT, testChannel); + call.start(new TestListener<>(), new Metadata()); + + clock.advanceNanos(200_000_000L); + testCall.deliverMessage(null); + testCall.close(Status.OK, new Metadata()); + + Collection metricData = reader.collectAllMetrics(); + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + assertThat(ttfbData).isNull(); + } + @Test public void interceptCall_disabledMetrics_doesNotRecord() { StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, false, false); @@ -262,6 +343,12 @@ public io.grpc.Attributes getAttributes() { return attributes; } + void deliverMessage(RespT message) { + if (listener != null) { + listener.onMessage(message); + } + } + void close(Status status, Metadata trailers) { if (listener != null) { listener.onClose(status, trailers); diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java index 0ce3d3376225..9adf49f1cba2 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java @@ -334,6 +334,121 @@ public LowLevelHttpResponse execute() throws IOException { .isEqualTo("storage.googleapis.com"); } + @Test + public void isMediaDownloadRequest_cases() throws IOException { + HttpTransport transport = new MockHttpTransport(); + HttpRequest getDownload = + transport + .createRequestFactory() + .buildGetRequest( + new GenericUrl( + "https://storage.googleapis.com/download/storage/v1/b/my-b/o/my-obj")); + assertThat(StorageHttpMetricsInterceptor.isMediaDownloadRequest(getDownload)).isTrue(); + + HttpRequest getAltMedia = + transport + .createRequestFactory() + .buildGetRequest( + new GenericUrl( + "https://storage.googleapis.com/storage/v1/b/my-b/o/my-obj?alt=media")); + assertThat(StorageHttpMetricsInterceptor.isMediaDownloadRequest(getAltMedia)).isTrue(); + + HttpRequest getMetadata = + transport + .createRequestFactory() + .buildGetRequest( + new GenericUrl("https://storage.googleapis.com/storage/v1/b/my-b/o/my-obj")); + assertThat(StorageHttpMetricsInterceptor.isMediaDownloadRequest(getMetadata)).isFalse(); + + HttpRequest postReq = + transport + .createRequestFactory() + .buildPostRequest( + new GenericUrl( + "https://storage.googleapis.com/download/storage/v1/b/my-b/o/my-obj"), + null); + assertThat(StorageHttpMetricsInterceptor.isMediaDownloadRequest(postReq)).isFalse(); + + assertThat(StorageHttpMetricsInterceptor.isMediaDownloadRequest(null)).isFalse(); + } + + @Test + public void initialize_mediaDownload_recordsTtfb() throws IOException { + StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); + StorageHttpMetricsInterceptor interceptor = + new StorageHttpMetricsInterceptor(null, metrics, clock, "https://storage.googleapis.com"); + + MockLowLevelHttpResponse lowLevelResponse = new MockLowLevelHttpResponse(); + lowLevelResponse.setStatusCode(200); + HttpTransport transport = + new MockHttpTransport() { + @Override + public LowLevelHttpRequest buildRequest(String method, String url) { + return new MockLowLevelHttpRequest() { + @Override + public LowLevelHttpResponse execute() throws IOException { + clock.advanceNanos(250_000_000L); // 0.25s network duration + return lowLevelResponse; + } + }; + } + }; + + HttpRequest request = + transport + .createRequestFactory(interceptor) + .buildGetRequest( + new GenericUrl( + "https://storage.googleapis.com/download/storage/v1/b/my-b/o/my-obj")); + request.execute(); + + Collection metricData = reader.collectAllMetrics(); + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + + assertThat(ttfbData).isNotNull(); + assertThat(ttfbData.getHistogramData().getPoints()).hasSize(1); + HistogramPointData ttfbPoint = ttfbData.getHistogramData().getPoints().iterator().next(); + assertThat(ttfbPoint.getSum()).isWithin(1e-6).of(0.25); + Attributes attrs = ttfbPoint.getAttributes(); + assertThat(attrs.get(StorageClientMetrics.KEY_RPC_SYSTEM_NAME)).isEqualTo("http"); + assertThat(attrs.get(StorageClientMetrics.KEY_RPC_METHOD)).isEqualTo("ReadObject"); + assertThat(attrs.get(StorageClientMetrics.KEY_NETWORK_PATH)) + .isEqualTo(StorageClientMetrics.NETWORK_PATH_CLOUDPATH); + assertThat(attrs.get(StorageClientMetrics.KEY_SERVER_ADDRESS)) + .isEqualTo("storage.googleapis.com"); + } + + @Test + public void initialize_nonDownloadGet_doesNotRecordTtfb() throws IOException { + StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); + StorageHttpMetricsInterceptor interceptor = + new StorageHttpMetricsInterceptor(null, metrics, clock, "https://storage.googleapis.com"); + + MockLowLevelHttpResponse lowLevelResponse = new MockLowLevelHttpResponse(); + lowLevelResponse.setStatusCode(200); + HttpTransport transport = + new MockHttpTransport.Builder().setLowLevelHttpResponse(lowLevelResponse).build(); + + HttpRequest request = + transport + .createRequestFactory(interceptor) + .buildGetRequest( + new GenericUrl("https://storage.googleapis.com/storage/v1/b/my-b/o/my-obj")); + request.execute(); + + Collection metricData = reader.collectAllMetrics(); + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + assertThat(ttfbData).isNull(); + } + @Test public void initialize_metricsDisabled_doesNotRecord() throws IOException { StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, false, false); From 4c2d80b8abcb1b1bd39a45c77178dcd956326dce Mon Sep 17 00:00:00 2001 From: Nidhi Nandwani Date: Tue, 3 Nov 2026 10:55:16 +0000 Subject: [PATCH 2/2] fix(storage): refine ttfb metric attributes, status guards, and interceptor checks Use KEY_GCP_STORAGE_OPERATION (ReadObject) for gcp.storage.client.ttfb across gRPC and HTTP interceptors, restrict HTTP TTFB recording to 2xx status codes, hoist recordDuration/recordTtfb checks, and expand unit test coverage. [Generated-by: AI] --- .../StorageGrpcMetricsInterceptor.java | 24 ++++-- .../StorageHttpMetricsInterceptor.java | 83 +++++++++++-------- .../StorageGrpcMetricsInterceptorTest.java | 65 ++++++++++++++- .../StorageHttpMetricsInterceptorTest.java | 79 +++++++++++++++++- 4 files changed, 205 insertions(+), 46 deletions(-) diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java index 5b8f9cca783e..aa100a31fa82 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageGrpcMetricsInterceptor.java @@ -53,13 +53,18 @@ final class StorageGrpcMetricsInterceptor implements ClientInterceptor { @Override public ClientCall interceptCall( MethodDescriptor method, CallOptions callOptions, Channel next) { - if (metrics == null - || (metrics.getRpcClientCallDuration() == null && metrics.getTtfb() == null)) { + 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); + } + String initialServerAddress = StorageOptions.extractServerAddress( defaultEndpoint != null ? defaultEndpoint : next.authority()); @@ -70,6 +75,10 @@ public ClientCall interceptCall( StorageMetricsContext metricsCtx = StorageMetricsContext.from(callOptions); Context otelContext = metricsCtx != null ? metricsCtx.getOtelContext() : Context.current(); + String operationName = + (metricsCtx != null && metricsCtx.getOperationName() != null) + ? metricsCtx.getOperationName() + : "ReadObject"; return new SimpleForwardingClientCall(next.newCall(method, callOptions)) { @Override @@ -82,9 +91,9 @@ public void start(Listener responseListener, Metadata headers) { new SimpleForwardingClientCallListener(responseListener) { @Override public void onMessage(RespT message) { - if (isReadMethod - && metrics.getTtfb() != null - && ttfbRecorded.compareAndSet(false, true)) { + // The first response message from the server (including metadata-only + // responses for 0-byte objects) marks Time To First Byte (TTFB). + if (recordTtfb && ttfbRecorded.compareAndSet(false, true)) { double ttfbSeconds = Math.max(0.0, (clock.nanoTime() - startTimeNanos) / 1_000_000_000.0); String networkPath = detectNetworkPath(getAttributes(), defaultEndpoint); @@ -92,7 +101,7 @@ public void onMessage(RespT message) { metrics .clientAttributesBuilder() .put(StorageClientMetrics.KEY_RPC_SYSTEM_NAME, "grpc") - .put(StorageClientMetrics.KEY_RPC_METHOD, shortMethod) + .put(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION, operationName) .put(StorageClientMetrics.KEY_NETWORK_PATH, networkPath) .put( StorageClientMetrics.KEY_SERVER_ADDRESS, @@ -105,8 +114,7 @@ public void onMessage(RespT message) { @Override public void onClose(Status status, Metadata trailers) { try { - if (callRecorded.compareAndSet(false, true) - && metrics.getRpcClientCallDuration() != null) { + if (recordDuration && callRecorded.compareAndSet(false, true)) { double durationSeconds = Math.max(0.0, (clock.nanoTime() - startTimeNanos) / 1_000_000_000.0); String networkPath = detectNetworkPath(getAttributes(), defaultEndpoint); diff --git a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java index e838ed47edb8..ed216a17f3a7 100644 --- a/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java +++ b/java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageHttpMetricsInterceptor.java @@ -55,8 +55,12 @@ public void initialize(HttpRequest request) throws java.io.IOException { if (delegate != null) { delegate.initialize(request); } - if (metrics == null - || (metrics.getHttpClientRequestDuration() == null && metrics.getTtfb() == null)) { + if (metrics == null) { + return; + } + boolean recordDuration = metrics.getHttpClientRequestDuration() != null; + boolean recordTtfb = metrics.getTtfb() != null; + if (!recordDuration && !recordTtfb) { return; } @@ -87,7 +91,7 @@ public void initialize(HttpRequest request) throws java.io.IOException { int statusCode = response.getStatusCode(); String serverAddress = resolveServerAddress(request.getUrl(), defaultServerAddress); - if (metrics.getHttpClientRequestDuration() != null) { + if (recordDuration) { String method = request.getRequestMethod() != null ? request.getRequestMethod() : "UNKNOWN"; String urlTemplate = extractUrlTemplate(request.getUrl()); @@ -115,12 +119,18 @@ public void initialize(HttpRequest request) throws java.io.IOException { .record(durationSeconds, attrBuilder.build(), Context.current()); } - if (metrics.getTtfb() != null && statusCode < 400 && isMediaDownloadRequest(request)) { + // Evaluate isMediaDownloadRequest(request) at response time rather than in + // initialize(HttpRequest), because callers such as MediaHttpDownloader may append + // query parameters (e.g. alt=media) to request.getUrl() after buildGetRequest(). + if (recordTtfb + && statusCode >= 200 + && statusCode < 300 + && isMediaDownloadRequest(request)) { AttributesBuilder ttfbAttrs = metrics .clientAttributesBuilder() .put(StorageClientMetrics.KEY_RPC_SYSTEM_NAME, "http") - .put(StorageClientMetrics.KEY_RPC_METHOD, "ReadObject") + .put(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION, "ReadObject") .put( StorageClientMetrics.KEY_NETWORK_PATH, StorageClientMetrics.NETWORK_PATH_CLOUDPATH) @@ -132,39 +142,40 @@ public void initialize(HttpRequest request) throws java.io.IOException { } }); - request.setIOExceptionHandler( - (req, supportsRetry) -> { - if (recorded.compareAndSet(false, true) - && metrics.getHttpClientRequestDuration() != null) { - double durationSeconds = - Math.max(0.0, (clock.nanoTime() - startTimeNanos.get()) / 1_000_000_000.0); - String method = req.getRequestMethod() != null ? req.getRequestMethod() : "UNKNOWN"; - String urlTemplate = extractUrlTemplate(req.getUrl()); - String serverAddress = resolveServerAddress(req.getUrl(), defaultServerAddress); + if (recordDuration) { + request.setIOExceptionHandler( + (req, supportsRetry) -> { + if (recorded.compareAndSet(false, true)) { + double durationSeconds = + Math.max(0.0, (clock.nanoTime() - startTimeNanos.get()) / 1_000_000_000.0); + String method = req.getRequestMethod() != null ? req.getRequestMethod() : "UNKNOWN"; + String urlTemplate = extractUrlTemplate(req.getUrl()); + String serverAddress = resolveServerAddress(req.getUrl(), defaultServerAddress); - AttributesBuilder attrBuilder = - metrics - .clientAttributesBuilder() - .put(StorageClientMetrics.KEY_HTTP_REQUEST_METHOD, method) - .put(StorageClientMetrics.KEY_HTTP_RESPONSE_STATUS_CODE, 0L) - .put( - StorageClientMetrics.KEY_NETWORK_PATH, - StorageClientMetrics.NETWORK_PATH_CLOUDPATH) - .put(StorageClientMetrics.KEY_URL_TEMPLATE, urlTemplate) - .put( - StorageClientMetrics.KEY_SERVER_ADDRESS, - serverAddress != null ? serverAddress : "") - .put( - StorageClientMetrics.KEY_ERROR_TYPE, - StorageErrorTypeMapper.STATUS_CONNECTIVITY); + AttributesBuilder attrBuilder = + metrics + .clientAttributesBuilder() + .put(StorageClientMetrics.KEY_HTTP_REQUEST_METHOD, method) + .put(StorageClientMetrics.KEY_HTTP_RESPONSE_STATUS_CODE, 0L) + .put( + StorageClientMetrics.KEY_NETWORK_PATH, + StorageClientMetrics.NETWORK_PATH_CLOUDPATH) + .put(StorageClientMetrics.KEY_URL_TEMPLATE, urlTemplate) + .put( + StorageClientMetrics.KEY_SERVER_ADDRESS, + serverAddress != null ? serverAddress : "") + .put( + StorageClientMetrics.KEY_ERROR_TYPE, + StorageErrorTypeMapper.STATUS_CONNECTIVITY); - metrics - .getHttpClientRequestDuration() - .record(durationSeconds, attrBuilder.build(), Context.current()); - } - return existingIoHandler != null - && existingIoHandler.handleIOException(req, supportsRetry); - }); + metrics + .getHttpClientRequestDuration() + .record(durationSeconds, attrBuilder.build(), Context.current()); + } + return existingIoHandler != null + && existingIoHandler.handleIOException(req, supportsRetry); + }); + } } static String resolveServerAddress( diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java index e96ee13539d8..78d4fd1bb501 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageGrpcMetricsInterceptorTest.java @@ -50,6 +50,14 @@ public final class StorageGrpcMetricsInterceptorTest { .setResponseMarshaller(new NoopMarshaller()) .build(); + private static final MethodDescriptor BIDI_READ_OBJECT_METHOD = + MethodDescriptor.newBuilder() + .setType(MethodDescriptor.MethodType.BIDI_STREAMING) + .setFullMethodName("google.storage.v2.Storage/BidiReadObject") + .setRequestMarshaller(new NoopMarshaller()) + .setResponseMarshaller(new NoopMarshaller()) + .build(); + private static final MethodDescriptor WRITE_OBJECT_METHOD = MethodDescriptor.newBuilder() .setType(MethodDescriptor.MethodType.CLIENT_STREAMING) @@ -221,6 +229,13 @@ public void interceptCall_callFails_recordsErrorType() throws Exception { .isEqualTo("NOT_FOUND"); assertThat(point.getAttributes().get(StorageClientMetrics.KEY_SERVER_ADDRESS)) .isEqualTo("storage.googleapis.com"); + + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + assertThat(ttfbData).isNull(); } @Test @@ -253,11 +268,16 @@ public void interceptCall_readObject_recordsTtfbOnFirstMessage() throws Exceptio assertThat(ttfbData).isNotNull(); assertThat(ttfbData.getHistogramData().getPoints()).hasSize(1); HistogramPointData ttfbPoint = ttfbData.getHistogramData().getPoints().iterator().next(); + assertThat(ttfbPoint.getCount()).isEqualTo(1L); assertThat(ttfbPoint.getSum()).isWithin(1e-6).of(0.2); assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_RPC_SYSTEM_NAME)) .isEqualTo("grpc"); - assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_RPC_METHOD)) + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION)) .isEqualTo("ReadObject"); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_NETWORK_PATH)) + .isEqualTo(StorageClientMetrics.NETWORK_PATH_UNKNOWN); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_SERVER_ADDRESS)) + .isEqualTo("storage.googleapis.com"); MetricData durationData = metricData.stream() @@ -270,6 +290,49 @@ public void interceptCall_readObject_recordsTtfbOnFirstMessage() throws Exceptio assertThat(durationPoint.getSum()).isWithin(1e-6).of(0.5); } + @Test + public void interceptCall_bidiReadObject_recordsTtfbOnFirstMessageWithUnifiedOperation() + throws Exception { + StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); + StorageGrpcMetricsInterceptor interceptor = + new StorageGrpcMetricsInterceptor(metrics, clock, "google-c2p:///storage.googleapis.com"); + + TestClientCall testCall = new TestClientCall<>(io.grpc.Attributes.EMPTY); + TestChannel testChannel = new TestChannel(testCall, "storage.googleapis.com"); + + ClientCall call = + interceptor.interceptCall(BIDI_READ_OBJECT_METHOD, CallOptions.DEFAULT, testChannel); + call.start(new TestListener<>(), new Metadata()); + + clock.advanceNanos(150_000_000L); // 0.15s + testCall.deliverMessage(null); + + clock.advanceNanos(250_000_000L); // 0.25s more (total 0.4s) + testCall.deliverMessage(null); + testCall.close(Status.OK, new Metadata()); + + Collection metricData = reader.collectAllMetrics(); + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + + assertThat(ttfbData).isNotNull(); + assertThat(ttfbData.getHistogramData().getPoints()).hasSize(1); + HistogramPointData ttfbPoint = ttfbData.getHistogramData().getPoints().iterator().next(); + assertThat(ttfbPoint.getCount()).isEqualTo(1L); + assertThat(ttfbPoint.getSum()).isWithin(1e-6).of(0.15); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_RPC_SYSTEM_NAME)) + .isEqualTo("grpc"); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION)) + .isEqualTo("ReadObject"); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_NETWORK_PATH)) + .isEqualTo(StorageClientMetrics.NETWORK_PATH_DIRECTPATH); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_SERVER_ADDRESS)) + .isEqualTo("storage.googleapis.com"); + } + @Test public void interceptCall_nonReadMethod_doesNotRecordTtfb() throws Exception { StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); diff --git a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java index 9adf49f1cba2..4e991e593473 100644 --- a/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java +++ b/java-storage/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageHttpMetricsInterceptorTest.java @@ -412,16 +412,93 @@ public LowLevelHttpResponse execute() throws IOException { assertThat(ttfbData).isNotNull(); assertThat(ttfbData.getHistogramData().getPoints()).hasSize(1); HistogramPointData ttfbPoint = ttfbData.getHistogramData().getPoints().iterator().next(); + assertThat(ttfbPoint.getCount()).isEqualTo(1L); assertThat(ttfbPoint.getSum()).isWithin(1e-6).of(0.25); Attributes attrs = ttfbPoint.getAttributes(); assertThat(attrs.get(StorageClientMetrics.KEY_RPC_SYSTEM_NAME)).isEqualTo("http"); - assertThat(attrs.get(StorageClientMetrics.KEY_RPC_METHOD)).isEqualTo("ReadObject"); + assertThat(attrs.get(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION)).isEqualTo("ReadObject"); assertThat(attrs.get(StorageClientMetrics.KEY_NETWORK_PATH)) .isEqualTo(StorageClientMetrics.NETWORK_PATH_CLOUDPATH); assertThat(attrs.get(StorageClientMetrics.KEY_SERVER_ADDRESS)) .isEqualTo("storage.googleapis.com"); } + @Test + public void initialize_mediaDownloadNon2xxResponse_doesNotRecordTtfb() throws IOException { + StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); + StorageHttpMetricsInterceptor interceptor = + new StorageHttpMetricsInterceptor(null, metrics, clock, "https://storage.googleapis.com"); + + for (int statusCode : new int[] {304, 404}) { + MockLowLevelHttpResponse lowLevelResponse = new MockLowLevelHttpResponse(); + lowLevelResponse.setStatusCode(statusCode); + HttpTransport transport = + new MockHttpTransport.Builder().setLowLevelHttpResponse(lowLevelResponse).build(); + + HttpRequest request = + transport + .createRequestFactory(interceptor) + .buildGetRequest( + new GenericUrl( + "https://storage.googleapis.com/download/storage/v1/b/my-b/o/my-obj?alt=media")); + request.setThrowExceptionOnExecuteError(false); + request.execute(); + } + + Collection metricData = reader.collectAllMetrics(); + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + assertThat(ttfbData).isNull(); + } + + @Test + public void initialize_altMediaAddedAfterBuildGetRequest_recordsTtfb() throws IOException { + StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false); + StorageHttpMetricsInterceptor interceptor = + new StorageHttpMetricsInterceptor(null, metrics, clock, "https://storage.googleapis.com"); + + MockLowLevelHttpResponse lowLevelResponse = new MockLowLevelHttpResponse(); + lowLevelResponse.setStatusCode(200); + HttpTransport transport = + new MockHttpTransport() { + @Override + public LowLevelHttpRequest buildRequest(String method, String url) { + return new MockLowLevelHttpRequest() { + @Override + public LowLevelHttpResponse execute() { + clock.advanceNanos(120_000_000L); + return lowLevelResponse; + } + }; + } + }; + + HttpRequest request = + transport + .createRequestFactory(interceptor) + .buildGetRequest( + new GenericUrl("https://storage.googleapis.com/storage/v1/b/my-b/o/my-obj")); + request.getUrl().put("alt", "media"); + request.execute(); + + Collection metricData = reader.collectAllMetrics(); + MetricData ttfbData = + metricData.stream() + .filter(m -> m.getName().equals(StorageClientMetrics.METRIC_GCP_STORAGE_CLIENT_TTFB)) + .findFirst() + .orElse(null); + + assertThat(ttfbData).isNotNull(); + HistogramPointData ttfbPoint = ttfbData.getHistogramData().getPoints().iterator().next(); + assertThat(ttfbPoint.getCount()).isEqualTo(1L); + assertThat(ttfbPoint.getSum()).isWithin(1e-6).of(0.12); + assertThat(ttfbPoint.getAttributes().get(StorageClientMetrics.KEY_GCP_STORAGE_OPERATION)) + .isEqualTo("ReadObject"); + } + @Test public void initialize_nonDownloadGet_doesNotRecordTtfb() throws IOException { StorageClientMetrics metrics = StorageClientMetrics.create(meterProvider, true, false);