feat(bigtable): expose a metric to track the number of outstanding rpcs (unary , streaming) in channel pool #2696
Conversation
product-auto-label
bot
added
size: m
sushanb
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
product-auto-label
bot
added
size: l
sushanb
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
sushanb
changed the title
| import com.google.cloud.bigtable.data.v2.stub.metrics.ErrorCountPerConnectionMetricTracker; | ||
| import com.google.cloud.bigtable.data.v2.stub.metrics.MetricsProvider; | ||
| import com.google.cloud.bigtable.data.v2.stub.metrics.NoopMetricsProvider; | ||
| import com.google.cloud.bigtable.data.v2.stub.metrics.*; |
There was a problem hiding this comment.
We don't want to import .*, can you revert this change? It's a setting in intellij: https://screenshot.googleplex.com/6XR4NrbcZ4RMuR3
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import javax.annotation.Nullable; | ||
|
|
There was a problem hiding this comment.
Add the @InternalApi annotation
| return; // Not registered yet | ||
| } | ||
| String lbPolicy = lbPolicyRef.get(); | ||
| if (lbPolicy == null) { |
There was a problem hiding this comment.
if the default is round robin maybe set the default value on line 39 when we define this variable? So we don't need to do this check every time.
| @Override | ||
| public void run() { | ||
| BigtableChannelPoolObserver channelInsightsProvider = bigtableChannelInsightsProviderRef.get(); | ||
| if (channelInsightsProvider == null) { |
| lbPolicy = "ROUND_ROBIN"; | ||
| } | ||
|
|
||
| // Build attributes if they haven't been built yet or were invalidated |
There was a problem hiding this comment.
when will they get invalidated?
|
|
||
| @VisibleForTesting | ||
| boolean retain(boolean isStreaming) { | ||
| AtomicInteger counter = isStreaming ? outstandingStreamingRpcs : outstandingUnaryRpcs; |
There was a problem hiding this comment.
I wonder if updating maxCounter should happen after the if statement? because the rpc is not actually sent on this channel so the max shouldn't be updated?
|
|
||
| @Override | ||
| public int getOutstandingStreamingRpcs() { | ||
| return 0; |
There was a problem hiding this comment.
return outstandingStreamingRpcs.get(); ?
And maybe add a test case? I'm a little concerned that this is not caught by any tests :)
...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsConstants.java
Show resolved
Hide resolved
.../src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/ChannelPoolMetricsTracker.java
Show resolved
Hide resolved
| InstantiatingGrpcChannelProvider instantiatingGrpcChannelProvider, | ||
| ChannelPrimer channelPrimer) { | ||
| ChannelPrimer channelPrimer, | ||
| OutstandingRpcsMetricTracker outstandingRpcsMetricTracker) { |
There was a problem hiding this comment.
I don't love that we're passing this in the ctor.. but I guess there isn't a better way because we need to register channel inside provider and load balancing strategy? maybe the OutstandingRpcsMetricTracker can be a generic channel level metric tracker. And I wonder if the PerConnectionErrorCount metric can be part of it? So maybe rename OutstandingRpcsMetricTracker to something like ChannelMetricTracker?
product-auto-label
bot
added
size: xl
3 participants
expose a metric to track the number of outstanding rpcs (unary , streaming) in channel pool