chore: refactor logic for Otel views

Author: lqiu96

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

@@ -179,7 +179,7 @@ public void close() throws Exception {
     } catch (Exception e) {
       logger.log(Level.WARNING, "Failed to close channels", e);
     }
-    // Close the default Metrics Recorder if exists
+    // Flush and shut down the built-in OTel SDK if active; no-op for user-provided instances.
     getOptions().getMetricsRecorder().close();
   }
 

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

@@ -55,6 +55,9 @@ public final class BuiltInDatastoreMetricsProvider {
   private static final Logger logger =
       Logger.getLogger(BuiltInDatastoreMetricsProvider.class.getName());
 
+  // volatile ensures that writes from one thread (first call to detectClientLocation) are
+  // immediately visible to all other threads sharing this singleton INSTANCE. Without it, a
+  // thread could read a stale null and re-enter the initialization branch.
   private static volatile String location;
   private static final String DEFAULT_LOCATION = "global";
 

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

@@ -74,6 +74,10 @@ private DatastoreBuiltInMetricsView() {}
    * @param metricExporter the exporter to use for metrics.
    * @param builder the builder to register views and the {@link PeriodicMetricReader} on.
    */
+  // Views are scoped to the SdkMeterProvider they are registered on. This method is called
+  // exclusively from BuiltInDatastoreMetricsProvider, which constructs a private SdkMeterProvider
+  // for the built-in Cloud Monitoring export path. A user-provided OpenTelemetry instance has its
+  // own independent MeterProvider, so these views never affect user-configured backends.
   static void registerBuiltinMetrics(
       MetricExporter metricExporter, SdkMeterProviderBuilder builder) {
     registerGaxViews(builder);
@@ -94,12 +98,11 @@ private static void registerGaxViews(SdkMeterProviderBuilder builder) {
     for (String metricName : TelemetryConstants.GAX_METRICS) {
       Aggregation aggregation = Aggregation.defaultAggregation();
 
-      // Differentiate the instrumentation type between `count` vs `latency`
       InstrumentType type = InstrumentType.COUNTER;
       String unit = "1";
 
       // Latency metrics use histograms with specific millisecond buckets.
-      if (metricName.endsWith("latency")) {
+      if (TelemetryConstants.GAX_HISTOGRAM_METRICS.contains(metricName)) {
         aggregation = AGGREGATION_WITH_MILLIS_HISTOGRAM;
         type = InstrumentType.HISTOGRAM;
         unit = "ms";

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

@@ -43,8 +43,6 @@
 @InternalExtensionOnly
 public interface DatastoreMetricsRecorder extends MetricsRecorder {
 
-  Logger logger = Logger.getLogger(DatastoreMetricsRecorder.class.getName());
-
   /**
    * Releases any resources held by this recorder.
    *
@@ -85,6 +83,7 @@ default void close() {}
    * @return a {@link DatastoreMetricsRecorder} that fans out to all configured backends
    */
   static DatastoreMetricsRecorder getInstance(@Nonnull DatastoreOptions datastoreOptions) {
+    Logger logger = Logger.getLogger(DatastoreMetricsRecorder.class.getName());
     DatastoreOpenTelemetryOptions otelOptions = datastoreOptions.getOpenTelemetryOptions();
     List<DatastoreMetricsRecorder> recorders = new ArrayList<>();
 
@@ -103,7 +102,7 @@ static DatastoreMetricsRecorder getInstance(@Nonnull DatastoreOptions datastoreO
         if (builtInOtel != null) {
           recorders.add(
               new OpenTelemetryDatastoreMetricsRecorder(
-                  builtInOtel, TelemetryConstants.METRIC_PREFIX, /* ownsOpenTelemetry= */ true));
+                  builtInOtel, TelemetryConstants.METRIC_PREFIX, /* isBuiltIn= */ true));
         }
       } catch (Exception e) {
         logger.log(

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

@@ -52,28 +52,28 @@ class OpenTelemetryDatastoreMetricsRecorder extends OpenTelemetryMetricsRecorder
   private final OpenTelemetry openTelemetry;
   // True when this recorder created the OpenTelemetry instance (built-in path) and therefore
   // owns its lifecycle. False when the instance was provided by the user.
-  private final boolean ownsOpenTelemetry;
+  private final boolean isBuiltIn;
 
   // Datastore-specific transaction metrics (registered under the Datastore meter).
   private final DoubleHistogram transactionLatency;
   private final LongCounter transactionAttemptCount;
 
   /** Creates a recorder backed by a user-provided {@link OpenTelemetry} instance. */
   OpenTelemetryDatastoreMetricsRecorder(@Nonnull OpenTelemetry openTelemetry, String metricPrefix) {
-    this(openTelemetry, metricPrefix, /* ownsOpenTelemetry= */ false);
+    this(openTelemetry, metricPrefix, /* isBuiltIn= */ false);
   }
 
   /**
    * Creates a recorder, specifying whether this instance owns the {@link OpenTelemetry} lifecycle.
    *
-   * @param ownsOpenTelemetry {@code true} if this recorder created the instance and should shut it
-   *     down on {@link #close()}; {@code false} if the user provided it.
+   * @param isBuiltIn {@code true} if this recorder created the instance and should shut it down on
+   *     {@link #close()}; {@code false} if the user provided it.
    */
   OpenTelemetryDatastoreMetricsRecorder(
-      @Nonnull OpenTelemetry openTelemetry, String metricPrefix, boolean ownsOpenTelemetry) {
+      @Nonnull OpenTelemetry openTelemetry, String metricPrefix, boolean isBuiltIn) {
     super(openTelemetry, metricPrefix);
     this.openTelemetry = openTelemetry;
-    this.ownsOpenTelemetry = ownsOpenTelemetry;
+    this.isBuiltIn = isBuiltIn;
 
     // Note: Standard GAX RPC metrics (operation_latency, attempt_latency, etc.) are handled by the
     // base OpenTelemetryMetricsRecorder class. Those metrics are inherited from the parent classes.
@@ -108,7 +108,7 @@ OpenTelemetry getOpenTelemetry() {
    */
   @Override
   public void close() {
-    if (ownsOpenTelemetry && openTelemetry instanceof OpenTelemetrySdk) {
+    if (isBuiltIn && openTelemetry instanceof OpenTelemetrySdk) {
       try {
         ((OpenTelemetrySdk) openTelemetry).close();
       } catch (Exception e) {

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

@@ -63,6 +63,11 @@ public class TelemetryConstants {
   // GAX-emitted metrics for the built-in Cloud Monitoring export pipeline.
   public static final Set<String> GAX_METRICS = GAX_METRIC_NAME_MAP.keySet();
 
+  // The subset of GAX_METRICS that are histograms (latency metrics). Explicit enumeration here
+  // avoids fragile name-suffix matching (endsWith("latency")) in view registration logic.
+  public static final Set<String> GAX_HISTOGRAM_METRICS =
+      ImmutableSet.of(METRIC_NAME_SHORT_OPERATION_LATENCY, METRIC_NAME_SHORT_ATTEMPT_LATENCY);
+
   // Monitored resource type for Cloud Monitoring
   public static final String DATASTORE_RESOURCE_TYPE = "global";
 
@@ -136,17 +141,21 @@ public class TelemetryConstants {
   /**
    * Metric name for the total latency of an operation (one full RPC call including retries).
    *
-   * <p>Renamed to the plural form ({@code operation_latencies}) by {@link
-   * DatastoreBuiltInMetricsView} during export.
+   * <p><b>Singular/plural naming:</b> This constant uses the singular form ({@code
+   * operation_latency}) to match the name recorded by the GAX layer and used by custom OTel
+   * backends (e.g., an in-memory reader in tests). The built-in Cloud Monitoring export path
+   * renames the metric to the plural form ({@code operation_latencies}) via an OTel View in {@link
+   * DatastoreBuiltInMetricsView}. This split is intentional and consistent with other Google Cloud
+   * client libraries.
    */
   public static final String METRIC_NAME_OPERATION_LATENCY =
       METRIC_PREFIX + "/" + METRIC_NAME_SHORT_OPERATION_LATENCY;
 
   /**
    * Metric name for the latency of a single RPC attempt.
    *
-   * <p>Renamed to the plural form ({@code attempt_latencies}) by {@link
-   * DatastoreBuiltInMetricsView} during export.
+   * <p>See {@link #METRIC_NAME_OPERATION_LATENCY} for the singular/plural naming rationale. The
+   * built-in Cloud Monitoring export renames this to {@code attempt_latencies} via an OTel View.
    */
   public static final String METRIC_NAME_ATTEMPT_LATENCY =
       METRIC_PREFIX + "/" + METRIC_NAME_SHORT_ATTEMPT_LATENCY;

java-datastore/google-cloud-datastore/src/test/java/com/google/cloud/datastore/DatastoreBuiltInAndCustomMetricsIT.java

@@ -175,17 +175,18 @@ public void bothBackendsActive_recorderIsComposite() {
   }
 
   /**
-   * Verifies that the built-in metrics export flag is on by default so that the Cloud Monitoring
-   * exporter path is active even when no additional configuration is provided.
+   * Verifies that the built-in metrics export flag is off by default. The flag is disabled until
+   * the Datastore namespace in Cloud Monitoring is deployed; it must be explicitly opted in via
+   * {@link DatastoreOpenTelemetryOptions.Builder#setExportBuiltinMetricsToGoogleCloudMonitoring}.
    */
   @Test
-  public void builtInMetricsExport_isEnabledByDefault() {
+  public void builtInMetricsExport_isDisabledByDefault() {
     assertThat(
             datastore
                 .getOptions()
                 .getOpenTelemetryOptions()
                 .isExportBuiltinMetricsToGoogleCloudMonitoring())
-        .isTrue();
+        .isFalse();
   }
 
   /**

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

@@ -173,15 +173,17 @@ public void testClientCacheReferenceCounting() {
     DatastoreCloudMonitoringExporter.METRICS_CLIENT_CACHE.put(key, cachedMetricsClient);
 
     DatastoreCloudMonitoringExporter exporter1 =
-        new DatastoreCloudMonitoringExporter(key, PROJECT_ID, DATABASE_ID, mockClient, Collections.emptyMap());
+        new DatastoreCloudMonitoringExporter(
+            key, PROJECT_ID, DATABASE_ID, mockClient, Collections.emptyMap());
 
     // First shutdown should decrement refCount to 1, but not close client
     exporter1.shutdown();
     assertThat(cachedMetricsClient.refCount.get()).isEqualTo(1);
     assertThat(DatastoreCloudMonitoringExporter.METRICS_CLIENT_CACHE.containsKey(key)).isTrue();
 
     DatastoreCloudMonitoringExporter exporter2 =
-        new DatastoreCloudMonitoringExporter(key, PROJECT_ID, DATABASE_ID, mockClient, Collections.emptyMap());
+        new DatastoreCloudMonitoringExporter(
+            key, PROJECT_ID, DATABASE_ID, mockClient, Collections.emptyMap());
 
     // Second shutdown should decrement refCount to 0, close client, and remove from cache
     exporter2.shutdown();