core,netty,okhttp,testing: nest TransportStats inside SocketStats for channelz (#4190)

Transport ststistics should really be a child member of SocketStats.
While we're at it, let's add the local and remote SocketAddress to
SocketStats, with a test.

2 files changed, 27 insertions, 14 deletions

diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
index 9ace0f74a..ac3187948 100644
--- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
+++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
@@ -37,7 +37,8 @@ import io.grpc.MethodDescriptor.MethodType;
 import io.grpc.Status;
 import io.grpc.Status.Code;
 import io.grpc.StatusException;
-import io.grpc.internal.Channelz.TransportStats;
+import io.grpc.internal.Channelz.Security;
+import io.grpc.internal.Channelz.SocketStats;
 import io.grpc.internal.ConnectionClientTransport;
 import io.grpc.internal.GrpcUtil;
 import io.grpc.internal.Http2Ping;
@@ -893,10 +894,14 @@ class OkHttpClientTransport implements ConnectionClientTransport {
   }
 
   @Override
-  public ListenableFuture<TransportStats> getStats() {
+  public ListenableFuture<SocketStats> getStats() {
     synchronized (lock) {
-      SettableFuture<TransportStats> ret = SettableFuture.create();
-      ret.set(transportTracer.getStats());
+      SettableFuture<SocketStats> ret = SettableFuture.create();
+      ret.set(new SocketStats(
+          transportTracer.getStats(),
+          socket.getLocalSocketAddress(),
+          socket.getRemoteSocketAddress(),
+          new Security()));
       return ret;
     }
   }
diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
index 5fc4961af..1a323af40 100644
--- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
+++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
@@ -61,10 +61,12 @@ import io.grpc.Status;
 import io.grpc.Status.Code;
 import io.grpc.StatusException;
 import io.grpc.internal.AbstractStream;
+import io.grpc.internal.Channelz.SocketStats;
 import io.grpc.internal.Channelz.TransportStats;
 import io.grpc.internal.ClientStreamListener;
 import io.grpc.internal.ClientTransport;
 import io.grpc.internal.GrpcUtil;
+import io.grpc.internal.Instrumented;
 import io.grpc.internal.ManagedClientTransport;
 import io.grpc.internal.TransportTracer;
 import io.grpc.okhttp.OkHttpClientTransport.ClientFrameHandler;
@@ -88,6 +90,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -547,7 +550,7 @@ public class OkHttpClientTransportTest {
   @Test
   public void transportTracer_windowSizeDefault() throws Exception {
     initTransport();
-    TransportStats stats = clientTransport.getStats().get();
+    TransportStats stats = getTransportStats(clientTransport);
     assertEquals(Utils.DEFAULT_WINDOW_SIZE, stats.remoteFlowControlWindow);
     // okhttp does not track local window sizes
     assertEquals(-1, stats.localFlowControlWindow);
@@ -556,13 +559,13 @@ public class OkHttpClientTransportTest {
   @Test
   public void transportTracer_windowSize_remote() throws Exception {
     initTransport();
-    TransportStats before = clientTransport.getStats().get();
+    TransportStats before = getTransportStats(clientTransport);
     assertEquals(Utils.DEFAULT_WINDOW_SIZE, before.remoteFlowControlWindow);
     // okhttp does not track local window sizes
     assertEquals(-1, before.localFlowControlWindow);
 
     frameHandler().windowUpdate(0, 1000);
-    TransportStats after = clientTransport.getStats().get();
+    TransportStats after = getTransportStats(clientTransport);
     assertEquals(Utils.DEFAULT_WINDOW_SIZE + 1000, after.remoteFlowControlWindow);
     // okhttp does not track local window sizes
     assertEquals(-1, after.localFlowControlWindow);
@@ -1278,11 +1281,11 @@ public class OkHttpClientTransportTest {
     initTransport();
     PingCallbackImpl callback1 = new PingCallbackImpl();
     clientTransport.ping(callback1, MoreExecutors.directExecutor());
-    assertEquals(1, clientTransport.getStats().get().keepAlivesSent);
+    assertEquals(1, getTransportStats(clientTransport).keepAlivesSent);
     // add'l ping will be added as listener to outstanding operation
     PingCallbackImpl callback2 = new PingCallbackImpl();
     clientTransport.ping(callback2, MoreExecutors.directExecutor());
-    assertEquals(1, clientTransport.getStats().get().keepAlivesSent);
+    assertEquals(1, getTransportStats(clientTransport).keepAlivesSent);
 
     ArgumentCaptor<Integer> captor1 = ArgumentCaptor.forClass(int.class);
     ArgumentCaptor<Integer> captor2 = ArgumentCaptor.forClass(int.class);
@@ -1315,7 +1318,7 @@ public class OkHttpClientTransportTest {
     // now that previous ping is done, next request returns a different future
     callback1 = new PingCallbackImpl();
     clientTransport.ping(callback1, MoreExecutors.directExecutor());
-    assertEquals(2, clientTransport.getStats().get().keepAlivesSent);
+    assertEquals(2, getTransportStats(clientTransport).keepAlivesSent);
     assertEquals(0, callback1.invocationCount);
     shutdownAndVerify();
   }
@@ -1325,7 +1328,7 @@ public class OkHttpClientTransportTest {
     initTransport();
     PingCallbackImpl callback = new PingCallbackImpl();
     clientTransport.ping(callback, MoreExecutors.directExecutor());
-    assertEquals(1, clientTransport.getStats().get().keepAlivesSent);
+    assertEquals(1, getTransportStats(clientTransport).keepAlivesSent);
     assertEquals(0, callback.invocationCount);
 
     clientTransport.shutdown(SHUTDOWN_REASON);
@@ -1337,7 +1340,7 @@ public class OkHttpClientTransportTest {
     // now that handler is in terminal state, all future pings fail immediately
     callback = new PingCallbackImpl();
     clientTransport.ping(callback, MoreExecutors.directExecutor());
-    assertEquals(1, clientTransport.getStats().get().keepAlivesSent);
+    assertEquals(1, getTransportStats(clientTransport).keepAlivesSent);
     assertEquals(1, callback.invocationCount);
     assertTrue(callback.failureCause instanceof StatusException);
     assertSame(SHUTDOWN_REASON, ((StatusException) callback.failureCause).getStatus());
@@ -1349,7 +1352,7 @@ public class OkHttpClientTransportTest {
     initTransport();
     PingCallbackImpl callback = new PingCallbackImpl();
     clientTransport.ping(callback, MoreExecutors.directExecutor());
-    assertEquals(1, clientTransport.getStats().get().keepAlivesSent);
+    assertEquals(1, getTransportStats(clientTransport).keepAlivesSent);
     assertEquals(0, callback.invocationCount);
 
     clientTransport.onException(new IOException());
@@ -1362,7 +1365,7 @@ public class OkHttpClientTransportTest {
     // now that handler is in terminal state, all future pings fail immediately
     callback = new PingCallbackImpl();
     clientTransport.ping(callback, MoreExecutors.directExecutor());
-    assertEquals(1, clientTransport.getStats().get().keepAlivesSent);
+    assertEquals(1, getTransportStats(clientTransport).keepAlivesSent);
     assertEquals(1, callback.invocationCount);
     assertTrue(callback.failureCause instanceof StatusException);
     assertEquals(Status.Code.UNAVAILABLE,
@@ -1970,4 +1973,9 @@ public class OkHttpClientTransportTest {
       delayed.set(null);
     }
   }
+
+  private static TransportStats getTransportStats(Instrumented<SocketStats> obj)
+      throws ExecutionException, InterruptedException {
+    return obj.getStats().get().data;
+  }
 }