chore: Always return CompositeMetricsRecorder

Author: lqiu96

java-datastore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/RetryAndTraceDatastoreRpcDecorator.java

@@ -24,8 +24,8 @@
 import com.google.cloud.RetryHelper;
 import com.google.cloud.RetryHelper.RetryHelperException;
 import com.google.cloud.datastore.spi.v1.DatastoreRpc;
+import com.google.cloud.datastore.telemetry.CompositeDatastoreMetricsRecorder;
 import com.google.cloud.datastore.telemetry.DatastoreMetricsRecorder;
-import com.google.cloud.datastore.telemetry.NoOpDatastoreMetricsRecorder;
 import com.google.cloud.datastore.telemetry.TelemetryConstants;
 import com.google.cloud.datastore.telemetry.TelemetryUtils;
 import com.google.cloud.datastore.telemetry.TraceUtil;
@@ -48,7 +48,9 @@
 import com.google.datastore.v1.RunAggregationQueryResponse;
 import com.google.datastore.v1.RunQueryRequest;
 import com.google.datastore.v1.RunQueryResponse;
+import java.util.ArrayList;
 import java.util.concurrent.Callable;
+import javax.annotation.Nonnull;
 
 /**
  * An implementation of {@link DatastoreRpc} which acts as a Decorator and decorates the underlying
@@ -58,7 +60,7 @@
 public class RetryAndTraceDatastoreRpcDecorator implements DatastoreRpc {
 
   private final DatastoreRpc datastoreRpc;
-  private final com.google.cloud.datastore.telemetry.TraceUtil otelTraceUtil;
+  private final TraceUtil otelTraceUtil;
   private final RetrySettings retrySettings;
   private final DatastoreOptions datastoreOptions;
   private final DatastoreMetricsRecorder metricsRecorder;
@@ -73,7 +75,7 @@ public RetryAndTraceDatastoreRpcDecorator(
     this.retrySettings = retrySettings;
     this.datastoreOptions = datastoreOptions;
     this.otelTraceUtil = otelTraceUtil;
-    this.metricsRecorder = new NoOpDatastoreMetricsRecorder();
+    this.metricsRecorder = new CompositeDatastoreMetricsRecorder(new ArrayList<>());
   }
 
   private RetryAndTraceDatastoreRpcDecorator(Builder builder) {
@@ -94,10 +96,8 @@ public static class Builder {
     private RetrySettings retrySettings;
     private DatastoreOptions datastoreOptions;
 
-    // Defaults configured for this class
-    private DatastoreMetricsRecorder metricsRecorder = new NoOpDatastoreMetricsRecorder();
-
-    private Builder() {}
+    private DatastoreMetricsRecorder metricsRecorder =
+        new CompositeDatastoreMetricsRecorder(new ArrayList<>());
 
     public Builder setDatastoreRpc(DatastoreRpc datastoreRpc) {
       this.datastoreRpc = datastoreRpc;
@@ -119,8 +119,8 @@ public Builder setDatastoreOptions(DatastoreOptions datastoreOptions) {
       return this;
     }
 
-    public Builder setMetricsRecorder(DatastoreMetricsRecorder metricsRecorder) {
-      Preconditions.checkNotNull(metricsRecorder, "metricsRecorder can not be null");
+    @InternalApi
+    public Builder setMetricsRecorder(@Nonnull DatastoreMetricsRecorder metricsRecorder) {
       this.metricsRecorder = metricsRecorder;
       return this;
     }
@@ -130,6 +130,7 @@ public RetryAndTraceDatastoreRpcDecorator build() {
       Preconditions.checkNotNull(otelTraceUtil, "otelTraceUtil is required");
       Preconditions.checkNotNull(retrySettings, "retrySettings is required");
       Preconditions.checkNotNull(datastoreOptions, "datastoreOptions is required");
+      Preconditions.checkNotNull(metricsRecorder, "metricsRecorder is required");
       return new RetryAndTraceDatastoreRpcDecorator(this);
     }
   }
@@ -176,9 +177,8 @@ public RunAggregationQueryResponse runAggregationQuery(RunAggregationQueryReques
     boolean isTransactional = readOptions.hasTransaction() || readOptions.hasNewTransaction();
     String spanName =
         (isTransactional
-            ? com.google.cloud.datastore.telemetry.TraceUtil
-                .SPAN_NAME_TRANSACTION_RUN_AGGREGATION_QUERY
-            : com.google.cloud.datastore.telemetry.TraceUtil.SPAN_NAME_RUN_AGGREGATION_QUERY);
+            ? TraceUtil.SPAN_NAME_TRANSACTION_RUN_AGGREGATION_QUERY
+            : TraceUtil.SPAN_NAME_RUN_AGGREGATION_QUERY);
     return invokeRpc(
         () -> datastoreRpc.runAggregationQuery(request),
         spanName,

java-datastore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/CompositeDatastoreMetricsRecorder.java

@@ -16,6 +16,8 @@
 
 package com.google.cloud.datastore.telemetry;
 
+import com.google.api.core.InternalApi;
+import com.google.common.annotations.VisibleForTesting;
 import java.util.List;
 import java.util.Map;
 import java.util.logging.Level;
@@ -28,17 +30,23 @@
  * <p>This allows simultaneous recording to both built-in (Cloud Monitoring) and custom
  * (user-provided) OpenTelemetry backends.
  */
