chore: Address PR feedback

- Pre-compute PID_AND_HOSTNAME statically to avoid per-client hostname lookup
- Remove cachedClientAttributes/getClientAttributes() from singleton; use buildClientAttributes() directly
- Include credentials hash in MetricServiceClient cache key to isolate clients with different credentials
- Remove EXPONENTIAL_HISTOGRAM cases (exporter will never receive them)
- Remove unused CLIENT_NAME_KEY constant

Author: lqiu96

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

@@ -29,7 +29,6 @@
 import java.lang.reflect.Method;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.UUID;
@@ -59,13 +58,12 @@ public final class BuiltInDatastoreMetricsProvider {
   private static volatile String location;
   private static final String DEFAULT_LOCATION = "global";
 
-  private final Map<String, String> cachedClientAttributes;
+  // Pre-computed once per JVM; hostname lookup can block, so we pay the cost at class-init time.
+  private static final String PID_AND_HOSTNAME = getProcessId() + "@" + getHostnameSafely();
 
-  private BuiltInDatastoreMetricsProvider() {
-    cachedClientAttributes = Collections.unmodifiableMap(buildClientAttributes());
-  }
+  private BuiltInDatastoreMetricsProvider() {}
 
-  private Map<String, String> buildClientAttributes() {
+  static Map<String, String> buildClientAttributes() {
     Map<String, String> attrs = new HashMap<>();
     attrs.put(TelemetryConstants.CLIENT_UID_KEY.getKey(), getDefaultTaskValue());
     attrs.put(TelemetryConstants.SERVICE_KEY.getKey(), TelemetryConstants.SERVICE_VALUE);
@@ -96,8 +94,6 @@ public OpenTelemetry createOpenTelemetry(
       @Nonnull String projectId, @Nonnull String databaseId, @Nonnull Credentials credentials) {
     SdkMeterProviderBuilder sdkMeterProviderBuilder = SdkMeterProvider.builder();
 
-    // Generate unique client attributes (including unique taskId) for this specific client
-    // instance.
     Map<String, String> clientAttributes = buildClientAttributes();
 
     if (credentials instanceof NoCredentials) {
@@ -158,45 +154,25 @@ Attributes createResourceAttributes(String projectId, String databaseId) {
     return attributesBuilder.build();
   }
 
-  /**
-   * Returns common client attributes added to every exported metric data point.
-   *
-   * <p>The returned map is pre-computed at construction time and shared across all export calls,
-   * since {@code client_name}, {@code client_uid}, and {@code service} are stable for the lifetime
-   * of the process.
-   *
-   * @return an unmodifiable map of client attributes.
-   */
-  Map<String, String> getClientAttributes() {
-    return cachedClientAttributes;
-  }
-
   /**
    * Generates a unique identifier for the {@code client_uid} metric field.
    *
-   * <p>Combines a random UUID with {@code RuntimeMXBean.getName()} (typically {@code
+   * <p>Combines a random UUID with the pre-computed {@code PID_AND_HOSTNAME} (typically {@code
    * pid@hostname}). The UUID prefix ensures uniqueness across process restarts that reuse the same
    * PID, preventing Cloud Monitoring from conflating time series from different process lifecycles.
    *
-   * <p>For Java 9 and later, the PID is obtained using the ProcessHandle API. For Java 8, the PID
-   * is extracted from ManagementFactory.getRuntimeMXBean().getName().
-   *
-   * <p><b>Note</b>: This method generates a new value every time it is called to ensure that each
-   * client instance gets a unique ID. It should be called sparingly (e.g., once per client
-   * creation) to avoid performance overhead from UUID generation and hostname lookup.
-   *
    * @return a unique identifier string.
    */
   private static String getDefaultTaskValue() {
-    String identifier = UUID.randomUUID().toString();
-    String pid = getProcessId();
+    return UUID.randomUUID().toString() + "@" + PID_AND_HOSTNAME;
+  }
 
+  private static String getHostnameSafely() {
     try {
-      String hostname = InetAddress.getLocalHost().getHostName();
-      return identifier + "@" + pid + "@" + hostname;
+      return InetAddress.getLocalHost().getHostName();
     } catch (UnknownHostException e) {
       logger.log(Level.CONFIG, "Unable to get the hostname.", e);
-      return identifier + "@" + pid + "@localhost";
+      return "localhost";
     }
   }
 

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

@@ -85,15 +85,19 @@ static class CachedMetricsClient {
   }
 
   /**
-   * Shared cache for {@link MetricServiceClient} instances, keyed by "projectId:databaseId". This
-   * prevents creating a new gRPC client for every exporter instance, reducing resource usage.
-   * Reference counting is used to safely shut down the client when no longer needed.
+   * Shared cache for {@link MetricServiceClient} instances, keyed by
+   * "projectId:databaseId:credentialsHashCode". Sharing a single gRPC channel across exporter
+   * instances that target the same project, database, and credentials avoids per-client channel
+   * overhead (threads, connections, memory). The credentials hash ensures that clients using
+   * different credentials get their own isolated channel. Reference counting is used to safely shut
+   * down the client when no longer needed.
    */
   static final ConcurrentHashMap<String, CachedMetricsClient> METRICS_CLIENT_CACHE =
       new ConcurrentHashMap<>();
 
   private final MetricServiceClient client;
   private final Map<String, String> clientAttributes;
+  private final String cacheKey;
 
   // This is the quota limit from Cloud Monitoring. More details in
   // https://cloud.google.com/monitoring/quotas#custom_metrics_quotas.
@@ -132,7 +136,8 @@ static DatastoreCloudMonitoringExporter create(
       String databaseId,
       Credentials credentials,
       Map<String, String> clientAttributes) {
-    String key = projectId + ":" + databaseId;
+    int credHash = credentials != null ? credentials.hashCode() : 0;
+    String key = projectId + ":" + databaseId + ":" + credHash;
 
     // Use compute to acquire or create the client atomically with reference counting.
     // If creation fails, we log the error and return null so it's not added to the map.
@@ -161,7 +166,7 @@ static DatastoreCloudMonitoringExporter create(
     }
 
     return new DatastoreCloudMonitoringExporter(
-        projectId, databaseId, cachedMetricsClient.client, clientAttributes);
+        key, projectId, databaseId, cachedMetricsClient.client, clientAttributes);
   }
 
   private static MetricServiceClient createMetricServiceClient(Credentials credentials)
@@ -185,10 +190,12 @@ private static MetricServiceClient createMetricServiceClient(Credentials credent
 
   @VisibleForTesting
   DatastoreCloudMonitoringExporter(
+      String cacheKey,
       String projectId,
       String databaseId,
       MetricServiceClient client,
       Map<String, String> clientAttributes) {
+    this.cacheKey = cacheKey;
     this.client = client;
     this.projectId = projectId;
     this.databaseId = databaseId;
@@ -307,10 +314,9 @@ public CompletableResultCode shutdown() {
     }
     CompletableResultCode shutdownResult = new CompletableResultCode();
     try {
-      String key = projectId + ":" + databaseId;
       // Atomically decrement reference count and cleanup if zero.
       METRICS_CLIENT_CACHE.compute(
-          key,
+          cacheKey,
           (k, v) -> {
             if (v != null && v.refCount.decrementAndGet() == 0) {
               v.client.shutdown();

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

@@ -132,7 +132,7 @@ static List<TimeSeries> convertToDatastoreTimeSeries(
    * <p>{@code clientAttributes} (e.g. {@code client_name}, {@code client_uid}) are injected here
    * rather than being looked up from a singleton so that this method is testable in isolation. The
    * caller ({@link DatastoreCloudMonitoringExporter}) is responsible for supplying them from {@link
-   * BuiltInDatastoreMetricsProvider#getClientAttributes()}.
+   * BuiltInDatastoreMetricsProvider#buildClientAttributes()}.
    */
   private static TimeSeries convertPointToDatastoreTimeSeries(
       MetricData metricData,
@@ -179,7 +179,6 @@ private static TimeSeries convertPointToDatastoreTimeSeries(
   private static MetricKind convertMetricKind(MetricData metricData) {
     switch (metricData.getType()) {
       case HISTOGRAM:
-      case EXPONENTIAL_HISTOGRAM:
         return convertHistogramType(metricData.getHistogramData());
       case LONG_GAUGE:
       case DOUBLE_GAUGE:
@@ -235,7 +234,6 @@ private static ValueType convertValueType(MetricDataType metricDataType) {
       case DOUBLE_SUM:
         return DOUBLE;
       case HISTOGRAM:
-      case EXPONENTIAL_HISTOGRAM:
         return DISTRIBUTION;
       default:
         return ValueType.UNRECOGNIZED;
@@ -248,7 +246,6 @@ private static Point createPoint(
     Point.Builder builder = Point.newBuilder().setInterval(timeInterval);
     switch (type) {
       case HISTOGRAM:
-      case EXPONENTIAL_HISTOGRAM:
         return builder
             .setValue(
                 TypedValue.newBuilder()

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

@@ -105,7 +105,6 @@ public class TelemetryConstants {
 
   // Metric attribute keys (used on metric data points)
   public static final AttributeKey<String> CLIENT_UID_KEY = AttributeKey.stringKey("client_uid");
-  public static final AttributeKey<String> CLIENT_NAME_KEY = AttributeKey.stringKey("client_name");
   public static final AttributeKey<String> METHOD_KEY = AttributeKey.stringKey("method");
   public static final AttributeKey<String> STATUS_KEY = AttributeKey.stringKey("status");
   public static final AttributeKey<String> SERVICE_KEY = AttributeKey.stringKey("service");

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

@@ -30,8 +30,8 @@ public class BuiltInDatastoreMetricsProviderTest {
   private static final String PROJECT_ID = "project-id";
 
   @Test
-  public void testGetClientAttributes() {
-    Map<String, String> attributes = BuiltInDatastoreMetricsProvider.INSTANCE.getClientAttributes();
+  public void testBuildClientAttributes() {
+    Map<String, String> attributes = BuiltInDatastoreMetricsProvider.buildClientAttributes();
     assertThat(attributes).containsKey(TelemetryConstants.CLIENT_UID_KEY.getKey());
     assertThat(attributes.get(TelemetryConstants.CLIENT_UID_KEY.getKey())).contains("@");
     assertThat(attributes).containsKey(TelemetryConstants.SERVICE_KEY.getKey());

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

@@ -78,13 +78,16 @@ public void setUp() {
     mockMetricServiceStub = createMock(MetricServiceStub.class);
     fakeMetricServiceClient = new FakeMetricServiceClient(mockMetricServiceStub);
 
-    Map<String, String> clientAttributes =
-        BuiltInDatastoreMetricsProvider.INSTANCE.getClientAttributes();
+    Map<String, String> clientAttributes = BuiltInDatastoreMetricsProvider.buildClientAttributes();
     this.clientUid = clientAttributes.get(CLIENT_UID_KEY.getKey());
 
     exporter =
         new DatastoreCloudMonitoringExporter(
-            PROJECT_ID, DATABASE_ID, fakeMetricServiceClient, clientAttributes);
+            PROJECT_ID + ":" + DATABASE_ID + ":0",
+            PROJECT_ID,
+            DATABASE_ID,
+            fakeMetricServiceClient,
+            clientAttributes);
 
     attributes =
         Attributes.builder()
@@ -163,24 +166,22 @@ public void testClientCacheReferenceCounting() {
     EasyMock.expectLastCall(); // Expect shutdown when refCount reaches 0
     replay(mockClient);
 
-    String key = PROJECT_ID + ":" + DATABASE_ID;
+    String key = PROJECT_ID + ":" + DATABASE_ID + ":0";
     DatastoreCloudMonitoringExporter.CachedMetricsClient cachedMetricsClient =
         new DatastoreCloudMonitoringExporter.CachedMetricsClient(mockClient);
     cachedMetricsClient.refCount.set(2); // Simulate 2 references
     DatastoreCloudMonitoringExporter.METRICS_CLIENT_CACHE.put(key, cachedMetricsClient);
 
     DatastoreCloudMonitoringExporter exporter1 =
-        new DatastoreCloudMonitoringExporter(
-            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(
-            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();