Skip to content

quiche: implement QUIC_HISTOGRAM api #10715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
danzh2010 wants to merge 3 commits into from
Closed

quiche: implement QUIC_HISTOGRAM api #10715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73948e0
initial checkin
danzh1989 Apr 8, 2020
4401f14
Merge branch 'master' into stats
danzh1989 Apr 9, 2020
a15dc9e
fix build
danzh1989 Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 172 additions & 6 deletions source/extensions/quic_listeners/quiche/platform/quic_server_stats_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,184 @@
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#define QUIC_SERVER_HISTOGRAM_ENUM_IMPL(name, sample, enum_size, docstring) \
#include "envoy/stats/scope.h"
#include "envoy/stats/stats_macros.h"

#define HISTOGRAM_UNIT_UNSPECIFIED Envoy::Stats::Histogram::Unit::Unspecified
#define HISTOGRAM_UNIT_MILLISECONDS Envoy::Stats::Histogram::Unit::Milliseconds
#define GAUGE_MODE_NEVER_IMPORT Envoy::Stats::Gauge::ImportMode::NeverImport

// A full list of QUICHE stats names which is synchoronized with QUICHE.
// New stats needs to be added to the list otherwise the build will complain
// about undefined variable.
// Deprecated stats nees to be removed from the list otherwise clang will
// trigger the Wunused-member-function warning.
// If a QUICHE stats name doesn't contain '.', it can be declared as one of
// HISTOGRAM, GAUGE, COUNTER and ENUM_HISTOGRAM. The format for such type is
// TYPE(name_in_quiche, [other_param_for_envoy_stats,] "name_in_envoy")
// If a QIUCHE stats name contains '.', it need to be declared as nested STRUCT, one layer for
// each token before '.', and the last token as one of HISTOGRAM, GAUGE, COUNTER and ENUM_HISTOGRAM.
// The format for stats name "prefix1.prefix2.prefix3.token" looks like STRUCT(prefix1,
// STRUCT(prefix2, STRUCT(prefix3, TYPE(token, [other_param_for_envoy_stats,] "name_in_envoy"))))
// Two QUICHE stats names can share common prefix in which case the different
// parts after the common prefix are wrapped in the same STRUCT, i.e.
// For histogram names "prefix1.token1" and "prefix1.token2" are like
// STRUCT(prefix1, HISTOGRAM(token1, HISTOGRAM_UNIT_UNSPECIFIED, "unique_name1")
// HISTOGRAM(token2, HISTOGRAM_UNIT_MILLISECONDS, "unique_name2"))
#define QUICHE_STATS(COUNTER, GAUGE, HISTOGRAM, ENUM_HISTOGRAM, STRUCT) \
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning against all this macro stuff from lines 12-82; these look like they are duplicates of include/envoy/stats/stats_macros.h. Why do you need them?

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macros in stats_macros.h doesn't support stats name with dot, like: QUIC_SERVER_HISTOGRAM_ENUM(test_trial.failure.reason, Reason1, 2, "doc");
and can't detect deprecated stats, and doesn't have enum map.
These macros here are following the patten in stats_macros.h, but added nested struct to support the above special case.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be really hard to review. None of the other subsystems which needs stats require a whole suite of new macros. Can we try and avoid this by instead manually taking the steps at each call-site? Then we can iterate on finding a functional way to express what you want.

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by manually taking the steps at each call-site? Are you talking about the nested struct?

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to say: let's assume you have nothing but the raw stats API provided by Envoy. How do your call-sites look?

Can we start from that? Don't even consider the existing envoy stats_macros.h: just use the raw APIs. Maybe iterate on one or two examples with me and then we can figure out how to factor out anything that looks repeated.

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some examples as comments in: test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

