reviewer comments

reviewable/pr16055/r8
ncteisen 7 years ago
parent 2428109608
commit 58db94e5d9
  1. 30
      src/core/ext/filters/client_channel/client_channel_channelz.cc
  2. 11
      src/core/ext/filters/client_channel/client_channel_channelz.h
  3. 4
      src/core/ext/filters/client_channel/subchannel.cc
  4. 39
      src/core/lib/channel/channelz.cc
  5. 41
      src/core/lib/channel/channelz.h
  6. 6
      src/core/lib/surface/call.cc
  7. 4
      src/core/lib/surface/channel.cc
  8. 25
      test/core/channel/channel_trace_test.cc
  9. 53
      test/core/channel/channelz_test.cc

@ -123,9 +123,14 @@ grpc_json* ClientChannelNode::RenderJson() {
GPR_ASSERT(target_view() != nullptr);
grpc_json_create_child(nullptr, json, "target", target_view(),
GRPC_JSON_STRING, false);
// as CallCountingAndTracingNode to populate trace and call count data.
counter_and_tracer()->PopulateTrace(json);
counter_and_tracer()->PopulateCallData(json);
// fill in the channel trace if applicable
grpc_json* trace_json = trace()->RenderJson();
if (trace_json != nullptr) {
trace_json->key = "trace"; // this object is named trace in channelz.proto
grpc_json_link_child(json, trace_json, nullptr);
}
// ask CallCountingHelper to populate trace and call count data.
call_counter()->PopulateCallData(json);
// reset to the top level
json = top_level_json;
PopulateChildRefs(json);
@ -150,11 +155,12 @@ SubchannelNode::SubchannelNode(grpc_subchannel* subchannel,
size_t channel_tracer_max_nodes)
: BaseNode(EntityType::kSubchannel),
subchannel_(subchannel),
target_(
UniquePtr<char>(gpr_strdup(grpc_subchannel_get_target(subchannel_)))),
counter_and_tracer_(channel_tracer_max_nodes) {}
target_(UniquePtr<char>(
gpr_strdup(grpc_subchannel_get_target(subchannel_)))) {
trace_.Init(channel_tracer_max_nodes);
}
SubchannelNode::~SubchannelNode() {}
SubchannelNode::~SubchannelNode() { trace_.Destroy(); }
void SubchannelNode::PopulateConnectivityState(grpc_json* json) {
grpc_connectivity_state state;
@ -192,8 +198,14 @@ grpc_json* SubchannelNode::RenderJson() {
GPR_ASSERT(target_.get() != nullptr);
grpc_json_create_child(nullptr, json, "target", target_.get(),
GRPC_JSON_STRING, false);
counter_and_tracer_.PopulateTrace(json);
counter_and_tracer_.PopulateCallData(json);
// fill in the channel trace if applicable
grpc_json* trace_json = trace_->RenderJson();
if (trace_json != nullptr) {
trace_json->key = "trace"; // this object is named trace in channelz.proto
grpc_json_link_child(json, trace_json, nullptr);
}
// ask CallCountingHelper to populate trace and call count data.
call_counter_.PopulateCallData(json);
return top_level_json;
}

@ -76,14 +76,17 @@ class SubchannelNode : public BaseNode {
grpc_json* RenderJson() override;
CallCountingAndTracingNode* counter_and_tracer() {
return &counter_and_tracer_;
}
// proxy methods to composed classes.
ChannelTrace* trace() { return trace_.get(); }
void RecordCallStarted() { call_counter_.RecordCallStarted(); }
void RecordCallFailed() { call_counter_.RecordCallFailed(); }
void RecordCallSucceeded() { call_counter_.RecordCallSucceeded(); }
private:
grpc_subchannel* subchannel_;
UniquePtr<char> target_;
CallCountingAndTracingNode counter_and_tracer_;
CallCountingHelper call_counter_;
ManualConstructor<ChannelTrace> trace_;
void PopulateConnectivityState(grpc_json* json);
};

@ -183,7 +183,7 @@ static void connection_destroy(void* arg, grpc_error* error) {
static void subchannel_destroy(void* arg, grpc_error* error) {
grpc_subchannel* c = static_cast<grpc_subchannel*>(arg);
if (c->channelz_subchannel != nullptr) {
c->channelz_subchannel->counter_and_tracer()->trace()->AddTraceEvent(
c->channelz_subchannel->trace()->AddTraceEvent(
grpc_core::channelz::ChannelTrace::Severity::Info,
grpc_slice_from_static_string("Subchannel destroyed"));
c->channelz_subchannel->MarkSubchannelDestroyed();
@ -397,7 +397,7 @@ grpc_subchannel* grpc_subchannel_create(grpc_connector* connector,
c->channelz_subchannel =
grpc_core::MakeRefCounted<grpc_core::channelz::SubchannelNode>(
c, channel_tracer_max_nodes);
c->channelz_subchannel->counter_and_tracer()->trace()->AddTraceEvent(
c->channelz_subchannel->trace()->AddTraceEvent(
grpc_core::channelz::ChannelTrace::Severity::Info,
grpc_slice_from_static_string("Subchannel created"));
}

@ -53,33 +53,20 @@ char* BaseNode::RenderJsonString() {
return json_str;
}
CallCountingAndTracingNode::CallCountingAndTracingNode(
size_t channel_tracer_max_nodes) {
trace_.Init(channel_tracer_max_nodes);
CallCountingHelper::CallCountingHelper() {
gpr_atm_no_barrier_store(&last_call_started_millis_,
(gpr_atm)ExecCtx::Get()->Now());
}
CallCountingAndTracingNode::~CallCountingAndTracingNode() { trace_.Destroy(); }
CallCountingHelper::~CallCountingHelper() {}
void CallCountingAndTracingNode::RecordCallStarted() {
void CallCountingHelper::RecordCallStarted() {
gpr_atm_no_barrier_fetch_add(&calls_started_, (gpr_atm)1);
gpr_atm_no_barrier_store(&last_call_started_millis_,
(gpr_atm)ExecCtx::Get()->Now());
}
void CallCountingAndTracingNode::PopulateTrace(grpc_json* json) {
// fill in the channel trace if applicable
grpc_json* trace_json = trace_->RenderJson();
if (trace_json != nullptr) {
// we manually link up and fill the child since it was created for us in
// ChannelTrace::RenderJson
trace_json->key = "trace"; // this object is named trace in channelz.proto
grpc_json_link_child(json, trace_json, nullptr);
}
}
void CallCountingAndTracingNode::PopulateCallData(grpc_json* json) {
void CallCountingHelper::PopulateCallData(grpc_json* json) {
grpc_json* json_iterator = nullptr;
if (calls_started_ != 0) {
json_iterator = grpc_json_add_number_string_child(
@ -105,10 +92,11 @@ ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes,
: BaseNode(is_top_level_channel ? EntityType::kTopLevelChannel
: EntityType::kInternalChannel),
channel_(channel),
target_(UniquePtr<char>(grpc_channel_get_target(channel_))),
counter_and_tracer_(channel_tracer_max_nodes) {}
target_(UniquePtr<char>(grpc_channel_get_target(channel_))) {
trace_.Init(channel_tracer_max_nodes);
}
ChannelNode::~ChannelNode() {}
ChannelNode::~ChannelNode() { trace_.Destroy(); }
grpc_json* ChannelNode::RenderJson() {
// We need to track these three json objects to build our object
@ -134,9 +122,14 @@ grpc_json* ChannelNode::RenderJson() {
GPR_ASSERT(target_.get() != nullptr);
grpc_json_create_child(nullptr, json, "target", target_.get(),
GRPC_JSON_STRING, false);
// as CallCountingAndTracingNode to populate trace and call count data.
counter_and_tracer_.PopulateTrace(json);
counter_and_tracer_.PopulateCallData(json);
// fill in the channel trace if applicable
grpc_json* trace_json = trace_->RenderJson();
if (trace_json != nullptr) {
trace_json->key = "trace"; // this object is named trace in channelz.proto
grpc_json_link_child(json, trace_json, nullptr);
}
// ask CallCountingHelper to populate trace and call count data.
call_counter_.PopulateCallData(json);
return top_level_json;
}

@ -43,7 +43,7 @@ namespace grpc_core {
namespace channelz {
namespace testing {
class CallCountingAndTracingNodePeer;
class CallCountingHelperPeer;
}
// base class for all channelz entities
@ -60,7 +60,7 @@ class BaseNode : public RefCounted<BaseNode> {
kSocket,
};
BaseNode(EntityType type);
explicit BaseNode(EntityType type);
virtual ~BaseNode();
// All children must implement this function.
@ -74,22 +74,20 @@ class BaseNode : public RefCounted<BaseNode> {
intptr_t uuid() const { return uuid_; }
private:
friend class ChannelTrace;
EntityType type_;
const EntityType type_;
const intptr_t uuid_;
};
// This class is the parent for the channelz entities that deal with Channels
// This class is a helper class for channelz entities that deal with Channels
// Subchannels, and Servers, since those have similar proto definitions.
// This class has the ability to:
// - track calls_{started,succeeded,failed}
// - track last_call_started_timestamp
// - hold the channel trace.
// - perform common rendering.
class CallCountingAndTracingNode {
// - perform rendering of the above items
class CallCountingHelper {
public:
CallCountingAndTracingNode(size_t channel_tracer_max_nodes);
~CallCountingAndTracingNode();
CallCountingHelper();
~CallCountingHelper();
void RecordCallStarted();
void RecordCallFailed() {
@ -98,23 +96,18 @@ class CallCountingAndTracingNode {
void RecordCallSucceeded() {
gpr_atm_no_barrier_fetch_add(&calls_succeeded_, (gpr_atm(1)));
}
ChannelTrace* trace() { return trace_.get(); }
// Common rendering of the channel trace.
void PopulateTrace(grpc_json* json);
// Common rendering of the call count data and last_call_started_timestamp.
void PopulateCallData(grpc_json* json);
private:
// testing peer friend.
friend class testing::CallCountingAndTracingNodePeer;
friend class testing::CallCountingHelperPeer;
gpr_atm calls_started_ = 0;
gpr_atm calls_succeeded_ = 0;
gpr_atm calls_failed_ = 0;
gpr_atm last_call_started_millis_ = 0;
ManualConstructor<ChannelTrace> trace_;
};
// Handles channelz bookkeeping for channels
@ -137,25 +130,31 @@ class ChannelNode : public BaseNode {
bool ChannelIsDestroyed() { return channel_ == nullptr; }
CallCountingAndTracingNode* counter_and_tracer() {
return &counter_and_tracer_;
}
// proxy methods to composed classes.
ChannelTrace* trace() { return trace_.get(); }
void RecordCallStarted() { call_counter_.RecordCallStarted(); }
void RecordCallFailed() { call_counter_.RecordCallFailed(); }
void RecordCallSucceeded() { call_counter_.RecordCallSucceeded(); }
protected:
// provides view of target for child.
char* target_view() { return target_.get(); }
// provides access to call_counter_ for child.
CallCountingHelper* call_counter() { return &call_counter_; }
private:
grpc_channel* channel_ = nullptr;
UniquePtr<char> target_;
CallCountingAndTracingNode counter_and_tracer_;
CallCountingHelper call_counter_;
ManualConstructor<ChannelTrace> trace_;
};
// Handles channelz bookkeeping for servers
// TODO(ncteisen): implement in subsequent PR.
class ServerNode : public BaseNode {
public:
ServerNode(size_t channel_tracer_max_nodes) : BaseNode(EntityType::kServer) {}
explicit ServerNode(size_t channel_tracer_max_nodes)
: BaseNode(EntityType::kServer) {}
~ServerNode() override {}
};

@ -489,7 +489,7 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args,
grpc_core::channelz::ChannelNode* channelz_channel =
grpc_channel_get_channelz_node(call->channel);
if (channelz_channel != nullptr) {
channelz_channel->counter_and_tracer()->RecordCallStarted();
channelz_channel->RecordCallStarted();
}
grpc_slice_unref_internal(path);
@ -1269,9 +1269,9 @@ static void post_batch_completion(batch_control* bctl) {
grpc_channel_get_channelz_node(call->channel);
if (channelz_channel != nullptr) {
if (*call->final_op.client.status != GRPC_STATUS_OK) {
channelz_channel->counter_and_tracer()->RecordCallFailed();
channelz_channel->RecordCallFailed();
} else {
channelz_channel->counter_and_tracer()->RecordCallSucceeded();
channelz_channel->RecordCallSucceeded();
}
}
GRPC_ERROR_UNREF(error);

@ -170,7 +170,7 @@ grpc_channel* grpc_channel_create_with_builder(
bool is_top_level_channel = channel->is_client && !internal_channel;
channel->channelz_channel = channel_node_create_func(
channel, channel_tracer_max_nodes, is_top_level_channel);
channel->channelz_channel->counter_and_tracer()->trace()->AddTraceEvent(
channel->channelz_channel->trace()->AddTraceEvent(
grpc_core::channelz::ChannelTrace::Severity::Info,
grpc_slice_from_static_string("Channel created"));
}
@ -417,7 +417,7 @@ void grpc_channel_internal_unref(grpc_channel* c REF_ARG) {
static void destroy_channel(void* arg, grpc_error* error) {
grpc_channel* channel = static_cast<grpc_channel*>(arg);
if (channel->channelz_channel != nullptr) {
channel->channelz_channel->counter_and_tracer()->trace()->AddTraceEvent(
channel->channelz_channel->trace()->AddTraceEvent(
grpc_core::channelz::ChannelTrace::Severity::Info,
grpc_slice_from_static_string("Channel destroyed"));
channel->channelz_channel->MarkChannelDestroyed();

@ -89,7 +89,6 @@ void ValidateChannelTrace(ChannelTrace* tracer,
grpc_json* json = tracer->RenderJson();
EXPECT_NE(json, nullptr);
char* json_str = grpc_json_dump_to_string(json, 0);
gpr_log(GPR_ERROR, "%s", json_str);
grpc_json_destroy(json);
grpc::testing::ValidateChannelTraceProtoJsonTranslation(json_str);
grpc_json* parsed_json = grpc_json_parse_string(json_str);
@ -161,14 +160,14 @@ TEST_P(ChannelTracerTest, ComplexTest) {
ChannelTrace::Severity::Info,
grpc_slice_from_static_string("subchannel one created"), sc1);
ValidateChannelTrace(&tracer, 3, GetParam());
AddSimpleTrace(sc1->counter_and_tracer()->trace());
AddSimpleTrace(sc1->counter_and_tracer()->trace());
AddSimpleTrace(sc1->counter_and_tracer()->trace());
ValidateChannelTrace(sc1->counter_and_tracer()->trace(), 3, GetParam());
AddSimpleTrace(sc1->counter_and_tracer()->trace());
AddSimpleTrace(sc1->counter_and_tracer()->trace());
AddSimpleTrace(sc1->counter_and_tracer()->trace());
ValidateChannelTrace(sc1->counter_and_tracer()->trace(), 6, GetParam());
AddSimpleTrace(sc1->trace());
AddSimpleTrace(sc1->trace());
AddSimpleTrace(sc1->trace());
ValidateChannelTrace(sc1->trace(), 3, GetParam());
AddSimpleTrace(sc1->trace());
AddSimpleTrace(sc1->trace());
AddSimpleTrace(sc1->trace());
ValidateChannelTrace(sc1->trace(), 6, GetParam());
AddSimpleTrace(&tracer);
AddSimpleTrace(&tracer);
ValidateChannelTrace(&tracer, 5, GetParam());
@ -208,20 +207,20 @@ TEST_P(ChannelTracerTest, TestNesting) {
ChannelTrace::Severity::Info,
grpc_slice_from_static_string("subchannel one created"), sc1);
ValidateChannelTrace(&tracer, 3, GetParam());
AddSimpleTrace(sc1->counter_and_tracer()->trace());
AddSimpleTrace(sc1->trace());
ChannelFixture channel2(GetParam());
RefCountedPtr<ChannelNode> conn1 =
MakeRefCounted<ChannelNode>(channel2.channel(), GetParam(), true);
// nesting one level deeper.
sc1->counter_and_tracer()->trace()->AddTraceEventWithReference(
sc1->trace()->AddTraceEventWithReference(
ChannelTrace::Severity::Info,
grpc_slice_from_static_string("connection one created"), conn1);
ValidateChannelTrace(&tracer, 3, GetParam());
AddSimpleTrace(conn1->counter_and_tracer()->trace());
AddSimpleTrace(conn1->trace());
AddSimpleTrace(&tracer);
AddSimpleTrace(&tracer);
ValidateChannelTrace(&tracer, 5, GetParam());
ValidateChannelTrace(conn1->counter_and_tracer()->trace(), 1, GetParam());
ValidateChannelTrace(conn1->trace(), 1, GetParam());
ChannelFixture channel3(GetParam());
RefCountedPtr<ChannelNode> sc2 =
MakeRefCounted<ChannelNode>(channel3.channel(), GetParam(), true);

@ -44,17 +44,16 @@ namespace channelz {
namespace testing {
// testing peer to access channel internals
class CallCountingAndTracingNodePeer {
class CallCountingHelperPeer {
public:
CallCountingAndTracingNodePeer(CallCountingAndTracingNode* node)
: node_(node) {}
CallCountingHelperPeer(CallCountingHelper* node) : node_(node) {}
grpc_millis last_call_started_millis() {
return (grpc_millis)gpr_atm_no_barrier_load(
&node_->last_call_started_millis_);
}
private:
CallCountingAndTracingNode* node_;
CallCountingHelper* node_;
};
namespace {
@ -164,8 +163,8 @@ void ValidateChannel(ChannelNode* channel, validate_channel_data_args args) {
gpr_free(core_api_json_str);
}
grpc_millis GetLastCallStartedMillis(CallCountingAndTracingNode* channel) {
CallCountingAndTracingNodePeer peer(channel);
grpc_millis GetLastCallStartedMillis(CallCountingHelper* channel) {
CallCountingHelperPeer peer(channel);
return peer.last_call_started_millis();
}
@ -201,46 +200,40 @@ TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) {
ChannelFixture channel(GetParam());
ChannelNode* channelz_channel =
grpc_channel_get_channelz_node(channel.channel());
channelz_channel->counter_and_tracer()->RecordCallStarted();
channelz_channel->counter_and_tracer()->RecordCallFailed();
channelz_channel->counter_and_tracer()->RecordCallSucceeded();
channelz_channel->RecordCallStarted();
channelz_channel->RecordCallFailed();
channelz_channel->RecordCallSucceeded();
ValidateChannel(channelz_channel, {1, 1, 1});
channelz_channel->counter_and_tracer()->RecordCallStarted();
channelz_channel->counter_and_tracer()->RecordCallFailed();
channelz_channel->counter_and_tracer()->RecordCallSucceeded();
channelz_channel->counter_and_tracer()->RecordCallStarted();
channelz_channel->counter_and_tracer()->RecordCallFailed();
channelz_channel->counter_and_tracer()->RecordCallSucceeded();
channelz_channel->RecordCallStarted();
channelz_channel->RecordCallFailed();
channelz_channel->RecordCallSucceeded();
channelz_channel->RecordCallStarted();
channelz_channel->RecordCallFailed();
channelz_channel->RecordCallSucceeded();
ValidateChannel(channelz_channel, {3, 3, 3});
}
TEST_P(ChannelzChannelTest, LastCallStartedMillis) {
grpc_core::ExecCtx exec_ctx;
ChannelFixture channel(GetParam());
ChannelNode* channelz_channel =
grpc_channel_get_channelz_node(channel.channel());
CallCountingHelper counter;
// start a call to set the last call started timestamp
channelz_channel->counter_and_tracer()->RecordCallStarted();
grpc_millis millis1 =
GetLastCallStartedMillis(channelz_channel->counter_and_tracer());
counter.RecordCallStarted();
grpc_millis millis1 = GetLastCallStartedMillis(&counter);
// time gone by should not affect the timestamp
ChannelzSleep(100);
grpc_millis millis2 =
GetLastCallStartedMillis(channelz_channel->counter_and_tracer());
grpc_millis millis2 = GetLastCallStartedMillis(&counter);
EXPECT_EQ(millis1, millis2);
// calls succeeded or failed should not affect the timestamp
ChannelzSleep(100);
channelz_channel->counter_and_tracer()->RecordCallFailed();
channelz_channel->counter_and_tracer()->RecordCallSucceeded();
grpc_millis millis3 =
GetLastCallStartedMillis(channelz_channel->counter_and_tracer());
counter.RecordCallFailed();
counter.RecordCallSucceeded();
grpc_millis millis3 = GetLastCallStartedMillis(&counter);
EXPECT_EQ(millis1, millis3);
// another call started should affect the timestamp
// sleep for extra long to avoid flakes (since we cache Now())
ChannelzSleep(5000);
channelz_channel->counter_and_tracer()->RecordCallStarted();
grpc_millis millis4 =
GetLastCallStartedMillis(channelz_channel->counter_and_tracer());
counter.RecordCallStarted();
grpc_millis millis4 = GetLastCallStartedMillis(&counter);
EXPECT_NE(millis1, millis4);
}

Loading…
Cancel
Save