From 7b9a85c385436b2aa9e6f034c3c83e7d604c0f2f Mon Sep 17 00:00:00 2001 From: Reza Karegar Date: Thu, 25 Jan 2024 02:27:36 +0000 Subject: [PATCH] Add version to the statsRecorder map. --- .../clirr-ignored-differences.xml | 6 ---- .../stats/BuiltinMeasureConstants.java | 3 +- .../bigtable/stats/StatsRecorderWrapper.java | 23 ++++++------ .../stats/StatsRecorderWrapperTest.java | 15 ++++---- .../v2/stub/metrics/BuiltinMetricsTracer.java | 5 ++- .../metrics/BuiltinMetricsTracerFactory.java | 10 +++++- .../metrics/BuiltinMetricsTracerTest.java | 35 +++++-------------- 7 files changed, 43 insertions(+), 54 deletions(-) diff --git a/google-cloud-bigtable-stats/clirr-ignored-differences.xml b/google-cloud-bigtable-stats/clirr-ignored-differences.xml index a7dcf1dac0..a920751495 100644 --- a/google-cloud-bigtable-stats/clirr-ignored-differences.xml +++ b/google-cloud-bigtable-stats/clirr-ignored-differences.xml @@ -19,10 +19,4 @@ com/google/cloud/bigtable/stats/StatsRecorderWrapper void putBatchRequestThrottled(long) - - - 7004 - com/google/cloud/bigtable/stats/StatsRecorderWrapper - * - diff --git a/google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/BuiltinMeasureConstants.java b/google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/BuiltinMeasureConstants.java index 2f51204d4b..12b96e14a2 100644 --- a/google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/BuiltinMeasureConstants.java +++ b/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"; diff --git a/google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/StatsRecorderWrapper.java b/google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/StatsRecorderWrapper.java index e0b67c472e..649dd4081d 100644 --- a/google-cloud-bigtable-stats/src/main/java/com/google/cloud/bigtable/stats/StatsRecorderWrapper.java +++ b/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 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; } diff --git a/google-cloud-bigtable-stats/src/test/java/com/google/cloud/bigtable/stats/StatsRecorderWrapperTest.java b/google-cloud-bigtable-stats/src/test/java/com/google/cloud/bigtable/stats/StatsRecorderWrapperTest.java index fd72a8b577..18b21680aa 100644 --- a/google-cloud-bigtable-stats/src/test/java/com/google/cloud/bigtable/stats/StatsRecorderWrapperTest.java +++ b/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); diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java index cc8f54e8ff..2d8262a93e 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracer.java +++ b/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); } } diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java index 794997071d..575aa591a5 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerFactory.java +++ b/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 st } private BuiltinMetricsTracerFactory(ImmutableMap statsAttributes) { - this.statsAttributes = statsAttributes; + this.statsAttributes = addClientVersionToAttributes(statsAttributes); + } + + private ImmutableMap addClientVersionToAttributes( + ImmutableMap statsAttributes) { + return new ImmutableMap.Builder().putAll(statsAttributes).put( + BuiltinMeasureConstants.CLIENT_VERSION.getName(), Version.VERSION).build(); } @Override diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java index e4641c43fb..bca49a1b2d 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsTracerTest.java +++ b/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 tableId; @Captor private ArgumentCaptor zone; @Captor private ArgumentCaptor cluster; - @Captor private ArgumentCaptor 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 {