fix: add client id to metrics to avoid collisions (#117)

338e136
commit 338e136
Author: Knut Olav Løite <koloite@gmail.com>
Date:   Fri Mar 20 04:32:04 2020 +0100

    fix: add client id to metrics to avoid collisions (#117)

    This PR adds an automatically generated identifier to all database clients that are created by the client library. This avoids collisions of the same metrics being registered multiple times, and makes it possible to distinguish different clients from each other in the monitoring.

    This PR does not allow the user to specify the id, but this could be added in a future change. That would need an API change by adding an overload to the method `getDatabaseClient(DatabaseId)` with an additional `clientId` parameter.

    This should also fix the build error in GoogleCloudPlatform/java-docs-samples#2323.

    Fixes #106

google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestWithClosedSessionsEnv.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java
google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java

google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java

@@ -37,9 +37,16 @@ private enum SessionMode {
     READ_WRITE
   }
 
+  @VisibleForTesting final String clientId;
   @VisibleForTesting final SessionPool pool;
 
+  @VisibleForTesting
   DatabaseClientImpl(SessionPool pool) {
+    this("", pool);
+  }
+
+  DatabaseClientImpl(String clientId, SessionPool pool) {
+    this.clientId = clientId;
     this.pool = pool;
   }
 

google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java

@@ -23,6 +23,8 @@
 class MetricRegistryConstants {
 
   // The label keys are used to uniquely identify timeseries.
+  private static final LabelKey CLIENT_ID =
+      LabelKey.create("client_id", "User defined database client id");
   private static final LabelKey DATABASE = LabelKey.create("database", "Target database");
   private static final LabelKey INSTANCE_ID =
       LabelKey.create("instance_id", "Name of the instance");
@@ -33,10 +35,10 @@ class MetricRegistryConstants {
   private static final LabelValue UNSET_LABEL = LabelValue.create(null);
 
   static final ImmutableList<LabelKey> SPANNER_LABEL_KEYS =
-      ImmutableList.of(DATABASE, INSTANCE_ID, LIBRARY_VERSION);
+      ImmutableList.of(CLIENT_ID, DATABASE, INSTANCE_ID, LIBRARY_VERSION);
 
   static final ImmutableList<LabelValue> SPANNER_DEFAULT_LABEL_VALUES =
-      ImmutableList.of(UNSET_LABEL, UNSET_LABEL, UNSET_LABEL);
+      ImmutableList.of(UNSET_LABEL, UNSET_LABEL, UNSET_LABEL, UNSET_LABEL);
 
   /** Unit to represent counts. */
   static final String COUNT = "1";

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java

@@ -62,6 +62,24 @@ class SpannerImpl extends BaseService<SpannerOptions> implements Spanner {
   static final String QUERY = "CloudSpannerOperation.ExecuteStreamingQuery";
   static final String READ = "CloudSpannerOperation.ExecuteStreamingRead";
 
+  private static final Object CLIENT_ID_LOCK = new Object();
+
+  @GuardedBy("CLIENT_ID_LOCK")
+  private static final Map<DatabaseId, Long> CLIENT_IDS = new HashMap<>();
+
+  private static String nextDatabaseClientId(DatabaseId databaseId) {
+    synchronized (CLIENT_ID_LOCK) {
+      Long id = CLIENT_IDS.get(databaseId);
+      if (id == null) {
+        id = 1L;
+      } else {
+        id++;
+      }
+      CLIENT_IDS.put(databaseId, id);
+      return String.format("client-%d", id);
+    }
+  }
+
   private final SpannerRpc gapicRpc;
 
   @GuardedBy("this")
@@ -153,24 +171,26 @@ public DatabaseClient getDatabaseClient(DatabaseId db) {
       if (dbClients.containsKey(db)) {
         return dbClients.get(db);
       } else {
+        String clientId = nextDatabaseClientId(db);
         List<LabelValue> labelValues =
             ImmutableList.of(
+                LabelValue.create(clientId),
                 LabelValue.create(db.getDatabase()),
                 LabelValue.create(db.getInstanceId().getName()),
                 LabelValue.create(GaxProperties.getLibraryVersion(getOptions().getClass())));
         SessionPool pool =
             SessionPool.createPool(
                 getOptions(), SpannerImpl.this.getSessionClient(db), labelValues);
-        DatabaseClientImpl dbClient = createDatabaseClient(pool);
+        DatabaseClientImpl dbClient = createDatabaseClient(clientId, pool);
         dbClients.put(db, dbClient);
         return dbClient;
       }
     }
   }
 
   @VisibleForTesting
-  DatabaseClientImpl createDatabaseClient(SessionPool pool) {
-    return new DatabaseClientImpl(pool);
+  DatabaseClientImpl createDatabaseClient(String clientId, SessionPool pool) {
+    return new DatabaseClientImpl(clientId, pool);
   }
 
   @Override

google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestWithClosedSessionsEnv.java

@@ -45,8 +45,8 @@ private static class SpannerWithClosedSessionsImpl extends SpannerImpl {
     }
 
     @Override
-    DatabaseClientImpl createDatabaseClient(SessionPool pool) {
-      return new DatabaseClientWithClosedSessionImpl(pool);
+    DatabaseClientImpl createDatabaseClient(String clientId, SessionPool pool) {
+      return new DatabaseClientWithClosedSessionImpl(clientId, pool);
     }
   }
 
@@ -58,8 +58,8 @@ public static class DatabaseClientWithClosedSessionImpl extends DatabaseClientIm
     private boolean invalidateNextSession = false;
     private boolean allowReplacing = true;
 
-    DatabaseClientWithClosedSessionImpl(SessionPool pool) {
-      super(pool);
+    DatabaseClientWithClosedSessionImpl(String clientId, SessionPool pool) {
+      super(clientId, pool);
     }
 
     /** Invalidate the next session that is checked out from the pool. */

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java

@@ -1579,6 +1579,7 @@ public void testSessionMetrics() throws Exception {
     FakeMetricRegistry metricRegistry = new FakeMetricRegistry();
     List<LabelValue> labelValues =
         Arrays.asList(
+            LabelValue.create("client1"),
             LabelValue.create("database1"),
             LabelValue.create("instance1"),
             LabelValue.create("1.0.0"));

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java

@@ -32,6 +32,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.UUID;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -185,6 +186,49 @@ public void testSpannerClosed() throws InterruptedException {
     spanner4.close();
   }
 
+  @Test
+  public void testClientId() {
+    // Create a unique database id to be sure it has not yet been used in the lifetime of this JVM.
+    String dbName =
+        String.format("projects/p1/instances/i1/databases/%s", UUID.randomUUID().toString());
+    DatabaseId db = DatabaseId.of(dbName);
+
+    Mockito.when(spannerOptions.getTransportOptions())
+        .thenReturn(GrpcTransportOptions.newBuilder().build());
+    Mockito.when(spannerOptions.getSessionPoolOptions())
+        .thenReturn(SessionPoolOptions.newBuilder().setMinSessions(0).build());
+
+    DatabaseClientImpl databaseClient = (DatabaseClientImpl) impl.getDatabaseClient(db);
+    assertThat(databaseClient.clientId).isEqualTo("client-1");
+
+    // Get same db client again.
+    DatabaseClientImpl databaseClient1 = (DatabaseClientImpl) impl.getDatabaseClient(db);
+    assertThat(databaseClient1.clientId).isEqualTo(databaseClient.clientId);
+
+    // Get a db client for a different database.
+    String dbName2 =
+        String.format("projects/p1/instances/i1/databases/%s", UUID.randomUUID().toString());
+    DatabaseId db2 = DatabaseId.of(dbName2);
+    DatabaseClientImpl databaseClient2 = (DatabaseClientImpl) impl.getDatabaseClient(db2);
+    assertThat(databaseClient2.clientId).isEqualTo("client-1");
+
+    // Create a new Spanner instance. This will generate new database clients with new ids.
+    try (Spanner spanner =
+        SpannerOptions.newBuilder()
+            .setProjectId("p1")
+            .setCredentials(NoCredentials.getInstance())
+            .build()
+            .getService()) {
+
+      // Get a database client for the same database as the first database. As this goes through a
+      // different Spanner instance with potentially different options, it will get a different
+      // client
+      // id.
+      DatabaseClientImpl databaseClient3 = (DatabaseClientImpl) spanner.getDatabaseClient(db);
+      assertThat(databaseClient3.clientId).isEqualTo("client-2");
+    }
+  }
+
   private SpannerOptions createSpannerOptions() {
     return SpannerOptions.newBuilder()
         .setProjectId("[PROJECT]")

synth.metadata

@@ -1,5 +1,5 @@
 {
-  "updateTime": "2020-03-25T18:53:18.618643Z",
+  "updateTime": "2020-03-25T18:48:37.098594Z",
   "sources": [
     {
       "generator": {