From 67a79dad722ad437212437f5a1093c1a27e702fe Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Tue, 21 Jun 2022 14:52:28 -0400 Subject: [PATCH] update comments --- .../data/v2/stub/EnhancedBigtableStub.java | 4 +- .../v2/stub/metrics/BuiltinMetricsTracer.java | 57 +++---------------- .../metrics/BuiltinMetricsTracerFactory.java | 29 ++-------- .../HeaderTracerStreamingCallable.java | 2 +- .../data/v2/stub/metrics/MetricsTracer.java | 35 ------------ .../metrics/BuiltinMetricsTracerTest.java | 44 ++++++++++++-- .../v2/stub/metrics/MetricsTracerTest.java | 12 ---- 7 files changed, 56 insertions(+), 127 deletions(-) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java index 6df57061dc..b6d17baadf 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStub.java @@ -90,7 +90,6 @@ import com.google.cloud.bigtable.data.v2.stub.readrows.ReadRowsUserCallable; import com.google.cloud.bigtable.data.v2.stub.readrows.RowMergingCallable; import com.google.cloud.bigtable.gaxx.retrying.ApiResultRetryAlgorithm; -import com.google.cloud.bigtable.stats.StatsWrapper; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -154,7 +153,6 @@ public static EnhancedBigtableStubSettings finalizeSettings( EnhancedBigtableStubSettings settings, Tagger tagger, StatsRecorder stats) throws IOException { EnhancedBigtableStubSettings.Builder builder = settings.toBuilder(); - StatsWrapper statsWrapper = StatsWrapper.create(); // TODO: this implementation is on the cusp of unwieldy, if we end up adding more features // consider splitting it up by feature. @@ -227,7 +225,7 @@ public static EnhancedBigtableStubSettings finalizeSettings( .build()), // Add OpenCensus Metrics MetricsTracerFactory.create(tagger, stats, attributes), - BuiltinMetricsTracerFactory.create(statsWrapper, builtinAttributes), + BuiltinMetricsTracerFactory.create(builtinAttributes), // Add user configured tracer settings.getTracerFactory()))); return builder.build(); 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 cf4c850e62..7a3ae478e7 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 @@ -15,12 +15,12 @@ */ package com.google.cloud.bigtable.data.v2.stub.metrics; -import com.google.api.gax.tracing.ApiTracerFactory; +import static com.google.api.gax.tracing.ApiTracerFactory.OperationType; + import com.google.api.gax.tracing.SpanName; import com.google.cloud.bigtable.stats.StatsRecorderWrapper; -import com.google.cloud.bigtable.stats.StatsWrapper; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; -import java.util.Map; import java.util.concurrent.CancellationException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -36,7 +36,7 @@ class BuiltinMetricsTracer extends BigtableTracer { private final StatsRecorderWrapper recorder; - private final ApiTracerFactory.OperationType operationType; + private final OperationType operationType; private final SpanName spanName; // Operation level metrics @@ -51,8 +51,7 @@ class BuiltinMetricsTracer extends BigtableTracer { // Total application latency // Total application latency needs to be atomic because it's accessed from different threads. E.g. - // request() from - // user thread and attempt failed from grpc thread. + // request() from user thread and attempt failed from grpc thread. private final AtomicLong totalApplicationLatency = new AtomicLong(0); // Stopwatch is not thread safe so this is a workaround to check if the stopwatch changes is // flushed to memory. @@ -67,20 +66,12 @@ class BuiltinMetricsTracer extends BigtableTracer { // gfe stats private AtomicLong gfeMissingHeaders = new AtomicLong(0); + @VisibleForTesting BuiltinMetricsTracer( - ApiTracerFactory.OperationType operationType, - SpanName spanName, - Map attributes, - StatsWrapper statsWrapper, - @Nullable StatsRecorderWrapper statsRecorderWrapper) { + OperationType operationType, SpanName spanName, StatsRecorderWrapper recorder) { this.operationType = operationType; this.spanName = spanName; - if (statsRecorderWrapper != null) { - // A workaround for test to pass in a mock StatsRecorderWrapper - this.recorder = statsRecorderWrapper; - } else { - this.recorder = new StatsRecorderWrapper(operationType, spanName, attributes, statsWrapper); - } + this.recorder = recorder; } @Override @@ -143,26 +134,6 @@ public void attemptFailed(Throwable error, Duration delay) { recordAttemptCompletion(error); } - @Override - public void attemptFailedRetriesExhausted(Throwable error) { - super.attemptFailedRetriesExhausted(error); - } - - @Override - public void attemptPermanentFailure(Throwable error) { - super.attemptPermanentFailure(error); - } - - @Override - public void lroStartFailed(Throwable error) { - super.lroStartFailed(error); - } - - @Override - public void lroStartSucceeded() { - super.lroStartSucceeded(); - } - @Override public void onRequest() { if (applicationLatencyTimerIsRunning.compareAndSet(true, false)) { @@ -181,16 +152,6 @@ public void responseReceived() { } } - @Override - public void requestSent() { - super.requestSent(); - } - - @Override - public void batchRequestSent(long elementCount, long requestSize) { - super.batchRequestSent(elementCount, requestSize); - } - @Override public int getAttempt() { return attempt; @@ -235,7 +196,7 @@ private void recordOperationCompletion(@Nullable Throwable status) { recorder.putOperationLatencies(operationTimer.elapsed(TimeUnit.MILLISECONDS)); - if (operationType == ApiTracerFactory.OperationType.ServerStreaming + if (operationType == OperationType.ServerStreaming && spanName.getMethodName().equals("ReadRows")) { recorder.putFirstResponseLatencies(firstResponsePerOpTimer.elapsed(TimeUnit.MILLISECONDS)); } 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 6a5a9b6910..794997071d 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,9 +20,7 @@ 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.stats.StatsRecorderWrapper; import com.google.cloud.bigtable.stats.StatsWrapper; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; /** @@ -33,35 +31,20 @@ public class BuiltinMetricsTracerFactory extends BaseApiTracerFactory { private final ImmutableMap statsAttributes; - private final StatsWrapper statsWrapper; - private final StatsRecorderWrapper statsRecorderWrapper; - public static BuiltinMetricsTracerFactory create( - StatsWrapper statsWrapper, ImmutableMap statsAttributes) { - return new BuiltinMetricsTracerFactory(statsWrapper, statsAttributes, null); + public static BuiltinMetricsTracerFactory create(ImmutableMap statsAttributes) { + return new BuiltinMetricsTracerFactory(statsAttributes); } - // A workaround for test to pass in a mock StatsRecorderWrapper - @VisibleForTesting - static BuiltinMetricsTracerFactory createWithRecorder( - StatsWrapper statsWrapper, - ImmutableMap statsAttributes, - StatsRecorderWrapper statsRecorderWrapper) { - return new BuiltinMetricsTracerFactory(statsWrapper, statsAttributes, statsRecorderWrapper); - } - - private BuiltinMetricsTracerFactory( - StatsWrapper statsWrapper, - ImmutableMap statsAttributes, - StatsRecorderWrapper statsRecorderWrapper) { + private BuiltinMetricsTracerFactory(ImmutableMap statsAttributes) { this.statsAttributes = statsAttributes; - this.statsWrapper = statsWrapper; - this.statsRecorderWrapper = statsRecorderWrapper; } @Override public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) { return new BuiltinMetricsTracer( - operationType, spanName, statsAttributes, statsWrapper, statsRecorderWrapper); + operationType, + spanName, + StatsWrapper.createRecorder(operationType, spanName, statsAttributes)); } } diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerStreamingCallable.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerStreamingCallable.java index 8f5c9871b4..b5af9d71b5 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerStreamingCallable.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/HeaderTracerStreamingCallable.java @@ -82,7 +82,7 @@ private class HeaderTracerResponseObserver implements ResponseObserve @Override public void onStart(final StreamController controller) { - final StreamController tracedController = new TracedStreamController(controller, tracer); + StreamController tracedController = new TracedStreamController(controller, tracer); outerObserver.onStart(tracedController); } diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracer.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracer.java index d27941bb71..3c63b1b5f7 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracer.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracer.java @@ -125,11 +125,6 @@ private void recordOperationCompletion(@Nullable Throwable throwable) { measures.record(tagCtx.build()); } - @Override - public void connectionSelected(String s) { - // noop: cardinality for connection ids is too high to use as tags - } - @Override public void attemptStarted(int attemptNumber) { attempt = attemptNumber; @@ -180,16 +175,6 @@ private void recordAttemptCompletion(@Nullable Throwable throwable) { measures.record(tagCtx.build()); } - @Override - public void lroStartFailed(Throwable throwable) { - // noop - } - - @Override - public void lroStartSucceeded() { - // noop - } - @Override public void responseReceived() { if (firstResponsePerOpTimer.isRunning()) { @@ -199,16 +184,6 @@ public void responseReceived() { operationResponseCount++; } - @Override - public void requestSent() { - // noop: no operations are client streaming - } - - @Override - public void batchRequestSent(long elementCount, long requestSize) { - // noop - } - @Override public int getAttempt() { return attempt; @@ -253,14 +228,4 @@ private TagContextBuilder newTagCtxBuilder() { return tagCtx; } - - @Override - public void setLocations(String zone, String cluster) { - // noop - } - - @Override - public void onRequest() { - // noop - } } 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 5e62c44748..37484871ce 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 @@ -15,15 +15,19 @@ */ package com.google.cloud.bigtable.data.v2.stub.metrics; +import static com.google.api.gax.tracing.ApiTracerFactory.OperationType; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import com.google.api.client.util.Lists; import com.google.api.core.SettableApiFuture; import com.google.api.gax.rpc.ClientContext; import com.google.api.gax.rpc.ResponseObserver; import com.google.api.gax.rpc.StreamController; +import com.google.api.gax.tracing.SpanName; import com.google.bigtable.v2.BigtableGrpc; import com.google.bigtable.v2.MutateRowRequest; import com.google.bigtable.v2.MutateRowResponse; @@ -37,9 +41,7 @@ import com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStub; import com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStubSettings; import com.google.cloud.bigtable.stats.StatsRecorderWrapper; -import com.google.cloud.bigtable.stats.StatsWrapper; import com.google.common.base.Stopwatch; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.Range; import com.google.protobuf.ByteString; import com.google.protobuf.BytesValue; @@ -59,6 +61,8 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; import org.mockito.Captor; import org.mockito.Mock; @@ -66,6 +70,7 @@ import org.mockito.junit.MockitoRule; import org.threeten.bp.Duration; +@RunWith(JUnit4.class) public class BuiltinMetricsTracerTest { private static final String PROJECT_ID = "fake-project"; private static final String INSTANCE_ID = "fake-instance"; @@ -118,6 +123,7 @@ public class BuiltinMetricsTracerTest { private EnhancedBigtableStub stub; + @Mock private BuiltinMetricsTracerFactory mockFactory; @Mock private StatsRecorderWrapper statsRecorderWrapper; @Captor private ArgumentCaptor longValue; @@ -169,9 +175,7 @@ public void sendHeaders(Metadata headers) { .mutateRowSettings() .retrySettings() .setInitialRetryDelay(Duration.ofMillis(200)); - stubSettingsBuilder.setTracerFactory( - BuiltinMetricsTracerFactory.createWithRecorder( - StatsWrapper.create(), ImmutableMap.of(), statsRecorderWrapper)); + stubSettingsBuilder.setTracerFactory(mockFactory); EnhancedBigtableStubSettings stubSettings = stubSettingsBuilder.build(); stub = new EnhancedBigtableStub(stubSettings, ClientContext.create(stubSettings)); @@ -185,6 +189,12 @@ public void tearDown() { @Test public void testOperationLatencies() { + when(mockFactory.newTracer(any(), any(), any())) + .thenReturn( + new BuiltinMetricsTracer( + OperationType.ServerStreaming, + SpanName.of("Bigtable", "ReadRows"), + statsRecorderWrapper)); Stopwatch stopwatch = Stopwatch.createStarted(); Lists.newArrayList(stub.readRowsCallable().call(Query.create(TABLE_ID)).iterator()); long elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS); @@ -196,6 +206,13 @@ public void testOperationLatencies() { @Test public void testGfeMetrics() { + when(mockFactory.newTracer(any(), any(), any())) + .thenReturn( + new BuiltinMetricsTracer( + OperationType.ServerStreaming, + SpanName.of("Bigtable", "ReadRows"), + statsRecorderWrapper)); + Lists.newArrayList(stub.readRowsCallable().call(Query.create(TABLE_ID))); verify(statsRecorderWrapper).putGfeLatencies(longValue.capture()); @@ -207,6 +224,13 @@ public void testGfeMetrics() { @Test public void testReadRowsApplicationLatency() throws Exception { + when(mockFactory.newTracer(any(), any(), any())) + .thenReturn( + new BuiltinMetricsTracer( + OperationType.ServerStreaming, + SpanName.of("Bigtable", "ReadRows"), + statsRecorderWrapper)); + final long applicationLatency = 1000; final SettableApiFuture future = SettableApiFuture.create(); final AtomicInteger counter = new AtomicInteger(0); @@ -258,6 +282,11 @@ public void onComplete() { @Test public void testRetryCount() { + when(mockFactory.newTracer(any(), any(), any())) + .thenReturn( + new BuiltinMetricsTracer( + OperationType.Unary, SpanName.of("Bigtable", "MutateRow"), statsRecorderWrapper)); + stub.mutateRowCallable() .call(RowMutation.create(TABLE_ID, "random-row").setCell("cf", "q", "value")); @@ -268,6 +297,11 @@ public void testRetryCount() { @Test public void testMutateRowAttempts() { + when(mockFactory.newTracer(any(), any(), any())) + .thenReturn( + new BuiltinMetricsTracer( + OperationType.Unary, SpanName.of("Bigtable", "MutateRow"), statsRecorderWrapper)); + stub.mutateRowCallable() .call(RowMutation.create(TABLE_ID, "random-row").setCell("cf", "q", "value")); diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java index 1176214de3..b1b966ee9d 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/MetricsTracerTest.java @@ -42,7 +42,6 @@ import com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStub; import com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStubSettings; import com.google.cloud.bigtable.data.v2.stub.mutaterows.MutateRowsBatchingDescriptor; -import com.google.cloud.bigtable.misc_utilities.MethodComparator; import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; @@ -59,8 +58,6 @@ import io.opencensus.tags.TagKey; import io.opencensus.tags.TagValue; import io.opencensus.tags.Tags; -import java.lang.reflect.Method; -import java.util.Arrays; import java.util.Iterator; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -485,15 +482,6 @@ public Object answer(InvocationOnMock invocation) { assertThat(throttledTimeMetric).isAtLeast(throttled); } - @Test - public void testMethodsOverride() { - Method[] baseMethods = BigtableTracer.class.getDeclaredMethods(); - Method[] metricsTracerMethods = MetricsTracer.class.getDeclaredMethods(); - assertThat(Arrays.asList(metricsTracerMethods)) - .comparingElementsUsing(MethodComparator.METHOD_CORRESPONDENCE) - .containsAtLeastElementsIn(baseMethods); - } - @SuppressWarnings("unchecked") private static StreamObserver anyObserver(Class returnType) { return (StreamObserver) any(returnType);