reviewer feedback, debuging helper

6 files changed, 42 insertions, 21 deletions

diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
index d7934ab7aa..d4665d5245 100644
--- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
+++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
@@ -31,6 +31,7 @@
 #include <grpc/support/log.h>
 #include <grpc/support/string_util.h>
 
+#include "src/core/ext/filters/client_channel/uri_parser.h"
 #include "src/core/ext/transport/chttp2/transport/frame_data.h"
 #include "src/core/ext/transport/chttp2/transport/internal.h"
 #include "src/core/ext/transport/chttp2/transport/varint.h"
@@ -38,6 +39,7 @@
 #include "src/core/lib/compression/stream_compression.h"
 #include "src/core/lib/debug/stats.h"
 #include "src/core/lib/gpr/env.h"
+#include "src/core/lib/gpr/host_port.h"
 #include "src/core/lib/gpr/string.h"
 #include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/http/parser.h"
@@ -395,9 +397,23 @@ static bool read_channel_args(grpc_chttp2_transport* t,
     }
   }
   if (channelz_enabled) {
+    // pick out just the host port (maybe trims off scheme prefix).
+    grpc_uri* uri = grpc_uri_parse(t->peer_string, false);
+    GPR_ASSERT(uri != nullptr);
+    const char* host_port = uri->path;
+    if (*host_port == '/') ++host_port;
+    char* host;
+    char* port;
+    GPR_ASSERT(gpr_split_host_port(host_port, &host, &port));
+    int port_num = -1;
+    if (port != nullptr) {
+      port_num = atoi(port);
+    }
     t->channelz_socket =
         grpc_core::MakeRefCounted<grpc_core::channelz::SocketNode>(
-            (t->peer_string));
+            grpc_core::UniquePtr<char>(host), port_num);
+    grpc_uri_destroy(uri);
+    gpr_free(port);
   }
   return enable_bdp;
 }
diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index bf8acebcd7..6da8a172b5 100644
--- a/src/core/lib/channel/channelz.cc
+++ b/src/core/lib/channel/channelz.cc
@@ -30,7 +30,6 @@
 
 #include "src/core/lib/channel/channelz_registry.h"
 #include "src/core/lib/channel/status_util.h"
-#include "src/core/lib/gpr/host_port.h"
 #include "src/core/lib/gpr/string.h"
 #include "src/core/lib/gpr/useful.h"
 #include "src/core/lib/gprpp/memory.h"
@@ -278,20 +277,10 @@ grpc_json* ServerNode::RenderJson() {
   return top_level_json;
 }
 
