From ff171d704cc7d59f18d95237ab3d82e1053471d7 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Fri, 3 May 2024 01:41:29 -0700 Subject: [PATCH 01/16] Adding OpenTelemetry API pom.xml dependency, DatastoreOptions API change and TraceUtil interface --- google-cloud-datastore/pom.xml | 8 + .../DatastoreOpenTelemetryOptions.java | 98 ++++++++++++ .../cloud/datastore/DatastoreOptions.java | 44 +++++ .../cloud/datastore/telemetry/TraceUtil.java | 151 ++++++++++++++++++ 4 files changed, 301 insertions(+) create mode 100644 google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java create mode 100644 google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java diff --git a/google-cloud-datastore/pom.xml b/google-cloud-datastore/pom.xml index 351e0dfae..998ca719d 100644 --- a/google-cloud-datastore/pom.xml +++ b/google-cloud-datastore/pom.xml @@ -111,6 +111,14 @@ jsr305 + + + io.opentelemetry + opentelemetry-api + 1.37.0 + + + ${project.groupId} diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java new file mode 100644 index 000000000..0827e6d5e --- /dev/null +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java @@ -0,0 +1,98 @@ +/* + * 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.datastore; + +import io.opentelemetry.api.OpenTelemetry; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +public class DatastoreOpenTelemetryOptions { + private final boolean enabled; + private final @Nullable OpenTelemetry openTelemetry; + + DatastoreOpenTelemetryOptions(Builder builder) { + this.enabled = builder.enabled; + this.openTelemetry = builder.openTelemetry; + } + + public boolean getEnabled() { + return enabled; + } + + @Nullable + public OpenTelemetry getOpenTelemetry() { + return openTelemetry; + } + + @Nonnull + public DatastoreOpenTelemetryOptions.Builder toBuilder() { + return new DatastoreOpenTelemetryOptions.Builder(this); + } + + @Nonnull + public static DatastoreOpenTelemetryOptions.Builder newBuilder() { + return new DatastoreOpenTelemetryOptions.Builder(); + } + + public static class Builder { + + private boolean enabled; + + @Nullable + private OpenTelemetry openTelemetry; + + private Builder() { + enabled = false; + openTelemetry = null; + } + + private Builder(DatastoreOpenTelemetryOptions options) { + this.enabled = options.enabled; + this.openTelemetry = options.openTelemetry; + } + + @Nonnull + public DatastoreOpenTelemetryOptions build() { + return new DatastoreOpenTelemetryOptions(this); + } + + /** + * Sets whether tracing should be enabled. + * + * @param enable Whether tracing should be enabled. + */ + @Nonnull + public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enable) { + this.enabled = enable; + return this; + } + + /** + * Sets the {@link OpenTelemetry} to use with this Firestore instance. If telemetry collection + * is enabled, but an `OpenTelemetry` is not provided, the Firestore SDK will attempt to use + * the `GlobalOpenTelemetry`. + * + * @param openTelemetry The OpenTelemetry that should be used by this Firestore instance. + */ + @Nonnull + public DatastoreOpenTelemetryOptions.Builder setOpenTelemetry( + @Nonnull OpenTelemetry openTelemetry) { + this.openTelemetry = openTelemetry; + return this; + } + } +} diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java index 8437c3e22..6cdf7afa3 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java @@ -18,6 +18,7 @@ import static com.google.cloud.datastore.Validator.validateNamespace; +import com.google.api.core.BetaApi; import com.google.cloud.ServiceDefaults; import com.google.cloud.ServiceOptions; import com.google.cloud.ServiceRpc; @@ -31,6 +32,8 @@ import java.lang.reflect.Method; import java.util.Objects; import java.util.Set; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; public class DatastoreOptions extends ServiceOptions { @@ -43,6 +46,9 @@ public class DatastoreOptions extends ServiceOptions { private String namespace; private String databaseId; + @Nullable + private DatastoreOpenTelemetryOptions openTelemetryOptions = null; + private Builder() {} private Builder(DatastoreOptions options) { super(options); namespace = options.namespace; databaseId = options.databaseId; + this.openTelemetryOptions = options.openTelemetryOptions; } @Override @@ -87,6 +108,9 @@ public Builder setTransportOptions(TransportOptions transportOptions) { @Override public DatastoreOptions build() { + if (this.openTelemetryOptions == null) { + this.setOpenTelemetryOptions(DatastoreOpenTelemetryOptions.newBuilder().build()); + } return new DatastoreOptions(this); } @@ -100,10 +124,30 @@ public Builder setDatabaseId(String databaseId) { this.databaseId = databaseId; return this; } + + /** + * Sets the {@link DatastoreOpenTelemetryOptions} to be used for this Firestore instance. + * + * @param openTelemetryOptions The `DatastoreOpenTelemetryOptions` to use. + */ + @BetaApi + @Nonnull + public Builder setOpenTelemetryOptions( + @Nonnull DatastoreOpenTelemetryOptions openTelemetryOptions) { + this.openTelemetryOptions = openTelemetryOptions; + return this; + } } private DatastoreOptions(Builder builder) { super(DatastoreFactory.class, DatastoreRpcFactory.class, builder, new DatastoreDefaults()); + + this.openTelemetryOptions = + builder.openTelemetryOptions != null + ? builder.openTelemetryOptions + : DatastoreOpenTelemetryOptions.newBuilder().build(); + this.traceUtil = com.google.cloud.datastore.telemetry.TraceUtil.getInstance(this); + namespace = MoreObjects.firstNonNull(builder.namespace, defaultNamespace()); databaseId = MoreObjects.firstNonNull(builder.databaseId, DEFAULT_DATABASE_ID); } diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java new file mode 100644 index 000000000..161f3675d --- /dev/null +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java @@ -0,0 +1,151 @@ +/* + * 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.datastore.telemetry; + +import com.google.api.core.ApiFunction; +import com.google.api.core.ApiFuture; +import com.google.cloud.datastore.DatastoreOptions; +import io.grpc.ManagedChannelBuilder; +import java.util.Map; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +public interface TraceUtil { + String ATTRIBUTE_SERVICE_PREFIX = "gcp.datastore."; + String SPAN_NAME_DOC_REF_CREATE = "DocumentReference.Create"; + String SPAN_NAME_DOC_REF_SET = "DocumentReference.Set"; + String SPAN_NAME_DOC_REF_UPDATE = "DocumentReference.Update"; + String SPAN_NAME_DOC_REF_DELETE = "DocumentReference.Delete"; + String SPAN_NAME_DOC_REF_GET = "DocumentReference.Get"; + String SPAN_NAME_DOC_REF_LIST_COLLECTIONS = "DocumentReference.ListCollections"; + String SPAN_NAME_COL_REF_ADD = "CollectionReference.Add"; + String SPAN_NAME_COL_REF_LIST_DOCUMENTS = "CollectionReference.ListDocuments"; + String SPAN_NAME_QUERY_GET = "Query.Get"; + String SPAN_NAME_AGGREGATION_QUERY_GET = "AggregationQuery.Get"; + String SPAN_NAME_RUN_QUERY = "RunQuery"; + String SPAN_NAME_RUN_AGGREGATION_QUERY = "RunAggregationQuery"; + String SPAN_NAME_BATCH_GET_DOCUMENTS = "BatchGetDocuments"; + String SPAN_NAME_TRANSACTION_RUN = "Transaction.Run"; + String SPAN_NAME_TRANSACTION_BEGIN = "Transaction.Begin"; + String SPAN_NAME_TRANSACTION_GET_QUERY = "Transaction.Get.Query"; + String SPAN_NAME_TRANSACTION_GET_AGGREGATION_QUERY = "Transaction.Get.AggregationQuery"; + String SPAN_NAME_TRANSACTION_GET_DOCUMENT = "Transaction.Get.Document"; + String SPAN_NAME_TRANSACTION_GET_DOCUMENTS = "Transaction.Get.Documents"; + String SPAN_NAME_TRANSACTION_ROLLBACK = "Transaction.Rollback"; + String SPAN_NAME_BATCH_COMMIT = "Batch.Commit"; + String SPAN_NAME_TRANSACTION_COMMIT = "Transaction.Commit"; + String SPAN_NAME_PARTITION_QUERY = "PartitionQuery"; + String SPAN_NAME_BULK_WRITER_COMMIT = "BulkWriter.Commit"; + + String ENABLE_TRACING_ENV_VAR = "DATASTORE_ENABLE_TRACING"; + String LIBRARY_NAME = "com.google.cloud.datastore"; + + /** + * Creates and returns an instance of the TraceUtil class. + * + * @param datastoreOptions The DatastoreOptions object that is requesting an instance of + * TraceUtil. + * @return An instance of the TraceUtil class. + */ + static TraceUtil getInstance(@Nonnull DatastoreOptions datastoreOptions) { + boolean createEnabledInstance = datastoreOptions.getOpenTelemetryOptions().getEnabled(); + + // The environment variable can override options to enable/disable telemetry collection. + String enableTracingEnvVar = System.getenv(ENABLE_TRACING_ENV_VAR); + if (enableTracingEnvVar != null) { + if (enableTracingEnvVar.equalsIgnoreCase("true") + || enableTracingEnvVar.equalsIgnoreCase("on")) { + createEnabledInstance = true; + } + if (enableTracingEnvVar.equalsIgnoreCase("false") + || enableTracingEnvVar.equalsIgnoreCase("off")) { + createEnabledInstance = false; + } + } + + if (createEnabledInstance) { + return new EnabledTraceUtil(datastoreOptions); + } else { + return new DisabledTraceUtil(); + } + } + + /** Returns a channel configurator for gRPC, or {@code null} if tracing is disabled. */ + @Nullable + ApiFunction getChannelConfigurator(); + + /** Represents a trace span. */ + interface Span { + /** 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); + + /** Adds the given attribute to this span. */ + Span setAttribute(String key, int value); + + /** Adds the given attribute to this span. */ + Span setAttribute(String key, String value); + + /** 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(); +} From 3f1dc1b820e2bd53e02dc62b6607f339bd85f2b3 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Fri, 3 May 2024 01:42:54 -0700 Subject: [PATCH 02/16] Adding EnabledTraceUtil, DisabledTraceUtil and TraceUtilTest - Annotating DatastoreOpenTelemetryOptions to be transient as they're not serializable --- google-cloud-datastore/pom.xml | 40 +++ .../cloud/datastore/DatastoreOptions.java | 4 +- .../telemetry/DisabledTraceUtil.java | 103 ++++++ .../datastore/telemetry/EnabledTraceUtil.java | 301 ++++++++++++++++++ .../cloud/datastore/telemetry/TraceUtil.java | 25 -- .../telemetry/DisabledTraceUtilTest.java | 53 +++ .../telemetry/EnabledTraceUtilTest.java | 101 ++++++ .../datastore/telemetry/TraceUtilTest.java | 61 ++++ 8 files changed, 661 insertions(+), 27 deletions(-) create mode 100644 google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/DisabledTraceUtil.java create mode 100644 google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java create mode 100644 google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/DisabledTraceUtilTest.java create mode 100644 google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java create mode 100644 google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java diff --git a/google-cloud-datastore/pom.xml b/google-cloud-datastore/pom.xml index 998ca719d..5bbcbf4fe 100644 --- a/google-cloud-datastore/pom.xml +++ b/google-cloud-datastore/pom.xml @@ -168,6 +168,46 @@ 1.4.2 test + + + io.opentelemetry + opentelemetry-sdk + 1.37.0 + test + + + io.opentelemetry + opentelemetry-sdk-testing + 1.37.0 + test + + + io.opentelemetry + opentelemetry-sdk-trace + 1.37.0 + test + + + io.opentelemetry + opentelemetry-sdk-common + 1.37.0 + test + + + + + com.google.api.grpc + proto-google-cloud-trace-v1 + 1.3.0 + test + + + com.google.cloud.opentelemetry + exporter-trace + 0.15.0 + test + + diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java index 6cdf7afa3..cbe0bc894 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java @@ -46,8 +46,8 @@ public class DatastoreOptions extends ServiceOptions void endAtFuture(ApiFuture futureValue) {} + + @Override + public TraceUtil.Span addEvent(String name) { + return this; + } + + @Override + public TraceUtil.Span addEvent(String name, Map attributes) { + return this; + } + + @Override + public TraceUtil.Span setAttribute(String key, int value) { + return this; + } + + @Override + public TraceUtil.Span setAttribute(String key, String value) { + return this; + } + + @Override + public Scope makeCurrent() { + return new Scope(); + } + } + + static class Context implements TraceUtil.Context { + @Override + public Scope makeCurrent() { + return new Scope(); + } + } + + static class Scope implements TraceUtil.Scope { + @Override + public void close() {} + } + + @Nullable + @Override + public ApiFunction getChannelConfigurator() { + return null; + } + + @Override + public Span startSpan(String spanName) { + return new Span(); + } + + @Override + public TraceUtil.Span startSpan(String spanName, TraceUtil.Context parent) { + return new Span(); + } + + @Nonnull + @Override + public TraceUtil.Span currentSpan() { + return new Span(); + } + + @Nonnull + @Override + public TraceUtil.Context currentContext() { + return new Context(); + } +} diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java new file mode 100644 index 000000000..dcd238b77 --- /dev/null +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java @@ -0,0 +1,301 @@ +/* + * 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.datastore.telemetry; + +import com.google.api.core.ApiFunction; +import com.google.api.core.ApiFuture; +import com.google.api.core.ApiFutureCallback; +import com.google.api.core.ApiFutures; +import com.google.cloud.datastore.DatastoreOptions; +import com.google.common.base.Throwables; +import io.grpc.ManagedChannelBuilder; +import io.opentelemetry.api.GlobalOpenTelemetry; +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; +import javax.annotation.Nullable; + +public class EnabledTraceUtil implements TraceUtil { + private final Tracer tracer; + private final OpenTelemetry openTelemetry; + private final DatastoreOptions datastoreOptions; + + EnabledTraceUtil(DatastoreOptions datastoreOptions) { + OpenTelemetry openTelemetry = datastoreOptions.getOpenTelemetryOptions().getOpenTelemetry(); + + // If tracing is enabled, but an OpenTelemetry instance is not provided, fall back + // to using GlobalOpenTelemetry. + if (openTelemetry == null) { + openTelemetry = GlobalOpenTelemetry.get(); + } + + this.datastoreOptions = datastoreOptions; + this.openTelemetry = openTelemetry; + this.tracer = openTelemetry.getTracer(LIBRARY_NAME); + } + + public OpenTelemetry getOpenTelemetry() { + return openTelemetry; + } + + @Override + @Nullable + public ApiFunction getChannelConfigurator() { + // TODO(jimit) Update this to return a gRPC Channel Configurator after gRPC upgrade. + return null; + } + + static class Span implements TraceUtil.Span { + private final io.opentelemetry.api.trace.Span span; + private final String spanName; + + public Span(io.opentelemetry.api.trace.Span span, String spanName) { + this.span = span; + this.spanName = spanName; + } + + /** Ends this span. */ + @Override + public void end() { + span.end(); + } + + /** Ends this span in an error. */ + @Override + public void end(Throwable error) { + span.setStatus(StatusCode.ERROR, error.getMessage()); + span.recordException( + error, + Attributes.builder() + .put("exception.message", error.getMessage()) + .put("exception.type", error.getClass().getName()) + .put("exception.stacktrace", Throwables.getStackTraceAsString(error)) + .build()); + span.end(); + } + + /** + * 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. + */ + @Override + public void endAtFuture(ApiFuture futureValue) { + io.opentelemetry.context.Context asyncContext = io.opentelemetry.context.Context.current(); + ApiFutures.addCallback( + futureValue, + new ApiFutureCallback() { + @Override + public void onFailure(Throwable t) { + try (io.opentelemetry.context.Scope scope = asyncContext.makeCurrent()) { + span.addEvent(spanName + " failed."); + end(t); + } + } + + @Override + public void onSuccess(T result) { + try (io.opentelemetry.context.Scope scope = asyncContext.makeCurrent()) { + span.addEvent(spanName + " succeeded."); + end(); + } + } + }); + } + + /** Adds the given event to this span. */ + @Override + public TraceUtil.Span addEvent(String name) { + span.addEvent(name); + return this; + } + + @Override + public TraceUtil.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 TraceUtil.Span setAttribute(String key, int value) { + span.setAttribute(ATTRIBUTE_SERVICE_PREFIX + key, value); + return this; + } + + @Override + public TraceUtil.Span setAttribute(String key, String value) { + span.setAttribute(ATTRIBUTE_SERVICE_PREFIX + key, value); + return this; + } + + @Override + public Scope makeCurrent() { + try (io.opentelemetry.context.Scope scope = span.makeCurrent()) { + return new Scope(scope); + } + } + } + + static class Scope implements TraceUtil.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 TraceUtil.Context { + private final io.opentelemetry.context.Context context; + + Context(io.opentelemetry.context.Context context) { + this.context = context; + } + + @Override + public Scope makeCurrent() { + try(io.opentelemetry.context.Scope scope = context.makeCurrent()) { + return new Scope(scope); + } + } + } + + /** Applies the current Firestore instance settings as attributes to the current Span */ + private SpanBuilder addSettingsAttributesToCurrentSpan(SpanBuilder spanBuilder) { + spanBuilder = + spanBuilder.setAllAttributes( + Attributes.builder() + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.databaseId", + datastoreOptions.getDatabaseId()) + .put(ATTRIBUTE_SERVICE_PREFIX + "settings.host", datastoreOptions.getHost()) + .build()); + + if (datastoreOptions.getCredentials() != null) { + spanBuilder = + spanBuilder.setAttribute( + ATTRIBUTE_SERVICE_PREFIX + "settings.credentials.authenticationType", + datastoreOptions.getCredentials().getAuthenticationType()); + } + + if (datastoreOptions.getRetrySettings() != null) { + spanBuilder = + spanBuilder.setAllAttributes( + Attributes.builder() + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.initialRetryDelay", + datastoreOptions.getRetrySettings().getInitialRetryDelay().toString()) + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.maxRetryDelay", + datastoreOptions.getRetrySettings().getMaxRetryDelay().toString()) + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.retryDelayMultiplier", + String.valueOf(datastoreOptions.getRetrySettings().getRetryDelayMultiplier())) + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.maxAttempts", + String.valueOf(datastoreOptions.getRetrySettings().getMaxAttempts())) + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.initialRpcTimeout", + datastoreOptions.getRetrySettings().getInitialRpcTimeout().toString()) + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.maxRpcTimeout", + datastoreOptions.getRetrySettings().getMaxRpcTimeout().toString()) + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.rpcTimeoutMultiplier", + String.valueOf(datastoreOptions.getRetrySettings().getRpcTimeoutMultiplier())) + .put( + ATTRIBUTE_SERVICE_PREFIX + "settings.retrySettings.totalTimeout", + datastoreOptions.getRetrySettings().getTotalTimeout().toString()) + .build()); + } + + // Add the memory utilization of the client at the time this trace was collected. + long totalMemory = Runtime.getRuntime().totalMemory(); + long freeMemory = Runtime.getRuntime().freeMemory(); + double memoryUtilization = ((double) (totalMemory - freeMemory)) / totalMemory; + spanBuilder.setAttribute( + ATTRIBUTE_SERVICE_PREFIX + "memoryUtilization", + String.format("%.2f", memoryUtilization * 100) + "%"); + + return spanBuilder; + } + + @Override + public Span startSpan(String spanName) { + SpanBuilder spanBuilder = tracer.spanBuilder(spanName).setSpanKind(SpanKind.PRODUCER); + io.opentelemetry.api.trace.Span span = + addSettingsAttributesToCurrentSpan(spanBuilder).startSpan(); + return new Span(span, spanName); + } + + @Override + public TraceUtil.Span startSpan(String spanName, TraceUtil.Context parent) { + assert (parent instanceof Context); + SpanBuilder spanBuilder = + tracer + .spanBuilder(spanName) + .setSpanKind(SpanKind.PRODUCER) + .setParent(((Context) parent).context); + io.opentelemetry.api.trace.Span span = + addSettingsAttributesToCurrentSpan(spanBuilder).startSpan(); + return new Span(span, spanName); + } + + @Nonnull + @Override + public TraceUtil.Span currentSpan() { + return new Span(io.opentelemetry.api.trace.Span.current(), ""); + } + + @Nonnull + @Override + public TraceUtil.Context currentContext() { + return new Context(io.opentelemetry.context.Context.current()); + } +} diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java index 161f3675d..aec51dbc1 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java @@ -26,31 +26,6 @@ public interface TraceUtil { String ATTRIBUTE_SERVICE_PREFIX = "gcp.datastore."; - String SPAN_NAME_DOC_REF_CREATE = "DocumentReference.Create"; - String SPAN_NAME_DOC_REF_SET = "DocumentReference.Set"; - String SPAN_NAME_DOC_REF_UPDATE = "DocumentReference.Update"; - String SPAN_NAME_DOC_REF_DELETE = "DocumentReference.Delete"; - String SPAN_NAME_DOC_REF_GET = "DocumentReference.Get"; - String SPAN_NAME_DOC_REF_LIST_COLLECTIONS = "DocumentReference.ListCollections"; - String SPAN_NAME_COL_REF_ADD = "CollectionReference.Add"; - String SPAN_NAME_COL_REF_LIST_DOCUMENTS = "CollectionReference.ListDocuments"; - String SPAN_NAME_QUERY_GET = "Query.Get"; - String SPAN_NAME_AGGREGATION_QUERY_GET = "AggregationQuery.Get"; - String SPAN_NAME_RUN_QUERY = "RunQuery"; - String SPAN_NAME_RUN_AGGREGATION_QUERY = "RunAggregationQuery"; - String SPAN_NAME_BATCH_GET_DOCUMENTS = "BatchGetDocuments"; - String SPAN_NAME_TRANSACTION_RUN = "Transaction.Run"; - String SPAN_NAME_TRANSACTION_BEGIN = "Transaction.Begin"; - String SPAN_NAME_TRANSACTION_GET_QUERY = "Transaction.Get.Query"; - String SPAN_NAME_TRANSACTION_GET_AGGREGATION_QUERY = "Transaction.Get.AggregationQuery"; - String SPAN_NAME_TRANSACTION_GET_DOCUMENT = "Transaction.Get.Document"; - String SPAN_NAME_TRANSACTION_GET_DOCUMENTS = "Transaction.Get.Documents"; - String SPAN_NAME_TRANSACTION_ROLLBACK = "Transaction.Rollback"; - String SPAN_NAME_BATCH_COMMIT = "Batch.Commit"; - String SPAN_NAME_TRANSACTION_COMMIT = "Transaction.Commit"; - String SPAN_NAME_PARTITION_QUERY = "PartitionQuery"; - String SPAN_NAME_BULK_WRITER_COMMIT = "BulkWriter.Commit"; - String ENABLE_TRACING_ENV_VAR = "DATASTORE_ENABLE_TRACING"; String LIBRARY_NAME = "com.google.cloud.datastore"; diff --git a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/DisabledTraceUtilTest.java b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/DisabledTraceUtilTest.java new file mode 100644 index 000000000..0f3f183cd --- /dev/null +++ b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/DisabledTraceUtilTest.java @@ -0,0 +1,53 @@ +/* + * 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.datastore.telemetry; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Test; + +public class DisabledTraceUtilTest { + @Test + public void disabledTraceUtilDoesNotProvideChannelConfigurator() { + DisabledTraceUtil traceUtil = new DisabledTraceUtil(); + assertThat(traceUtil.getChannelConfigurator()).isNull(); + } + + @Test + public void usesDisabledContext() { + DisabledTraceUtil traceUtil = new DisabledTraceUtil(); + assertThat(traceUtil.currentContext() instanceof DisabledTraceUtil.Context).isTrue(); + } + + @Test + public void usesDisabledSpan() { + DisabledTraceUtil traceUtil = new DisabledTraceUtil(); + assertThat(traceUtil.currentSpan() instanceof DisabledTraceUtil.Span).isTrue(); + assertThat(traceUtil.startSpan("foo") instanceof DisabledTraceUtil.Span).isTrue(); + assertThat( + traceUtil.startSpan("foo", traceUtil.currentContext()) + instanceof DisabledTraceUtil.Span) + .isTrue(); + } + + @Test + public void usesDisabledScope() { + DisabledTraceUtil traceUtil = new DisabledTraceUtil(); + assertThat(traceUtil.currentContext().makeCurrent() instanceof DisabledTraceUtil.Scope) + .isTrue(); + assertThat(traceUtil.currentSpan().makeCurrent() instanceof DisabledTraceUtil.Scope).isTrue(); + } +} diff --git a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java new file mode 100644 index 000000000..2331047ab --- /dev/null +++ b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java @@ -0,0 +1,101 @@ +/* + * 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.datastore.telemetry; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.cloud.datastore.DatastoreOpenTelemetryOptions; +import com.google.cloud.datastore.DatastoreOptions; +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.sdk.OpenTelemetrySdk; +import org.junit.Before; +import org.junit.Test; + +public class EnabledTraceUtilTest { + @Before + public void setUp() { + GlobalOpenTelemetry.resetForTest(); + } + + DatastoreOptions.Builder getBaseOptions() { + return DatastoreOptions.newBuilder().setProjectId("test-project").setDatabaseId("(default)"); + } + + DatastoreOptions getTracingEnabledOptions() { + return getBaseOptions() + .setOpenTelemetryOptions( + DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build()) + .build(); + } + + EnabledTraceUtil newEnabledTraceUtil() { + return new EnabledTraceUtil(getTracingEnabledOptions()); + } + + @Test + public void usesOpenTelemetryFromOptions() { + OpenTelemetrySdk myOpenTelemetrySdk = OpenTelemetrySdk.builder().build(); + DatastoreOptions firestoreOptions = + getBaseOptions() + .setOpenTelemetryOptions( + DatastoreOpenTelemetryOptions.newBuilder() + .setTracingEnabled(true) + .setOpenTelemetry(myOpenTelemetrySdk) + .build()) + .build(); + EnabledTraceUtil traceUtil = new EnabledTraceUtil(firestoreOptions); + assertThat(traceUtil.getOpenTelemetry()).isEqualTo(myOpenTelemetrySdk); + } + + @Test + public void usesGlobalOpenTelemetryIfOpenTelemetryInstanceNotProvided() { + OpenTelemetrySdk globalOpenTelemetrySdk = OpenTelemetrySdk.builder().buildAndRegisterGlobal(); + DatastoreOptions firestoreOptions = + getBaseOptions() + .setOpenTelemetryOptions( + DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build()) + .build(); + EnabledTraceUtil traceUtil = new EnabledTraceUtil(firestoreOptions); + assertThat(traceUtil.getOpenTelemetry()).isEqualTo(GlobalOpenTelemetry.get()); + } + + @Test + public void enabledTraceUtilProvidesChannelConfigurator() { + assertThat(newEnabledTraceUtil().getChannelConfigurator()).isNull(); + } + + @Test + public void usesEnabledContext() { + assertThat(newEnabledTraceUtil().currentContext() instanceof EnabledTraceUtil.Context).isTrue(); + } + + @Test + public void usesEnabledSpan() { + EnabledTraceUtil traceUtil = newEnabledTraceUtil(); + assertThat(traceUtil.currentSpan() instanceof EnabledTraceUtil.Span).isTrue(); + assertThat(traceUtil.startSpan("foo") != null).isTrue(); + assertThat( + traceUtil.startSpan("foo", traceUtil.currentContext()) instanceof EnabledTraceUtil.Span) + .isTrue(); + } + + @Test + public void usesEnabledScope() { + EnabledTraceUtil traceUtil = newEnabledTraceUtil(); + assertThat(traceUtil.currentContext().makeCurrent() instanceof EnabledTraceUtil.Scope).isTrue(); + assertThat(traceUtil.currentSpan().makeCurrent() instanceof EnabledTraceUtil.Scope).isTrue(); + } +} diff --git a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java new file mode 100644 index 000000000..469dc3b39 --- /dev/null +++ b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java @@ -0,0 +1,61 @@ +/* + * 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.datastore.telemetry; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.cloud.datastore.DatastoreOpenTelemetryOptions; +import com.google.cloud.datastore.DatastoreOptions; +import org.junit.Test; + +public class TraceUtilTest { + @Test + public void defaultOptionsUseDisabledTraceUtil() { + TraceUtil traceUtil = + TraceUtil.getInstance( + DatastoreOptions.newBuilder() + .setProjectId("test-project") + .setDatabaseId("(default)") + .build()); + assertThat(traceUtil instanceof DisabledTraceUtil).isTrue(); + } + + @Test + public void tracingDisabledOptionsUseDisabledTraceUtil() { + TraceUtil traceUtil = + TraceUtil.getInstance( + DatastoreOptions.newBuilder() + .setProjectId("test-project") + .setDatabaseId("(default)") + .setOpenTelemetryOptions( + DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(false).build()) + .build()); + assertThat(traceUtil instanceof DisabledTraceUtil).isTrue(); + } + + @Test + public void tracingEnabledOptionsUseEnabledTraceUtil() { + TraceUtil traceUtil = + TraceUtil.getInstance( + DatastoreOptions.newBuilder() + .setProjectId("test-project") + .setDatabaseId("(default)") + .setOpenTelemetryOptions( + DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build()) + .build()); + assertThat(traceUtil instanceof EnabledTraceUtil).isTrue(); + } +} From 1639b920c775655a0e1ecb526fdb2b93b4abd8fa Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Fri, 3 May 2024 13:21:41 -0700 Subject: [PATCH 03/16] Fixing unused dependencies --- google-cloud-datastore/pom.xml | 39 +++++----------------------------- 1 file changed, 5 insertions(+), 34 deletions(-) diff --git a/google-cloud-datastore/pom.xml b/google-cloud-datastore/pom.xml index 5bbcbf4fe..9dc10aede 100644 --- a/google-cloud-datastore/pom.xml +++ b/google-cloud-datastore/pom.xml @@ -117,6 +117,11 @@ opentelemetry-api 1.37.0 + + io.opentelemetry + opentelemetry-context + 1.37.0 + @@ -168,46 +173,12 @@ 1.4.2 test - io.opentelemetry opentelemetry-sdk 1.37.0 test - - io.opentelemetry - opentelemetry-sdk-testing - 1.37.0 - test - - - io.opentelemetry - opentelemetry-sdk-trace - 1.37.0 - test - - - io.opentelemetry - opentelemetry-sdk-common - 1.37.0 - test - - - - - com.google.api.grpc - proto-google-cloud-trace-v1 - 1.3.0 - test - - - com.google.cloud.opentelemetry - exporter-trace - 0.15.0 - test - - From 26228103e884be6d9875d2c9a22262bbe479952b Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Fri, 3 May 2024 13:30:57 -0700 Subject: [PATCH 04/16] Fixing linter issues --- .../DatastoreOpenTelemetryOptions.java | 119 +++++++++--------- .../cloud/datastore/DatastoreOptions.java | 3 +- .../datastore/telemetry/EnabledTraceUtil.java | 2 +- 3 files changed, 61 insertions(+), 63 deletions(-) diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java index 0827e6d5e..957af51a0 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java @@ -21,78 +21,77 @@ import javax.annotation.Nullable; public class DatastoreOpenTelemetryOptions { - private final boolean enabled; - private final @Nullable OpenTelemetry openTelemetry; + private final boolean enabled; + private final @Nullable OpenTelemetry openTelemetry; DatastoreOpenTelemetryOptions(Builder builder) { - this.enabled = builder.enabled; - this.openTelemetry = builder.openTelemetry; - } + this.enabled = builder.enabled; + this.openTelemetry = builder.openTelemetry; + } + + public boolean getEnabled() { + return enabled; + } + + @Nullable + public OpenTelemetry getOpenTelemetry() { + return openTelemetry; + } + + @Nonnull + public DatastoreOpenTelemetryOptions.Builder toBuilder() { + return new DatastoreOpenTelemetryOptions.Builder(this); + } + + @Nonnull + public static DatastoreOpenTelemetryOptions.Builder newBuilder() { + return new DatastoreOpenTelemetryOptions.Builder(); + } - public boolean getEnabled() { - return enabled; + public static class Builder { + + private boolean enabled; + + @Nullable private OpenTelemetry openTelemetry; + + private Builder() { + enabled = false; + openTelemetry = null; } - @Nullable - public OpenTelemetry getOpenTelemetry() { - return openTelemetry; + private Builder(DatastoreOpenTelemetryOptions options) { + this.enabled = options.enabled; + this.openTelemetry = options.openTelemetry; } @Nonnull - public DatastoreOpenTelemetryOptions.Builder toBuilder() { - return new DatastoreOpenTelemetryOptions.Builder(this); + public DatastoreOpenTelemetryOptions build() { + return new DatastoreOpenTelemetryOptions(this); } + /** + * Sets whether tracing should be enabled. + * + * @param enable Whether tracing should be enabled. + */ @Nonnull - public static DatastoreOpenTelemetryOptions.Builder newBuilder() { - return new DatastoreOpenTelemetryOptions.Builder(); + public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enable) { + this.enabled = enable; + return this; } - public static class Builder { - - private boolean enabled; - - @Nullable - private OpenTelemetry openTelemetry; - - private Builder() { - enabled = false; - openTelemetry = null; - } - - private Builder(DatastoreOpenTelemetryOptions options) { - this.enabled = options.enabled; - this.openTelemetry = options.openTelemetry; - } - - @Nonnull - public DatastoreOpenTelemetryOptions build() { - return new DatastoreOpenTelemetryOptions(this); - } - - /** - * Sets whether tracing should be enabled. - * - * @param enable Whether tracing should be enabled. - */ - @Nonnull - public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enable) { - this.enabled = enable; - return this; - } - - /** - * Sets the {@link OpenTelemetry} to use with this Firestore instance. If telemetry collection - * is enabled, but an `OpenTelemetry` is not provided, the Firestore SDK will attempt to use - * the `GlobalOpenTelemetry`. - * - * @param openTelemetry The OpenTelemetry that should be used by this Firestore instance. - */ - @Nonnull - public DatastoreOpenTelemetryOptions.Builder setOpenTelemetry( - @Nonnull OpenTelemetry openTelemetry) { - this.openTelemetry = openTelemetry; - return this; - } + /** + * Sets the {@link OpenTelemetry} to use with this Firestore instance. If telemetry collection + * is enabled, but an `OpenTelemetry` is not provided, the Firestore SDK will attempt to use the + * `GlobalOpenTelemetry`. + * + * @param openTelemetry The OpenTelemetry that should be used by this Firestore instance. + */ + @Nonnull + public DatastoreOpenTelemetryOptions.Builder setOpenTelemetry( + @Nonnull OpenTelemetry openTelemetry) { + this.openTelemetry = openTelemetry; + return this; } + } } diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java index cbe0bc894..4f7837507 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java @@ -85,8 +85,7 @@ public static class Builder extends ServiceOptions.Builder Date: Fri, 3 May 2024 13:32:12 -0700 Subject: [PATCH 05/16] Adding google-auth-library-credentials dependency due to https://github.com/googleapis/java-datastore/actions/runs/8944472794/job/24571458116?pr=1431 --- google-cloud-datastore/pom.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/google-cloud-datastore/pom.xml b/google-cloud-datastore/pom.xml index 9dc10aede..d1a0c85e1 100644 --- a/google-cloud-datastore/pom.xml +++ b/google-cloud-datastore/pom.xml @@ -38,6 +38,10 @@ com.google.cloud.datastore datastore-v1-proto-client + + com.google.auth + google-auth-library-credentials + io.grpc grpc-api From f9e0c112f9d6a5865edb0b6e86e0ada1d992f69d Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Mon, 6 May 2024 14:57:05 -0700 Subject: [PATCH 06/16] Code review comments --- .../cloud/datastore/DatastoreOpenTelemetryOptions.java | 10 +++++----- .../google/cloud/datastore/telemetry/TraceUtil.java | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java index 957af51a0..1fa3dcb90 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java @@ -25,7 +25,7 @@ public class DatastoreOpenTelemetryOptions { private final @Nullable OpenTelemetry openTelemetry; DatastoreOpenTelemetryOptions(Builder builder) { - this.enabled = builder.enabled; + this.enabled = builder.isEnabled; this.openTelemetry = builder.openTelemetry; } @@ -50,17 +50,17 @@ public static DatastoreOpenTelemetryOptions.Builder newBuilder() { public static class Builder { - private boolean enabled; + private boolean isEnabled; @Nullable private OpenTelemetry openTelemetry; private Builder() { - enabled = false; + isEnabled = false; openTelemetry = null; } private Builder(DatastoreOpenTelemetryOptions options) { - this.enabled = options.enabled; + this.isEnabled = options.enabled; this.openTelemetry = options.openTelemetry; } @@ -76,7 +76,7 @@ public DatastoreOpenTelemetryOptions build() { */ @Nonnull public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enable) { - this.enabled = enable; + this.isEnabled = enable; return this; } diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java index aec51dbc1..c87960b58 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java @@ -25,9 +25,9 @@ import javax.annotation.Nullable; public interface TraceUtil { - String ATTRIBUTE_SERVICE_PREFIX = "gcp.datastore."; - String ENABLE_TRACING_ENV_VAR = "DATASTORE_ENABLE_TRACING"; - String LIBRARY_NAME = "com.google.cloud.datastore"; + static final String ATTRIBUTE_SERVICE_PREFIX = "gcp.datastore."; + static final String ENABLE_TRACING_ENV_VAR = "DATASTORE_ENABLE_TRACING"; + static final String LIBRARY_NAME = "com.google.cloud.datastore"; /** * Creates and returns an instance of the TraceUtil class. From cd8ed6a4a91513bf11c066569e57bce5ede08530 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 10:11:46 -0700 Subject: [PATCH 07/16] Code review: fix pom to derive dependency from shared dependency --- google-cloud-datastore/pom.xml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/google-cloud-datastore/pom.xml b/google-cloud-datastore/pom.xml index d1a0c85e1..103813bfb 100644 --- a/google-cloud-datastore/pom.xml +++ b/google-cloud-datastore/pom.xml @@ -16,6 +16,7 @@ google-cloud-datastore + 1.37.0 @@ -119,12 +120,12 @@ io.opentelemetry opentelemetry-api - 1.37.0 + ${opentelemetry.version} io.opentelemetry opentelemetry-context - 1.37.0 + ${opentelemetry.version} @@ -180,7 +181,7 @@ io.opentelemetry opentelemetry-sdk - 1.37.0 + ${opentelemetry.version} test From 0b682ef6f52b5d4060d96d6f86fa80f40c4415e4 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 10:13:49 -0700 Subject: [PATCH 08/16] Code review: s/Firestore/Datastore --- .../cloud/datastore/DatastoreOpenTelemetryOptions.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java index 1fa3dcb90..778ff5877 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java @@ -81,11 +81,11 @@ public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enable) { } /** - * Sets the {@link OpenTelemetry} to use with this Firestore instance. If telemetry collection - * is enabled, but an `OpenTelemetry` is not provided, the Firestore SDK will attempt to use the + * Sets the {@link OpenTelemetry} to use with this Datastore instance. If telemetry collection + * is enabled, but an `OpenTelemetry` is not provided, the Datastore SDK will attempt to use the * `GlobalOpenTelemetry`. * - * @param openTelemetry The OpenTelemetry that should be used by this Firestore instance. + * @param openTelemetry The OpenTelemetry that should be used by this Datastore instance. */ @Nonnull public DatastoreOpenTelemetryOptions.Builder setOpenTelemetry( From 376d15c858d40dfb187c2535fa15e5ff3d1a3026 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 10:19:27 -0700 Subject: [PATCH 09/16] Code review: remove redundant opentelemetryOptions initialization --- .../main/java/com/google/cloud/datastore/DatastoreOptions.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java index 4f7837507..cef40eedd 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOptions.java @@ -107,9 +107,6 @@ public Builder setTransportOptions(TransportOptions transportOptions) { @Override public DatastoreOptions build() { - if (this.openTelemetryOptions == null) { - this.setOpenTelemetryOptions(DatastoreOpenTelemetryOptions.newBuilder().build()); - } return new DatastoreOptions(this); } From 5c2c2631b6b0e70435d28fc2b29588cb75e5b2af Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 10:27:29 -0700 Subject: [PATCH 10/16] Code review: block comments and API use annotation --- .../google/cloud/datastore/telemetry/DisabledTraceUtil.java | 6 ++++++ .../google/cloud/datastore/telemetry/EnabledTraceUtil.java | 5 +++++ .../com/google/cloud/datastore/telemetry/TraceUtil.java | 3 +++ 3 files changed, 14 insertions(+) diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/DisabledTraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/DisabledTraceUtil.java index 1accf3963..21321897d 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/DisabledTraceUtil.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/DisabledTraceUtil.java @@ -18,11 +18,17 @@ import com.google.api.core.ApiFunction; import com.google.api.core.ApiFuture; +import com.google.api.core.InternalApi; import io.grpc.ManagedChannelBuilder; import java.util.Map; import javax.annotation.Nonnull; import javax.annotation.Nullable; +/** + * Tracing utility implementation, used to stub out tracing instrumentation when tracing is + * disabled. + */ +@InternalApi public class DisabledTraceUtil implements TraceUtil { static class Span implements TraceUtil.Span { diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java index 2193ef3bd..c0541fe06 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java @@ -20,6 +20,7 @@ import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutureCallback; import com.google.api.core.ApiFutures; +import com.google.api.core.InternalApi; import com.google.cloud.datastore.DatastoreOptions; import com.google.common.base.Throwables; import io.grpc.ManagedChannelBuilder; @@ -35,6 +36,10 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +/** + * Tracing utility implementation, used to stub out tracing instrumentation when tracing is enabled. + */ +@InternalApi public class EnabledTraceUtil implements TraceUtil { private final Tracer tracer; private final OpenTelemetry openTelemetry; diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java index c87960b58..c477edc44 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java @@ -18,12 +18,15 @@ import com.google.api.core.ApiFunction; import com.google.api.core.ApiFuture; +import com.google.api.core.InternalExtensionOnly; import com.google.cloud.datastore.DatastoreOptions; import io.grpc.ManagedChannelBuilder; import java.util.Map; import javax.annotation.Nonnull; import javax.annotation.Nullable; +/** Utility interface to manage OpenTelemetry tracing instrumentation based on the configuration. */ +@InternalExtensionOnly public interface TraceUtil { static final String ATTRIBUTE_SERVICE_PREFIX = "gcp.datastore."; static final String ENABLE_TRACING_ENV_VAR = "DATASTORE_ENABLE_TRACING"; From b05645d68149c33232f2ccf6225e59e5ef197d31 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 11:11:19 -0700 Subject: [PATCH 11/16] Code review: s/Firestore/Datastore --- .../com/google/cloud/datastore/telemetry/EnabledTraceUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java index c0541fe06..8a5b0b36e 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/EnabledTraceUtil.java @@ -211,7 +211,7 @@ public Scope makeCurrent() { } } - /** Applies the current Firestore instance settings as attributes to the current Span */ + /** Applies the current Datastore instance settings as attributes to the current Span */ private SpanBuilder addSettingsAttributesToCurrentSpan(SpanBuilder spanBuilder) { spanBuilder = spanBuilder.setAllAttributes( From 59dc0ce5dfb6ad99f0c7ad8cbb48b0c74dff544f Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 11:32:07 -0700 Subject: [PATCH 12/16] Code review: Test changes --- .../cloud/datastore/telemetry/EnabledTraceUtilTest.java | 5 ++++- .../google/cloud/datastore/telemetry/TraceUtilTest.java | 7 ++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java index 2331047ab..e88e1a849 100644 --- a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java +++ b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/EnabledTraceUtilTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.cloud.NoCredentials; import com.google.cloud.datastore.DatastoreOpenTelemetryOptions; import com.google.cloud.datastore.DatastoreOptions; import io.opentelemetry.api.GlobalOpenTelemetry; @@ -31,7 +32,9 @@ public void setUp() { } DatastoreOptions.Builder getBaseOptions() { - return DatastoreOptions.newBuilder().setProjectId("test-project").setDatabaseId("(default)"); + return DatastoreOptions.newBuilder() + .setProjectId("test-project") + .setCredentials(NoCredentials.getInstance()); } DatastoreOptions getTracingEnabledOptions() { diff --git a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java index 469dc3b39..f1cce8006 100644 --- a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java +++ b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/TraceUtilTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.cloud.NoCredentials; import com.google.cloud.datastore.DatastoreOpenTelemetryOptions; import com.google.cloud.datastore.DatastoreOptions; import org.junit.Test; @@ -28,7 +29,7 @@ public void defaultOptionsUseDisabledTraceUtil() { TraceUtil.getInstance( DatastoreOptions.newBuilder() .setProjectId("test-project") - .setDatabaseId("(default)") + .setCredentials(NoCredentials.getInstance()) .build()); assertThat(traceUtil instanceof DisabledTraceUtil).isTrue(); } @@ -39,7 +40,7 @@ public void tracingDisabledOptionsUseDisabledTraceUtil() { TraceUtil.getInstance( DatastoreOptions.newBuilder() .setProjectId("test-project") - .setDatabaseId("(default)") + .setCredentials(NoCredentials.getInstance()) .setOpenTelemetryOptions( DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(false).build()) .build()); @@ -52,7 +53,7 @@ public void tracingEnabledOptionsUseEnabledTraceUtil() { TraceUtil.getInstance( DatastoreOptions.newBuilder() .setProjectId("test-project") - .setDatabaseId("(default)") + .setCredentials(NoCredentials.getInstance()) .setOpenTelemetryOptions( DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build()) .build()); From 29e088da14058c89982f493c2a788a5cb5e69520 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 14:29:44 -0700 Subject: [PATCH 13/16] Code review: DatastoreOptionsTest --- .../cloud/datastore/DatastoreOptionsTest.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java index a545580e2..87d2535c0 100644 --- a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java +++ b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java @@ -70,6 +70,20 @@ public void testHost() { assertEquals("http://localhost:" + PORT, options.build().getHost()); } + @Test + public void testOpenTelemetryOptionsEnabled() { + options.setOpenTelemetryOptions( + DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build()); + assertTrue(options.build().getOpenTelemetryOptions().getEnabled()); + } + + @Test + public void testOpenTelemetryOptionsDisabled() { + options.setOpenTelemetryOptions( + DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(false).build()); + assertTrue(!options.build().getOpenTelemetryOptions().getEnabled()); + } + @Test public void testNamespace() { assertTrue(options.build().getNamespace().isEmpty()); From f46058a9aed0a84c102537d9cde4acf8edba9dc9 Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Tue, 7 May 2024 14:32:24 -0700 Subject: [PATCH 14/16] Code review: Method name s/getEnabled/isEnabled --- .../datastore/DatastoreOpenTelemetryOptions.java | 12 ++++++------ .../google/cloud/datastore/telemetry/TraceUtil.java | 2 +- .../google/cloud/datastore/DatastoreOptionsTest.java | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java index 778ff5877..ffdedb8dc 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java @@ -25,11 +25,11 @@ public class DatastoreOpenTelemetryOptions { private final @Nullable OpenTelemetry openTelemetry; DatastoreOpenTelemetryOptions(Builder builder) { - this.enabled = builder.isEnabled; + this.enabled = builder.enabled; this.openTelemetry = builder.openTelemetry; } - public boolean getEnabled() { + public boolean isEnabled() { return enabled; } @@ -50,17 +50,17 @@ public static DatastoreOpenTelemetryOptions.Builder newBuilder() { public static class Builder { - private boolean isEnabled; + private boolean enabled; @Nullable private OpenTelemetry openTelemetry; private Builder() { - isEnabled = false; + enabled = false; openTelemetry = null; } private Builder(DatastoreOpenTelemetryOptions options) { - this.isEnabled = options.enabled; + this.enabled = options.enabled; this.openTelemetry = options.openTelemetry; } @@ -76,7 +76,7 @@ public DatastoreOpenTelemetryOptions build() { */ @Nonnull public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enable) { - this.isEnabled = enable; + this.enabled = enable; return this; } diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java index c477edc44..4e55a1ff6 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/TraceUtil.java @@ -40,7 +40,7 @@ public interface TraceUtil { * @return An instance of the TraceUtil class. */ static TraceUtil getInstance(@Nonnull DatastoreOptions datastoreOptions) { - boolean createEnabledInstance = datastoreOptions.getOpenTelemetryOptions().getEnabled(); + boolean createEnabledInstance = datastoreOptions.getOpenTelemetryOptions().isEnabled(); // The environment variable can override options to enable/disable telemetry collection. String enableTracingEnvVar = System.getenv(ENABLE_TRACING_ENV_VAR); diff --git a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java index 87d2535c0..85703f739 100644 --- a/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java +++ b/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreOptionsTest.java @@ -74,14 +74,14 @@ public void testHost() { public void testOpenTelemetryOptionsEnabled() { options.setOpenTelemetryOptions( DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(true).build()); - assertTrue(options.build().getOpenTelemetryOptions().getEnabled()); + assertTrue(options.build().getOpenTelemetryOptions().isEnabled()); } @Test public void testOpenTelemetryOptionsDisabled() { options.setOpenTelemetryOptions( DatastoreOpenTelemetryOptions.newBuilder().setTracingEnabled(false).build()); - assertTrue(!options.build().getOpenTelemetryOptions().getEnabled()); + assertTrue(!options.build().getOpenTelemetryOptions().isEnabled()); } @Test From 2dcc12174b0b7065355d580d27b87192d93ecffa Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Wed, 8 May 2024 10:53:53 -0700 Subject: [PATCH 15/16] Code review: redundant dependency, s/enable/enabled --- google-cloud-datastore/pom.xml | 4 ---- .../cloud/datastore/DatastoreOpenTelemetryOptions.java | 6 +++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/google-cloud-datastore/pom.xml b/google-cloud-datastore/pom.xml index 103813bfb..23f61c213 100644 --- a/google-cloud-datastore/pom.xml +++ b/google-cloud-datastore/pom.xml @@ -39,10 +39,6 @@ com.google.cloud.datastore datastore-v1-proto-client - - com.google.auth - google-auth-library-credentials - io.grpc grpc-api diff --git a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java index ffdedb8dc..ac266562e 100644 --- a/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java +++ b/google-cloud-datastore/src/main/java/com/google/cloud/datastore/DatastoreOpenTelemetryOptions.java @@ -72,11 +72,11 @@ public DatastoreOpenTelemetryOptions build() { /** * Sets whether tracing should be enabled. * - * @param enable Whether tracing should be enabled. + * @param enabled Whether tracing should be enabled. */ @Nonnull - public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enable) { - this.enabled = enable; + public DatastoreOpenTelemetryOptions.Builder setTracingEnabled(boolean enabled) { + this.enabled = enabled; return this; } From 33d08d600dd3835a6cc3aba17575f24c70af176c Mon Sep 17 00:00:00 2001 From: jimit-j-shah <57637300+jimit-j-shah@users.noreply.github.com> Date: Wed, 8 May 2024 11:05:07 -0700 Subject: [PATCH 16/16] Code review: reinstating dependency after build failure https://github.com/googleapis/java-datastore/actions/runs/9006313814/job/24743556228 --- google-cloud-datastore/pom.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/google-cloud-datastore/pom.xml b/google-cloud-datastore/pom.xml index 23f61c213..ced4d4069 100644 --- a/google-cloud-datastore/pom.xml +++ b/google-cloud-datastore/pom.xml @@ -39,6 +39,10 @@ com.google.cloud.datastore datastore-v1-proto-client + + com.google.auth + google-auth-library-credentials + io.grpc grpc-api