Reviewer comments

src/core/ext/filters/client_channel/subchannel.cc

@@ -35,8 +35,6 @@
 #include "src/core/ext/filters/client_channel/subchannel_index.h"
 #include "src/core/ext/filters/client_channel/uri_parser.h"
 #include "src/core/lib/backoff/backoff.h"
-#include "src/core/lib/channel/channel_args.h"
-#include "src/core/lib/channel/channel_trace.h"
 #include "src/core/lib/channel/connected_channel.h"
 #include "src/core/lib/debug/stats.h"
 #include "src/core/lib/gprpp/debug_location.h"

src/core/lib/channel/channel_trace.cc

@@ -134,16 +134,51 @@ void ChannelTrace::AddTraceEventReferencingSubchannel(
 
 namespace {
 
-// returns an allocated string that represents tm according to RFC-3339.
+// returns an allocated string that represents tm according to RFC-3339, and,
+// more specifically, follows:
+// https://developers.google.com/protocol-buffers/docs/proto3#json
+//
+// "Uses RFC 3339, where generated output will always be Z-normalized and uses
+// 0, 3, 6 or 9 fractional digits."
 char* fmt_time(gpr_timespec tm) {
-  char buffer[35];
+  char time_buffer[35];
+  char ns_buffer[11];  // '.' + 9 digits of precision
   struct tm* tm_info = localtime((const time_t*)&tm.tv_sec);
-  strftime(buffer, sizeof(buffer), "%Y-%m-%dT%H:%M:%S", tm_info);
+  strftime(time_buffer, sizeof(time_buffer), "%Y-%m-%dT%H:%M:%S", tm_info);
+  snprintf(ns_buffer, 11, ".%09d", tm.tv_nsec);
+  // This loop trims off trailing zeros by inserting a null character that the
+  // right point. We iterate in chunks of three because we want 0, 3, 6, or 9
+  // fractional digits.
+  for (int i = 7; i >= 1; i -= 3) {
+    if (ns_buffer[i] == '0' && ns_buffer[i + 1] == '0' &&
+        ns_buffer[i + 2] == '0') {
+      ns_buffer[i] = 0;
+      // Specially case in which all fractional digits were 0.
+      if (i == 1) {
+        ns_buffer[0] = 0;
+      }
+    } else {
+      break;
+    }
+  }
   char* full_time_str;
-  gpr_asprintf(&full_time_str, "%s.%09dZ", buffer, tm.tv_nsec);
+  gpr_asprintf(&full_time_str, "%s%sZ", time_buffer, ns_buffer);
   return full_time_str;
 }
 
+const char* severity_string(ChannelTrace::Severity severity) {
+  switch (severity) {
+    case ChannelTrace::Severity::Info:
+      return "INFO";
+    case ChannelTrace::Severity::Warning:
+      return "WARNING";
+    case ChannelTrace::Severity::Error:
+      return "ERROR";
+    default:
+      GPR_UNREACHABLE_CODE(return "UNKNOWN");
+  }
+}
+
 }  // anonymous namespace
 
 void ChannelTrace::TraceEvent::RenderTraceEvent(grpc_json* json) const {
@@ -151,10 +186,9 @@ void ChannelTrace::TraceEvent::RenderTraceEvent(grpc_json* json) const {
   json_iterator = grpc_json_create_child(json_iterator, json, "description",
                                          grpc_slice_to_c_string(data_),
                                          GRPC_JSON_STRING, true);
-  char* trace_level_str;
-  gpr_asprintf(&trace_level_str, "%d", static_cast<int32_t>(severity_));
-  json_iterator = grpc_json_create_child(
-      json_iterator, json, "severity", trace_level_str, GRPC_JSON_NUMBER, true);
+  json_iterator = grpc_json_create_child(json_iterator, json, "severity",
+                                         severity_string(severity_),
+                                         GRPC_JSON_STRING, false);
   json_iterator =
       grpc_json_create_child(json_iterator, json, "timestamp",
                              fmt_time(timestamp_), GRPC_JSON_STRING, true);

src/proto/grpc/channelz/channelz.proto

@@ -109,10 +109,15 @@ message ChannelData {
 message ChannelTraceEvent {
   // High level description of the event.
   string description = 1;
-  // the severity of the trace event. Options are INFO, WARNING, and ERROR,
-  // which are represented by the values 1, 2, and 3, respectively. Any other
-  // value will be treated as ERROR.
-  int32 severity = 2;
+  // The supported severity levels of trace events.
+  enum Severity {
+    UNKNOWN = 0;
+    INFO = 1;
+    WARNING = 2;
+    ERROR = 3;
+  }
+  // the severity of the trace event
+  Severity severity = 2;
   // When this event occurred.
   google.protobuf.Timestamp timestamp = 3;
   // ref of referenced channel or subchannel.

test/core/channel/channel_trace_test.cc

@@ -33,6 +33,11 @@
 #include "test/core/util/test_config.h"
 #include "test/cpp/util/channel_trace_proto_helper.h"
 
+// remove me
+#include <grpc/support/string_util.h>
+#include <stdlib.h>
+#include <string.h>
+
 namespace grpc_core {
 namespace testing {
 namespace {