chore(datastore): address review comments on built-in metrics

Address comments regarding:
- Removing @nullable from createOpenTelemetry
- Avoiding wrapping no-op OpenTelemetry in DatastoreMetricsRecorder
- Using isSameInstanceAs in tests

TAG=agy
CONV=5a246380-6d14-49d4-b850-ec05f8f89bcb

Author: lqiu96

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

@@ -36,7 +36,6 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
 
 /**
  * Provides a built-in {@link OpenTelemetry} instance for Datastore client-side metrics.
@@ -116,9 +115,8 @@ private static String hashClientUId(String uuid) {
    * the lifetime of their Datastore client.
    *
    * @param options Datastore Client Options
-   * @return a new {@link OpenTelemetry} instance, or {@code null} if it could not be created.
+   * @return a new {@link OpenTelemetry} instance.
    */
-  @Nullable
   public OpenTelemetry createOpenTelemetry(@Nonnull DatastoreOptions options) {
     // If built-in metrics export is disabled, return no-op OpenTelemetry to avoid instantiating
     // a real SdkMeterProvider. Otherwise, the SDK-internal PeriodicMetricReader will start

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

@@ -103,7 +103,7 @@ static DatastoreMetricsRecorder getInstance(
     // When using a local emulator, there is no need to configure a built-in Otel instance
     if (otelOptions.isExportBuiltinMetricsToGoogleCloudMonitoring()) {
       try {
-        if (builtInOtel != null) {
+        if (builtInOtel != null && builtInOtel != OpenTelemetry.noop()) {
           recorders.add(
               new OpenTelemetryDatastoreMetricsRecorder(
                   builtInOtel, TelemetryConstants.METRIC_PREFIX));

java-datastore/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/BuiltInDatastoreMetricsProviderTest.java

@@ -55,7 +55,7 @@ public void testCreateOpenTelemetry_returnsNoOp() {
             .setCredentials(NoCredentials.getInstance())
             .build();
     OpenTelemetry otel = BuiltInDatastoreMetricsProvider.INSTANCE.createOpenTelemetry(options);
-    assertThat(otel).isInstanceOf(OpenTelemetry.noop().getClass());
+    assertThat(otel).isSameInstanceAs(OpenTelemetry.noop());
   }
 
   @Test
@@ -145,7 +145,7 @@ public void testCreateOpenTelemetry_withEmulatorHostProperty_returnsNoOp() {
               .setCredentials(NoCredentials.getInstance())
               .build();
       OpenTelemetry otel = BuiltInDatastoreMetricsProvider.INSTANCE.createOpenTelemetry(options);
-      assertThat(otel).isInstanceOf(OpenTelemetry.noop().getClass());
+      assertThat(otel).isSameInstanceAs(OpenTelemetry.noop());
     } finally {
       System.clearProperty(DatastoreOptions.LOCAL_HOST_ENV_VAR);
     }

java-datastore/google-cloud-datastore/src/test/java/com/google/cloud/datastore/telemetry/DatastoreMetricsRecorderTest.java

@@ -21,6 +21,7 @@
 import com.google.cloud.datastore.DatastoreOpenTelemetryOptions;
 import com.google.cloud.datastore.DatastoreOptions;
 import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
 import org.easymock.EasyMock;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -79,7 +80,7 @@ public void tracingEnabledButMetricsDisabledAndBuiltInDisabled_returnsNoRecorder
   }
 
   @Test
-  public void defaultOptionsWithBuiltInMetricsEnabled_butNoCredentials_returnsOneRecorder() {
+  public void defaultOptionsWithBuiltInMetricsEnabled_butNoCredentials_returnsNoRecorders() {
     // Explicitly enable built-in metrics export
     DatastoreOptions options =
         baseOptions() // Uses NoCredentials by default
@@ -92,12 +93,12 @@ public void defaultOptionsWithBuiltInMetricsEnabled_butNoCredentials_returnsOneR
         DatastoreMetricsRecorder.getInstance(options, OpenTelemetry.noop());
 
     // Since baseOptions() uses NoCredentials, the provider returns OpenTelemetry.noop().
-    // This NoOp instance is passed to getInstance, which adds it to the recorders list.
-    // So we have 1 recorder (the NoOp one).
+    // This NoOp instance is passed to getInstance, which should NOT add it to the recorders list.
+    // So we have 0 recorders.
     assertThat(recorder).isInstanceOf(CompositeDatastoreMetricsRecorder.class);
     CompositeDatastoreMetricsRecorder compositeRecorder =
         (CompositeDatastoreMetricsRecorder) recorder;
-    assertThat(compositeRecorder.getMetricRecorders().size()).isEqualTo(1);
+    assertThat(compositeRecorder.getMetricRecorders().size()).isEqualTo(0);
   }
 
   @Test
@@ -142,7 +143,7 @@ public void bothEnabled_returnsTwoRecorders() {
                     .setOpenTelemetry(OpenTelemetry.noop())
                     .build())
             .build();
-    OpenTelemetry builtInOtel = OpenTelemetry.noop();
+    OpenTelemetry builtInOtel = OpenTelemetrySdk.builder().build();
 
     DatastoreMetricsRecorder recorder = DatastoreMetricsRecorder.getInstance(options, builtInOtel);
     assertThat(recorder).isInstanceOf(CompositeDatastoreMetricsRecorder.class);