-SocketNode::SocketNode(const char* remote_peer_string)
-    : BaseNode(EntityType::kSocket) {
-  const char* ipv6_prefix = "ipv6:";
-  const int ipv6_prefix_len = 5;
-  char* host;
-  char* port;
-  int offset = (strncmp(remote_peer_string, ipv6_prefix, ipv6_prefix_len) == 0)
-                   ? ipv6_prefix_len
-                   : 0;
-  GPR_ASSERT(gpr_split_host_port(remote_peer_string + offset, &host, &port));
-  remote_host_ = UniquePtr<char>(host);
-  remote_port_ = atoi(port);
-  gpr_free(port);
-}
+SocketNode::SocketNode(UniquePtr<char> remote_host, int remote_port)
+    : BaseNode(EntityType::kSocket),
+      remote_host_(std::move(remote_host)),
+      remote_port_(remote_port) {}
 
 void SocketNode::RecordStreamStartedFromLocal() {
   gpr_atm_no_barrier_fetch_add(&streams_started_, static_cast<gpr_atm>(1));
diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h
index ba10b1a5bf..404d8341ff 100644
--- a/src/core/lib/channel/channelz.h
+++ b/src/core/lib/channel/channelz.h
@@ -232,7 +232,7 @@ class ServerNode : public BaseNode {
 // Handles channelz bookkeeping for sockets
 class SocketNode : public BaseNode {
  public:
-  SocketNode(const char* remote_peer_string);
+  SocketNode(UniquePtr<char> remote_host, int remote_port);
   ~SocketNode() override {}
 
   grpc_json* RenderJson() override;
diff --git a/src/core/lib/channel/channelz_registry.cc b/src/core/lib/channel/channelz_registry.cc
index 8fc1b58adc..bc23b90a66 100644
--- a/src/core/lib/channel/channelz_registry.cc
+++ b/src/core/lib/channel/channelz_registry.cc
@@ -210,6 +210,17 @@ char* ChannelzRegistry::InternalGetServers(intptr_t start_server_id) {
   return json_str;
 }
 
+void ChannelzRegistry::InternalLogAllEntities() {
+  MutexLock lock(&mu_);
+  for (size_t i = 0; i < entities_.size(); ++i) {
+    if (entities_[i] != nullptr) {
+      char* json = entities_[i]->RenderJsonString();
+      gpr_log(GPR_INFO, "%s", json);
+      gpr_free(json);
+    }
+  }
+}
+
 }  // namespace channelz
 }  // namespace grpc_core
 
diff --git a/src/core/lib/channel/channelz_registry.h b/src/core/lib/channel/channelz_registry.h
index 326f0201c7..73b330785d 100644
--- a/src/core/lib/channel/channelz_registry.h
+++ b/src/core/lib/channel/channelz_registry.h
@@ -62,6 +62,10 @@ class ChannelzRegistry {
     return Default()->InternalGetServers(start_server_id);
   }
 
+  // Test only helper function to dump the JSON representation to std out.
+  // This can aid in debugging channelz code.
+  static void LogAllEntities() { Default()->InternalLogAllEntities(); }
+
  private:
   GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
   GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
@@ -96,6 +100,8 @@ class ChannelzRegistry {
   // Else, will return idx of the first uuid higher than the target.
   int FindByUuidLocked(intptr_t uuid, bool direct_hit_needed);
 
+  void InternalLogAllEntities();
+
   // protects members
   gpr_mu mu_;
   InlinedVector<BaseNode*, 20> entities_;
diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc
index 7c61b7910b..f860bdfc17 100644
--- a/test/core/end2end/tests/channelz.cc
+++ b/test/core/end2end/tests/channelz.cc
@@ -29,6 +29,7 @@
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 #include <grpc/support/time.h>
+#include "src/core/lib/channel/channelz_registry.h"
 #include "src/core/lib/gpr/string.h"
 #include "test/core/end2end/cq_verifier.h"
 
@@ -238,7 +239,6 @@ static void test_channelz(grpc_end2end_test_config config) {
 
   json = channelz_channel->RenderJsonString();
   GPR_ASSERT(json != nullptr);
-  gpr_log(GPR_INFO, "%s", json);
   GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"2\""));
   GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\""));
   GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"1\""));
@@ -250,7 +250,6 @@ static void test_channelz(grpc_end2end_test_config config) {
 
   json = channelz_server->RenderJsonString();
   GPR_ASSERT(json != nullptr);
-  gpr_log(GPR_INFO, "%s", json);
   GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"2\""));
   GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\""));
   GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"1\""));
@@ -264,6 +263,8 @@ static void test_channelz(grpc_end2end_test_config config) {
   GPR_ASSERT(nullptr != strstr(json, "\"socketRef\":"));
   gpr_free(json);
 
+  grpc_core::channelz::ChannelzRegistry::LogAllEntities();
+
   end_test(&f);
   config.tear_down_data(&f);
 }
@@ -292,7 +293,6 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) {
 
   char* json = channelz_channel->RenderJsonString();
   GPR_ASSERT(json != nullptr);
-  gpr_log(GPR_INFO, "%s", json);
   GPR_ASSERT(nullptr != strstr(json, "\"trace\""));
   GPR_ASSERT(nullptr != strstr(json, "\"description\":\"Channel created\""));
   GPR_ASSERT(nullptr != strstr(json, "\"severity\":\"CT_INFO\""));
@@ -300,7 +300,6 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) {
 
   json = channelz_server->RenderJsonString();
   GPR_ASSERT(json != nullptr);
-  gpr_log(GPR_INFO, "%s", json);
   GPR_ASSERT(nullptr != strstr(json, "\"trace\""));
   GPR_ASSERT(nullptr != strstr(json, "\"description\":\"Server created\""));
   GPR_ASSERT(nullptr != strstr(json, "\"severity\":\"CT_INFO\""));