-class CompositeDatastoreMetricsRecorder implements DatastoreMetricsRecorder {
+@InternalApi
+public class CompositeDatastoreMetricsRecorder implements DatastoreMetricsRecorder {
 
   private static final Logger logger =
       Logger.getLogger(CompositeDatastoreMetricsRecorder.class.getName());
 
   private final List<DatastoreMetricsRecorder> recorders;
 
-  CompositeDatastoreMetricsRecorder(List<DatastoreMetricsRecorder> recorders) {
+  public CompositeDatastoreMetricsRecorder(List<DatastoreMetricsRecorder> recorders) {
     this.recorders = recorders;
   }
 
+  @VisibleForTesting
+  List<DatastoreMetricsRecorder> getMetricRecorders() {
+    return recorders;
+  }
+
   /** {@inheritDoc} */
   @Override
   public void recordTransactionLatency(double latencyMs, Map<String, String> attributes) {

java-datastore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/DatastoreMetricsRecorder.java

@@ -125,14 +125,6 @@ static DatastoreMetricsRecorder getInstance(@Nonnull DatastoreOptions datastoreO
           new OpenTelemetryDatastoreMetricsRecorder(customOtel, TelemetryConstants.METRIC_PREFIX));
     }
 
-    // Default metrics are disabled and user has no custom Otel instance
-    if (recorders.isEmpty()) {
-      return new NoOpDatastoreMetricsRecorder();
-    }
-    // CompositeMetricsRecorder is not needed for one recorder
-    if (recorders.size() == 1) {
-      return recorders.get(0);
-    }
     return new CompositeDatastoreMetricsRecorder(recorders);
   }
 }

java-datastore/google-cloud-datastore/src/main/java/com/google/cloud/datastore/telemetry/NoOpDatastoreMetricsRecorder.java

@@ -41,8 +41,9 @@ private DatastoreOptions.Builder baseOptions() {
   }
 
   @Test
-  public void defaultOptionsWithBuiltInMetricsDisabled_returnsNoOp() {
-    // When both custom metrics and built-in metrics export are disabled, should return NoOp
+  public void defaultOptionsWithBuiltInMetricsDisabled_returnsNoRecorders() {
+    // When both custom metrics and built-in metrics export are disabled,
+    // there should be no recorders
     DatastoreOptions options =
         baseOptions()
             .setOpenTelemetryOptions(
@@ -51,11 +52,14 @@ public void defaultOptionsWithBuiltInMetricsDisabled_returnsNoOp() {
                     .build())
             .build();
     DatastoreMetricsRecorder recorder = DatastoreMetricsRecorder.getInstance(options);
-    assertThat(recorder).isInstanceOf(NoOpDatastoreMetricsRecorder.class);
+    assertThat(recorder).isInstanceOf(CompositeDatastoreMetricsRecorder.class);
+    CompositeDatastoreMetricsRecorder compositeRecorder =
+        (CompositeDatastoreMetricsRecorder) recorder;
+    assertThat(compositeRecorder.getMetricRecorders().size()).isEqualTo(0);
   }
 
   @Test
-  public void tracingEnabledButMetricsDisabledAndBuiltInDisabled_returnsNoOp() {
+  public void tracingEnabledButMetricsDisabledAndBuiltInDisabled_returnsNoRecorders() {
     // Enabling tracing alone should not enable metrics
     DatastoreOptions options =
         baseOptions()
@@ -66,11 +70,14 @@ public void tracingEnabledButMetricsDisabledAndBuiltInDisabled_returnsNoOp() {
                     .build())
             .build();
     DatastoreMetricsRecorder recorder = DatastoreMetricsRecorder.getInstance(options);
-    assertThat(recorder).isInstanceOf(NoOpDatastoreMetricsRecorder.class);
+    assertThat(recorder).isInstanceOf(CompositeDatastoreMetricsRecorder.class);
+    CompositeDatastoreMetricsRecorder compositeRecorder =
+        (CompositeDatastoreMetricsRecorder) recorder;
+    assertThat(compositeRecorder.getMetricRecorders().size()).isEqualTo(0);
   }
 
   @Test
-  public void defaultOptionsWithBuiltInMetricsEnabled_butNoCredentials_returnsNoOpRecorder() {
+  public void defaultOptionsWithBuiltInMetricsEnabled_butNoCredentials_returnsNoRecorders() {
     // Explicitly enable built-in metrics export
     DatastoreOptions options =
         baseOptions() // Uses NoCredentials by default
@@ -81,9 +88,12 @@ public void defaultOptionsWithBuiltInMetricsEnabled_butNoCredentials_returnsNoOp
             .build();
     DatastoreMetricsRecorder recorder = DatastoreMetricsRecorder.getInstance(options);
 
-    // Since baseOptions() uses NoCredentials, it should return NoOpDatastoreMetricsRecorder
+    // Since baseOptions() uses NoCredentials, it should not have any recorders
     // as we don't want to send metrics for local emulator logic.
-    assertThat(recorder).isInstanceOf(NoOpDatastoreMetricsRecorder.class);
+    assertThat(recorder).isInstanceOf(CompositeDatastoreMetricsRecorder.class);
+    CompositeDatastoreMetricsRecorder compositeRecorder =
+        (CompositeDatastoreMetricsRecorder) recorder;
+    assertThat(compositeRecorder.getMetricRecorders().size()).isEqualTo(0);
   }
 
   @Test