Add version to the statsRecorder map.

google-cloud-bigtable-stats/clirr-ignored-differences.xml

@@ -19,10 +19,4 @@
         <className>com/google/cloud/bigtable/stats/StatsRecorderWrapper</className>
         <method>void putBatchRequestThrottled(long)</method>
     </difference>
-    <difference>
-        <!-- change method args is ok because StatsRecorderWrapper is InternalApi -->
-        <differenceType>7004</differenceType>
-        <className>com/google/cloud/bigtable/stats/StatsRecorderWrapper</className>
-        <method>*</method>
-    </difference>
 </differences>

google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/BuiltinMeasureConstants.java

@@ -20,7 +20,7 @@
 import io.opencensus.tags.TagKey;
 
 /** Built-in metrics that will be readable under bigtable.googleapis.com/client namespace */
-class BuiltinMeasureConstants {
+public class BuiltinMeasureConstants {
   // Monitored resource TagKeys
   static final TagKey PROJECT_ID = TagKey.create("project_id");
   static final TagKey INSTANCE_ID = TagKey.create("instance");
@@ -35,6 +35,7 @@ class BuiltinMeasureConstants {
   static final TagKey STREAMING = TagKey.create("streaming");
   static final TagKey STATUS = TagKey.create("status");
   static final TagKey CLIENT_NAME = TagKey.create("client_name");
+  public static final TagKey CLIENT_VERSION = TagKey.create("client_version");
 
   // Units
   private static final String COUNT = "1";

google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/StatsRecorderWrapper.java

@@ -27,6 +27,7 @@
 import io.opencensus.tags.Tagger;
 import io.opencensus.tags.Tags;
 import java.util.Map;
+import java.util.Objects;
 
 /** A wrapper to record built-in metrics */
 @InternalApi("For internal use only")
@@ -59,10 +60,9 @@ public StatsRecorderWrapper(
     this.operationMeasureMap = statsRecorder.newMeasureMap();
   }
 
-  public void recordOperation(
-      String status, String tableId, String zone, String cluster, String clientVersion) {
+  public void recordOperation(String status, String tableId, String zone, String cluster) {
     TagContextBuilder tagCtx =
-        newTagContextBuilder(tableId, zone, cluster, clientVersion)
+        newTagContextBuilder(tableId, zone, cluster)
             .putLocal(BuiltinMeasureConstants.STATUS, TagValue.create(status));
 
     boolean isStreaming = operationType == OperationType.ServerStreaming;
@@ -74,10 +74,9 @@ public void recordOperation(
     operationMeasureMap = statsRecorder.newMeasureMap();
   }
 
-  public void recordAttempt(
-      String status, String tableId, String zone, String cluster, String clientVersion) {
+  public void recordAttempt(String status, String tableId, String zone, String cluster) {
     TagContextBuilder tagCtx =
-        newTagContextBuilder(tableId, zone, cluster, clientVersion)
+        newTagContextBuilder(tableId, zone, cluster)
             .putLocal(BuiltinMeasureConstants.STATUS, TagValue.create(status));
 
     boolean isStreaming = operationType == OperationType.ServerStreaming;
@@ -121,20 +120,24 @@ public void putClientBlockingLatencies(long clientBlockingLatency) {
     operationMeasureMap.put(BuiltinMeasureConstants.THROTTLING_LATENCIES, clientBlockingLatency);
   }
 
-  private TagContextBuilder newTagContextBuilder(
-      String tableId, String zone, String cluster, String clientVersion) {
+  private TagContextBuilder newTagContextBuilder(String tableId, String zone, String cluster) {
     TagContextBuilder tagContextBuilder =
         tagger
             .toBuilder(parentContext)
             .putLocal(
                 BuiltinMeasureConstants.CLIENT_NAME,
-                TagValue.create("bigtable-java/" + clientVersion))
+                TagValue.create("bigtable-java/" + statsAttributes.get(
+                    BuiltinMeasureConstants.CLIENT_VERSION.getName())))
             .putLocal(BuiltinMeasureConstants.METHOD, TagValue.create(spanName.toString()))
             .putLocal(BuiltinMeasureConstants.TABLE, TagValue.create(tableId))
             .putLocal(BuiltinMeasureConstants.ZONE, TagValue.create(zone))
             .putLocal(BuiltinMeasureConstants.CLUSTER, TagValue.create(cluster));
     for (Map.Entry<String, String> entry : statsAttributes.entrySet()) {
-      tagContextBuilder.putLocal(TagKey.create(entry.getKey()), TagValue.create(entry.getValue()));
+      // Client version is appended to the client name to keep metric attributes constant.
+      if (!Objects.equals(entry.getKey(), BuiltinMeasureConstants.CLIENT_VERSION.getName())) {
+        tagContextBuilder.putLocal(TagKey.create(entry.getKey()),
+            TagValue.create(entry.getValue()));
+      }
     }
     return tagContextBuilder;
   }

google-cloud-bigtable-stats/src/test/java/com/google/cloud/bigtable/stats/StatsRecorderWrapperTest.java

@@ -75,7 +75,9 @@ public void testStreamingOperation() throws InterruptedException {
                 BuiltinMeasureConstants.INSTANCE_ID.getName(),
                 INSTANCE_ID,
                 BuiltinMeasureConstants.APP_PROFILE.getName(),
-                APP_PROFILE_ID),
+                APP_PROFILE_ID,
+                BuiltinMeasureConstants.CLIENT_VERSION.getName(),
+                VERSION),
             statsComponent.getStatsRecorder());
 
     long operationLatency = 1234;
@@ -96,8 +98,8 @@ public void testStreamingOperation() throws InterruptedException {
     recorderWrapper.putFirstResponseLatencies(firstResponseLatency);
     recorderWrapper.putClientBlockingLatencies(throttlingLatency);
 
-    recorderWrapper.recordOperation("OK", TABLE_ID, ZONE, CLUSTER, VERSION);
-    recorderWrapper.recordAttempt("OK", TABLE_ID, ZONE, CLUSTER, VERSION);
+    recorderWrapper.recordOperation("OK", TABLE_ID, ZONE, CLUSTER);
+    recorderWrapper.recordAttempt("OK", TABLE_ID, ZONE, CLUSTER);
 
     Thread.sleep(100);
 
@@ -274,7 +276,8 @@ public void testUnaryOperations() throws InterruptedException {
             ImmutableMap.of(
                 BuiltinMeasureConstants.PROJECT_ID.getName(), PROJECT_ID,
                 BuiltinMeasureConstants.INSTANCE_ID.getName(), INSTANCE_ID,
-                BuiltinMeasureConstants.APP_PROFILE.getName(), APP_PROFILE_ID),
+                BuiltinMeasureConstants.APP_PROFILE.getName(), APP_PROFILE_ID,
+                BuiltinMeasureConstants.CLIENT_VERSION.getName(), VERSION),
             statsComponent.getStatsRecorder());
 
     long operationLatency = 1234;
@@ -295,8 +298,8 @@ public void testUnaryOperations() throws InterruptedException {
     recorderWrapper.putFirstResponseLatencies(firstResponseLatency);
     recorderWrapper.putClientBlockingLatencies(throttlingLatency);
 
-    recorderWrapper.recordOperation("UNAVAILABLE", TABLE_ID, ZONE, CLUSTER, VERSION);
-    recorderWrapper.recordAttempt("UNAVAILABLE", TABLE_ID, ZONE, CLUSTER, VERSION);
+    recorderWrapper.recordOperation("UNAVAILABLE", TABLE_ID, ZONE, CLUSTER);
+    recorderWrapper.recordAttempt("UNAVAILABLE", TABLE_ID, ZONE, CLUSTER);
 
     Thread.sleep(100);
 

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java

@@ -19,7 +19,6 @@
 
 import com.google.api.gax.retrying.ServerStreamingAttemptException;
 import com.google.api.gax.tracing.SpanName;
-import com.google.cloud.bigtable.Version;
 import com.google.cloud.bigtable.stats.StatsRecorderWrapper;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Stopwatch;
@@ -259,7 +258,7 @@ private void recordOperationCompletion(@Nullable Throwable status) {
       recorder.putFirstResponseLatencies(firstResponsePerOpTimer.elapsed(TimeUnit.MILLISECONDS));
     }
 
-    recorder.recordOperation(Util.extractStatus(status), tableId, zone, cluster, Version.VERSION);
+    recorder.recordOperation(Util.extractStatus(status), tableId, zone, cluster);
   }
 
   private void recordAttemptCompletion(@Nullable Throwable status) {
@@ -285,6 +284,6 @@ private void recordAttemptCompletion(@Nullable Throwable status) {
     }
 
     recorder.putAttemptLatencies(attemptTimer.elapsed(TimeUnit.MILLISECONDS));
-    recorder.recordAttempt(Util.extractStatus(status), tableId, zone, cluster, Version.VERSION);
+    recorder.recordAttempt(Util.extractStatus(status), tableId, zone, cluster);
   }
 }

google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java

@@ -20,8 +20,10 @@
 import com.google.api.gax.tracing.ApiTracerFactory;
 import com.google.api.gax.tracing.BaseApiTracerFactory;
 import com.google.api.gax.tracing.SpanName;
+import com.google.cloud.bigtable.Version;
 import com.google.cloud.bigtable.stats.StatsWrapper;
 import com.google.common.collect.ImmutableMap;
+import com.google.cloud.bigtable.stats.BuiltinMeasureConstants;
 
 /**
  * {@link ApiTracerFactory} that will generate OpenCensus metrics by using the {@link ApiTracer}
@@ -37,7 +39,13 @@ public static BuiltinMetricsTracerFactory create(ImmutableMap<String, String> st
   }
 
   private BuiltinMetricsTracerFactory(ImmutableMap<String, String> statsAttributes) {
-    this.statsAttributes = statsAttributes;
+    this.statsAttributes = addClientVersionToAttributes(statsAttributes);
+  }
+
+  private ImmutableMap<String, String> addClientVersionToAttributes(
+      ImmutableMap<String, String> statsAttributes) {
+    return new ImmutableMap.Builder<String, String>().putAll(statsAttributes).put(
+        BuiltinMeasureConstants.CLIENT_VERSION.getName(), Version.VERSION).build();
   }
 
   @Override

google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java

@@ -43,7 +43,6 @@
 import com.google.bigtable.v2.ReadRowsRequest;
 import com.google.bigtable.v2.ReadRowsResponse;
 import com.google.bigtable.v2.ResponseParams;
-import com.google.cloud.bigtable.Version;
 import com.google.cloud.bigtable.data.v2.BigtableDataSettings;
 import com.google.cloud.bigtable.data.v2.FakeServiceBuilder;
 import com.google.cloud.bigtable.data.v2.models.Query;
@@ -107,7 +106,6 @@ public class BuiltinMetricsTracerTest {
   private static final String BAD_TABLE_ID = "non-exist-table";
   private static final String ZONE = "us-west-1";
   private static final String CLUSTER = "cluster-0";
-  private static final String VERSION = Version.VERSION;
   private static final long FAKE_SERVER_TIMING = 50;
   private static final long SERVER_LATENCY = 100;
   private static final long APPLICATION_LATENCY = 200;
@@ -129,7 +127,6 @@ public class BuiltinMetricsTracerTest {
   @Captor private ArgumentCaptor<String> tableId;
   @Captor private ArgumentCaptor<String> zone;
   @Captor private ArgumentCaptor<String> cluster;
-  @Captor private ArgumentCaptor<String> version;
 
   private int batchElementCount = 2;
 
@@ -267,15 +264,13 @@ public void testReadRowsOperationLatencies() {
             status.capture(),
             tableId.capture(),
             zone.capture(),
-            cluster.capture(),
-            version.capture());
+            cluster.capture());
 
     assertThat(operationLatency.getValue()).isIn(Range.closed(SERVER_LATENCY, elapsed));
     assertThat(status.getAllValues()).containsExactly("OK");
     assertThat(tableId.getAllValues()).containsExactly(TABLE_ID);
     assertThat(zone.getAllValues()).containsExactly(ZONE);
     assertThat(cluster.getAllValues()).containsExactly(CLUSTER);
-    assertThat(version.getAllValues()).containsExactly(VERSION);
   }
 
   @Test
@@ -299,8 +294,7 @@ public void testGfeMetrics() {
             status.capture(),
             tableId.capture(),
             zone.capture(),
-            cluster.capture(),
-            version.capture());
+            cluster.capture());
 
     // The request was retried and gfe latency is only recorded in the retry attempt
     verify(statsRecorderWrapper).putGfeLatencies(gfeLatency.capture());
@@ -315,7 +309,6 @@ public void testGfeMetrics() {
     assertThat(tableId.getAllValues()).containsExactly(TABLE_ID, TABLE_ID);
     assertThat(zone.getAllValues()).containsExactly("global", ZONE);
     assertThat(cluster.getAllValues()).containsExactly("unspecified", CLUSTER);
-    assertThat(version.getAllValues()).containsExactly(VERSION, VERSION);
   }
 
   @Test
@@ -370,8 +363,7 @@ public void onComplete() {
             status.capture(),
             tableId.capture(),
             zone.capture(),
-            cluster.capture(),
-            version.capture());
+            cluster.capture());
 
     assertThat(counter.get()).isEqualTo(fakeService.getResponseCounter().get());
     // Thread.sleep might not sleep for the requested amount depending on the interrupt period
@@ -381,7 +373,6 @@ public void onComplete() {
         .isAtLeast((APPLICATION_LATENCY - SLEEP_VARIABILITY) * counter.get());
     assertThat(applicationLatency.getValue())
         .isAtMost(operationLatency.getValue() - SERVER_LATENCY);
-    assertThat(version.getAllValues()).containsExactly(VERSION);
   }
 
   @Test
@@ -414,8 +405,7 @@ public void testReadRowsApplicationLatencyWithManualFlowControl() throws Excepti
             status.capture(),
             tableId.capture(),
             zone.capture(),
-            cluster.capture(),
-            version.capture());
+            cluster.capture());
 
     // For manual flow control, the last application latency shouldn't count, because at that point
     // the server already sent back all the responses.
@@ -424,7 +414,6 @@ public void testReadRowsApplicationLatencyWithManualFlowControl() throws Excepti
         .isAtLeast(APPLICATION_LATENCY * (counter - 1) - SERVER_LATENCY);
     assertThat(applicationLatency.getValue())
         .isAtMost(operationLatency.getValue() - SERVER_LATENCY);
-    assertThat(version.getAllValues()).containsExactly(VERSION);
   }
 
   @Test
@@ -473,13 +462,11 @@ public void testMutateRowAttemptsTagValues() {
             status.capture(),
             tableId.capture(),
             zone.capture(),
-            cluster.capture(),
-            version.capture());
+            cluster.capture());
     assertThat(zone.getAllValues()).containsExactly("global", "global", ZONE);
     assertThat(cluster.getAllValues()).containsExactly("unspecified", "unspecified", CLUSTER);
     assertThat(status.getAllValues()).containsExactly("UNAVAILABLE", "UNAVAILABLE", "OK");
     assertThat(tableId.getAllValues()).containsExactly(TABLE_ID, TABLE_ID, TABLE_ID);
-    assertThat(version.getAllValues()).containsExactly(VERSION, VERSION, VERSION);
   }
 
   @Test
@@ -503,12 +490,10 @@ public void testReadRowsAttemptsTagValues() {
             status.capture(),
             tableId.capture(),
             zone.capture(),
-            cluster.capture(),
-            version.capture());
+            cluster.capture());
     assertThat(zone.getAllValues()).containsExactly("global", ZONE);
     assertThat(cluster.getAllValues()).containsExactly("unspecified", CLUSTER);
     assertThat(status.getAllValues()).containsExactly("UNAVAILABLE", "OK");
-    assertThat(version.getAllValues()).containsExactly(VERSION, VERSION);
   }
 
   @Test
@@ -540,12 +525,10 @@ public void testBatchBlockingLatencies() throws InterruptedException {
               status.capture(),
               tableId.capture(),
               zone.capture(),
-              cluster.capture(),
-              version.capture());
+              cluster.capture());
 
       assertThat(zone.getAllValues()).containsExactly(ZONE, ZONE, ZONE);
       assertThat(cluster.getAllValues()).containsExactly(CLUSTER, CLUSTER, CLUSTER);
-      assertThat(version.getAllValues()).containsExactly(VERSION, VERSION, VERSION);
     }
   }
 
@@ -610,14 +593,12 @@ public void testPermanentFailure() {
             status.capture(),
             tableId.capture(),
             zone.capture(),
-            cluster.capture(),
-            version.capture());
+            cluster.capture());
 
     assertThat(status.getValue()).isEqualTo("NOT_FOUND");
     assertThat(tableId.getValue()).isEqualTo(BAD_TABLE_ID);
     assertThat(cluster.getValue()).isEqualTo("unspecified");
     assertThat(zone.getValue()).isEqualTo("global");
-    assertThat(version.getAllValues()).containsExactly(VERSION);
   }
 
   private static class FakeService extends BigtableGrpc.BigtableImplBase {