reviewer feedback

1 files changed, 69 insertions, 53 deletions

diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc
index 95d1f41094..64e206ec63 100644
--- a/src/core/ext/filters/client_channel/client_channel.cc
+++ b/src/core/ext/filters/client_channel/client_channel.cc
@@ -131,8 +131,8 @@ typedef struct client_channel_channel_data {
   grpc_core::UniquePtr<char> info_service_config_json;
   /* backpointer to grpc_channel's channelz node */
   grpc_core::channelz::ClientChannelNode* channelz_channel;
-  /* caches if the last resolution event led to zero addresses */
-  bool previous_resolution_zero_num_addresses;
+  /* caches if the last resolution event contained addresses */
+  bool previous_resolution_contained_addresses;
 } channel_data;
 
 typedef struct {
@@ -403,6 +403,8 @@ static void request_reresolution_locked(void* arg, grpc_error* error) {
   chand->lb_policy->SetReresolutionClosureLocked(&args->closure);
 }
 
+using TraceStringVector = grpc_core::InlinedVector<char*, 3>;
+
 // Creates a new LB policy, replacing any previous one.
 // If the new policy is created successfully, sets *connectivity_state and
 // *connectivity_error to its initial connectivity state; otherwise,
@@ -410,7 +412,7 @@ static void request_reresolution_locked(void* arg, grpc_error* error) {
 static void create_new_lb_policy_locked(
     channel_data* chand, char* lb_policy_name,
     grpc_connectivity_state* connectivity_state,
-    grpc_error** connectivity_error) {
+    grpc_error** connectivity_error, TraceStringVector* trace_strings) {
   grpc_core::LoadBalancingPolicy::Args lb_policy_args;
   lb_policy_args.combiner = chand->combiner;
   lb_policy_args.client_channel_factory = chand->client_channel_factory;
@@ -420,11 +422,21 @@ static void create_new_lb_policy_locked(
           lb_policy_name, lb_policy_args);
   if (GPR_UNLIKELY(new_lb_policy == nullptr)) {
     gpr_log(GPR_ERROR, "could not create LB policy \"%s\"", lb_policy_name);
+    if (chand->channelz_channel != nullptr) {
+      char* str;
+      gpr_asprintf(&str, "Could not create LB policy \'%s\'", lb_policy_name);
+      trace_strings->push_back(str);
+    }
   } else {
     if (grpc_client_channel_trace.enabled()) {
       gpr_log(GPR_INFO, "chand=%p: created new LB policy \"%s\" (%p)", chand,
               lb_policy_name, new_lb_policy.get());
     }
+    if (chand->channelz_channel != nullptr) {
+      char* str;
+      gpr_asprintf(&str, "Created new LB policy \'%s\'", lb_policy_name);
+      trace_strings->push_back(str);
+    }
     // Swap out the LB policy and update the fds in
     // chand->interested_parties.
     if (chand->lb_policy != nullptr) {
@@ -499,6 +511,51 @@ get_service_config_from_resolver_result_locked(channel_data* chand) {
   return grpc_core::UniquePtr<char>(gpr_strdup(service_config_json));
 }
 
+static void check_for_important_resolution_change(
+    channel_data* chand, TraceStringVector* trace_strings) {
+  int resolution_contains_addresses = false;
+  const grpc_arg* channel_arg =
+      grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES);
+  if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) {
+    grpc_lb_addresses* addresses =
+        static_cast<grpc_lb_addresses*>(channel_arg->value.pointer.p);
+    if (addresses->num_addresses > 0) {
+      resolution_contains_addresses = true;
+    }
+  }
+  if (!resolution_contains_addresses &&
+      chand->previous_resolution_contained_addresses) {
+    trace_strings->push_back(gpr_strdup("Address list became empty"));
+  } else if (resolution_contains_addresses &&
+             !chand->previous_resolution_contained_addresses) {
+    trace_strings->push_back(gpr_strdup("Address list became non-empty"));
+  }
+  chand->previous_resolution_contained_addresses =
+      resolution_contains_addresses;
+}
+
+static void concatenate_and_add_channel_trace(
+    channel_data* chand, TraceStringVector* trace_strings) {
+  if (!trace_strings->empty()) {
+    gpr_strvec v;
+    gpr_strvec_init(&v);
+    gpr_strvec_add(&v, gpr_strdup("Resolution event: "));
+    bool is_first = 1;
+    for (size_t i = 0; i < trace_strings->size(); ++i) {
+      if (!is_first) gpr_strvec_add(&v, gpr_strdup(", "));
+      is_first = false;
+      gpr_strvec_add(&v, (*trace_strings)[i]);
+    }
+    char* flat;
+    size_t flat_len = 0;
+    flat = gpr_strvec_flatten(&v, &flat_len);
+    chand->channelz_channel->AddTraceEvent(
+        grpc_core::channelz::ChannelTrace::Severity::Info,
+        grpc_slice_new(flat, flat_len, gpr_free));
+    gpr_strvec_destroy(&v);
+  }
+}
+
 // Callback invoked when a resolver result is available.
 static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
   channel_data* chand = static_cast<channel_data*>(arg);
@@ -529,7 +586,7 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
   // (d) Address resolution that causes a new LB policy to be created.
   //
   // we track a list of strings to eventually be concatenated and traced.
-  grpc_core::InlinedVector<char*, 3> trace_strings;
+  TraceStringVector trace_strings;
   grpc_connectivity_state connectivity_state = GRPC_CHANNEL_TRANSIENT_FAILURE;
   grpc_error* connectivity_error =
       GRPC_ERROR_CREATE_FROM_STATIC_STRING("No load balancing policy");
@@ -564,12 +621,8 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
     } else {
       // Instantiate new LB policy.
       create_new_lb_policy_locked(chand, lb_policy_name.get(),
-                                  &connectivity_state, &connectivity_error);
-      if (chand->channelz_channel != nullptr) {
-        char* str;
-        gpr_asprintf(&str, "Switched LB policy to %s", lb_policy_name.get());
-        trace_strings.push_back(str);
-      }
+                                  &connectivity_state, &connectivity_error,
+                                  &trace_strings);
     }
     // Find service config.
     grpc_core::UniquePtr<char> service_config_json =
@@ -579,54 +632,17 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) {
     // only thing that modifies its value, and it can only be invoked
     // once at any given time.
     if (chand->channelz_channel != nullptr) {
-      if ((service_config_json == nullptr &&
-           chand->info_service_config_json != nullptr) ||
-          (service_config_json != nullptr &&
-           chand->info_service_config_json == nullptr) ||
+      if (((service_config_json == nullptr) !=
+           (chand->info_service_config_json == nullptr)) ||
           (service_config_json != nullptr &&
-           chand->info_service_config_json != nullptr &&
            strcmp(service_config_json.get(),
                   chand->info_service_config_json.get()) != 0)) {
         // TODO(ncteisen): might be worth somehow including a snippet of the
         // config in the trace, at the risk of bloating the trace logs.
-        trace_strings.push_back(gpr_strdup("Service config reloaded"));
-      }
-      int zero_num_addresses = true;
-      const grpc_arg* channel_arg =
-          grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES);
-      if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) {
-        grpc_lb_addresses* addresses =
-            static_cast<grpc_lb_addresses*>(channel_arg->value.pointer.p);
-        if (addresses->num_addresses > 0) {
-          zero_num_addresses = false;
-        }
-      }
-      if (zero_num_addresses &&
-          !chand->previous_resolution_zero_num_addresses) {
-        trace_strings.push_back(gpr_strdup("Address list became empty"));
-      } else if (!zero_num_addresses &&
-                 chand->previous_resolution_zero_num_addresses) {
-        trace_strings.push_back(gpr_strdup("Address list became non-empty"));
-      }
-      chand->previous_resolution_zero_num_addresses = zero_num_addresses;
-      if (!trace_strings.empty()) {
-        gpr_strvec v;
-        gpr_strvec_init(&v);
-        gpr_strvec_add(&v, gpr_strdup("Resolution event: "));
-        bool is_first = 1;
-        for (size_t i = 0; i < trace_strings.size(); ++i) {
-          if (!is_first) gpr_strvec_add(&v, gpr_strdup(", "));
-          is_first = false;
-          gpr_strvec_add(&v, trace_strings[i]);
-        }
-        char* flat;
-        size_t flat_len = 0;
-        flat = gpr_strvec_flatten(&v, &flat_len);
-        chand->channelz_channel->AddTraceEvent(
-            grpc_core::channelz::ChannelTrace::Severity::Info,
-            grpc_slice_new(flat, flat_len, gpr_free));
-        gpr_strvec_destroy(&v);
+        trace_strings.push_back(gpr_strdup("Service config changed"));
       }
+      check_for_important_resolution_change(chand, &trace_strings);
+      concatenate_and_add_channel_trace(chand, &trace_strings);
     }
     // Swap out the data used by cc_get_channel_info().
     gpr_mu_lock(&chand->info_mu);
@@ -796,7 +812,7 @@ static grpc_error* cc_init_channel_elem(grpc_channel_element* elem,
   arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_ENABLE_RETRIES);
   chand->enable_retries = grpc_channel_arg_get_bool(arg, true);
   chand->channelz_channel = nullptr;
-  chand->previous_resolution_zero_num_addresses = true;
+  chand->previous_resolution_contained_addresses = false;
   // Record client channel factory.
   arg = grpc_channel_args_find(args->channel_args,
                                GRPC_ARG_CLIENT_CHANNEL_FACTORY);