quiche: implement QUIC_HISTOGRAM api #10715
quiche: implement QUIC_HISTOGRAM api #10715danzh2010 wants to merge 3 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
| scope_ = nullptr; | ||
| } | ||
|
|
||
| Envoy::Stats::Counter& createCounter(std::string name) { return scope_->counterFromString(name); } |
There was a problem hiding this comment.
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_; }
|
|
||
| private: | ||
| Envoy::Stats::Scope* scope_; | ||
| std::unique_ptr<QuicheStats> stats_; |
There was a problem hiding this comment.
add here:
Envoy::Stats::StatNamePool pool_
Envoy::Stats::StatName quiche_stat_name_;
| // 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) \ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What do you mean by manually taking the steps at each call-site? Are you talking about the nested struct?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added some examples as comments in: test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
| QuicheStats& stats() { | ||
| RELEASE_ASSERT(stats_ != nullptr, "Quiche stats is not initialized_."); | ||
| return *stats_; | ||
| } |
There was a problem hiding this comment.
add here:
Envoy::Stats::SymbolTable& symbolTable() { return scope_->symbolTable(); }
| return stats_impl; | ||
| } | ||
|
|
||
| #define QUIC_SERVER_HISTOGRAM_ENUM_IMPL(enum_name, sample, enum_size, docstring) \ |
There was a problem hiding this comment.
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.
| 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))); \ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have a draft PR: #10735
ptal at the pattern as encapsulated there.
There was a problem hiding this comment.
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::variantso 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| NiceMock<Envoy::ThreadLocal::MockInstance> tls_; | ||
| Envoy::Stats::AllocatorImpl alloc_; | ||
| Envoy::Stats::MockSink sink_; | ||
| std::unique_ptr<Envoy::Stats::ThreadLocalStoreImpl> store_; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
danzh2010
left a comment
danzh2010
left a comment
There was a problem hiding this comment.
Thanks for the feedback!
| // 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) \ |
There was a problem hiding this comment.
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.
| return stats_impl; | ||
| } | ||
|
|
||
| #define QUIC_SERVER_HISTOGRAM_ENUM_IMPL(enum_name, sample, enum_size, docstring) \ |
| 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))); \ |
There was a problem hiding this comment.
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()
| NiceMock<Envoy::ThreadLocal::MockInstance> tls_; | ||
| Envoy::Stats::AllocatorImpl alloc_; | ||
| Envoy::Stats::MockSink sink_; | ||
| std::unique_ptr<Envoy::Stats::ThreadLocalStoreImpl> store_; |
There was a problem hiding this comment.
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.
| 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"); |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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?
| Reason2, | ||
| }; | ||
| size_t num_counters = store_->counters().size(); | ||
| QUIC_SERVER_HISTOGRAM_ENUM(test_trial.failure.reason, Reason1, 2, "doc"); |
There was a problem hiding this comment.
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();
| // 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) \ |
There was a problem hiding this comment.
I added some examples as comments in: test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
/wait |
stale
bot
removed
the
stale
|
/assign @antoniovicente |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
4 participants
Signed-off-by: Dan Zhang danzh@google.com
Create a QUICHE stats instantiation and reference approach in Envoy which is applied to platform implementation of QUIC_HISTOGRAM_XXX(...), and will be applied to implement QUIC counters which will be added in QUICHE later.
An upstream change is need to change the stats name in QUIC_HISTOGRAM_XXX(name, ... ) from string to a token.
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]