From 84722d197413f7a4cf0939fc16ea7d427d0184e2 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Thu, 17 Oct 2024 14:05:32 -0700 Subject: [PATCH 1/6] feat: Add OpenTelemetry to GRPC --- .../google/cloud/storage/GrpcStorageImpl.java | 24 +++++++++++++++---- .../cloud/storage/GrpcStorageOptions.java | 19 +++++++++++---- .../cloud/storage/ITOpenTelemetryTest.java | 24 +++++++++++++++++++ 3 files changed, 58 insertions(+), 9 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java index 6dff8996e9..e139220de2 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java @@ -68,6 +68,7 @@ import com.google.cloud.storage.UnifiedOpts.Opts; import com.google.cloud.storage.UnifiedOpts.ProjectId; import com.google.cloud.storage.UnifiedOpts.UserProject; +import com.google.cloud.storage.otel.OpenTelemetryTraceUtil; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -168,6 +169,7 @@ final class GrpcStorageImpl extends BaseService // workaround for https://github.com/googleapis/java-storage/issues/1736 private final Opts defaultOpts; @Deprecated private final ProjectId defaultProjectId; + private final OpenTelemetryTraceUtil openTelemetryTraceUtil; GrpcStorageImpl( GrpcStorageOptions options, @@ -184,6 +186,7 @@ final class GrpcStorageImpl extends BaseService this.retryAlgorithmManager = options.getRetryAlgorithmManager(); this.syntaxDecoders = new SyntaxDecoders(); this.defaultProjectId = UnifiedOpts.projectId(options.getProjectId()); + this.openTelemetryTraceUtil = OpenTelemetryTraceUtil.getInstance(options); } @Override @@ -198,6 +201,8 @@ public void close() throws Exception { @Override public Bucket create(BucketInfo bucketInfo, BucketTargetOption... options) { + OpenTelemetryTraceUtil.Span otelSpan = + openTelemetryTraceUtil.startSpan("create(Bucket, BucketTargetOption"); Opts opts = Opts.unwrap(options).resolveFrom(bucketInfo).prepend(defaultOpts); GrpcCallContext grpcCallContext = opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); @@ -212,11 +217,20 @@ public Bucket create(BucketInfo bucketInfo, BucketTargetOption... options) { .setParent("projects/_"); CreateBucketRequest req = opts.createBucketsRequest().apply(builder).build(); GrpcCallContext merge = Utils.merge(grpcCallContext, Retrying.newCallContext()); - return Retrying.run( - getOptions(), - retryAlgorithmManager.getFor(req), - () -> storageClient.createBucketCallable().call(req, merge), - syntaxDecoders.bucket); + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { + return Retrying.run( + getOptions(), + retryAlgorithmManager.getFor(req), + () -> storageClient.createBucketCallable().call(req, merge), + syntaxDecoders.bucket); + } catch (Exception ex) { + otelSpan.recordException(ex); + otelSpan.setStatus( + io.opentelemetry.api.trace.StatusCode.ERROR, ex.getClass().getSimpleName()); + throw StorageException.coalesce(ex); + } finally { + otelSpan.end(); + } } @Override 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 cd574e97bc..2ba8259d40 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 @@ -119,6 +119,7 @@ public final class GrpcStorageOptions extends StorageOptions private final boolean grpcClientMetricsManuallyEnabled; private final GrpcInterceptorProvider grpcInterceptorProvider; private final BlobWriteSessionConfig blobWriteSessionConfig; + private OpenTelemetrySdk openTelemetrySdk; private GrpcStorageOptions(Builder builder, GrpcStorageDefaults serviceDefaults) { super(builder, serviceDefaults); @@ -134,6 +135,7 @@ private GrpcStorageOptions(Builder builder, GrpcStorageDefaults serviceDefaults) this.grpcClientMetricsManuallyEnabled = builder.grpcMetricsManuallyEnabled; this.grpcInterceptorProvider = builder.grpcInterceptorProvider; this.blobWriteSessionConfig = builder.blobWriteSessionConfig; + this.openTelemetrySdk = builder.openTelemetrySdk; } @Override @@ -350,7 +352,7 @@ private Tuple> resolveSettingsAndOpts() throw @Override public OpenTelemetrySdk getOpenTelemetrySdk() { - return null; + return openTelemetrySdk; } /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ @@ -435,6 +437,8 @@ public static final class Builder extends StorageOptions.Builder { private boolean grpcMetricsManuallyEnabled = false; + private OpenTelemetrySdk openTelemetrySdk; + Builder() {} Builder(StorageOptions options) { @@ -446,6 +450,7 @@ public static final class Builder extends StorageOptions.Builder { this.enableGrpcClientMetrics = gso.enableGrpcClientMetrics; this.grpcInterceptorProvider = gso.grpcInterceptorProvider; this.blobWriteSessionConfig = gso.blobWriteSessionConfig; + this.openTelemetrySdk = gso.openTelemetrySdk; } /** @@ -634,9 +639,15 @@ public GrpcStorageOptions.Builder setBlobWriteSessionConfig( return this; } - @Override - public StorageOptions.Builder setOpenTelemetrySdk(@NonNull OpenTelemetrySdk openTelemetrySdk) { - return null; + /** + * Enable OpenTelemetry Tracing and provide an instance for the client to use. + * + * @param openTelemetrySdk + */ + public GrpcStorageOptions.Builder setOpenTelemetrySdk(OpenTelemetrySdk openTelemetrySdk) { + requireNonNull(openTelemetrySdk, "openTelemetry must be non null"); + this.openTelemetrySdk = openTelemetrySdk; + return this; } /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ 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 e07cf659cc..e13a77c2c7 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 @@ -68,6 +68,30 @@ public void checkInstrumentation() { getAttributeValue(spanData, "gcp.client.artifact")); } + @Test + public void checkInstrumentationGrpc() { + SpanExporter exporter = new TestExporter(); + + OpenTelemetrySdk openTelemetrySdk = + OpenTelemetrySdk.builder() + .setTracerProvider( + SdkTracerProvider.builder() + .addSpanProcessor(SimpleSpanProcessor.create(exporter)) + .build()) + .build(); + StorageOptions storageOptions = + StorageOptions.grpc().setOpenTelemetrySdk(openTelemetrySdk).build(); + Storage storage = storageOptions.getService(); + storage.create(BucketInfo.of(generator.randomBucketName())); + 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")); + } + @Test public void noOpDoesNothing() { StorageOptions storageOptions = StorageOptions.http().build(); From d8f263dae2114ca08cf1bdb199d979e672976c06 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Fri, 18 Oct 2024 11:11:49 -0700 Subject: [PATCH 2/6] adding rpc.system value --- .../com/google/cloud/storage/otel/OpenTelemetryInstance.java | 5 +++++ .../java/com/google/cloud/storage/ITOpenTelemetryTest.java | 2 ++ 2 files changed, 7 insertions(+) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java index 4fd1f0b6ff..788d6d8ef3 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java @@ -18,6 +18,7 @@ import com.google.api.core.ApiFuture; import com.google.api.gax.core.GaxProperties; +import com.google.cloud.storage.GrpcStorageOptions; import com.google.cloud.storage.StorageOptions; import com.google.cloud.storage.otel.OpenTelemetryTraceUtil.Context; import com.google.cloud.storage.otel.OpenTelemetryTraceUtil.Span; @@ -38,11 +39,14 @@ class OpenTelemetryInstance implements OpenTelemetryTraceUtil { private static final String LIBRARY_NAME = "cloud.google.com/java/storage"; + private final String transport; + public OpenTelemetryInstance(StorageOptions storageOptions) { this.storageOptions = storageOptions; this.openTelemetry = storageOptions.getOpenTelemetrySdk(); this.tracer = openTelemetry.getTracer(LIBRARY_NAME, GaxProperties.getLibraryVersion(this.getClass())); + this.transport = storageOptions instanceof GrpcStorageOptions ? "grpc" : "http"; } static class Span implements OpenTelemetryTraceUtil.Span { @@ -146,6 +150,7 @@ public Scope makeCurrent() { public OpenTelemetryTraceUtil.Span startSpan(String methodName) { String formatSpanName = String.format("%s.%s/%s", "storage", "client", methodName); SpanBuilder spanBuilder = tracer.spanBuilder(formatSpanName).setSpanKind(SpanKind.CLIENT); + spanBuilder.setAttribute("rpc.system", transport); io.opentelemetry.api.trace.Span span = addSettingsAttributesToCurrentSpan(spanBuilder).startSpan(); return new Span(span, formatSpanName); 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 e13a77c2c7..e466af461d 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 @@ -66,6 +66,7 @@ public void checkInstrumentation() { Assert.assertEquals( "com.google.cloud.google-cloud-storage", getAttributeValue(spanData, "gcp.client.artifact")); + Assert.assertEquals("http", getAttributeValue(spanData, "rpc.system")); } @Test @@ -90,6 +91,7 @@ public void checkInstrumentationGrpc() { Assert.assertEquals( "com.google.cloud.google-cloud-storage", getAttributeValue(spanData, "gcp.client.artifact")); + Assert.assertEquals("grpc", getAttributeValue(spanData, "rpc.system")); } @Test From e1e37ae15d675c10e2f010617b1a2ec64a89b7de Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Fri, 18 Oct 2024 11:23:40 -0700 Subject: [PATCH 3/6] refactor test for both transports --- .../cloud/storage/ITOpenTelemetryTest.java | 49 ++++++++++++------- 1 file changed, 32 insertions(+), 17 deletions(-) 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 e466af461d..6f55c54b89 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,12 +16,7 @@ package com.google.cloud.storage; -import com.google.cloud.storage.TransportCompatibility.Transport; -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.CrossRun; -import com.google.cloud.storage.it.runner.annotations.Inject; -import com.google.cloud.storage.it.runner.registry.Generator; +import com.google.cloud.storage.testing.RemoteStorageHelper; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.sdk.common.CompletableResultCode; @@ -33,16 +28,11 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.UUID; import org.junit.Assert; import org.junit.Test; -import org.junit.runner.RunWith; -@RunWith(StorageITRunner.class) -@CrossRun( - backends = {Backend.PROD}, - transports = {Transport.HTTP}) public final class ITOpenTelemetryTest { - @Inject public Generator generator; @Test public void checkInstrumentation() { @@ -58,7 +48,8 @@ public void checkInstrumentation() { StorageOptions storageOptions = StorageOptions.http().setOpenTelemetrySdk(openTelemetrySdk).build(); Storage storage = storageOptions.getService(); - storage.create(BucketInfo.of(generator.randomBucketName())); + 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")); @@ -67,6 +58,9 @@ public void checkInstrumentation() { "com.google.cloud.google-cloud-storage", getAttributeValue(spanData, "gcp.client.artifact")); Assert.assertEquals("http", getAttributeValue(spanData, "rpc.system")); + + // Cleanup + RemoteStorageHelper.forceDelete(storage, bucket); } @Test @@ -83,7 +77,8 @@ public void checkInstrumentationGrpc() { StorageOptions storageOptions = StorageOptions.grpc().setOpenTelemetrySdk(openTelemetrySdk).build(); Storage storage = storageOptions.getService(); - storage.create(BucketInfo.of(generator.randomBucketName())); + 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")); @@ -92,21 +87,41 @@ public void checkInstrumentationGrpc() { "com.google.cloud.google-cloud-storage", getAttributeValue(spanData, "gcp.client.artifact")); Assert.assertEquals("grpc", getAttributeValue(spanData, "rpc.system")); + + // Cleanup + RemoteStorageHelper.forceDelete(storage, bucket); } @Test public void noOpDoesNothing() { - StorageOptions storageOptions = StorageOptions.http().build(); - Storage storage = storageOptions.getService(); - storage.create(BucketInfo.of(generator.randomBucketName())); + 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); } private String getAttributeValue(SpanData spanData, String key) { return spanData.getAttributes().get(AttributeKey.stringKey(key)).toString(); } + + public String randomBucketName() { + return "java-storage-grpc-rand-" + UUID.randomUUID(); + } } class TestExporter implements SpanExporter { + public final List exportedSpans = Collections.synchronizedList(new ArrayList<>()); @Override From 9ed968778dddcbc5bf17adb78cd09b1fb47c98f6 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Fri, 18 Oct 2024 12:37:53 -0700 Subject: [PATCH 4/6] lint --- .../google/cloud/storage/otel/OpenTelemetryInstance.java | 8 +++++++- .../com/google/cloud/storage/ITOpenTelemetryTest.java | 6 +++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java index 788d6d8ef3..9c71f4842a 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java @@ -23,6 +23,7 @@ import com.google.cloud.storage.otel.OpenTelemetryTraceUtil.Context; import com.google.cloud.storage.otel.OpenTelemetryTraceUtil.Span; import io.opentelemetry.api.OpenTelemetry; +import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.trace.SpanBuilder; @@ -60,7 +61,12 @@ static class Span implements OpenTelemetryTraceUtil.Span { @Override public OpenTelemetryTraceUtil.Span recordException(Throwable error) { - span.recordException(error); + span.recordException( + error, + Attributes.of( + AttributeKey.stringKey("exception.message"), error.getMessage(), + AttributeKey.stringKey("exception.type"), error.getClass().getName(), + AttributeKey.stringKey("exception.stacktrace"), error.getStackTrace().toString())); return this; } 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 6f55c54b89..c3261de542 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 @@ -86,9 +86,9 @@ public void checkInstrumentationGrpc() { Assert.assertEquals( "com.google.cloud.google-cloud-storage", getAttributeValue(spanData, "gcp.client.artifact")); - Assert.assertEquals("grpc", getAttributeValue(spanData, "rpc.system")); + Assert.assertEquals("grpc", getAttributeValue(spanData, "rpc.system")); - // Cleanup + // Cleanup RemoteStorageHelper.forceDelete(storage, bucket); } @@ -101,7 +101,7 @@ public void noOpDoesNothing() { Storage storageHttp = storageOptionsHttp.getService(); storageHttp.create(BucketInfo.of(httpBucket)); - //NoOp for Grpc + // NoOp for Grpc StorageOptions storageOptionsGrpc = StorageOptions.grpc().build(); Storage storageGrpc = storageOptionsGrpc.getService(); storageGrpc.create(BucketInfo.of(grpcBucket)); From 63157f8e1b08b65d79e9dfa04985a5c21109eea4 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Tue, 22 Oct 2024 08:27:21 -0700 Subject: [PATCH 5/6] add branch protection rules to otel feature branch --- .github/sync-repo-settings.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/sync-repo-settings.yaml b/.github/sync-repo-settings.yaml index f55594b387..f28f5575df 100644 --- a/.github/sync-repo-settings.yaml +++ b/.github/sync-repo-settings.yaml @@ -162,6 +162,22 @@ branchProtectionRules: - 'Kokoro - Test: Java GraalVM Native Image' - 'Kokoro - Test: Java 17 GraalVM Native Image' - javadoc + - pattern: otel-v1-branch + isAdminEnforced: true + requiredApprovingReviewCount: 1 + requiresCodeOwnerReviews: true + requiresStrictStatusChecks: false + requiredStatusCheckContexts: + - dependencies (17) + - lint + - clirr + - units (8) + - units (11) + - 'Kokoro - Test: Integration' + - cla/google + - 'Kokoro - Test: Java GraalVM Native Image' + - 'Kokoro - Test: Java 17 GraalVM Native Image' + - javadoc permissionRules: - team: yoshi-admins permission: admin From 171fdb5104c8c655bac33d88e7c2e96cb320c7d8 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Tue, 22 Oct 2024 09:19:34 -0700 Subject: [PATCH 6/6] pr comment --- .../main/java/com/google/cloud/storage/GrpcStorageOptions.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 2ba8259d40..92d05403ee 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 @@ -642,7 +642,7 @@ public GrpcStorageOptions.Builder setBlobWriteSessionConfig( /** * Enable OpenTelemetry Tracing and provide an instance for the client to use. * - * @param openTelemetrySdk + * @param openTelemetrySdk User defined instance of OpenTelemetry SDK to be used by the library */ public GrpcStorageOptions.Builder setOpenTelemetrySdk(OpenTelemetrySdk openTelemetrySdk) { requireNonNull(openTelemetrySdk, "openTelemetry must be non null");