From 92989f520dec8f48c0e8f1a966c0ef94fa28c10e Mon Sep 17 00:00:00 2001 From: Sydney Munro <97561403+sydney-munro@users.noreply.github.com> Date: Wed, 18 Dec 2024 15:10:13 -0800 Subject: [PATCH] pr comments --- .../cloud/storage/GrpcStorageOptions.java | 4 +- .../cloud/storage/HttpStorageOptions.java | 8 +- .../storage/ITGrpcOpenTelemetryTest.java | 1 + .../storage/ITHttpOpenTelemetryTest.java | 1 + .../cloud/storage/ITOpenTelemetryTest.java | 107 ++++++++---------- .../storage/it/runner/registry/TestBench.java | 1 + .../cloud/storage/otel/TestExporter.java | 2 +- 7 files changed, 55 insertions(+), 69 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java index bda0ac9cc..0e2a75f92 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java @@ -120,7 +120,7 @@ public final class GrpcStorageOptions extends StorageOptions private final boolean grpcClientMetricsManuallyEnabled; private final GrpcInterceptorProvider grpcInterceptorProvider; private final BlobWriteSessionConfig blobWriteSessionConfig; - private OpenTelemetrySdk openTelemetrySdk; + private final OpenTelemetrySdk openTelemetrySdk; private GrpcStorageOptions(Builder builder, GrpcStorageDefaults serviceDefaults) { super(builder, serviceDefaults); @@ -634,7 +634,9 @@ public GrpcStorageOptions.Builder setBlobWriteSessionConfig( * Enable OpenTelemetry Tracing and provide an instance for the client to use. * * @param openTelemetrySdk User defined instance of OpenTelemetry SDK to be used by the library + * @since 2.46.1 This new api is in preview and is subject to breaking changes. */ + @BetaApi public GrpcStorageOptions.Builder setOpenTelemetrySdk(OpenTelemetrySdk openTelemetrySdk) { requireNonNull(openTelemetrySdk, "openTelemetry must be non null"); this.openTelemetrySdk = openTelemetrySdk; diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpStorageOptions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpStorageOptions.java index bb391a061..77d9aee31 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpStorageOptions.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/HttpStorageOptions.java @@ -60,7 +60,7 @@ public class HttpStorageOptions extends StorageOptions { private transient RetryDependenciesAdapter retryDepsAdapter; private final BlobWriteSessionConfig blobWriteSessionConfig; - private OpenTelemetrySdk openTelemetrySdk; + private final OpenTelemetrySdk openTelemetrySdk; private HttpStorageOptions(Builder builder, StorageDefaults serviceDefaults) { super(builder, serviceDefaults); @@ -139,10 +139,6 @@ RetryingDependencies asRetryDependencies() { return retryDepsAdapter; } - public void setOpenTelemetrySdk(OpenTelemetrySdk openTelemetrySdk) { - this.openTelemetrySdk = openTelemetrySdk; - } - public static class Builder extends StorageOptions.Builder { private StorageRetryStrategy storageRetryStrategy; @@ -288,7 +284,9 @@ public HttpStorageOptions build() { * Enable OpenTelemetry Tracing and provide an instance for the client to use. * * @param openTelemetrySdk + * @since 2.34.0 This new api is in preview and is subject to breaking changes. */ + @BetaApi public HttpStorageOptions.Builder setOpenTelemetrySdk(OpenTelemetrySdk openTelemetrySdk) { requireNonNull(openTelemetrySdk, "openTelemetry must be non null"); this.openTelemetrySdk = openTelemetrySdk; diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcOpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcOpenTelemetryTest.java index 96ac92b7d..aedbd6549 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcOpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGrpcOpenTelemetryTest.java @@ -30,6 +30,7 @@ import com.google.cloud.storage.it.runner.annotations.SingleBackend; import com.google.cloud.storage.it.runner.registry.Generator; import com.google.cloud.storage.it.runner.registry.TestBench; +import com.google.cloud.storage.otel.TestExporter; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.sdk.trace.SdkTracerProvider; 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 def465d21..36f6e0c76 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 @@ -30,6 +30,7 @@ import com.google.cloud.storage.it.runner.annotations.SingleBackend; import com.google.cloud.storage.it.runner.registry.Generator; import com.google.cloud.storage.it.runner.registry.TestBench; +import com.google.cloud.storage.otel.TestExporter; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.sdk.trace.SdkTracerProvider; diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java index 363f1fa08..99bc1386c 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java @@ -16,18 +16,14 @@ package com.google.cloud.storage; +import com.google.cloud.storage.otel.TestExporter; import com.google.cloud.storage.testing.RemoteStorageHelper; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.sdk.OpenTelemetrySdk; -import io.opentelemetry.sdk.common.CompletableResultCode; import io.opentelemetry.sdk.trace.SdkTracerProvider; import io.opentelemetry.sdk.trace.data.SpanData; import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor; import io.opentelemetry.sdk.trace.export.SpanExporter; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.List; import java.util.UUID; import org.junit.Assert; import org.junit.Test; @@ -49,18 +45,21 @@ public void checkInstrumentation() { StorageOptions.http().setOpenTelemetrySdk(openTelemetrySdk).build(); Storage storage = storageOptions.getService(); String bucket = randomBucketName(); - 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")); - - // Cleanup - RemoteStorageHelper.forceDelete(storage, bucket); + try { + 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")); + } finally { + // Cleanup + RemoteStorageHelper.forceDelete(storage, bucket); + } } @Test @@ -79,36 +78,45 @@ public void checkInstrumentationGrpc() { Storage storage = storageOptions.getService(); String bucket = randomBucketName(); 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("grpc", getAttributeValue(spanData, "rpc.system")); - - // Cleanup - RemoteStorageHelper.forceDelete(storage, bucket); + try { + 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("grpc", getAttributeValue(spanData, "rpc.system")); + } finally { + // Cleanup + RemoteStorageHelper.forceDelete(storage, bucket); + } } @Test public void noOpDoesNothing() { String httpBucket = randomBucketName(); String grpcBucket = randomBucketName(); - // NoOp for HTTP StorageOptions storageOptionsHttp = StorageOptions.http().build(); Storage storageHttp = storageOptionsHttp.getService(); - storageHttp.create(BucketInfo.of(httpBucket)); - - // NoOp for Grpc StorageOptions storageOptionsGrpc = StorageOptions.grpc().build(); Storage storageGrpc = storageOptionsGrpc.getService(); - storageGrpc.create(BucketInfo.of(grpcBucket)); - - // cleanup - RemoteStorageHelper.forceDelete(storageHttp, httpBucket); - RemoteStorageHelper.forceDelete(storageGrpc, grpcBucket); + try { + // NoOp for HTTP + storageHttp.create(BucketInfo.of(httpBucket)); + + // NoOp for Grpc + storageGrpc.create(BucketInfo.of(grpcBucket)); + + Assert.assertNull(storageOptionsGrpc.getOpenTelemetrySdk()); + Assert.assertNull(storageOptionsHttp.getOpenTelemetrySdk()); + } finally { + // cleanup + RemoteStorageHelper.forceDelete(storageHttp, httpBucket); + RemoteStorageHelper.forceDelete(storageGrpc, grpcBucket); + } } private String getAttributeValue(SpanData spanData, String key) { @@ -119,28 +127,3 @@ public String randomBucketName() { return "java-storage-grpc-rand-" + UUID.randomUUID(); } } - -class TestExporter implements SpanExporter { - - public final List exportedSpans = Collections.synchronizedList(new ArrayList<>()); - - @Override - public CompletableResultCode export(Collection spans) { - exportedSpans.addAll(spans); - return CompletableResultCode.ofSuccess(); - } - - @Override - public CompletableResultCode flush() { - return null; - } - - @Override - public CompletableResultCode shutdown() { - return null; - } - - public List getExportedSpans() { - return exportedSpans; - } -} diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/runner/registry/TestBench.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/runner/registry/TestBench.java index 0a0dfac5a..3c595337a 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/runner/registry/TestBench.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/runner/registry/TestBench.java @@ -201,6 +201,7 @@ public void start() { tempDirectory = Files.createTempDirectory(containerName); outPath = tempDirectory.resolve("stdout"); errPath = tempDirectory.resolve("stderr"); + File outFile = outPath.toFile(); File errFile = errPath.toFile(); LOGGER.info("Redirecting server stdout to: " + outFile.getAbsolutePath()); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/otel/TestExporter.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/otel/TestExporter.java index 93ac4b348..1532fa836 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/otel/TestExporter.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/otel/TestExporter.java @@ -24,7 +24,7 @@ import java.util.Collections; import java.util.List; -class TestExporter implements SpanExporter { +public class TestExporter implements SpanExporter { public final List exportedSpans = Collections.synchronizedList(new ArrayList<>());