chore(datastore): Address PR feedback on OTel metrics

Author: lqiu96

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

@@ -168,10 +168,9 @@ DatastoreOpenTelemetryOptions.Builder setMetricsEnabled(boolean enabled) {
     /**
      * Sets whether built-in metrics should be exported to Google Cloud Monitoring.
      *
-     * <p>When enabled (the default), client-side metrics are automatically exported to Google Cloud
-     * Monitoring using the Cloud Monitoring API. This can be disabled to prevent metrics from being
-     * sent to Cloud Monitoring while still allowing metrics to flow to a custom OpenTelemetry
-     * backend.
+     * <p>When enabled, client-side metrics are automatically exported to Google Cloud Monitoring
+     * using the Cloud Monitoring API. This can be disabled to prevent metrics from being sent to
+     * Cloud Monitoring while still allowing metrics to flow to a custom OpenTelemetry backend.
      *
      * @param exportBuiltinMetrics Whether built-in metrics should be exported to Cloud Monitoring.
      * @return this builder instance.

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

@@ -81,11 +81,8 @@ private Map<String, String> buildClientAttributes() {
    * DatastoreCloudMonitoringExporter}. No global or shared state is modified.
    *
    * <p><b>Lifecycle:</b> The returned instance is owned by the caller. It should be closed by
-   * calling {@link OpenTelemetrySdk#close()} (or via {@link
+   * calling {@link io.opentelemetry.sdk.OpenTelemetrySdk#close()} (or via {@link
    * OpenTelemetryDatastoreMetricsRecorder#close()}) when the associated Datastore client is closed.
-   * A JVM shutdown hook is also registered as a last-resort safety net for cases where the caller
-   * does not explicitly close the client. Note that each call to this method adds a new shutdown
-   * hook; callers should avoid creating an unbounded number of short-lived clients.
    *
    * <p>No caching is performed here; callers are responsible for holding the returned instance for
    * the lifetime of their Datastore client.
@@ -107,8 +104,6 @@ public OpenTelemetry createOpenTelemetry(
       sdkMeterProviderBuilder.setResource(
           Resource.create(createResourceAttributes(projectId, databaseId)));
       SdkMeterProvider sdkMeterProvider = sdkMeterProviderBuilder.build();
-      // Ensure cleanup on shutdown.
-      Runtime.getRuntime().addShutdownHook(new Thread(sdkMeterProvider::close));
       return OpenTelemetrySdk.builder().setMeterProvider(sdkMeterProvider).build();
     } catch (IOException ex) {
       logger.log(

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

@@ -18,6 +18,8 @@
 
 import java.util.List;
 import java.util.Map;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * A {@link DatastoreMetricsRecorder} implementation that fans out recording calls to multiple
@@ -28,6 +30,9 @@
  */
 class CompositeDatastoreMetricsRecorder implements DatastoreMetricsRecorder {
 
+  private static final Logger logger =
+      Logger.getLogger(CompositeDatastoreMetricsRecorder.class.getName());
+
   private final List<DatastoreMetricsRecorder> recorders;
 
   CompositeDatastoreMetricsRecorder(List<DatastoreMetricsRecorder> recorders) {
@@ -86,8 +91,12 @@ public void recordOperationCount(long count, Map<String, String> attributes) {
    */
   @Override
   public void close() {
-    for (DatastoreMetricsRecorder recorder : recorders) {
-      recorder.close();
+    for (int i = recorders.size() - 1; i >= 0; i--) {
+      try {
+        recorders.get(i).close();
+      } catch (Exception e) {
+        logger.log(Level.WARNING, "Failed to close metrics recorder", e);
+      }
     }
   }
 }

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

@@ -0,0 +1,88 @@
+/*
+ * Copyright 2026 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 java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class CompositeDatastoreMetricsRecorderTest {
+
+  @Test
+  public void testCloseLIFOAndExceptionSafe() {
+    List<Integer> closeOrder = new ArrayList<>();
+    DatastoreMetricsRecorder recorder1 = new MockRecorder(1, closeOrder, false);
+    DatastoreMetricsRecorder recorder2 = new MockRecorder(2, closeOrder, true); // will throw
+    DatastoreMetricsRecorder recorder3 = new MockRecorder(3, closeOrder, false);
+
+    CompositeDatastoreMetricsRecorder composite =
+        new CompositeDatastoreMetricsRecorder(Arrays.asList(recorder1, recorder2, recorder3));
+
+    composite.close();
+
+    // LIFO order means 3, then 2, then 1.
+    // Even though 2 throws, 1 should still be closed.
+    assertThat(closeOrder).containsExactly(3, 2, 1).inOrder();
+  }
+
+  private static class MockRecorder implements DatastoreMetricsRecorder {
+    private final int id;
+    private final List<Integer> closeOrder;
+    private final boolean throwOnClose;
+
+    MockRecorder(int id, List<Integer> closeOrder, boolean throwOnClose) {
+      this.id = id;
+      this.closeOrder = closeOrder;
+      this.throwOnClose = throwOnClose;
+    }
+
+    @Override
+    public void close() {
+      closeOrder.add(id);
+      if (throwOnClose) {
+        throw new RuntimeException("Mock exception on close");
+      }
+    }
+
+    @Override
+    public void recordTransactionLatency(
+        double latencyMs, java.util.Map<String, String> attributes) {}
+
+    @Override
+    public void recordTransactionAttemptCount(
+        long count, java.util.Map<String, String> attributes) {}
+
+    @Override
+    public void recordAttemptLatency(double latencyMs, java.util.Map<String, String> attributes) {}
+
+    @Override
+    public void recordAttemptCount(long count, java.util.Map<String, String> attributes) {}
+
+    @Override
+    public void recordOperationLatency(
+        double latencyMs, java.util.Map<String, String> attributes) {}
+
+    @Override
+    public void recordOperationCount(long count, java.util.Map<String, String> attributes) {}
+  }
+}