From 76ad953c3639cc42f76b04cd8798e4d64da47e04 Mon Sep 17 00:00:00 2001
From: Chengyuan Zhang <chengyuanzhang@google.com>
Date: Fri, 13 Nov 2020 10:28:44 -0800
Subject: [PATCH] interop-testing: fix wrong semantics for RPC failure stats in
 xDS test client (#7618)

The proto field is named as num_failures but its comment is saying it is for number of RPCs that failed to record a remote peer. RPC failed == RPC failed to record a remote peer was true previously (so no existing tests should be affected by this changed) as server completed RPCs immediately. It is no longer true with server capability to keep the call open/delayed.

This change clarifies the proto definition for stats RPC. rpcs_by_peer is for recording RPCs succeeded and num_failures is for RPCs failed. RPCs in the flight when the stats call times out are not counted towards any of the stats.
---
 .../grpc/testing/integration/XdsTestClient.java   | 15 +++++++--------
 .../src/main/proto/grpc/testing/messages.proto    |  8 ++++----
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/interop-testing/src/main/java/io/grpc/testing/integration/XdsTestClient.java b/interop-testing/src/main/java/io/grpc/testing/integration/XdsTestClient.java
index 47cde98969..1cb3b3e00b 100644
--- a/interop-testing/src/main/java/io/grpc/testing/integration/XdsTestClient.java
+++ b/interop-testing/src/main/java/io/grpc/testing/integration/XdsTestClient.java
@@ -326,7 +326,7 @@ public final class XdsTestClient {
 
                 @Override
                 public void onError(Throwable t) {
-                  handleRpcError(requestId, rpcType, hostnameRef.get(), savedWatchers);
+                  handleRpcError(requestId, rpcType, savedWatchers);
                 }
 
                 @Override
@@ -347,7 +347,7 @@ public final class XdsTestClient {
                   if (printResponse) {
                     logger.log(Level.WARNING, "Rpc failed: {0}", t);
                   }
-                  handleRpcError(requestId, rpcType, hostnameRef.get(), savedWatchers);
+                  handleRpcError(requestId, rpcType, savedWatchers);
                 }
 
                 @Override
@@ -395,8 +395,7 @@ public final class XdsTestClient {
         notifyWatchers(watchers, rpcType, requestId, hostname);
       }
 
-      private void handleRpcError(long requestId, RpcType rpcType, String hostname,
-          Set<XdsStatsWatcher> watchers) {
+      private void handleRpcError(long requestId, RpcType rpcType, Set<XdsStatsWatcher> watchers) {
         synchronized (lock) {
           Integer failedBase = rpcsFailedByMethod.get(rpcType.name());
           if (failedBase == null) {
@@ -404,7 +403,7 @@ public final class XdsTestClient {
           }
           rpcsFailedByMethod.put(rpcType.name(), failedBase + 1);
         }
-        notifyWatchers(watchers, rpcType, requestId, hostname);
+        notifyWatchers(watchers, rpcType, requestId, null);
       }
     }
 
@@ -508,7 +507,7 @@ public final class XdsTestClient {
     private final EnumMap<RpcType, Map<String, Integer>> rpcsByTypeAndPeer =
         new EnumMap<>(RpcType.class);
     private final Object lock = new Object();
-    private int noRemotePeer;
+    private int rpcsFailed;
 
     private XdsStatsWatcher(long startId, long endId) {
       latch = new CountDownLatch(Ints.checkedCast(endId - startId));
@@ -539,7 +538,7 @@ public final class XdsTestClient {
               rpcsByTypeAndPeer.put(rpcType, rpcMap);
             }
           } else {
-            noRemotePeer += 1;
+            rpcsFailed += 1;
           }
           latch.countDown();
         }
@@ -565,7 +564,7 @@ public final class XdsTestClient {
           rpcs.putAllRpcsByPeer(entry.getValue());
           builder.putRpcsByMethod(getRpcTypeString(entry.getKey()), rpcs.build());
         }
-        builder.setNumFailures(noRemotePeer + (int) latch.getCount());
+        builder.setNumFailures(rpcsFailed);
       }
       return builder.build();
     }
diff --git a/interop-testing/src/main/proto/grpc/testing/messages.proto b/interop-testing/src/main/proto/grpc/testing/messages.proto
index 797adb72d6..81724a5062 100644
--- a/interop-testing/src/main/proto/grpc/testing/messages.proto
+++ b/interop-testing/src/main/proto/grpc/testing/messages.proto
@@ -191,15 +191,15 @@ message LoadBalancerStatsRequest {
 }
 
 message LoadBalancerStatsResponse {
-  // The number of completed RPCs for each peer.
+  // The number of RPCs succeeded for each peer.
   map<string, int32> rpcs_by_peer = 1;
-  // The number of RPCs that failed to record a remote peer.
+  // The number of RPCs that failed.
   int32 num_failures = 2;
   message RpcsByPeer {
-    // The number of completed RPCs for each peer.
+    // The number of RPCs succeeded for each peer.
     map<string, int32> rpcs_by_peer = 1;
   }
-  // The number of completed RPCs for each type (UnaryCall or EmptyCall).
+  // The number of RPCs succeeded for each type (UnaryCall or EmptyCall).
   map<string, RpcsByPeer> rpcs_by_method = 3;
 }