HISTOGRAM(test_latency, HISTOGRAM_UNIT_MILLISECONDS, "test_latency") \
STRUCT(test_trial, HISTOGRAM(success, HISTOGRAM_UNIT_UNSPECIFIED, "test_trial.success") \
STRUCT(failure, ENUM_HISTOGRAM(reason, "test_trial.failure.reason")) \
HISTOGRAM(cancel, HISTOGRAM_UNIT_UNSPECIFIED, "test_trial.cancel")) \
STRUCT(test_session, STRUCT(a, STRUCT(b, STRUCT(c, HISTOGRAM(d, HISTOGRAM_UNIT_UNSPECIFIED, \
"test_session.a.b.c.d"))))) \
/* To be added when QUICHE interfaceis updated \
COUNTER(test_counter , "quiche_test_counter") \
GAUGE(test_gauge , GAUGE_MODE_NEVER_IMPORT, "quiche_test_gauge") \
HISTOGRAM(quic_server_num_written_packets_per_write, HISTOGRAM_UNIT_UNSPECIFIED, \
"quic_server_num_written_packets_per_write") \
ENUM_HISTOGRAM(quic_server_connection_close_errors, "quic_server_connection_close_errors") \
ENUM_HISTOGRAM(quic_client_connection_close_errors, "quic_client_connection_close_errors") \
*/

using CounterMap = absl::flat_hash_map<uint32_t, std::reference_wrapper<Envoy::Stats::Counter>>;

#define STRUCT_MEMBER_HISTOGRAM(a, other_param, name_str) \
Envoy::Stats::Histogram& a; \
Envoy::Stats::Histogram& a##_() { return a; }
#define STRUCT_MEMBER_COUNTER(a, name_str) \
Envoy::Stats::Counter& a; \
Envoy::Stats::Counter& a##_() { return a; }
#define STRUCT_MEMBER_GAUGE(a, other_param, name_str) \
Envoy::Stats::Gauge& a; \
Envoy::Stats::Gauge& a##_() { return a; }
// Used to support QUIC_SERVER_HISTOGRAM_ENUM. Envoy histogram is not designed
// for collecting enum values. Because the bucketization is hidden from the interface.
// A walk around is to use a group of couters mapped by enum value. Each value
// mapped counter is not instantiated till it is used.
#define STRUCT_MEMBER_ENUM_HISTOGRAM(a, name_str) \
CounterMap a; \
CounterMap& a##_() { return a; }
// Used to support '.' concatinated stats name, i.e. QUIC_SERVER_HISTOGRAM_BOOL(a.b.c.d, false, ...)
#define STRUCT_MEMBER_STRUCT(a, X) \
struct a##_t { \
X \
} a;

#define EMPTY_MAP(args...) \
{}

#define INIT_STATS_HISTOGRAM(a, other_param, name_str) \
scope.histogramFromString("quiche." name_str, other_param),
#define INIT_STATS_COUNTER(a, name_str) scope.counterFromString("quiche." name_str),
#define INIT_STATS_GAUGE(a, other_param, name_str) \
scope.gaugeFromString("quiche." name_str, other_param),
#define INIT_STATS_ENUM_HISTOGRAM(a, name_str) EMPTY_MAP(a, name_str),
#define INIT_STATS_STRUCT(name, X...) {X},

namespace {

// In order to detect deprecated stats, these entries need to be declared in a struct as public
// fields with corresponding getters. And they should be only accessed via their getters as if they
// are private field. The reason for them to be public is for the accessibility of stats like
// "a.b.c.d". When a stats is removed from QUICHE code, its getter is left unused. And this struct
// must be declared in an annonymous namespace. This struct looks like:
// struct QuicheStats {
// // QUIC_SERVER_HISTOGRAM_BOOL(a.b.c.d, ...)
// // QUIC_SERVER_HISTOGRAM_ENUM(a.b.c.e, ...)
// struct a_t{
// struct b_t{
// struct c_t{
// Envoy::Stats::Metric& d;
// Envoy::Stats::Metric& d_() { return d; }
// EnumMap e;
// EnumMap& e_() { return e; }
// } c;
// } b;
// } a;
//
// // QUIC_SERVER_HISTOGRAM_COUNTER(aaa, ...)
// Envoy::Stats::Histogram& aaa;
// Envoy::Stats::Histogram& aaa_() { return aaa; }
//
// // QUIC_SERVER_HISTOGRAM_ENUM(bbb, ...)
// // Counters are not instantiated until incremented.
// EnumMap bbb;
// EnumMap& bbb_() { return bbb; }
// ...
// };
struct QuicheStats {
QUICHE_STATS(STRUCT_MEMBER_COUNTER, STRUCT_MEMBER_GAUGE, STRUCT_MEMBER_HISTOGRAM,
STRUCT_MEMBER_ENUM_HISTOGRAM, STRUCT_MEMBER_STRUCT)
};

} // namespace

