Repository navigation
feat(storage): add gcp.storage.client.ttfb metric for read operations #14745
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,14 @@ public final class StorageGrpcMetricsInterceptorTest { | |
| .setResponseMarshaller(new NoopMarshaller()) | ||
| .build(); | ||
|
|
||
| private static final MethodDescriptor<Void, Void> WRITE_OBJECT_METHOD = | ||
| MethodDescriptor.<Void, Void>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<Void, Void> testCall = new TestClientCall<>(io.grpc.Attributes.EMPTY); | ||
| TestChannel testChannel = new TestChannel(testCall, "storage.googleapis.com"); | ||
|
|
||
| ClientCall<Void, Void> 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> 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<Void, Void> testCall = new TestClientCall<>(io.grpc.Attributes.EMPTY); | ||
| TestChannel testChannel = new TestChannel(testCall, "storage.googleapis.com"); | ||
|
|
||
| ClientCall<Void, Void> 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> 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); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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> 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> 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); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relaxing this guard to allow the interceptor to run when
metrics.getRpcClientCallDuration()is null (butmetrics.getTtfb()is non-null) will cause aNullPointerExceptioninonClosebecauseonCloselacks a null check forrpcClientCallDuration.In
StorageHttpMetricsInterceptor.java, a null check was correctly added around the recording ofhttpClientRequestDuration(line 90). A similar null check must be added inonClosein this file.Please update
onCloseto check for null before recording: