Fixup: Change the fallback order to prefer custom metrics over app util

Author: sauravzg

xds/src/main/java/io/grpc/xds/WeightedRoundRobinLoadBalancer.java

@@ -393,18 +393,21 @@ public void onLoadReport(MetricReport report) {
       }
 
       /**
-       * Returns the utilization value computed from the specified metric names. If the application
-       * utilization is present and valid, it is returned. Otherwise, the maximum of the custom
-       * metrics specified is returned. If none of the custom metrics are present, the CPU
+       * Returns the utilization value computed from the specified metric names. If the custom
+       * metrics are present and valid, the maximum of the custom metrics is returned. Otherwise,
+       * if application utilization is > 0, it is returned. If neither are present, the CPU
        * utilization is returned.
        */
       private double getUtilization(MetricReport report, ImmutableList<String> metricNames) {
+        OptionalDouble customUtil = getCustomMetricUtilization(report, metricNames);
+        if (customUtil.isPresent()) {
+          return customUtil.getAsDouble();
+        }
         double appUtil = report.getApplicationUtilization();
         if (appUtil > 0) {
           return appUtil;
         }
-        return getCustomMetricUtilization(report, metricNames)
-            .orElse(report.getCpuUtilization());
+        return report.getCpuUtilization();
       }
 
       /**

xds/src/test/java/io/grpc/xds/WeightedRoundRobinLoadBalancerTest.java

@@ -1334,7 +1334,7 @@ public void metricWithRealChannel() throws Exception {
 
 
   @Test
-  public void customMetric_priority_appUtilStillPreferred() {
+  public void customMetric_priority_overAppUtil() {
     weightedConfig = WeightedRoundRobinLoadBalancerConfig.newBuilder().setBlackoutPeriodNanos(0)
         .setMetricNamesForComputingUtilization(ImmutableList.of("named_metrics.cost")).build();
     wrr = new WeightedRoundRobinLoadBalancer(helper, fakeClock.getDeadlineTicker());
@@ -1359,6 +1359,42 @@ public void customMetric_priority_appUtilStillPreferred() {
     MetricReport report = InternalCallMetricRecorder.createMetricReport(0.1, 0.8, 0.1, 1, 0,
         new HashMap<>(), new HashMap<>(), namedMetrics);
     listener.onLoadReport(report);
+    // Custom metrics now take priority over app_util
+    // qps=1, util=0.5 -> weight=2.0
+    fakeClock.forwardTime(1100, TimeUnit.MILLISECONDS);
+    verify(mockMetricRecorder).recordDoubleHistogram(
+        argThat(instr -> instr.getName().equals("grpc.lb.wrr.endpoint_weights")), eq(2.0), any(),
+        any());
+  }
+
+  @Test
+  public void customMetric_invalid_fallbackToAppUtil() {
+    weightedConfig = WeightedRoundRobinLoadBalancerConfig.newBuilder().setBlackoutPeriodNanos(0)
+        .setMetricNamesForComputingUtilization(ImmutableList.of("named_metrics.cost")).build();
+    wrr = new WeightedRoundRobinLoadBalancer(helper, fakeClock.getDeadlineTicker());
+
+    syncContext.execute(
+        () -> wrr.acceptResolvedAddresses(ResolvedAddresses.newBuilder().setAddresses(servers)
+            .setLoadBalancingPolicyConfig(weightedConfig).setAttributes(affinity).build()));
+
+    Iterator<Subchannel> it = subchannels.values().iterator();
+    Subchannel readySubchannel = it.next();
+    getSubchannelStateListener(readySubchannel)
+        .onSubchannelState(ConnectivityStateInfo.forNonError(ConnectivityState.READY));
+
+    WeightedChildLbState weightedChild =
+        (WeightedChildLbState) wrr.getChildLbStates().iterator().next();
+    WeightedChildLbState.OrcaReportListener listener = weightedChild.getOrCreateOrcaListener(
+        weightedConfig.errorUtilizationPenalty, weightedConfig.metricNamesForComputingUtilization);
+
+    // custom metric is NaN, App util = 0.8
+    Map<String, Double> namedMetrics = new HashMap<>();
+    namedMetrics.put("cost", Double.NaN);
+    MetricReport report = InternalCallMetricRecorder.createMetricReport(0.1, 0.8, 0.1, 1, 0,
+        new HashMap<>(), new HashMap<>(), namedMetrics);
+    listener.onLoadReport(report);
+
+    // Should fallback to App Util (0.8)
     // qps=1, util=0.8 -> weight=1.25
     fakeClock.forwardTime(1100, TimeUnit.MILLISECONDS);
     verify(mockMetricRecorder).recordDoubleHistogram(