reviewer feedback

3 files changed, 22 insertions, 11 deletions

diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc
index 3227fe53a1..fe81acb617 100644
--- a/src/core/lib/channel/channel_trace.cc
+++ b/src/core/lib/channel/channel_trace.cc
@@ -48,18 +48,16 @@ ChannelTrace::TraceEvent::TraceEvent(Severity severity, grpc_slice data,
       timestamp_(grpc_millis_to_timespec(grpc_core::ExecCtx::Get()->Now(),
                                          GPR_CLOCK_REALTIME)),
       next_(nullptr),
-      referenced_entity_(std::move(referenced_entity)) {
-  memory_usage_ = sizeof(TraceEvent) + grpc_slice_memory_usage(data);
-}
+      referenced_entity_(std::move(referenced_entity)),
+      memory_usage_(sizeof(TraceEvent) + grpc_slice_memory_usage(data)) {}
 
 ChannelTrace::TraceEvent::TraceEvent(Severity severity, grpc_slice data)
     : severity_(severity),
       data_(data),
       timestamp_(grpc_millis_to_timespec(grpc_core::ExecCtx::Get()->Now(),
                                          GPR_CLOCK_REALTIME)),
-      next_(nullptr) {
-  memory_usage_ = sizeof(TraceEvent) + grpc_slice_memory_usage(data);
-}
+      next_(nullptr),
+      memory_usage_(sizeof(TraceEvent) + grpc_slice_memory_usage(data)) {}
 
 ChannelTrace::TraceEvent::~TraceEvent() { grpc_slice_unref_internal(data_); }
 
diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h
index 0e9ce5f648..8ff91ee8c8 100644
--- a/src/core/lib/channel/channel_trace.h
+++ b/src/core/lib/channel/channel_trace.h
@@ -30,6 +30,10 @@
 namespace grpc_core {
 namespace channelz {
 
+namespace testing {
+size_t GetSizeofTraceEvent(void);
+}
+
 class BaseNode;
 
 // Object used to hold live data for a channel. This data is exposed via the
@@ -49,6 +53,12 @@ class ChannelTrace {
 
   // Adds a new trace event to the tracing object
   //
+  // NOTE: each ChannelTrace tracks the memory used by its list of trace
+  // events, so adding an event with a large amount of data could cause other
+  // trace event to be evicted. If a single trace is larger than the limit, it
+  // will cause all events to be evicted. The limit is set with the arg:
+  // GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE.
+  //
   // TODO(ncteisen): as this call is used more and more throughout the gRPC
   // stack, determine if it makes more sense to accept a char* instead of a
   // slice.
@@ -59,9 +69,9 @@ class ChannelTrace {
   // channel has created a new subchannel, then it would record that with
   // a TraceEvent referencing the new subchannel.
   //
-  // TODO(ncteisen): as this call is used more and more throughout the gRPC
-  // stack, determine if it makes more sense to accept a char* instead of a
-  // slice.
+  // NOTE: see the note in the method above.
+  //
+  // TODO(ncteisen): see the todo in the method above.
   void AddTraceEventWithReference(Severity severity, grpc_slice data,
                                   RefCountedPtr<BaseNode> referenced_entity);
 
@@ -70,6 +80,8 @@ class ChannelTrace {
   grpc_json* RenderJson() const;
 
  private:
+  friend size_t testing::GetSizeofTraceEvent(void);
+
   // Private class to encapsulate all the data and bookkeeping needed for a
   // a trace event.
   class TraceEvent {
@@ -92,7 +104,7 @@ class ChannelTrace {
     TraceEvent* next() const { return next_; }
     void set_next(TraceEvent* next) { next_ = next; }
 
-    size_t memory_usage() { return memory_usage_; }
+    size_t memory_usage() const { return memory_usage_; }
 
    private:
     Severity severity_;
diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h
index 97eef9868d..30ec66ced2 100644
--- a/src/core/lib/channel/channelz.h
+++ b/src/core/lib/channel/channelz.h
@@ -43,7 +43,8 @@
  * GRPC_ARG_ENABLE_CHANNELZ is set, it will override this default value. */
 #define GRPC_ENABLE_CHANNELZ_DEFAULT false
 
-/** This is the default value for TODO. If
+/** This is the default value for the maximum amount of memory used by trace
+ * events per channel trace node. If
  * GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE is set, it will override
  * this default value. */
 #define GRPC_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE_DEFAULT 0