From a64cb54de152017d6c3c968ec9c22a98c405a8ba Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 2 Oct 2018 17:33:07 -0700 Subject: Channel trace is limited by memory --- include/grpc/impl/codegen/grpc_types.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'include/grpc') diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 9ed5b3c1d4..672f130bab 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -285,9 +285,11 @@ typedef struct { #define GRPC_ARG_SOCKET_MUTATOR "grpc.socket_mutator" /** The grpc_socket_factory instance to create and bind sockets. A pointer. */ #define GRPC_ARG_SOCKET_FACTORY "grpc.socket_factory" -/** The maximum number of trace events to keep in the tracer for each channel or - * subchannel. The default is 10. If set to 0, channel tracing is disabled. */ -#define GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE \ +/** The maximum ammount of memory that the trace event can use per channel + * trace node. Once the maximum is reached, subsequent events will evict the + * oldest events from the buffer. The unit for this knob is bytes. Setting + * it to zero causes channel tracing to be disabled. */ +#define GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE \ "grpc.max_channel_trace_events_per_node" /** If non-zero, gRPC library will track stats and information at at per channel * level. Disabling channelz naturally disables channel tracing. The default -- cgit v1.2.3 From 664178164a7ecd3b108b9dd61e7821b716848792 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 4 Oct 2018 16:32:01 -0700 Subject: reviewer feedback --- include/grpc/impl/codegen/grpc_types.h | 8 ++++---- src/core/lib/channel/channel_trace.cc | 10 ++++------ src/core/lib/channel/channel_trace.h | 20 ++++++++++++++++---- src/core/lib/channel/channelz.h | 3 ++- test/core/channel/channel_trace_test.cc | 23 ++++++++++++----------- test/core/end2end/tests/channelz.cc | 18 +++++++----------- 6 files changed, 45 insertions(+), 37 deletions(-) (limited to 'include/grpc') diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 42daf3bb6e..3ce88a8264 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -285,10 +285,10 @@ typedef struct { #define GRPC_ARG_SOCKET_MUTATOR "grpc.socket_mutator" /** The grpc_socket_factory instance to create and bind sockets. A pointer. */ #define GRPC_ARG_SOCKET_FACTORY "grpc.socket_factory" -/** The maximum ammount of memory that the trace event can use per channel - * trace node. Once the maximum is reached, subsequent events will evict the - * oldest events from the buffer. The unit for this knob is bytes. Setting - * it to zero causes channel tracing to be disabled. */ +/** The maximum amount of memory used by trace events per channel trace node. + * Once the maximum is reached, subsequent events will evict the oldest events + * from the buffer. The unit for this knob is bytes. Setting it to zero causes + * channel tracing to be disabled. */ #define GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE \ "grpc.max_channel_trace_event_memory_per_node" /** If non-zero, gRPC library will track stats and information at at per channel 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 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 diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index 8385475e7d..a569444fed 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -30,6 +30,7 @@ #include "src/core/lib/gpr/useful.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/json/json.h" +#include "src/core/lib/surface/channel.h" #include "test/core/util/test_config.h" #include "test/cpp/util/channel_trace_proto_helper.h" @@ -51,6 +52,8 @@ class ChannelNodePeer { ChannelNode* node_; }; +size_t GetSizeofTraceEvent() { return sizeof(ChannelTrace::TraceEvent); } + namespace { grpc_json* GetJsonChild(grpc_json* parent, const char* key) { @@ -119,11 +122,9 @@ void ValidateChannelTrace(ChannelTrace* tracer, size_t num_events_logged) { class ChannelFixture { public: ChannelFixture(int max_tracer_event_memory) { - grpc_arg client_a; - client_a.type = GRPC_ARG_INTEGER; - client_a.key = - const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE); - client_a.value.integer = max_tracer_event_memory; + grpc_arg client_a = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE), + max_tracer_event_memory); grpc_channel_args client_args = {1, &client_a}; channel_ = grpc_insecure_channel_create("fake_target", &client_args, nullptr); @@ -286,9 +287,9 @@ TEST(ChannelTracerTest, TestSmallMemoryLimit) { TEST(ChannelTracerTest, TestEviction) { grpc_core::ExecCtx exec_ctx; - // This depends on sizeof(ChannelTrace). If that struct has been updated, + // This depends on sizeof(ChannelEvent). If that struct has been updated, // this will too. - const int kTraceEventSize = 80; + const int kTraceEventSize = GetSizeofTraceEvent(); const int kNumEvents = 5; ChannelTrace tracer(kTraceEventSize * kNumEvents); for (int i = 1; i <= kNumEvents; ++i) { @@ -305,9 +306,9 @@ TEST(ChannelTracerTest, TestEviction) { TEST(ChannelTracerTest, TestMultipleEviction) { grpc_core::ExecCtx exec_ctx; - // This depends on sizeof(ChannelTrace). If that struct has been updated, + // This depends on sizeof(ChannelEvent). If that struct has been updated, // this will too. - const int kTraceEventSize = 80; + const int kTraceEventSize = GetSizeofTraceEvent(); const int kNumEvents = 5; ChannelTrace tracer(kTraceEventSize * kNumEvents); for (int i = 1; i <= kNumEvents; ++i) { @@ -326,9 +327,9 @@ TEST(ChannelTracerTest, TestMultipleEviction) { TEST(ChannelTracerTest, TestTotalEviction) { grpc_core::ExecCtx exec_ctx; - // This depends on sizeof(ChannelTrace). If that struct has been updated, + // This depends on sizeof(ChannelEvent). If that struct has been updated, // this will too. - const int kTraceEventSize = 80; + const int kTraceEventSize = GetSizeofTraceEvent(); const int kNumEvents = 5; ChannelTrace tracer(kTraceEventSize * kNumEvents); for (int i = 1; i <= kNumEvents; ++i) { diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index 58ffdc18da..0e3ad6a476 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/test/core/end2end/tests/channelz.cc @@ -199,10 +199,8 @@ static void run_one_request(grpc_end2end_test_config config, static void test_channelz(grpc_end2end_test_config config) { grpc_end2end_test_fixture f; - grpc_arg arg; - arg.type = GRPC_ARG_INTEGER; - arg.key = const_cast(GRPC_ARG_ENABLE_CHANNELZ); - arg.value.integer = true; + grpc_arg arg = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_ENABLE_CHANNELZ), true); grpc_channel_args args = {1, &arg}; f = begin_test(config, "test_channelz", &args, &args); @@ -266,13 +264,11 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) { grpc_end2end_test_fixture f; grpc_arg arg[2]; - arg[0].type = GRPC_ARG_INTEGER; - arg[0].key = - const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE); - arg[0].value.integer = 1024; - arg[1].type = GRPC_ARG_INTEGER; - arg[1].key = const_cast(GRPC_ARG_ENABLE_CHANNELZ); - arg[1].value.integer = true; + arg[0] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE), + 1024 * 1024); + arg[1] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_ENABLE_CHANNELZ), true); grpc_channel_args args = {GPR_ARRAY_SIZE(arg), arg}; f = begin_test(config, "test_channelz_with_channel_trace", &args, &args); -- cgit v1.2.3