class QuicStatsImpl {
public:
void initializeStats(Envoy::Stats::Scope& scope) {
scope_ = &scope;
std::string prefix("quiche");
stats_.reset(
new QuicheStats{// // QUIC_SERVER_HISTOGRAM_COUNTER(aaa, ...)
// scope.histogram("quiche.aaa", ...),
//
// // QUIC_SERVER_HISTOGRAM_ENUM(bbb, ...)
// {},
//
// // QUIC_SERVER_HISTOGRAM_ENUM(a.b.c.d, ...)
// // QUIC_SERVER_HISTOGRAM_ENUM(a.b.c.e, ...)
// {{{ {},{} }}},
QUICHE_STATS(INIT_STATS_COUNTER, INIT_STATS_GAUGE, INIT_STATS_HISTOGRAM,
INIT_STATS_ENUM_HISTOGRAM, INIT_STATS_STRUCT)});
}

void resetForTest() {
stats_.reset();
scope_ = nullptr;
}

Envoy::Stats::Counter& createCounter(std::string name) { return scope_->counterFromString(name); }
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a pretty strong anti-pattern that will take a global lock unconditionally. It will fail 'check_format' also for this reason.

I recommend deleting this here and I'll comment at the single call-site how to avoid this. Instead, add here:

  Envoy::Stats::SymbolTable& symbolTable() { return scope_->symbolTable(); }
  Envoy::Stats::SymbolTable& scope() { return *scope_; }
  Envoy::Stats::StatName quicheStatName() { return quiche_stat_name_; }


QuicheStats& stats() {
RELEASE_ASSERT(stats_ != nullptr, "Quiche stats is not initialized_.");
return *stats_;
}
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add here:

  Envoy::Stats::SymbolTable& symbolTable() { return scope_->symbolTable(); }


private:
Envoy::Stats::Scope* scope_;
std::unique_ptr<QuicheStats> stats_;
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add here:

  Envoy::Stats::StatNamePool pool_
  Envoy::Stats::StatName quiche_stat_name_;

};

QuicStatsImpl& getQuicheStats() {
// Each worker thread has its own set of references to counters, gauges and
// histograms.
static thread_local QuicStatsImpl stats_impl = QuicStatsImpl();
return stats_impl;
}

#define QUIC_SERVER_HISTOGRAM_ENUM_IMPL(enum_name, sample, enum_size, docstring) \
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we please make this a wrapper to an actual function-call where you do all the stringification you need in the macro and do the real work in the function? I really don't like these huge macros. Hard to debug, slow to compile.

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

