From 39e6e59cf3bb78f53181c7ebfa8c84d3b2f0b684 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Wed, 30 Oct 2024 11:16:25 -0700 Subject: [PATCH 1/4] feat: Instrument HTTP Reads and Writes --- .../cloud/storage/spi/v1/HttpStorageRpc.java | 41 ++++++++--- .../storage/ITHttpOpenTelemetryTest.java | 71 +++++++++++++++++++ 2 files changed, 104 insertions(+), 8 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java index e244903d09..327220ea61 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java @@ -848,9 +848,10 @@ private Get createReadRequest(StorageObject from, Map options) throws @Override public long read( StorageObject from, Map options, long position, OutputStream outputStream) { + OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("read"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_READ); Scope scope = tracer.withSpan(span); - try { + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { Get req = createReadRequest(from, options); Boolean shouldReturnRawInputStream = Option.RETURN_RAW_INPUT_STREAM.getBoolean(options); if (shouldReturnRawInputStream != null) { @@ -867,6 +868,8 @@ public long read( req.executeMedia().download(outputStream); return mediaHttpDownloader.getNumBytesDownloaded(); } catch (IOException ex) { + otelSpan.recordException(ex); + otelSpan.setStatus(StatusCode.ERROR, ex.getClass().getSimpleName()); span.setStatus(Status.UNKNOWN.withDescription(ex.getMessage())); StorageException serviceException = translate(ex); if (serviceException.getCode() == SC_REQUESTED_RANGE_NOT_SATISFIABLE) { @@ -874,6 +877,7 @@ public long read( } throw serviceException; } finally { + otelSpan.end(); scope.close(); span.end(HttpStorageRpcSpans.END_SPAN_OPTIONS); } @@ -882,9 +886,10 @@ public long read( @Override public Tuple read( StorageObject from, Map options, long position, int bytes) { + OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("read"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_READ); Scope scope = tracer.withSpan(span); - try { + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()){ checkArgument(position >= 0, "Position should be non-negative, is " + position); Get req = createReadRequest(from, options); Boolean shouldReturnRawInputStream = Option.RETURN_RAW_INPUT_STREAM.getBoolean(options); @@ -902,6 +907,8 @@ public Tuple read( String etag = req.getLastResponseHeaders().getETag(); return Tuple.of(etag, output.toByteArray()); } catch (IOException ex) { + otelSpan.recordException(ex); + otelSpan.setStatus(StatusCode.ERROR, ex.getClass().getSimpleName()); span.setStatus(Status.UNKNOWN.withDescription(ex.getMessage())); StorageException serviceException = StorageException.translate(ex); if (serviceException.getCode() == SC_REQUESTED_RANGE_NOT_SATISFIABLE) { @@ -909,6 +916,7 @@ public Tuple read( } throw serviceException; } finally { + otelSpan.end(); scope.close(); span.end(HttpStorageRpcSpans.END_SPAN_OPTIONS); } @@ -1158,11 +1166,13 @@ public String open(String signedURL) { @Override public RewriteResponse openRewrite(RewriteRequest rewriteRequest) { + OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("openRewrite"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_OPEN_REWRITE); Scope scope = tracer.withSpan(span); - try { - return rewrite(rewriteRequest, null); + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { + return rewrite(rewriteRequest, null, openTelemetryTraceUtil.currentContext()); } finally { + otelSpan.end(); scope.close(); span.end(HttpStorageRpcSpans.END_SPAN_OPTIONS); } @@ -1170,18 +1180,21 @@ public RewriteResponse openRewrite(RewriteRequest rewriteRequest) { @Override public RewriteResponse continueRewrite(RewriteResponse previousResponse) { + OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("continueRewrite"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_CONTINUE_REWRITE); Scope scope = tracer.withSpan(span); - try { - return rewrite(previousResponse.rewriteRequest, previousResponse.rewriteToken); + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()){ + return rewrite(previousResponse.rewriteRequest, previousResponse.rewriteToken, openTelemetryTraceUtil.currentContext()); } finally { + otelSpan.end(); scope.close(); span.end(HttpStorageRpcSpans.END_SPAN_OPTIONS); } } - private RewriteResponse rewrite(RewriteRequest req, String token) { - try { + private RewriteResponse rewrite(RewriteRequest req, String token, OpenTelemetryTraceUtil.Context ctx) { + OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("rewrite", ctx); + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()){ String userProject = Option.USER_PROJECT.getString(req.sourceOptions); if (userProject == null) { userProject = Option.USER_PROJECT.getString(req.targetOptions); @@ -1232,8 +1245,12 @@ private RewriteResponse rewrite(RewriteRequest req, String token) { rewriteResponse.getRewriteToken(), rewriteResponse.getTotalBytesRewritten().longValue()); } catch (IOException ex) { + otelSpan.recordException(ex); + otelSpan.setStatus(StatusCode.ERROR, ex.getClass().getSimpleName()); tracer.getCurrentSpan().setStatus(Status.UNKNOWN.withDescription(ex.getMessage())); throw translate(ex); + } finally { + otelSpan.end(); } } @@ -1835,4 +1852,12 @@ private static StorageException buildStorageException(int statusCode, String sta error.setMessage(statusMessage); return translate(error); } + private static StorageException buildStorageException(int statusCode, String statusMessage, OpenTelemetryTraceUtil.Span otelSpan) { + GoogleJsonError error = new GoogleJsonError(); + error.setCode(statusCode); + error.setMessage(statusMessage); + StorageException ex = translate(error); + otelSpan.recordException(ex); + return ex; + } } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java index c4b43fd7ad..4afb2dd063 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java @@ -16,9 +16,14 @@ package com.google.cloud.storage; +import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.fail; import com.google.cloud.NoCredentials; +import com.google.cloud.storage.Storage.BlobSourceOption; +import com.google.cloud.storage.Storage.BlobTargetOption; +import com.google.cloud.storage.Storage.CopyRequest; import com.google.cloud.storage.it.runner.StorageITRunner; import com.google.cloud.storage.it.runner.annotations.Backend; import com.google.cloud.storage.it.runner.annotations.Inject; @@ -31,6 +36,12 @@ import io.opentelemetry.sdk.trace.data.SpanData; import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor; import io.opentelemetry.sdk.trace.export.SpanExporter; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; import java.util.List; import org.junit.Assert; import org.junit.Before; @@ -43,10 +54,14 @@ public class ITHttpOpenTelemetryTest { @Inject public TestBench testBench; private StorageOptions options; private SpanExporter exporter; + private BlobId blobId; private Storage storage; + private static final byte[] helloWorldTextBytes = "hello world".getBytes(); + private static final byte[] helloWorldGzipBytes = TestUtils.gzipBytes(helloWorldTextBytes); @Inject public Generator generator; @Inject public BucketInfo testBucket; + @Before public void setUp() { exporter = new TestExporter(); @@ -65,6 +80,8 @@ public void setUp() { .setOpenTelemetrySdk(openTelemetrySdk) .build(); storage = options.getService(); + String objectString = generator.randomObjectName(); + blobId = BlobId.of(testBucket.getName(), objectString); } @Test @@ -97,6 +114,60 @@ public void runCreateBlob() { } } + @Test + public void runRead() throws IOException { + BlobInfo blobInfo = + BlobInfo.newBuilder(blobId).setContentEncoding("gzip").setContentType("text/plain").build(); + storage.create(blobInfo, helloWorldGzipBytes); + Path helloWorldTxtGz = File.createTempFile(blobId.getName(), ".txt.gz").toPath(); + storage.downloadTo( + blobId, helloWorldTxtGz, Storage.BlobSourceOption.shouldReturnRawInputStream(true)); + TestExporter testExported = (TestExporter) exporter; + List spanData = testExported.getExportedSpans(); + for (SpanData span : spanData) { + Assert.assertEquals("Storage", getAttributeValue(span, "gcp.client.service")); + Assert.assertEquals("googleapis/java-storage", getAttributeValue(span, "gcp.client.repo")); + Assert.assertEquals( + "com.google.cloud.google-cloud-storage", getAttributeValue(span, "gcp.client.artifact")); + Assert.assertEquals("http", getAttributeValue(span, "rpc.system")); + } + Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("read"))); + } + @Test + public void runCopy() { + + byte[] expected = "Hello, World!".getBytes(StandardCharsets.UTF_8); + + BlobInfo info = + BlobInfo.newBuilder(testBucket.getName(), generator.randomObjectName() + "copy/src").build(); + Blob cpySrc = storage.create(info, expected, BlobTargetOption.doesNotExist()); + + BlobInfo dst = + BlobInfo.newBuilder(testBucket.getName(), generator.randomObjectName() + "copy/dst").build(); + + CopyRequest copyRequest = + CopyRequest.newBuilder() + .setSource(cpySrc.getBlobId()) + .setSourceOptions(BlobSourceOption.generationMatch(cpySrc.getGeneration())) + .setTarget(dst, BlobTargetOption.doesNotExist()) + .build(); + + CopyWriter copyWriter = storage.copy(copyRequest); + copyWriter.getResult(); + TestExporter testExported = (TestExporter) exporter; + List spanData = testExported.getExportedSpans(); + for (SpanData span : spanData) { + Assert.assertEquals("Storage", getAttributeValue(span, "gcp.client.service")); + Assert.assertEquals("googleapis/java-storage", getAttributeValue(span, "gcp.client.repo")); + Assert.assertEquals( + "com.google.cloud.google-cloud-storage", getAttributeValue(span, "gcp.client.artifact")); + Assert.assertEquals("http", getAttributeValue(span, "rpc.system")); + } + Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("openRewrite"))); + Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("rewrite"))); + + } + private String getAttributeValue(SpanData spanData, String key) { return spanData.getAttributes().get(AttributeKey.stringKey(key)).toString(); } From e4cb91a3b701ca15f96e4b60bbf1091456351f80 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Mon, 4 Nov 2024 15:51:53 -0800 Subject: [PATCH 2/4] Add tests --- .../com/google/cloud/storage/spi/v1/HttpStorageRpc.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java index 327220ea61..ded5b42c7d 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java @@ -1852,12 +1852,4 @@ private static StorageException buildStorageException(int statusCode, String sta error.setMessage(statusMessage); return translate(error); } - private static StorageException buildStorageException(int statusCode, String statusMessage, OpenTelemetryTraceUtil.Span otelSpan) { - GoogleJsonError error = new GoogleJsonError(); - error.setCode(statusCode); - error.setMessage(statusMessage); - StorageException ex = translate(error); - otelSpan.recordException(ex); - return ex; - } } From fc519623ee82656b39025852a7afd6eac290d530 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Tue, 5 Nov 2024 08:13:42 -0800 Subject: [PATCH 3/4] lint --- .../cloud/storage/spi/v1/HttpStorageRpc.java | 14 +++++++++----- .../cloud/storage/ITHttpOpenTelemetryTest.java | 13 +++++-------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java index ded5b42c7d..bb69d83000 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java @@ -889,7 +889,7 @@ public Tuple read( OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("read"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_READ); Scope scope = tracer.withSpan(span); - try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()){ + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { checkArgument(position >= 0, "Position should be non-negative, is " + position); Get req = createReadRequest(from, options); Boolean shouldReturnRawInputStream = Option.RETURN_RAW_INPUT_STREAM.getBoolean(options); @@ -1183,8 +1183,11 @@ public RewriteResponse continueRewrite(RewriteResponse previousResponse) { OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("continueRewrite"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_CONTINUE_REWRITE); Scope scope = tracer.withSpan(span); - try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()){ - return rewrite(previousResponse.rewriteRequest, previousResponse.rewriteToken, openTelemetryTraceUtil.currentContext()); + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { + return rewrite( + previousResponse.rewriteRequest, + previousResponse.rewriteToken, + openTelemetryTraceUtil.currentContext()); } finally { otelSpan.end(); scope.close(); @@ -1192,9 +1195,10 @@ public RewriteResponse continueRewrite(RewriteResponse previousResponse) { } } - private RewriteResponse rewrite(RewriteRequest req, String token, OpenTelemetryTraceUtil.Context ctx) { + private RewriteResponse rewrite( + RewriteRequest req, String token, OpenTelemetryTraceUtil.Context ctx) { OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("rewrite", ctx); - try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()){ + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { String userProject = Option.USER_PROJECT.getString(req.sourceOptions); if (userProject == null) { userProject = Option.USER_PROJECT.getString(req.targetOptions); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java index 4afb2dd063..a8547cea9e 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java @@ -16,9 +16,7 @@ package com.google.cloud.storage; -import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.junit.Assert.fail; import com.google.cloud.NoCredentials; import com.google.cloud.storage.Storage.BlobSourceOption; @@ -39,9 +37,7 @@ import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.nio.file.Files; import java.nio.file.Path; -import java.util.Arrays; import java.util.List; import org.junit.Assert; import org.junit.Before; @@ -61,7 +57,6 @@ public class ITHttpOpenTelemetryTest { @Inject public Generator generator; @Inject public BucketInfo testBucket; - @Before public void setUp() { exporter = new TestExporter(); @@ -133,17 +128,20 @@ public void runRead() throws IOException { } Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("read"))); } + @Test public void runCopy() { byte[] expected = "Hello, World!".getBytes(StandardCharsets.UTF_8); BlobInfo info = - BlobInfo.newBuilder(testBucket.getName(), generator.randomObjectName() + "copy/src").build(); + BlobInfo.newBuilder(testBucket.getName(), generator.randomObjectName() + "copy/src") + .build(); Blob cpySrc = storage.create(info, expected, BlobTargetOption.doesNotExist()); BlobInfo dst = - BlobInfo.newBuilder(testBucket.getName(), generator.randomObjectName() + "copy/dst").build(); + BlobInfo.newBuilder(testBucket.getName(), generator.randomObjectName() + "copy/dst") + .build(); CopyRequest copyRequest = CopyRequest.newBuilder() @@ -165,7 +163,6 @@ public void runCopy() { } Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("openRewrite"))); Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("rewrite"))); - } private String getAttributeValue(SpanData spanData, String key) { From 8f5f8d8001e8bb065717ca0255425e9dd0076b4a Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Thu, 7 Nov 2024 10:50:21 -0800 Subject: [PATCH 4/4] refactor test logic --- .../storage/ITHttpOpenTelemetryTest.java | 34 ++++++------------- 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java index a8547cea9e..afe7884445 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITHttpOpenTelemetryTest.java @@ -84,13 +84,8 @@ public void runCreateBucket() { String bucket = "random-bucket"; storage.create(BucketInfo.of(bucket)); TestExporter testExported = (TestExporter) exporter; - SpanData spanData = testExported.getExportedSpans().get(0); - Assert.assertEquals("Storage", getAttributeValue(spanData, "gcp.client.service")); - Assert.assertEquals("googleapis/java-storage", getAttributeValue(spanData, "gcp.client.repo")); - Assert.assertEquals( - "com.google.cloud.google-cloud-storage", - getAttributeValue(spanData, "gcp.client.artifact")); - Assert.assertEquals("http", getAttributeValue(spanData, "rpc.system")); + List spanData = testExported.getExportedSpans(); + checkCommonAttributes(spanData); } @Test @@ -100,13 +95,7 @@ public void runCreateBlob() { storage.create(BlobInfo.newBuilder(toCreate).build(), content); TestExporter testExported = (TestExporter) exporter; List spanData = testExported.getExportedSpans(); - for (SpanData span : spanData) { - Assert.assertEquals("Storage", getAttributeValue(span, "gcp.client.service")); - Assert.assertEquals("googleapis/java-storage", getAttributeValue(span, "gcp.client.repo")); - Assert.assertEquals( - "com.google.cloud.google-cloud-storage", getAttributeValue(span, "gcp.client.artifact")); - Assert.assertEquals("http", getAttributeValue(span, "rpc.system")); - } + checkCommonAttributes(spanData); } @Test @@ -119,13 +108,7 @@ public void runRead() throws IOException { blobId, helloWorldTxtGz, Storage.BlobSourceOption.shouldReturnRawInputStream(true)); TestExporter testExported = (TestExporter) exporter; List spanData = testExported.getExportedSpans(); - for (SpanData span : spanData) { - Assert.assertEquals("Storage", getAttributeValue(span, "gcp.client.service")); - Assert.assertEquals("googleapis/java-storage", getAttributeValue(span, "gcp.client.repo")); - Assert.assertEquals( - "com.google.cloud.google-cloud-storage", getAttributeValue(span, "gcp.client.artifact")); - Assert.assertEquals("http", getAttributeValue(span, "rpc.system")); - } + checkCommonAttributes(spanData); Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("read"))); } @@ -149,11 +132,16 @@ public void runCopy() { .setSourceOptions(BlobSourceOption.generationMatch(cpySrc.getGeneration())) .setTarget(dst, BlobTargetOption.doesNotExist()) .build(); - CopyWriter copyWriter = storage.copy(copyRequest); copyWriter.getResult(); TestExporter testExported = (TestExporter) exporter; List spanData = testExported.getExportedSpans(); + checkCommonAttributes(spanData); + Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("openRewrite"))); + Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("rewrite"))); + } + + private void checkCommonAttributes(List spanData) { for (SpanData span : spanData) { Assert.assertEquals("Storage", getAttributeValue(span, "gcp.client.service")); Assert.assertEquals("googleapis/java-storage", getAttributeValue(span, "gcp.client.repo")); @@ -161,8 +149,6 @@ public void runCopy() { "com.google.cloud.google-cloud-storage", getAttributeValue(span, "gcp.client.artifact")); Assert.assertEquals("http", getAttributeValue(span, "rpc.system")); } - Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("openRewrite"))); - Assert.assertTrue(spanData.stream().anyMatch(x -> x.getName().contains("rewrite"))); } private String getAttributeValue(SpanData spanData, String key) {