From a1d19327febde9c26fff8f59f7825ffe15dda4c7 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Wed, 1 May 2024 16:19:56 -0700 Subject: rls: Add the target label to RLS counter metrics (#11142) --- .../main/java/io/grpc/rls/CachingRlsLbClient.java | 30 +++++++++++----------- .../java/io/grpc/rls/CachingRlsLbClientTest.java | 5 ++++ .../test/java/io/grpc/rls/RlsLoadBalancerTest.java | 14 ++++++++-- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java index 54c4c411f..5263975b4 100644 --- a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java +++ b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java @@ -24,7 +24,6 @@ import com.google.common.base.Converter; import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects.ToStringHelper; import com.google.common.base.Ticker; -import com.google.common.collect.Lists; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; @@ -61,6 +60,8 @@ import io.grpc.stub.StreamObserver; import io.grpc.util.ForwardingLoadBalancerHelper; import java.net.URI; import java.net.URISyntaxException; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -127,16 +128,16 @@ final class CachingRlsLbClient { = MetricInstrumentRegistry.getDefaultRegistry(); DEFAULT_TARGET_PICKS_COUNTER = metricInstrumentRegistry.registerLongCounter( "grpc.lb.rls.default_target_picks", "Number of LB picks sent to the default target", "pick", - Lists.newArrayList("grpc.target", "grpc.lb.rls.server_target", - "grpc.lb.rls.data_plane_target", "grpc.lb.pick_result"), Lists.newArrayList(), true); + Arrays.asList("grpc.target", "grpc.lb.rls.server_target", + "grpc.lb.rls.data_plane_target", "grpc.lb.pick_result"), Collections.emptyList(), true); TARGET_PICKS_COUNTER = metricInstrumentRegistry.registerLongCounter("grpc.lb.rls.target_picks", "Number of LB picks sent to each RLS target", "pick", - Lists.newArrayList("grpc.target", "grpc.lb.rls.server_target", - "grpc.lb.rls.data_plane_target", "grpc.lb.pick_result"), Lists.newArrayList(), true); + Arrays.asList("grpc.target", "grpc.lb.rls.server_target", + "grpc.lb.rls.data_plane_target", "grpc.lb.pick_result"), Collections.emptyList(), true); FAILED_PICKS_COUNTER = metricInstrumentRegistry.registerLongCounter("grpc.lb.rls.failed_picks", "Number of LB picks failed due to either a failed RLS request or the RLS channel being " - + "throttled", "pick", Lists.newArrayList("grpc.target", "grpc.lb.rls.server_target"), - Lists.newArrayList(), true); + + "throttled", "pick", Arrays.asList("grpc.target", "grpc.lb.rls.server_target"), + Collections.emptyList(), true); } private CachingRlsLbClient(Builder builder) { @@ -968,11 +969,11 @@ final class CachingRlsLbClient { // Happy path logger.log(ChannelLogLevel.DEBUG, "Returning PickResult"); PickResult pickResult = picker.pickSubchannel(args); - // TODO: include the "grpc.target" label once target is available here. if (pickResult.hasResult()) { helper.getMetricRecorder().addLongCounter(TARGET_PICKS_COUNTER, 1, - Lists.newArrayList("", lookupService, childPolicyWrapper.getTarget(), - determineMetricsPickResult(pickResult)), Lists.newArrayList()); + Arrays.asList(helper.getChannelTarget(), lookupService, + childPolicyWrapper.getTarget(), determineMetricsPickResult(pickResult)), + Collections.emptyList()); } return pickResult; } else if (response.hasError()) { @@ -982,9 +983,8 @@ final class CachingRlsLbClient { return useFallback(args); } logger.log(ChannelLogLevel.DEBUG, "No RLS fallback, returning PickResult with an error"); - // TODO: include the "grpc.target" label once target is available here. helper.getMetricRecorder().addLongCounter(FAILED_PICKS_COUNTER, 1, - Lists.newArrayList("", lookupService), Lists.newArrayList()); + Arrays.asList(helper.getChannelTarget(), lookupService), Collections.emptyList()); return PickResult.withError( convertRlsServerStatus(response.getStatus(), lbPolicyConfig.getRouteLookupConfig().lookupService())); @@ -1007,10 +1007,10 @@ final class CachingRlsLbClient { } PickResult pickResult = picker.pickSubchannel(args); if (pickResult.hasResult()) { - // TODO: include the grpc.target label once target is available here. helper.getMetricRecorder().addLongCounter(DEFAULT_TARGET_PICKS_COUNTER, 1, - Lists.newArrayList("", lookupService, fallbackChildPolicyWrapper.getTarget(), - determineMetricsPickResult(pickResult)), Lists.newArrayList()); + Arrays.asList(helper.getChannelTarget(), lookupService, + fallbackChildPolicyWrapper.getTarget(), determineMetricsPickResult(pickResult)), + Collections.emptyList()); } return pickResult; } diff --git a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java index 362e961fb..9cef95c2a 100644 --- a/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java +++ b/rls/src/test/java/io/grpc/rls/CachingRlsLbClientTest.java @@ -900,6 +900,11 @@ public class CachingRlsLbClientTest { public MetricRecorder getMetricRecorder() { return mockMetricRecorder; } + + @Override + public String getChannelTarget() { + return "channelTarget"; + } } private static final class FakeThrottler implements Throttler { diff --git a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java index 2137dfeb3..fd29c871a 100644 --- a/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java +++ b/rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java @@ -124,6 +124,7 @@ public class RlsLoadBalancerTest { private final FakeRlsServerImpl fakeRlsServerImpl = new FakeRlsServerImpl(); private final Deque subchannels = new LinkedList<>(); private final FakeThrottler fakeThrottler = new FakeThrottler(); + private final String channelTarget = "channelTarget"; @Mock private Marshaller mockMarshaller; @Captor @@ -296,6 +297,7 @@ public class RlsLoadBalancerTest { PickResult res = picker.pickSubchannel(searchSubchannelArgs); // create subchannel assertThat(res.getStatus().getCode()).isEqualTo(Code.UNAVAILABLE); inOrder.verify(helper).getMetricRecorder(); + inOrder.verify(helper).getChannelTarget(); inOrder.verifyNoMoreInteractions(); verifyFailedPicksCounterAdd(1, 1); verifyNoMoreInteractions(mockMetricRecorder); @@ -319,6 +321,7 @@ public class RlsLoadBalancerTest { assertThat(fallbackSubchannel).isNotNull(); assertThat(subchannelIsReady(fallbackSubchannel)).isTrue(); inOrder.verify(helper).getMetricRecorder(); + inOrder.verify(helper).getChannelTarget(); inOrder.verifyNoMoreInteractions(); verifyLongCounterAdd("grpc.lb.rls.default_target_picks", 1, 1, "defaultTarget", "complete"); verifyNoMoreInteractions(mockMetricRecorder); @@ -393,6 +396,7 @@ public class RlsLoadBalancerTest { FakeSubchannel searchSubchannel = (FakeSubchannel) markReadyAndGetPickResult(inOrder, searchSubchannelArgs).getSubchannel(); inOrder.verify(helper).getMetricRecorder(); + inOrder.verify(helper).getChannelTarget(); inOrder.verifyNoMoreInteractions(); assertThat(subchannelIsReady(searchSubchannel)).isTrue(); assertThat(subchannels.getLast()).isSameInstanceAs(searchSubchannel); @@ -468,6 +472,7 @@ public class RlsLoadBalancerTest { verifyLongCounterAdd("grpc.lb.rls.target_picks", 1, 1, "wilderness", "complete"); inOrder.verify(helper).getMetricRecorder(); + inOrder.verify(helper).getChannelTarget(); inOrder.verifyNoMoreInteractions(); rlsLb.handleNameResolutionError(Status.UNAVAILABLE); @@ -559,7 +564,7 @@ public class RlsLoadBalancerTest { return longCounterInstrument.getName().equals(name); } }), eq(value), - eq(Lists.newArrayList("", "localhost:8972", dataPlaneTargetLabel, pickResult)), + eq(Lists.newArrayList(channelTarget, "localhost:8972", dataPlaneTargetLabel, pickResult)), eq(Lists.newArrayList())); } @@ -573,7 +578,7 @@ public class RlsLoadBalancerTest { return longCounterInstrument.getName().equals("grpc.lb.rls.failed_picks"); } }), eq(value), - eq(Lists.newArrayList("", "localhost:8972")), + eq(Lists.newArrayList(channelTarget, "localhost:8972")), eq(Lists.newArrayList())); } @@ -675,6 +680,11 @@ public class RlsLoadBalancerTest { public MetricRecorder getMetricRecorder() { return mockMetricRecorder; } + + @Override + public String getChannelTarget() { + return channelTarget; + } } private static final class FakeRlsServerImpl -- cgit v1.2.3