From f5f064421f9ca970bb8b6bb19c61a17aa35500b0 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Tue, 15 Oct 2024 15:31:36 -0700 Subject: [PATCH 1/7] feat: Instrument HTTP with OpenTelemetry --- .../cloud/storage/GrpcStorageOptions.java | 61 +++++- .../cloud/storage/HttpStorageOptions.java | 26 +++ .../google/cloud/storage/StorageOptions.java | 6 + .../otel/NoOpOpenTelemetryInstance.java | 45 ++++ .../storage/otel/OpenTelemetryInstance.java | 192 ++++++++++++++++++ .../storage/otel/OpenTelemetryTraceUtil.java | 94 +++++++++ .../cloud/storage/spi/v1/HttpStorageRpc.java | 14 +- .../cloud/storage/OpenTelemetryTest.java | 99 +++++++++ 8 files changed, 531 insertions(+), 6 deletions(-) create mode 100644 google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java create mode 100644 google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java create mode 100644 google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryTraceUtil.java create mode 100644 google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java 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 10d7066987..20822a29ef 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 @@ -30,6 +30,7 @@ import com.google.api.gax.grpc.GrpcStubCallableFactory; import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider; import com.google.api.gax.retrying.RetrySettings; +import com.google.api.gax.retrying.StreamResumptionStrategy; import com.google.api.gax.rpc.ClientContext; import com.google.api.gax.rpc.HeaderProvider; import com.google.api.gax.rpc.NoHeaderProvider; @@ -78,6 +79,7 @@ import io.grpc.MethodDescriptor; import io.grpc.Status; import io.grpc.protobuf.ProtoUtils; +import io.opentelemetry.sdk.OpenTelemetrySdk; import java.io.Closeable; import java.io.IOException; import java.io.InputStream; @@ -97,6 +99,7 @@ import java.util.Objects; import java.util.Set; import org.checkerframework.checker.nullness.qual.NonNull; +import org.checkerframework.checker.nullness.qual.Nullable; import org.threeten.bp.Duration; /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ @@ -334,12 +337,18 @@ private Tuple> resolveSettingsAndOpts() throw .startResumableWriteSettings() .setRetrySettings(baseRetrySettings) .setRetryableCodes(startResumableWriteRetryableCodes); - // for ReadObject disable retries and move the total timeout to the idle timeout + // for ReadObject we are configuring the server stream handling to do its own retries, so wire + // things through. Retryable codes will be controlled closer to the use site as idempotency + // considerations need to be made. builder .readObjectSettings() .setRetrySettings(readRetrySettings) - // disable gapic retries because we're handling it ourselves - .setRetryableCodes(Collections.emptySet()) + // even though we might want to default to the empty set for retryable codes, don't ever + // actually do this. Doing so prevents any retry capability from being wired into the stream + // pipeline, ever. + // For our use, we will always set it one way or the other to ensure it's appropriate + // DO NOT: .setRetryableCodes(Collections.emptySet()) + .setResumptionStrategy(new ReadObjectResumptionStrategy()) // for reads, the stream can be held open for a long time in order to read all bytes, // this is totally valid. instead we want to monitor if the stream is doing work and if not // timeout. @@ -347,6 +356,11 @@ private Tuple> resolveSettingsAndOpts() throw return Tuple.of(builder.build(), defaultOpts); } + @Override + public OpenTelemetrySdk getOpenTelemetrySdk() { + return null; + } + /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ @BetaApi @Override @@ -628,6 +642,11 @@ public GrpcStorageOptions.Builder setBlobWriteSessionConfig( return this; } + @Override + public StorageOptions.Builder setOpenTelemetrySdk(@NonNull OpenTelemetrySdk openTelemetrySdk) { + return null; + } + /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ @BetaApi @Override @@ -689,7 +708,7 @@ public Duration getTerminationAwaitDuration() { /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ @BetaApi public boolean isAttemptDirectPath() { - return true; + return false; } /** @since 2.41.0 This new api is in preview and is subject to breaking changes. */ @@ -837,6 +856,40 @@ public ServiceRpc create(StorageOptions options) { } } + // TODO: See if we can change gax to allow shifting this to callable.withContext so it doesn't + // have to be set globally + private static class ReadObjectResumptionStrategy + implements StreamResumptionStrategy { + private long readOffset = 0; + + @NonNull + @Override + public StreamResumptionStrategy createNew() { + return new ReadObjectResumptionStrategy(); + } + + @NonNull + @Override + public ReadObjectResponse processResponse(ReadObjectResponse response) { + readOffset += response.getChecksummedData().getContent().size(); + return response; + } + + @Nullable + @Override + public ReadObjectRequest getResumeRequest(ReadObjectRequest originalRequest) { + if (readOffset != 0) { + return originalRequest.toBuilder().setReadOffset(readOffset).build(); + } + return originalRequest; + } + + @Override + public boolean canResume() { + return true; + } + } + // setInternalHeaderProvider is protected so we need to open its scope in order to set it // we are adding an entry for gccl which is set via this provider private static final class GapicStorageSettingsBuilder extends StorageSettings.Builder { 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 9f6781f6c4..f58da29e18 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 @@ -38,6 +38,7 @@ import com.google.cloud.storage.spi.v1.StorageRpc; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableSet; +import io.opentelemetry.sdk.OpenTelemetrySdk; import java.io.IOException; import java.io.ObjectInputStream; import java.io.Serializable; @@ -60,6 +61,8 @@ public class HttpStorageOptions extends StorageOptions { private transient RetryDependenciesAdapter retryDepsAdapter; private final BlobWriteSessionConfig blobWriteSessionConfig; + private OpenTelemetrySdk openTelemetrySdk; + private HttpStorageOptions(Builder builder, StorageDefaults serviceDefaults) { super(builder, serviceDefaults); this.retryAlgorithmManager = @@ -68,6 +71,7 @@ private HttpStorageOptions(Builder builder, StorageDefaults serviceDefaults) { builder.storageRetryStrategy, defaults().getStorageRetryStrategy())); retryDepsAdapter = new RetryDependenciesAdapter(); blobWriteSessionConfig = builder.blobWriteSessionConfig; + openTelemetrySdk = builder.openTelemetrySdk; } @Override @@ -85,6 +89,11 @@ StorageRpc getStorageRpcV1() { return (StorageRpc) getRpc(); } + @Override + public OpenTelemetrySdk getOpenTelemetrySdk() { + return openTelemetrySdk; + } + @Override public HttpStorageOptions.Builder toBuilder() { return new HttpStorageOptions.Builder(this); @@ -131,11 +140,16 @@ RetryingDependencies asRetryDependencies() { return retryDepsAdapter; } + public void setOpenTelemetrySdk(OpenTelemetrySdk openTelemetrySdk) { + this.openTelemetrySdk = openTelemetrySdk; + } + public static class Builder extends StorageOptions.Builder { private StorageRetryStrategy storageRetryStrategy; private BlobWriteSessionConfig blobWriteSessionConfig = HttpStorageDefaults.INSTANCE.getDefaultStorageWriterConfig(); + private OpenTelemetrySdk openTelemetrySdk; Builder() {} @@ -144,6 +158,7 @@ public static class Builder extends StorageOptions.Builder { HttpStorageOptions hso = (HttpStorageOptions) options; this.storageRetryStrategy = hso.retryAlgorithmManager.retryStrategy; this.blobWriteSessionConfig = hso.blobWriteSessionConfig; + this.openTelemetrySdk = hso.getOpenTelemetrySdk(); } @Override @@ -269,6 +284,17 @@ public HttpStorageOptions build() { } return options; } + + /** + * Enable OpenTelemetry Tracing and provide an instance for the client to use. + * + * @param openTelemetrySdk + */ + public HttpStorageOptions.Builder setOpenTelemetrySdk(OpenTelemetrySdk openTelemetrySdk) { + requireNonNull(openTelemetrySdk, "openTelemetry must be non null"); + this.openTelemetrySdk = openTelemetrySdk; + return this; + } } public static final class HttpStorageDefaults extends StorageDefaults { diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageOptions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageOptions.java index ab32532ae1..703f927509 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageOptions.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageOptions.java @@ -29,6 +29,7 @@ import com.google.cloud.storage.Storage.BlobWriteOption; import com.google.cloud.storage.TransportCompatibility.Transport; import com.google.cloud.storage.spi.StorageRpcFactory; +import io.opentelemetry.sdk.OpenTelemetrySdk; import java.io.IOException; import java.io.InputStream; import java.util.Properties; @@ -110,6 +111,9 @@ public abstract static class Builder public abstract StorageOptions.Builder setBlobWriteSessionConfig( @NonNull BlobWriteSessionConfig blobWriteSessionConfig); + public abstract StorageOptions.Builder setOpenTelemetrySdk( + @NonNull OpenTelemetrySdk openTelemetrySdk); + @Override public abstract StorageOptions build(); } @@ -144,6 +148,8 @@ public static String version() { return VERSION; } + public abstract OpenTelemetrySdk getOpenTelemetrySdk(); + @SuppressWarnings("unchecked") @Override public abstract StorageOptions.Builder toBuilder(); diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java new file mode 100644 index 0000000000..545512996e --- /dev/null +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java @@ -0,0 +1,45 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.google.cloud.storage.otel; + +import javax.annotation.Nonnull; + +class NoOpOpenTelemetryInstance implements OpenTelemetryTraceUtil { + + @Override + public Span startSpan(String spanName) { + return null; + } + + @Override + public Span startSpan(String spanName, Context parent) { + return null; + } + + @Nonnull + @Override + public Span currentSpan() { + return null; + } + + @Nonnull + @Override + public Context currentContext() { + return null; + } +} 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 new file mode 100644 index 0000000000..025f286704 --- /dev/null +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryInstance.java @@ -0,0 +1,192 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.google.cloud.storage.otel; + +import com.google.api.core.ApiFuture; +import com.google.api.gax.core.GaxProperties; +import com.google.cloud.storage.StorageOptions; +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.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.api.trace.SpanBuilder; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.api.trace.StatusCode; +import io.opentelemetry.api.trace.Tracer; +import java.util.Map; +import javax.annotation.Nonnull; + +class OpenTelemetryInstance implements OpenTelemetryTraceUtil { + private final Tracer tracer; + private final OpenTelemetry openTelemetry; + private final StorageOptions storageOptions; + + private static final String LIBRARY_NAME = "cloud.google.com/java/storage"; + + public OpenTelemetryInstance(StorageOptions storageOptions) { + this.storageOptions = storageOptions; + this.openTelemetry = storageOptions.getOpenTelemetrySdk(); + this.tracer = + openTelemetry.getTracer(LIBRARY_NAME, GaxProperties.getLibraryVersion(this.getClass())); + } + + static class Span implements OpenTelemetryTraceUtil.Span { + private final io.opentelemetry.api.trace.Span span; + private final String spanName; + + Span(io.opentelemetry.api.trace.Span span, String spanName) { + this.span = span; + this.spanName = spanName; + } + + @Override + public OpenTelemetryTraceUtil.Span recordException(Throwable error) { + span.recordException(error); + return this; + } + + @Override + public OpenTelemetryTraceUtil.Span setStatus(StatusCode status, String name) { + span.setStatus(status, name); + return this; + } + + @Override + public OpenTelemetryTraceUtil.Span addEvent(String name) { + span.addEvent(name); + return this; + } + + @Override + public OpenTelemetryTraceUtil.Span addEvent(String name, Map attributes) { + AttributesBuilder attributesBuilder = Attributes.builder(); + attributes.forEach( + (key, value) -> { + if (value instanceof Integer) { + attributesBuilder.put(key, (int) value); + } else if (value instanceof Long) { + attributesBuilder.put(key, (long) value); + } else if (value instanceof Double) { + attributesBuilder.put(key, (double) value); + } else if (value instanceof Float) { + attributesBuilder.put(key, (float) value); + } else if (value instanceof Boolean) { + attributesBuilder.put(key, (boolean) value); + } else if (value instanceof String) { + attributesBuilder.put(key, (String) value); + } else { + // OpenTelemetry APIs do not support any other type. + throw new IllegalArgumentException( + "Unknown attribute type:" + value.getClass().getSimpleName()); + } + }); + span.addEvent(name, attributesBuilder.build()); + return this; + } + + @Override + public Scope makeCurrent() { + return new Scope(span.makeCurrent()); + } + + @Override + public void end() { + span.end(); + } + + @Override + public void end(Throwable error) {} + + @Override + public void endAtFuture(ApiFuture futureValue) {} + } + + static class Scope implements OpenTelemetryTraceUtil.Scope { + private final io.opentelemetry.context.Scope scope; + + Scope(io.opentelemetry.context.Scope scope) { + this.scope = scope; + } + + @Override + public void close() { + scope.close(); + } + } + + static class Context implements OpenTelemetryTraceUtil.Context { + private final io.opentelemetry.context.Context context; + + Context(io.opentelemetry.context.Context context) { + this.context = context; + } + + @Override + public Scope makeCurrent() { + return new Scope(context.makeCurrent()); + } + } + + @Override + public OpenTelemetryTraceUtil.Span startSpan(String methodName) { + String formatSpanName = String.format("%s.%s/%s", "storage", "client", methodName); + SpanBuilder spanBuilder = tracer.spanBuilder(formatSpanName).setSpanKind(SpanKind.CLIENT); + io.opentelemetry.api.trace.Span span = + addSettingsAttributesToCurrentSpan(spanBuilder).startSpan(); + return new Span(span, formatSpanName); + } + + @Override + public OpenTelemetryTraceUtil.Span startSpan( + String spanName, OpenTelemetryTraceUtil.Context parent) { + assert (parent instanceof OpenTelemetryInstance.Context); + SpanBuilder spanBuilder = + tracer + .spanBuilder(spanName) + .setSpanKind(SpanKind.CLIENT) + .setParent(((OpenTelemetryInstance.Context) parent).context); + io.opentelemetry.api.trace.Span span = + addSettingsAttributesToCurrentSpan(spanBuilder).startSpan(); + return new Span(span, spanName); + } + + @Nonnull + @Override + public OpenTelemetryTraceUtil.Span currentSpan() { + return new Span(io.opentelemetry.api.trace.Span.current(), ""); + } + + @Nonnull + @Override + public OpenTelemetryTraceUtil.Context currentContext() { + return new Context(io.opentelemetry.context.Context.current()); + } + + private SpanBuilder addSettingsAttributesToCurrentSpan(SpanBuilder spanBuilder) { + spanBuilder = spanBuilder.setAttribute("gcp.client.service", "Storage"); + spanBuilder = + spanBuilder.setAllAttributes( + Attributes.builder() + .put("gcp.client.version", GaxProperties.getLibraryVersion(this.getClass())) + .put("gcp.client.repo", "googleapis/java-storage") + .put("gcp.client.artifact", "com.google.cloud.google-cloud-storage") + .build()); + return spanBuilder; + } +} diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryTraceUtil.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryTraceUtil.java new file mode 100644 index 0000000000..932457d05e --- /dev/null +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/OpenTelemetryTraceUtil.java @@ -0,0 +1,94 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.storage.otel; + +import com.google.api.core.ApiFuture; +import com.google.cloud.storage.StorageOptions; +import io.opentelemetry.api.trace.StatusCode; +import java.util.Map; +import javax.annotation.Nonnull; + +public interface OpenTelemetryTraceUtil { + + static OpenTelemetryTraceUtil getInstance(@Nonnull StorageOptions storageOptions) { + boolean createNoOp = storageOptions.getOpenTelemetrySdk() == null; + + if (createNoOp) { + return new NoOpOpenTelemetryInstance(); + } else { + return new OpenTelemetryInstance(storageOptions); + } + } + + /** Represents a trace span. */ + interface Span { + Span recordException(Throwable error); + + Span setStatus(StatusCode status, String name); + /** Adds the given event to this span. */ + Span addEvent(String name); + + /** Adds the given event with the given attributes to this span. */ + Span addEvent(String name, Map attributes); + + /** Marks this span as the current span. */ + Scope makeCurrent(); + + /** Ends this span. */ + void end(); + + /** Ends this span in an error. */ + void end(Throwable error); + + /** + * If an operation ends in the future, its relevant span should end _after_ the future has been + * completed. This method "appends" the span completion code at the completion of the given + * future. In order for telemetry info to be recorded, the future returned by this method should + * be completed. + */ + void endAtFuture(ApiFuture futureValue); + } + + /** Represents a trace context. */ + interface Context { + /** Makes this context the current context. */ + Scope makeCurrent(); + } + + /** Represents a trace scope. */ + interface Scope extends AutoCloseable { + /** Closes the current scope. */ + void close(); + } + + /** Starts a new span with the given name, sets it as the current span, and returns it. */ + Span startSpan(String spanName); + + /** + * Starts a new span with the given name and the given context as its parent, sets it as the + * current span, and returns it. + */ + Span startSpan(String spanName, Context parent); + + /** Returns the current span. */ + @Nonnull + Span currentSpan(); + + /** Returns the current Context. */ + @Nonnull + Context currentContext(); +} 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 5341051a25..d74c011677 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 @@ -67,6 +67,7 @@ import com.google.cloud.http.HttpTransportOptions; import com.google.cloud.storage.StorageException; import com.google.cloud.storage.StorageOptions; +import com.google.cloud.storage.otel.OpenTelemetryTraceUtil; import com.google.common.base.Function; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @@ -80,6 +81,7 @@ import io.opencensus.trace.Status; import io.opencensus.trace.Tracer; import io.opencensus.trace.Tracing; +import io.opentelemetry.api.trace.StatusCode; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; @@ -113,6 +115,7 @@ public class HttpStorageRpc implements StorageRpc { private final Storage storage; private final Tracer tracer = Tracing.getTracer(); private final HttpRequestInitializer batchRequestInitializer; + private final OpenTelemetryTraceUtil openTelemetryTraceUtil; private static final long MEGABYTE = 1024L * 1024L; private static final FileNameMap FILE_NAME_MAP = URLConnection.getFileNameMap(); @@ -144,6 +147,8 @@ public HttpStorageRpc(StorageOptions options, JsonFactory jsonFactory) { .setRootUrl(options.getHost()) .setApplicationName(applicationName) .build(); + // Get instance of OpenTelemetry + openTelemetryTraceUtil = OpenTelemetryTraceUtil.getInstance(options); } public Storage getStorage() { @@ -355,9 +360,11 @@ private Span startSpan(String spanName) { @Override public Bucket create(Bucket bucket, Map options) { + OpenTelemetryTraceUtil.Span otelSpan = + openTelemetryTraceUtil.startSpan("create(Bucket,Map)"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_CREATE_BUCKET); Scope scope = tracer.withSpan(span); - try { + try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { return storage .buckets() .insert(this.options.getProjectId(), bucket) @@ -368,9 +375,12 @@ public Bucket create(Bucket bucket, Map options) { .setEnableObjectRetention(Option.ENABLE_OBJECT_RETENTION.getBoolean(options)) .execute(); } catch (IOException ex) { + otelSpan.recordException(ex); + otelSpan.setStatus(StatusCode.ERROR, ex.getClass().getSimpleName()); span.setStatus(Status.UNKNOWN.withDescription(ex.getMessage())); throw translate(ex); } finally { + otelSpan.end(); scope.close(); span.end(HttpStorageRpcSpans.END_SPAN_OPTIONS); } @@ -1822,4 +1832,4 @@ private static StorageException buildStorageException(int statusCode, String sta error.setMessage(statusMessage); return translate(error); } -} +} \ No newline at end of file diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java new file mode 100644 index 0000000000..417ad0c018 --- /dev/null +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java @@ -0,0 +1,99 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +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 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 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 OpenTelemetryTest { + @Inject public Generator generator; + + @Test + public void checkInstrumentation() { + SpanExporter exporter = new TestExporter(); + + OpenTelemetrySdk openTelemetrySdk = + OpenTelemetrySdk.builder() + .setTracerProvider( + SdkTracerProvider.builder() + .addSpanProcessor(SimpleSpanProcessor.create(exporter)) + .build()) + .build(); + StorageOptions storageOptions = + StorageOptions.http().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")); + } + + private String getAttributeValue(SpanData spanData, String key) { + return spanData.getAttributes().get(AttributeKey.stringKey(key)).toString(); + } +} + +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; + } +} From 0286d4fde4c0a1fbc36e7c60b6b455c4e233fde5 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Wed, 16 Oct 2024 10:10:01 -0700 Subject: [PATCH 2/7] Implement noop methods and write noop test --- .../otel/NoOpOpenTelemetryInstance.java | 63 +++++++++++++++++-- .../cloud/storage/OpenTelemetryTest.java | 8 +++ 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java index 545512996e..9e3c895043 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java @@ -17,29 +17,80 @@ package com.google.cloud.storage.otel; +import com.google.api.core.ApiFuture; +import io.opentelemetry.api.trace.StatusCode; +import java.util.Map; import javax.annotation.Nonnull; class NoOpOpenTelemetryInstance implements OpenTelemetryTraceUtil { @Override - public Span startSpan(String spanName) { - return null; + public OpenTelemetryTraceUtil.Span startSpan(String spanName) { + return new Span(); } @Override - public Span startSpan(String spanName, Context parent) { - return null; + public OpenTelemetryTraceUtil.Span startSpan(String spanName, + OpenTelemetryTraceUtil.Context parent) { + return new Span(); } @Nonnull @Override public Span currentSpan() { - return null; + return new Span(); } @Nonnull @Override public Context currentContext() { - return null; + return new Context(); + } + + static class Span implements OpenTelemetryTraceUtil.Span { + @Override + public void end() {} + + @Override + public void end(Throwable error) {} + + @Override + public void endAtFuture(ApiFuture futureValue) {} + + @Override + public OpenTelemetryTraceUtil.Span recordException(Throwable error) { + return this; + } + + @Override + public OpenTelemetryTraceUtil.Span setStatus(StatusCode status, String name) { + return this; + } + + @Override + public OpenTelemetryTraceUtil.Span addEvent(String name) { + return this; + } + + @Override + public OpenTelemetryTraceUtil.Span addEvent(String name, Map attributes) { + return this; + } + + @Override + public Scope makeCurrent() { + return new Scope(); + } + } + static class Context implements OpenTelemetryTraceUtil.Context { + @Override + public Scope makeCurrent() { + return new Scope(); + } + } + + static class Scope implements OpenTelemetryTraceUtil.Scope { + @Override + public void close() {} } } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java index 417ad0c018..cd519a267f 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java @@ -69,6 +69,14 @@ public void checkInstrumentation() { getAttributeValue(spanData, "gcp.client.artifact")); } + @Test + public void noOpDoesNothing() { + StorageOptions storageOptions = + StorageOptions.http().build(); + Storage storage = storageOptions.getService(); + storage.create(BucketInfo.of(generator.randomBucketName())); + } + private String getAttributeValue(SpanData spanData, String key) { return spanData.getAttributes().get(AttributeKey.stringKey(key)).toString(); } From bb7e0111da2cafd5585b0ebebd8526f73c101cec Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Wed, 16 Oct 2024 10:28:01 -0700 Subject: [PATCH 3/7] linter --- .../google/cloud/storage/otel/NoOpOpenTelemetryInstance.java | 5 +++-- .../java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java | 5 ++--- .../java/com/google/cloud/storage/OpenTelemetryTest.java | 3 +-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java index 9e3c895043..417bd21a27 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java @@ -30,8 +30,8 @@ public OpenTelemetryTraceUtil.Span startSpan(String spanName) { } @Override - public OpenTelemetryTraceUtil.Span startSpan(String spanName, - OpenTelemetryTraceUtil.Context parent) { + public OpenTelemetryTraceUtil.Span startSpan( + String spanName, OpenTelemetryTraceUtil.Context parent) { return new Span(); } @@ -82,6 +82,7 @@ public Scope makeCurrent() { return new Scope(); } } + static class Context implements OpenTelemetryTraceUtil.Context { @Override public Scope makeCurrent() { 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 d74c011677..fdfec46199 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 @@ -360,8 +360,7 @@ private Span startSpan(String spanName) { @Override public Bucket create(Bucket bucket, Map options) { - OpenTelemetryTraceUtil.Span otelSpan = - openTelemetryTraceUtil.startSpan("create(Bucket,Map)"); + OpenTelemetryTraceUtil.Span otelSpan = openTelemetryTraceUtil.startSpan("create(Bucket,Map)"); Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_CREATE_BUCKET); Scope scope = tracer.withSpan(span); try (OpenTelemetryTraceUtil.Scope unused = otelSpan.makeCurrent()) { @@ -1832,4 +1831,4 @@ private static StorageException buildStorageException(int statusCode, String sta error.setMessage(statusMessage); return translate(error); } -} \ No newline at end of file +} diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java index cd519a267f..fe1164f7f4 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java @@ -71,8 +71,7 @@ public void checkInstrumentation() { @Test public void noOpDoesNothing() { - StorageOptions storageOptions = - StorageOptions.http().build(); + StorageOptions storageOptions = StorageOptions.http().build(); Storage storage = storageOptions.getService(); storage.create(BucketInfo.of(generator.randomBucketName())); } From 38483ea4c3e53a673d3064ee5d3b3ce82caea65a Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Wed, 16 Oct 2024 10:31:54 -0700 Subject: [PATCH 4/7] removing accidental commit --- .../cloud/storage/GrpcStorageOptions.java | 50 ++----------------- 1 file changed, 4 insertions(+), 46 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 20822a29ef..cd574e97bc 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 @@ -30,7 +30,6 @@ import com.google.api.gax.grpc.GrpcStubCallableFactory; import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider; import com.google.api.gax.retrying.RetrySettings; -import com.google.api.gax.retrying.StreamResumptionStrategy; import com.google.api.gax.rpc.ClientContext; import com.google.api.gax.rpc.HeaderProvider; import com.google.api.gax.rpc.NoHeaderProvider; @@ -99,7 +98,6 @@ import java.util.Objects; import java.util.Set; import org.checkerframework.checker.nullness.qual.NonNull; -import org.checkerframework.checker.nullness.qual.Nullable; import org.threeten.bp.Duration; /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ @@ -337,18 +335,12 @@ private Tuple> resolveSettingsAndOpts() throw .startResumableWriteSettings() .setRetrySettings(baseRetrySettings) .setRetryableCodes(startResumableWriteRetryableCodes); - // for ReadObject we are configuring the server stream handling to do its own retries, so wire - // things through. Retryable codes will be controlled closer to the use site as idempotency - // considerations need to be made. + // for ReadObject disable retries and move the total timeout to the idle timeout builder .readObjectSettings() .setRetrySettings(readRetrySettings) - // even though we might want to default to the empty set for retryable codes, don't ever - // actually do this. Doing so prevents any retry capability from being wired into the stream - // pipeline, ever. - // For our use, we will always set it one way or the other to ensure it's appropriate - // DO NOT: .setRetryableCodes(Collections.emptySet()) - .setResumptionStrategy(new ReadObjectResumptionStrategy()) + // disable gapic retries because we're handling it ourselves + .setRetryableCodes(Collections.emptySet()) // for reads, the stream can be held open for a long time in order to read all bytes, // this is totally valid. instead we want to monitor if the stream is doing work and if not // timeout. @@ -708,7 +700,7 @@ public Duration getTerminationAwaitDuration() { /** @since 2.14.0 This new api is in preview and is subject to breaking changes. */ @BetaApi public boolean isAttemptDirectPath() { - return false; + return true; } /** @since 2.41.0 This new api is in preview and is subject to breaking changes. */ @@ -856,40 +848,6 @@ public ServiceRpc create(StorageOptions options) { } } - // TODO: See if we can change gax to allow shifting this to callable.withContext so it doesn't - // have to be set globally - private static class ReadObjectResumptionStrategy - implements StreamResumptionStrategy { - private long readOffset = 0; - - @NonNull - @Override - public StreamResumptionStrategy createNew() { - return new ReadObjectResumptionStrategy(); - } - - @NonNull - @Override - public ReadObjectResponse processResponse(ReadObjectResponse response) { - readOffset += response.getChecksummedData().getContent().size(); - return response; - } - - @Nullable - @Override - public ReadObjectRequest getResumeRequest(ReadObjectRequest originalRequest) { - if (readOffset != 0) { - return originalRequest.toBuilder().setReadOffset(readOffset).build(); - } - return originalRequest; - } - - @Override - public boolean canResume() { - return true; - } - } - // setInternalHeaderProvider is protected so we need to open its scope in order to set it // we are adding an entry for gccl which is set via this provider private static final class GapicStorageSettingsBuilder extends StorageSettings.Builder { From ab66b77079c22b725e546ffd25dd3e6564f3ab8a Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Wed, 16 Oct 2024 10:51:06 -0700 Subject: [PATCH 5/7] clirr --- google-cloud-storage/clirr-ignored-differences.xml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/google-cloud-storage/clirr-ignored-differences.xml b/google-cloud-storage/clirr-ignored-differences.xml index d81c37bb24..03e37c174e 100644 --- a/google-cloud-storage/clirr-ignored-differences.xml +++ b/google-cloud-storage/clirr-ignored-differences.xml @@ -96,4 +96,17 @@ boolean equals(java.lang.Object) + + 7013 + com/google/cloud/storage/StorageOptions$Builder + com.google.cloud.storage.StorageOptions$Builder setOpenTelemetrySdk(io.opentelemetry.sdk.OpenTelemetrySdk) + + + + 7013 + com/google/cloud/storage/StorageOptions + io.opentelemetry.sdk.OpenTelemetrySdk getOpenTelemetrySdk() + + + From d159731c035cc01f5b3c14896856c5180055639e Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Wed, 16 Oct 2024 11:02:31 -0700 Subject: [PATCH 6/7] fix copyright --- .../storage/otel/NoOpOpenTelemetryInstance.java | 13 ++++++------- .../cloud/storage/otel/OpenTelemetryInstance.java | 13 ++++++------- ...elemetryTest.java => ITOpenTelemetryTest.java} | 15 +++++++-------- 3 files changed, 19 insertions(+), 22 deletions(-) rename google-cloud-storage/src/test/java/com/google/cloud/storage/{OpenTelemetryTest.java => ITOpenTelemetryTest.java} (88%) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java index 417bd21a27..814dbafdc4 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/otel/NoOpOpenTelemetryInstance.java @@ -5,14 +5,13 @@ * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * http://www.apache.org/licenses/LICENSE-2.0 * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ package com.google.cloud.storage.otel; 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 025f286704..4fd1f0b6ff 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 @@ -5,14 +5,13 @@ * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * http://www.apache.org/licenses/LICENSE-2.0 * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ package com.google.cloud.storage.otel; diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java similarity index 88% rename from google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java rename to google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java index fe1164f7f4..e07cf659cc 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/OpenTelemetryTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITOpenTelemetryTest.java @@ -5,14 +5,13 @@ * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * http://www.apache.org/licenses/LICENSE-2.0 * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. */ package com.google.cloud.storage; @@ -42,7 +41,7 @@ @CrossRun( backends = {Backend.PROD}, transports = {Transport.HTTP}) -public final class OpenTelemetryTest { +public final class ITOpenTelemetryTest { @Inject public Generator generator; @Test From 166ff4d2ecc7032b1a7ce32c4c67a8170ad35eb4 Mon Sep 17 00:00:00 2001 From: Sydney Munro Date: Wed, 16 Oct 2024 12:42:24 -0700 Subject: [PATCH 7/7] declare opentelemetry dependency --- google-cloud-storage/pom.xml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/google-cloud-storage/pom.xml b/google-cloud-storage/pom.xml index 2b3413e7f3..02ef4f2611 100644 --- a/google-cloud-storage/pom.xml +++ b/google-cloud-storage/pom.xml @@ -83,6 +83,10 @@ io.opencensus opencensus-api + + io.opentelemetry + opentelemetry-context + com.google.api.grpc proto-google-iam-v1 @@ -205,6 +209,11 @@ grpc-googleapis runtime + + io.opentelemetry + opentelemetry-sdk-trace + test +