do { \
CounterMap& enum_map = getQuicheStats().stats().enum_name##_(); \
uint32_t key = static_cast<uint32_t>(sample); \
auto it = enum_map.find(key); \
if (it == enum_map.end()) { \
auto result = enum_map.emplace( \
key, getQuicheStats().createCounter(absl::StrCat("quiche." #enum_name ".", sample))); \
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the above comments where "quiche" is captured as a StatName in your structure, you can use join() without taking. a lock. What I'm about to write is not completely coherent as I don't fully understand your structures and maps, but this is the basic pattern you want, where you join a pre-symbolized prefix to a dynamically allocated string. I'm ignoring 'sample' for the moment until I get more clarity on how this works. Ideally one of 'enum' or 'sample' should be pre-symbolized like quiche_stat_name_, to reduce the amount of dynamic allocation we need to do.

   if (it == enum_map.end()) { 
     QuicStatsImpl& quiche_stats = getQuicheStats();
     Envoy::Stats::SymbolTable& symbol_table = quiche_stats.symbolTable();
     Envoy::Stats::StatNameDynamicStorage storage(#enum_name, symbol_table);
     Envoy::Stats::SymbolTable::StoragePtr joined = symbol_table.join({
        quiche_stats.quicheStatName(), storage.statName()});
    EXPECT_EQ(absl::StrCat("a.b.", stat_str, ".c.d"), table_->toString(StatName(joined.get())));
    auto result = quiche_stats.scope().counterFromStatName(StatName(joined.get());
    ...

You could also wrap that in a helper method on QuicheStatsImpl, though you'd need several variants of that helper method for gauges, histograms. Maybe with some helper methods.

But the above block will not take a global lock, and it will only use memory-inefficient dynamic allocation for #enum_name and not for the "quiche" prefix.

Speaking of which, what's the threading story on your CounterMap?

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patten! Just one thing I'm trying to understand, the cost of counterFromStringName() should only occur once per counter. Afterwards their references should be kept in enum_map. So my question is: are we doing all the pre-symbolization and counterFromStatName() instead just for avoiding the one time only lock contention for each counter?

enum_name is the stats name; sample is the enum value passed in which is the key in enum_map. The stats name for a particular enum value in envoy could be "quiche.stats_name_in_quiche.enum_int_value". Does StatNameDynamicStorage supports enum_name like a.b.c.d ?

CounterMap should be thread local as QuicStatsImpl object is thread local, see: getQuicheStats()

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Let's not re-implement source/common/common/thread_local_store.cc here. It's very expensive and not necessary to make this thread-local. A central tenant of the stats architecture is that the stat name strings when fully elaborated are incredibly expensive in memory , and we don't want to have thread-local maps of strings.

However I do think the pattern I suggest itself should be abstracted into the stats system, and that would also simplify other existing call-sites. Your PR convinces me to work on doing that now, but please don't let that block this PR. If mine comes in after yours I'll refactor this to clean it up. Here's a sample of a unit-test which I'm working on, which

  ScopePtr scope = store_->createScope("scope.");
  Counter& c1 = Utility::counterFromElements(*scope, {"a", "b"});
  EXPECT_EQ("scope.a.b", c1.name());
  StatName token = pool_.add("token");
  Counter& c2 = Utility::counterFromElements(*scope, {"a", token, "b"});
  EXPECT_EQ("scope.a.token.b", c2.name());
  StatName suffix = pool_.add("suffix");
  Counter& c3 = Utility::counterFromElements(*scope, {token, suffix});
  EXPECT_EQ("scope.token.suffix", c3.name());

This uses the nifty absl::variant so we can mix pre-symbolized tokens and tokens specified via string_view.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a draft PR: #10735

ptal at the pattern as encapsulated there.

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Let's not re-implement source/common/common/thread_local_store.cc here. It's very expensive and not necessary to make this thread-local. A central tenant of the stats architecture is that the stat name strings when fully elaborated are incredibly expensive in memory , and we don't want to have thread-local maps of strings.

I actually don't know ThreadLocalStore well. I think it's a totally different thing from the stats struct I created here which is merely a sets of references. The Scope instance I used in initializeStats() is actually supposed to be a ThreadLocalStore instance. Please let me know if my understanding is wrong.

thread-localizing QuicStatsImpl is for QUICHE code to access the stats struct instance as QUICHE doesn't have interface to pass it in. We are adding a context to QUICHE actually, but it's not for platform abstraction purpose. I don't know how the api would turn out to be and if it can be adopted by platform stuff. But for now, making QuicStatsImpl thread-local is the only way to make it accessible in QUICHE. As to the stat name strings, I don't think they are kept anywhere in the stats struct I proposed. The struct instantiated is similar to what stats_macro.h does, with all the stats name as variable name, and the enum map is keyed on integer.

However I do think the pattern I suggest itself should be abstracted into the stats system, and that would also simplify other existing call-sites. Your PR convinces me to work on doing that now, but please don't let that block this PR. If mine comes in after yours I'll refactor this to clean it up. Here's a sample of a unit-test which I'm working on, which

  ScopePtr scope = store_->createScope("scope.");
  Counter& c1 = Utility::counterFromElements(*scope, {"a", "b"});
  EXPECT_EQ("scope.a.b", c1.name());
  StatName token = pool_.add("token");
  Counter& c2 = Utility::counterFromElements(*scope, {"a", token, "b"});
  EXPECT_EQ("scope.a.token.b", c2.name());
  StatName suffix = pool_.add("suffix");
  Counter& c3 = Utility::counterFromElements(*scope, {token, suffix});
  EXPECT_EQ("scope.token.suffix", c3.name());

This uses the nifty absl::variant so we can mix pre-symbolized tokens and tokens specified via string_view.

Could you confirm that Envoy::Stats::StatNameDynamicStorage storage(#enum_name, symbol_table) also works if enum_name is a.b? If not, we need to split it before creating StatNameDynamicStorage for them.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against adding string-maps into thread-local storage. It's very wasteful to have copies of all these strings. I don't think you need to make these stats structs thread-local. You can use thread-safe APIs that don't take locks, based on my suggestions above. LMK if you need more help.

RE "works if enum_name is a.b": yes it does. However if either "a" or "b" can be known at compile-time it would be preferable to record a symbolized-stat on construction of your context, but please don't replicate that per thread.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn't reading carefully enough: your CounterMap is from the enum int value not the string, so you are not duplicating the strings. My bad.

I still think the solution I propose is simpler and cleaner than having another layer of TLS-maps. Note that we already have TLS-maps in source/common/stats/thread_local_store.cc, so having TLS-maps in front of TLS-maps just seems needlessly complex.

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the solution I propose is simpler and cleaner than having another layer of TLS-maps. Note that we already have TLS-maps in source/common/stats/thread_local_store.cc, so having TLS-maps in front of TLS-maps just seems needlessly complex.

The stats struct I was trying to create is just for fast accessing those counters and histograms. Are you saying search in ThreadLocalStore via something like counterFromStatName() is efficient enough so that we don't need to keep a reference of these stats instances? My understanding is that for the stats which are instantiated at start up via StatNameManagedStorage (so called pre-symbolized?) is not thread local so accessing them needs lock and so we want to instantiate them at start up and hold a reference to them. Is it correct?

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably about the same as your thread-local structure. You are not gaining much by putting another tls-scoped cache in front of the tls-scoped cache.

ASSERT(result.second); \
it = result.first; \
} \
it->second.get().inc(); \
} while (0)

#define QUIC_SERVER_HISTOGRAM_BOOL_IMPL(name, sample, docstring) \
do { \
} while (0)
QUIC_SERVER_HISTOGRAM_TIMES_IMPL(name, sample, 0, 1, 2, docstring)

#define QUIC_SERVER_HISTOGRAM_TIMES_IMPL(name, sample, min, max, bucket_count, docstring) \
#define QUIC_SERVER_HISTOGRAM_TIMES_IMPL(time_name, sample, min, max, bucket_count, docstring) \
do { \
static_cast<Envoy::Stats::Histogram&>(getQuicheStats().stats().time_name##_()) \
.recordValue(sample); \
} while (0)

#define QUIC_SERVER_HISTOGRAM_COUNTS_IMPL(name, sample, min, max, bucket_count, docstring) \
do { \
} while (0)
QUIC_SERVER_HISTOGRAM_TIMES_IMPL(name, sample, min, max, bucket_count, docstring)
8 changes: 7 additions & 1 deletion test/extensions/quic_listeners/quiche/platform/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,24 @@ envoy_cc_test(
"//bazel:linux": ["quic_platform_test.cc"],
"//conditions:default": [],
}),
copts = ["-Wno-unused-parameter"],
copts = [
"-Wno-unused-parameter",
"-Wunused-member-function",
],
data = ["//test/extensions/transport_sockets/tls/test_data:certs"],
external_deps = ["quiche_quic_platform"],
tags = ["nofips"],
deps = [
":quic_platform_epoll_clock_lib",
"//source/common/memory:stats_lib",
"//source/common/stats:thread_local_store_lib",
"//source/extensions/quic_listeners/quiche/platform:flags_impl_lib",
"//test/common/buffer:utility_lib",
"//test/common/stats:stat_test_utility_lib",
"//test/extensions/transport_sockets/tls:ssl_test_utils",
"//test/mocks/api:api_mocks",
"//test/mocks/event:event_mocks",
"//test/mocks/thread_local:thread_local_mocks",
"//test/test_common:logging_lib",
"//test/test_common:threadsafe_singleton_injector_lib",
"//test/test_common:utility_lib",
Expand Down
82 changes: 72 additions & 10 deletions test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

#include "common/memory/stats.h"
#include "common/network/utility.h"
#include "common/stats/symbol_table_impl.h"
#include "common/stats/thread_local_store.h"

#include "extensions/quic_listeners/quiche/platform/flags_impl.h"

Expand All @@ -19,6 +21,8 @@
#include "test/extensions/quic_listeners/quiche/platform/quic_epoll_clock.h"
#include "test/extensions/transport_sockets/tls/ssl_test_utility.h"
#include "test/mocks/api/mocks.h"
#include "test/mocks/event/mocks.h"
#include "test/mocks/thread_local/mocks.h"
#include "test/test_common/environment.h"
#include "test/test_common/logging.h"
#include "test/test_common/network_utility.h"
Expand Down Expand Up @@ -70,26 +74,43 @@

using testing::_;
using testing::HasSubstr;
using testing::NiceMock;
using testing::Ref;
using testing::Return;

namespace quic {
namespace {

class QuicPlatformTest : public testing::Test {
protected:
QuicPlatformTest()
: log_level_(GetLogger().level()), verbosity_log_threshold_(GetVerbosityLogThreshold()) {
: log_level_(GetLogger().level()), verbosity_log_threshold_(GetVerbosityLogThreshold()),
symbol_table_(Envoy::Stats::SymbolTableCreator::makeSymbolTable()), alloc_(*symbol_table_),
store_(std::make_unique<Envoy::Stats::ThreadLocalStoreImpl>(alloc_)) {
SetVerbosityLogThreshold(0);
GetLogger().set_level(ERROR);
store_->addSink(sink_);
}

void SetUp() override {
QuicStatsImpl& stats = getQuicheStats();
stats.initializeStats(*store_);
}

void TearDown() override { getQuicheStats().resetForTest(); }

~QuicPlatformTest() override {
SetVerbosityLogThreshold(verbosity_log_threshold_);
GetLogger().set_level(log_level_);
}

const QuicLogLevel log_level_;
const int verbosity_log_threshold_;
Envoy::Stats::SymbolTablePtr symbol_table_;
NiceMock<Envoy::Event::MockDispatcher> main_thread_dispatcher_;
NiceMock<Envoy::ThreadLocal::MockInstance> tls_;
Envoy::Stats::AllocatorImpl alloc_;
Envoy::Stats::MockSink sink_;
std::unique_ptr<Envoy::Stats::ThreadLocalStoreImpl> store_;
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the test, see if you can use Envoy::Stats::TestUtil::TestStore. That will allow you in the test to do string based lookups of stats that have mixed dynamic and symbolic name tokens.

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestStore inherits from IsolatedStoreImpl which doesn't write histogram into sink, so I didn't consider about it. Maybe I can overide deliverHistogramToSinks() in TestStore.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point: it would be better if we can actually test real histogram functionality with TestStore. Could you instead just simply add deliverHistogramToSinks into TestStore itself rather than overriding?

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

};

TEST_F(QuicPlatformTest, QuicAlignOf) { EXPECT_LT(0, QUIC_ALIGN_OF(int)); }
Expand Down Expand Up @@ -133,13 +154,14 @@ TEST_F(QuicPlatformTest, QuicExpectBug) {
}

TEST_F(QuicPlatformTest, QuicExportedStats) {
// Just make sure they compile.
/*
QUIC_HISTOGRAM_ENUM("my.enum.histogram", TestEnum::ONE, TestEnum::COUNT, "doc");
QUIC_HISTOGRAM_BOOL("my.bool.histogram", false, "doc");
QUIC_HISTOGRAM_TIMES("my.timing.histogram", QuicTime::Delta::FromSeconds(5),
QuicTime::Delta::FromSeconds(1), QuicTime::Delta::FromSecond(3600), 100,
"doc");
QUIC_HISTOGRAM_COUNTS("my.count.histogram", 123, 0, 1000, 100, "doc");
*/
}

TEST_F(QuicPlatformTest, QuicHostnameUtils) {
Expand Down Expand Up @@ -229,13 +251,53 @@ TEST_F(QuicPlatformTest, QuicMockLog) {
}

TEST_F(QuicPlatformTest, QuicServerStats) {
// Just make sure they compile.
QUIC_SERVER_HISTOGRAM_ENUM("my.enum.histogram", TestEnum::ONE, TestEnum::COUNT, "doc");
QUIC_SERVER_HISTOGRAM_BOOL("my.bool.histogram", false, "doc");
QUIC_SERVER_HISTOGRAM_TIMES("my.timing.histogram", QuicTime::Delta::FromSeconds(5),
QuicTime::Delta::FromSeconds(1), QuicTime::Delta::FromSecond(3600),
100, "doc");
QUIC_SERVER_HISTOGRAM_COUNTS("my.count.histogram", 123, 0, 1000, 100, "doc");
Envoy::Stats::Histogram& h1 = store_->histogramFromString(
"quiche.test_latency", Envoy::Stats::Histogram::Unit::Milliseconds);
EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 100));
QUIC_SERVER_HISTOGRAM_TIMES(test_latency, 100, 0, 500, 10, "bbb");

Envoy::Stats::Histogram& h2 = store_->histogramFromString(
"quiche.test_trial.success", Envoy::Stats::Histogram::Unit::Unspecified);
EXPECT_CALL(sink_, onHistogramComplete(Ref(h2), 0));
QUIC_SERVER_HISTOGRAM_BOOL(test_trial.success, false, "just for test");
Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With out the stats struct it would look like:

    QuicStatsImpl& quiche_stats = getQuicheStats();
     Envoy::Stats::SymbolTable& symbol_table = quiche_stats.symbolTable();
    // Assuming we don't support : nvoy::Stats::StatNameDynamicStorage storage1("test_trial.success", symbol_table);
     Envoy::Stats::StatNameDynamicStorage storage1("test_trial", symbol_table);
     Envoy::Stats::StatNameDynamicStorage storage2("success", symbol_table);
     Envoy::Stats::SymbolTable::StoragePtr joined = symbol_table.join({
        quiche_stats.quicheStatName(), storage1.statName(), storage2.statsName});
    auto result = quiche_stats.scope().histogramFromStatName(StatName(joined.get());
    result.recordValue(false);

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, and once #10715 is merged this would be:

    QuicStatsImpl& quiche_stats = getQuicheStats();
    auto hist = Envoy::Stats::Utility::histogramFromElements(quiche_stats.scope(), {
      quiche_stats.quicheStatName(), "test_trial.success"});
    hist.recordValue(false);

Though it could be simplified further; if you are always prefixing with "quiche" then when constructing QuicheStatsImpl, you would just make a nested scope in your ctor:

   : scope_(scope.createScope("quiche")) { ... }

and use that. Then it simplifies to one line if you really want.

Envoy::Stats::Utility::histogramFromElements(getQuicheStats().scope(), {"test_trial.success"}).recordValue(false);

though we really want to do factor out any common components from your dot-separated names and save them on construction in the QuicheStatsImpl.

And I guess if I bit the bullet and made histogramFromElements a Scope method then it would be a little cleaner still:

getQuicheStats().scope().histogramFromElements({"test_trial.success"}).recordValue(false);

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between ThreadLocalStore::histogramFromString(name) and ThreadLocalStore::histogramFromElements(name) then? Or I'm wondering how to choose between these two interface when we instantiate or search for histograms?


enum FailureReason {
Reason1 = 0,
Reason2,
};
size_t num_counters = store_->counters().size();
QUIC_SERVER_HISTOGRAM_ENUM(test_trial.failure.reason, Reason1, 2, "doc");
Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like:

QuicStatsImpl& quiche_stats = getQuicheStats();
Envoy::Stats::SymbolTable& symbol_table = quiche_stats.symbolTable();
Envoy::Stats::StatNameDynamicStorage storage1("test_trial", symbol_table);
Envoy::Stats::StatNameDynamicStorage storage2("failure", symbol_table);
Envoy::Stats::StatNameDynamicStorage storage3("reason", symbol_table);
auto value = static_cat<int32_t>(Reason1);
Envoy::Stats::StatNameDynamicStorage storage4(std::StrCat(value), symbol_table);
Envoy::Stats::SymbolTable::StoragePtr joined = symbol_table.join({
quiche_stats.quicheStatName(), storage1.statName(), storage2.statsName, storage3.statsName, storage4.statsName});
auto result = quiche_stats.scope().histogramFromStatName(StatName(joined.get()));
result.recordValue();

EXPECT_EQ(num_counters + 1, store_->counters().size());
Envoy::Stats::Counter& c1 =
store_->counterFromString(absl::StrCat("quiche.test_trial.failure.reason.", Reason1));
EXPECT_EQ(1u, c1.value());

Envoy::Stats::Histogram& h3 = store_->histogramFromString(
"quiche.test_trial.cancel", Envoy::Stats::Histogram::Unit::Unspecified);
EXPECT_FALSE(h3.used());
EXPECT_CALL(sink_, onHistogramComplete(Ref(h3), 5));
QUIC_SERVER_HISTOGRAM_COUNTS(test_trial.cancel, 5, 0, 10, 10, "aaaa");

Envoy::Stats::Histogram& h4 = store_->histogramFromString(
"quiche.test_session.a.b.c.d", Envoy::Stats::Histogram::Unit::Unspecified);
EXPECT_FALSE(h4.used());
EXPECT_CALL(sink_, onHistogramComplete(Ref(h4), 16));
QUIC_SERVER_HISTOGRAM_COUNTS(test_session.a.b.c.d, 16, 0, 100, 10, "bbbb");

/*
Envoy::Stats::Histogram& h1 =
store_->histogram("quiche.quic_server_num_written_packets_per_write",
Envoy::Stats::Histogram::Unit::Unspecified); EXPECT_FALSE(h1.used()); EXPECT_CALL(sink_,
onHistogramComplete(Ref(h1), 16));
QUIC_SERVER_HISTOGRAM_COUNTS(quic_server_num_written_packets_per_write, 16, 0, 100, 10, "aaaa");

size_t num_counters = store_->counters().size();
QUIC_SERVER_HISTOGRAM_ENUM(quic_server_connection_close_errors, QUIC_INTERNAL_ERROR,
QUIC_LAST_ERROR, "doc"); EXPECT_EQ(num_counters + 1, store_->counters().size());
Envoy::Stats::Counter& c1 =
store_->counter(absl::StrCat("quiche.quic_server_connection_close_errors.", QUIC_INTERNAL_ERROR));
EXPECT_EQ(1u, c1.value());
*/
}

TEST_F(QuicPlatformTest, QuicStackTraceTest) {
Expand Down