From 9179a0a9d4e163b8a95885deceecfcea919745d5 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Thu, 19 Sep 2024 15:59:05 -0400 Subject: [PATCH] fix: fix client blocking latency --- .../v2/stub/metrics/BigtableGrpcStreamTracer.java | 11 +---------- .../bigtable/data/v2/stub/metrics/BigtableTracer.java | 7 +++++++ .../data/v2/stub/metrics/BuiltinMetricsTracer.java | 4 ++-- .../data/v2/stub/metrics/CompositeTracer.java | 7 +++++++ .../v2/stub/metrics/BuiltinMetricsTracerTest.java | 3 +-- .../data/v2/stub/metrics/CompositeTracerTest.java | 7 +++++++ 6 files changed, 25 insertions(+), 14 deletions(-) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableGrpcStreamTracer.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableGrpcStreamTracer.java index 3b2242385a..80fcdd0419 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableGrpcStreamTracer.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableGrpcStreamTracer.java @@ -15,11 +15,8 @@ */ package com.google.cloud.bigtable.data.v2.stub.metrics; -import com.google.common.base.Stopwatch; -import io.grpc.Attributes; import io.grpc.ClientStreamTracer; import io.grpc.Metadata; -import java.util.concurrent.TimeUnit; /** * Records the time a request is enqueued in a grpc channel queue. This a bridge between gRPC stream @@ -28,21 +25,15 @@ */ class BigtableGrpcStreamTracer extends ClientStreamTracer { - private final Stopwatch stopwatch = Stopwatch.createUnstarted(); private final BigtableTracer tracer; public BigtableGrpcStreamTracer(BigtableTracer tracer) { this.tracer = tracer; } - @Override - public void streamCreated(Attributes transportAttrs, Metadata headers) { - stopwatch.start(); - } - @Override public void outboundMessageSent(int seqNo, long optionalWireSize, long optionalUncompressedSize) { - tracer.grpcChannelQueuedLatencies(stopwatch.elapsed(TimeUnit.NANOSECONDS)); + tracer.grpcMessageSent(); } static class Factory extends ClientStreamTracer.Factory { diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracer.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracer.java index 3445514f7b..d0e307d510 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracer.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableTracer.java @@ -83,7 +83,14 @@ public void setLocations(String zone, String cluster) { // noop } + @Deprecated + /** @deprecated {@link #grpcMessageSent()} is called instead. */ public void grpcChannelQueuedLatencies(long queuedTimeMs) { // noop } + + /** Called when the message is sent on a grpc channel. */ + public void grpcMessageSent() { + // noop + } } 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 abd214d760..954bd61825 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 @@ -263,8 +263,8 @@ public void batchRequestThrottled(long throttledTimeNanos) { } @Override - public void grpcChannelQueuedLatencies(long queuedTimeNanos) { - totalClientBlockingTime.addAndGet(queuedTimeNanos); + public void grpcMessageSent() { + totalClientBlockingTime.addAndGet(attemptTimer.elapsed(TimeUnit.NANOSECONDS)); } @Override diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracer.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracer.java index 774c6d9f22..d89aa90c6b 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracer.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracer.java @@ -225,4 +225,11 @@ public void grpcChannelQueuedLatencies(long queuedTimeMs) { tracer.grpcChannelQueuedLatencies(queuedTimeMs); } } + + @Override + public void grpcMessageSent() { + for (BigtableTracer tracer : bigtableTracers) { + tracer.grpcMessageSent(); + } + } } 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 d37a2562bf..a2d6458f5d 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 @@ -692,9 +692,8 @@ public void testQueuedOnChannelUnaryLatencies() { .put(CLIENT_NAME_KEY, CLIENT_NAME) .build(); - long expected = CHANNEL_BLOCKING_LATENCY * 2 / 3; long actual = getAggregatedValue(clientLatency, attributes); - assertThat(actual).isAtLeast(expected); + assertThat(actual).isAtLeast(CHANNEL_BLOCKING_LATENCY); } @Test diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracerTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracerTest.java index 11dd0b5095..cb0916ad28 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracerTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/CompositeTracerTest.java @@ -258,4 +258,11 @@ public void testRequestBlockedOnChannel() { verify(child3, times(1)).grpcChannelQueuedLatencies(5L); verify(child4, times(1)).grpcChannelQueuedLatencies(5L); } + + @Test + public void testGrpcMessageSent() { + compositeTracer.grpcMessageSent(); + verify(child3, times(1)).grpcMessageSent(); + verify(child4, times(1)).grpcMessageSent(); + } }