From 956623075fbbe74b19b5192ec3e85699ef1bfc37 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 16 Oct 2019 21:31:34 -0400 Subject: [PATCH] stats: Remember recent lookups and display them in an admin endpoint (#8116) * stats: Remember recent lookups and display them in an admin endpoint Signed-off-by: Joshua Marantz --- docs/root/intro/version_history.rst | 2 + docs/root/operations/admin.rst | 51 +++++- include/envoy/stats/symbol_table.h | 26 +++ source/common/stats/BUILD | 1 + source/common/stats/fake_symbol_table_impl.h | 4 + source/common/stats/symbol_table_impl.cc | 104 +++++++++++- source/common/stats/symbol_table_impl.h | 28 +++- source/docs/stats.md | 81 +++++---- source/server/http/admin.cc | 48 ++++++ source/server/http/admin.h | 13 ++ source/server/server.cc | 3 + source/server/server.h | 5 +- test/common/stats/symbol_table_impl_test.cc | 31 ++++ test/integration/BUILD | 1 + test/integration/integration_admin_test.cc | 166 +++++++++++-------- test/server/BUILD | 2 + test/server/http/admin_test.cc | 14 ++ test/server/server_test.cc | 57 ++++++- tools/spelling_dictionary.txt | 1 + 19 files changed, 524 insertions(+), 114 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index a7351555b52b..9f8386f1d2d1 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -9,6 +9,8 @@ Version history * access log: reintroduce :ref:`filesystem ` stats and added the `write_failed` counter to track failed log writes * admin: added ability to configure listener :ref:`socket options `. * admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump `. +* admin: added :http:get:`/stats/recentlookups`, :http:post:`/stats/recentlookups/clear`, + :http:post:`/stats/recentlookups/disable`, and :http:post:`/stats/recentlookups/enable` endpoints. * api: added ::ref:`set_node_on_first_message_only ` option to omit the node identifier from the subsequent discovery requests on the same stream. * buffer filter: the buffer filter populates content-length header if not present, behavior can be disabled using the runtime feature `envoy.reloadable_features.buffer_filter_populate_content_length`. * config: added support for :ref:`delta xDS ` (including ADS) delivery diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index c681deb55951..2cec683f2f60 100644 --- a/docs/root/operations/admin.rst +++ b/docs/root/operations/admin.rst @@ -200,7 +200,7 @@ modify different aspects of the server: .. http:post:: /reset_counters Reset all counters to zero. This is useful along with :http:get:`/stats` during debugging. Note - that this does not drop any data sent to statsd. It just effects local output of the + that this does not drop any data sent to statsd. It just affects local output of the :http:get:`/stats` command. .. http:get:: /server_info @@ -354,6 +354,55 @@ modify different aspects of the server: Envoy has updated (counters incremented at least once, gauges changed at least once, and histograms added to at least once) + .. http:get:: /stats/recentlookups + + This endpoint helps Envoy developers debug potential contention + issues in the stats system. Initially, only the count of StatName + lookups is acumulated, not the specific names that are being looked + up. In order to see specific recent requests, you must enable the + feature by POSTing to `/stats/recentlookups/enable`. There may be + approximately 40-100 nanoseconds of added overhead per lookup. + + When enabled, this endpoint emits a table of stat names that were + recently accessed as strings by Envoy. Ideally, strings should be + converted into StatNames, counters, gauges, and histograms by Envoy + code only during startup or when receiving a new configuration via + xDS. This is because when stats are looked up as strings they must + take a global symbol table lock. During startup this is acceptable, + but in response to user requests on high core-count machines, this + can cause performance issues due to mutex contention. + + This admin endpoint requires Envoy to be started with option + `--use-fake-symbol-table 0`. + + See :repo:`source/docs/stats.md` for more details. + + Note also that actual mutex contention can be tracked via :http:get:`/contention`. + + .. http:post:: /stats/recentlookups/enable + + Turns on collection of recent lookup of stat-names, thus enabling + `/stats/recentlookups`. + + See :repo:`source/docs/stats.md` for more details. + + .. http:post:: /stats/recentlookups/disable + + Turns off collection of recent lookup of stat-names, thus disabling + `/stats/recentlookups`. It also clears the list of lookups. However, + the total count, visible as stat `server.stats_recent_lookups`, is + not cleared, and continues to accumulate. + + See :repo:`source/docs/stats.md` for more details. + + .. http:post:: /stats/recentlookups/clear + + Clears all outstanding lookups and counts. This clears all recent + lookups data as well as the count, but collection continues if + it is enabled. + + See :repo:`source/docs/stats.md` for more details. + .. _operations_admin_interface_runtime: .. http:get:: /runtime diff --git a/include/envoy/stats/symbol_table.h b/include/envoy/stats/symbol_table.h index 204cc227cb91..d1aded28d630 100644 --- a/include/envoy/stats/symbol_table.h +++ b/include/envoy/stats/symbol_table.h @@ -146,6 +146,32 @@ class SymbolTable { virtual void callWithStringView(StatName stat_name, const std::function& fn) const PURE; + using RecentLookupsFn = std::function; + + /** + * Calls the provided function with the name of the most recently looked-up + * symbols, including lookups on any StatNameSets, and with a count of + * the recent lookups on that symbol. + * + * @param iter the function to call for every recent item. + */ + virtual uint64_t getRecentLookups(const RecentLookupsFn& iter) const PURE; + + /** + * Clears the recent-lookups structures. + */ + virtual void clearRecentLookups() PURE; + + /** + * Sets the recent-lookup capacity. + */ + virtual void setRecentLookupCapacity(uint64_t capacity) PURE; + + /** + * @return The configured recent-lookup tracking capacity. + */ + virtual uint64_t recentLookupCapacity() const PURE; + /** * Creates a StatNameSet. * diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 36325b71d36f..19dcbf43d257 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -156,6 +156,7 @@ envoy_cc_library( hdrs = ["symbol_table_impl.h"], external_deps = ["abseil_base"], deps = [ + ":recent_lookups_lib", "//include/envoy/stats:symbol_table_interface", "//source/common/common:assert_lib", "//source/common/common:logger_lib", diff --git a/source/common/stats/fake_symbol_table_impl.h b/source/common/stats/fake_symbol_table_impl.h index 058cb20f23df..b2a86d6cfdad 100644 --- a/source/common/stats/fake_symbol_table_impl.h +++ b/source/common/stats/fake_symbol_table_impl.h @@ -130,6 +130,10 @@ class FakeSymbolTableImpl : public SymbolTable { return StatNameSetPtr(new StatNameSet(*this, name)); } void forgetSet(StatNameSet&) override {} + uint64_t getRecentLookups(const RecentLookupsFn&) const override { return 0; } + void clearRecentLookups() override {} + void setRecentLookupCapacity(uint64_t) override {} + uint64_t recentLookupCapacity() const override { return 0; } private: absl::string_view toStringView(const StatName& stat_name) const { diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index af009ce9ceaf..9fd7a6be2e65 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -126,6 +126,7 @@ void SymbolTableImpl::addTokensToEncoding(const absl::string_view name, Encoding // ref-counts in this. { Thread::LockGuard lock(lock_); + recent_lookups_.lookup(name); for (auto& token : tokens) { symbols.push_back(toSymbol(token)); } @@ -207,16 +208,94 @@ void SymbolTableImpl::free(const StatName& stat_name) { } } -StatNameSetPtr SymbolTableImpl::makeSet(absl::string_view name) { +uint64_t SymbolTableImpl::getRecentLookups(const RecentLookupsFn& iter) const { + uint64_t total = 0; + absl::flat_hash_map name_count_map; + + // We don't want to hold stat_name_set_mutex while calling the iterator, so + // buffer lookup_data. + { + Thread::LockGuard lock(stat_name_set_mutex_); + for (StatNameSet* stat_name_set : stat_name_sets_) { + total += + stat_name_set->getRecentLookups([&name_count_map](absl::string_view str, uint64_t count) { + name_count_map[std::string(str)] += count; + }); + } + } + + // We also don't want to hold lock_ while calling the iterator, but we need it + // to access recent_lookups_. + { + Thread::LockGuard lock(lock_); + recent_lookups_.forEach( + [&name_count_map](absl::string_view str, uint64_t count) + NO_THREAD_SAFETY_ANALYSIS { name_count_map[std::string(str)] += count; }); + total += recent_lookups_.total(); + } + + // Now we have the collated name-count map data: we need to vectorize and + // sort. We define the pair with the count first as std::pair::operator< + // prioritizes its first element over its second. + using LookupCount = std::pair; + std::vector lookup_data; + lookup_data.reserve(name_count_map.size()); + for (const auto& iter : name_count_map) { + lookup_data.emplace_back(LookupCount(iter.second, iter.first)); + } + std::sort(lookup_data.begin(), lookup_data.end()); + for (const LookupCount& lookup_count : lookup_data) { + iter(lookup_count.second, lookup_count.first); + } + return total; +} + +void SymbolTableImpl::setRecentLookupCapacity(uint64_t capacity) { + { + Thread::LockGuard lock(stat_name_set_mutex_); + for (StatNameSet* stat_name_set : stat_name_sets_) { + stat_name_set->setRecentLookupCapacity(capacity); + } + } + + { + Thread::LockGuard lock(lock_); + recent_lookups_.setCapacity(capacity); + } +} + +void SymbolTableImpl::clearRecentLookups() { + { + Thread::LockGuard lock(stat_name_set_mutex_); + for (StatNameSet* stat_name_set : stat_name_sets_) { + stat_name_set->clearRecentLookups(); + } + } + { + Thread::LockGuard lock(lock_); + recent_lookups_.clear(); + } +} + +uint64_t SymbolTableImpl::recentLookupCapacity() const { Thread::LockGuard lock(lock_); - // make_unique does not work with private ctor, even though FakeSymbolTableImpl is a friend. + return recent_lookups_.capacity(); +} + +StatNameSetPtr SymbolTableImpl::makeSet(absl::string_view name) { + const uint64_t capacity = recentLookupCapacity(); + // make_unique does not work with private ctor, even though SymbolTableImpl is a friend. StatNameSetPtr stat_name_set(new StatNameSet(*this, name)); - stat_name_sets_.insert(stat_name_set.get()); + stat_name_set->setRecentLookupCapacity(capacity); + { + Thread::LockGuard lock(stat_name_set_mutex_); + stat_name_sets_.insert(stat_name_set.get()); + } return stat_name_set; } void SymbolTableImpl::forgetSet(StatNameSet& stat_name_set) { - Thread::LockGuard lock(lock_); + Thread::LockGuard lock(stat_name_set_mutex_); stat_name_sets_.erase(&stat_name_set); } @@ -484,10 +563,27 @@ StatName StatNameSet::getDynamic(absl::string_view token) { Stats::StatName& stat_name_ref = dynamic_stat_names_[token]; if (stat_name_ref.empty()) { // Note that builtin_stat_names_ already has one for "". stat_name_ref = pool_.add(token); + recent_lookups_.lookup(token); } return stat_name_ref; } } +uint64_t StatNameSet::getRecentLookups(const RecentLookups::IterFn& iter) const { + absl::MutexLock lock(&mutex_); + recent_lookups_.forEach(iter); + return recent_lookups_.total(); +} + +void StatNameSet::clearRecentLookups() { + absl::MutexLock lock(&mutex_); + recent_lookups_.clear(); +} + +void StatNameSet::setRecentLookupCapacity(uint64_t capacity) { + absl::MutexLock lock(&mutex_); + recent_lookups_.setCapacity(capacity); +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index d0317b9870ff..75b5a07b936a 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -18,6 +18,7 @@ #include "common/common/stack_array.h" #include "common/common/thread.h" #include "common/common/utility.h" +#include "common/stats/recent_lookups.h" #include "absl/container/flat_hash_map.h" #include "absl/strings/str_join.h" @@ -161,6 +162,10 @@ class SymbolTableImpl : public SymbolTable { StatNameSetPtr makeSet(absl::string_view name) override; void forgetSet(StatNameSet& stat_name_set) override; + uint64_t getRecentLookups(const RecentLookupsFn&) const override; + void clearRecentLookups() override; + void setRecentLookupCapacity(uint64_t capacity) override; + uint64_t recentLookupCapacity() const override; private: friend class StatName; @@ -176,6 +181,9 @@ class SymbolTableImpl : public SymbolTable { // This must be held during both encode() and free(). mutable Thread::MutexBasicLockable lock_; + // This must be held while updating stat_name_sets_. + mutable Thread::MutexBasicLockable stat_name_set_mutex_; + /** * Decodes a vector of symbols back into its period-delimited stat name. If * decoding fails on any part of the symbol_vec, we release_assert and crash @@ -241,7 +249,9 @@ class SymbolTableImpl : public SymbolTable { // TODO(ambuc): There might be an optimization here relating to storing ranges of freed symbols // using an Envoy::IntervalSet. std::stack pool_ GUARDED_BY(lock_); - absl::flat_hash_set stat_name_sets_ GUARDED_BY(lock_); + RecentLookups recent_lookups_ GUARDED_BY(lock_); + + absl::flat_hash_set stat_name_sets_ GUARDED_BY(stat_name_set_mutex_); }; /** @@ -709,19 +719,33 @@ class StatNameSet { return pool_.add(str); } + /** + * Clears recent lookups. + */ + void clearRecentLookups(); + + /** + * Sets the number of names recorded in the recent-lookups set. + * + * @param capacity the capacity to configure. + */ + void setRecentLookupCapacity(uint64_t capacity); + private: friend class FakeSymbolTableImpl; friend class SymbolTableImpl; StatNameSet(SymbolTable& symbol_table, absl::string_view name); + uint64_t getRecentLookups(const RecentLookups::IterFn& iter) const; const std::string name_; Stats::SymbolTable& symbol_table_; Stats::StatNamePool pool_ GUARDED_BY(mutex_); - absl::Mutex mutex_; + mutable absl::Mutex mutex_; using StringStatNameMap = absl::flat_hash_map; StringStatNameMap builtin_stat_names_; StringStatNameMap dynamic_stat_names_ GUARDED_BY(mutex_); + RecentLookups recent_lookups_ GUARDED_BY(mutex_); }; } // namespace Stats diff --git a/source/docs/stats.md b/source/docs/stats.md index 965dcb569d16..c247495e6372 100644 --- a/source/docs/stats.md +++ b/source/docs/stats.md @@ -12,10 +12,9 @@ binary program restarts. The metrics are tracked as: In order to support restarting the Envoy binary program without losing counter and gauge values, they are passed from parent to child in an RPC protocol. They were previously held in shared memory, which imposed various restrictions. -Unlike the shared memory implementation, the RPC passing *requires special indication -in source/common/stats/stat_merger.cc when simple addition is not appropriate for -combining two instances of a given stat*. - +Unlike the shared memory implementation, the RPC passing *requires a mode-bit specified +when constructing gauges indicating whether it should be accumulated across hot-restarts*. + ## Performance and Thread Local Storage A key tenant of the Envoy architecture is high performance on machines with @@ -80,7 +79,8 @@ followed. Stat names are replicated in several places in various forms. - * Held with the stat values, in `HeapStatData` + * Held with the stat values, in `CounterImpl` and `GaugeImpl`, which are defined in + [allocator_impl.cc](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/allocator_impl.cc) * In [MetricImpl](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/metric_impl.h) in a transformed state, with tags extracted into vectors of name/value strings. * In static strings across the codebase where stats are referenced @@ -90,7 +90,7 @@ Stat names are replicated in several places in various forms. There are stat maps in `ThreadLocalStore` for capturing all stats in a scope, and each per-thread caches. However, they don't duplicate the stat names. -Instead, they reference the `char*` held in the `HeapStatData` itself, and thus +Instead, they reference the `StatName` held in the `CounterImpl` or `GaugeImpl`, and thus are relatively cheap; effectively those maps are all pointer-to-pointer. For this to be safe, cache lookups from locally scoped strings must use `.find` @@ -120,36 +120,49 @@ etc, must explicitly store partial stat-names their class instances, which later can be composed dynamically at runtime in order to fully elaborate counters, gauges, etc, without taking symbol-table locks, via `SymbolTable::join()`. +### `StatNamePool` and `StatNameSet` + +These two helper classes evolved to make it easy to deploy the symbol table API +across the codebase. + +`StatNamePool` provides pooled allocation for any number of +`StatName` objects, and is intended to be held in a data structure alongside the +`const StatName` member variables. Most names should be established during +process initializion or in response to xDS updates. + +`StatNameSet` provides some associative lookups at runtime, using two maps: a +static map and a dynamic map. + ### Current State and Strategy To Deploy Symbol Tables -As of April 1, 2019, there are a fairly large number of files that directly -lookup stats by name, e.g. via `Stats::Scope::counter(const std::string&)` in -the request path. In most cases, this runtime lookup concatenates the scope name -with a string literal or other request-dependent token to form the stat name, so -it is not possible to fully memoize the stats at startup; there must be a -runtime name lookup. - -If a PR is issued that changes the underlying representation of a stat name to -be a symbol table entry then each stat-name will need to be transformed -whenever names are looked up, which would add CPU overhead and lock contention -in the request-path, violating one of the principles of Envoy's [threading -model](https://blog.envoyproxy.io/envoy-threading-model-a8d44b922310). Before -issuing such a PR we need to first iterate through the codebase memoizing the -symbols that are used to form stat-names. - -To resolve this chicken-and-egg challenge of switching to symbol-table stat-name -representation without suffering a temporary loss of performance, we employ a -["fake" symbol table -implementation](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/fake_symbol_table_impl.h). -This implemenation uses elaborated strings as an underlying representation, but -implements the same API as the ["real" -implemention](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/symbol_table_impl.h). -The underlying string representation means that there is minimal runtime -overhead compared to the current state. But once all stat-allocation call-sites -have been converted to use the abstract [SymbolTable -API](https://github.com/envoyproxy/envoy/blob/master/include/envoy/stats/symbol_table.h), -the real implementation can be swapped in, the space savings realized, and the -fake implementation deleted. +As of September 5, 2019, the symbol table API has been integrated into the +production code, using a temporary ["fake" symbol table +implementation](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/fake_symbol_table_impl.h). This +fake has enabled us to incrementally transform the codebase to pre-symbolize +names as much as possible, avoiding contention in the hot-path. + +There are no longer any explicit production calls to create counters +or gauges directly from a string via `Stats::Scope::counter(const +std::string&)`, though they are ubiquitous in tests. There is also a +`check_format` protection against reintroducting production calls to +`counter()`. + +However, there are still several ways to create hot-path contention +looking up stats by name, and there is no bulletproof way to prevent it from +occurring. + * The [stats macros](https://github.com/envoyproxy/envoy/blob/master/include/envoy/stats/stats_macros.h) may be used in a data structure which is constructed in response to requests. + * An explicit symbol-table lookup, via `StatNamePool` or `StatNameSet` can be + made in the hot path. + +It is difficult to search for those scenarios in the source code or prevent them +with a format-check, but we can determine whether symbol-table lookups are +occurring during via an admin endpoint that shows 20 recent lookups by name, at +`ENVOY_HOST:ADMIN_PORT/stats?recentlookups`. This works only when real symbol +tables are enabled, via command-line option `--use-fake-symbol-table 0`. + +Once we are confident we've removed all hot-path symbol-table lookups, ideally +through usage of real symbol tables in production, examining that endpoint, we +can enable real symbol tables by default. ## Tags and Tag Extraction diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index e3ddf7dff5c0..1b303250dd2c 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -140,6 +140,8 @@ const char AdminHtmlEnd[] = R"( const std::regex PromRegex("[^a-zA-Z0-9_]"); +const uint64_t RecentLookupsCapacity = 100; + void populateFallbackResponseHeaders(Http::Code code, Http::HeaderMap& header_map) { header_map.insertStatus().value(std::to_string(enumToInt(code))); const auto& headers = Http::Headers::get(); @@ -702,7 +704,45 @@ Http::Code AdminImpl::handlerResetCounters(absl::string_view, Http::HeaderMap&, for (const Stats::CounterSharedPtr& counter : server_.stats().counters()) { counter->reset(); } + server_.stats().symbolTable().clearRecentLookups(); + response.add("OK\n"); + return Http::Code::OK; +} + +Http::Code AdminImpl::handlerStatsRecentLookups(absl::string_view, Http::HeaderMap&, + Buffer::Instance& response, AdminStream&) { + Stats::SymbolTable& symbol_table = server_.stats().symbolTable(); + std::string table; + const uint64_t total = + symbol_table.getRecentLookups([&table](absl::string_view name, uint64_t count) { + table += fmt::format("{:8d} {}\n", count, name); + }); + if (table.empty() && symbol_table.recentLookupCapacity() == 0) { + table = "Lookup tracking is not enabled. Use /stats/recentlookups/enable to enable.\n"; + } else { + response.add(" Count Lookup\n"); + } + response.add(absl::StrCat(table, "\ntotal: ", total, "\n")); + return Http::Code::OK; +} + +Http::Code AdminImpl::handlerStatsRecentLookupsClear(absl::string_view, Http::HeaderMap&, + Buffer::Instance& response, AdminStream&) { + server_.stats().symbolTable().clearRecentLookups(); + response.add("OK\n"); + return Http::Code::OK; +} + +Http::Code AdminImpl::handlerStatsRecentLookupsDisable(absl::string_view, Http::HeaderMap&, + Buffer::Instance& response, AdminStream&) { + server_.stats().symbolTable().setRecentLookupCapacity(0); + response.add("OK\n"); + return Http::Code::OK; +} +Http::Code AdminImpl::handlerStatsRecentLookupsEnable(absl::string_view, Http::HeaderMap&, + Buffer::Instance& response, AdminStream&) { + server_.stats().symbolTable().setRecentLookupCapacity(RecentLookupsCapacity); response.add("OK\n"); return Http::Code::OK; } @@ -1231,6 +1271,14 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server) {"/stats", "print server stats", MAKE_ADMIN_HANDLER(handlerStats), false, false}, {"/stats/prometheus", "print server stats in prometheus format", MAKE_ADMIN_HANDLER(handlerPrometheusStats), false, false}, + {"/stats/recentlookups", "Show recent stat-name lookups", + MAKE_ADMIN_HANDLER(handlerStatsRecentLookups), false, false}, + {"/stats/recentlookups/clear", "clear list of stat-name lookups", + MAKE_ADMIN_HANDLER(handlerStatsRecentLookupsClear), false, true}, + {"/stats/recentlookups/disable", "disable recording of reset stat-name lookup names", + MAKE_ADMIN_HANDLER(handlerStatsRecentLookupsDisable), false, true}, + {"/stats/recentlookups/enable", "reset all counters to zero", + MAKE_ADMIN_HANDLER(handlerStatsRecentLookupsEnable), false, true}, {"/listeners", "print listener info", MAKE_ADMIN_HANDLER(handlerListenerInfo), false, false}, {"/runtime", "print runtime values", MAKE_ADMIN_HANDLER(handlerRuntime), false, false}, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 9efb966e758c..ad1e13099b6f 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -153,6 +153,7 @@ class AdminImpl : public Admin, Http::HeaderMap& response_headers, std::string& body) override; void closeSocket(); void addListenerToHandler(Network::ConnectionHandler* handler) override; + Server::Instance& server() { return server_; } private: /** @@ -288,6 +289,18 @@ class AdminImpl : public Admin, Http::Code handlerResetCounters(absl::string_view path_and_query, Http::HeaderMap& response_headers, Buffer::Instance& response, AdminStream&); + Http::Code handlerStatsRecentLookups(absl::string_view path_and_query, + Http::HeaderMap& response_headers, + Buffer::Instance& response, AdminStream&); + Http::Code handlerStatsRecentLookupsClear(absl::string_view path_and_query, + Http::HeaderMap& response_headers, + Buffer::Instance& response, AdminStream&); + Http::Code handlerStatsRecentLookupsDisable(absl::string_view path_and_query, + Http::HeaderMap& response_headers, + Buffer::Instance& response, AdminStream&); + Http::Code handlerStatsRecentLookupsEnable(absl::string_view path_and_query, + Http::HeaderMap& response_headers, + Buffer::Instance& response, AdminStream&); Http::Code handlerServerInfo(absl::string_view path_and_query, Http::HeaderMap& response_headers, Buffer::Instance& response, AdminStream&); Http::Code handlerReady(absl::string_view path_and_query, Http::HeaderMap& response_headers, diff --git a/source/server/server.cc b/source/server/server.cc index 8aba537de9b2..3700c2c30586 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -198,6 +198,9 @@ void InstanceImpl::flushStatsInternal() { sslContextManager().daysUntilFirstCertExpires()); server_stats_->state_.set( enumToInt(Utility::serverState(initManager().state(), healthCheckFailed()))); + server_stats_->stats_recent_lookups_.set( + stats_store_.symbolTable().getRecentLookups([](absl::string_view, uint64_t) {})); + InstanceUtil::flushMetricsToSinks(config_.statsSinks(), stats_store_); // TODO(ramaraochavali): consider adding different flush interval for histograms. if (stat_flush_timer_ != nullptr) { diff --git a/source/server/server.h b/source/server/server.h index 1993e2276f01..9f3c2ad6ffd8 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -49,9 +49,9 @@ namespace Server { * All server wide stats. @see stats_macros.h */ #define ALL_SERVER_STATS(COUNTER, GAUGE, HISTOGRAM) \ - COUNTER(static_unknown_fields) \ - COUNTER(dynamic_unknown_fields) \ COUNTER(debug_assertion_failures) \ + COUNTER(dynamic_unknown_fields) \ + COUNTER(static_unknown_fields) \ GAUGE(concurrency, NeverImport) \ GAUGE(days_until_first_cert_expiring, Accumulate) \ GAUGE(hot_restart_epoch, NeverImport) \ @@ -60,6 +60,7 @@ namespace Server { GAUGE(memory_heap_size, Accumulate) \ GAUGE(parent_connections, Accumulate) \ GAUGE(state, NeverImport) \ + GAUGE(stats_recent_lookups, NeverImport) \ GAUGE(total_connections, Accumulate) \ GAUGE(uptime, Accumulate) \ GAUGE(version, NeverImport) \ diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index c67bcd313b54..293e292035d1 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -575,6 +575,37 @@ TEST_P(StatNameTest, StatNameSet) { EXPECT_NE(dynamic2.data(), dynamic.data()); } +TEST_P(StatNameTest, RecentLookups) { + if (GetParam() == SymbolTableType::Fake) { + return; + } + + StatNameSetPtr set1(table_->makeSet("set1")); + table_->setRecentLookupCapacity(10); + StatNameSetPtr set2(table_->makeSet("set2")); + set1->getDynamic("dynamic.stat1"); + set2->getDynamic("dynamic.stat2"); + encodeDecode("direct.stat"); + + std::vector accum; + uint64_t total = table_->getRecentLookups([&accum](absl::string_view name, uint64_t count) { + accum.emplace_back(absl::StrCat(count, ": ", name)); + }); + EXPECT_EQ(5, total); + std::string recent_lookups_str = StringUtil::join(accum, " "); + + EXPECT_EQ("1: direct.stat " + "2: dynamic.stat1 " // Combines entries from set and symbol-table. + "2: dynamic.stat2", + recent_lookups_str); + + table_->clearRecentLookups(); + uint32_t num_calls = 0; + EXPECT_EQ(0, + table_->getRecentLookups([&num_calls](absl::string_view, uint64_t) { ++num_calls; })); + EXPECT_EQ(0, num_calls); +} + TEST_P(StatNameTest, StatNameEmptyEquivalent) { StatName empty1; StatName empty2 = makeStat(""); diff --git a/test/integration/BUILD b/test/integration/BUILD index ba9a4b35eac3..2a526b446aeb 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -326,6 +326,7 @@ envoy_cc_test( "//source/common/stats:stats_matcher_lib", "//source/extensions/filters/http/buffer:config", "//source/extensions/filters/http/health_check:config", + "//test/common/stats:stat_test_utility_lib", "@envoy_api//envoy/admin/v2alpha:pkg_cc_proto", "@envoy_api//envoy/config/metrics/v2:pkg_cc_proto", ], diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 41085b762d9c..cb3df8a82293 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -10,6 +10,7 @@ #include "common/profiler/profiler.h" #include "common/stats/stats_matcher_impl.h" +#include "test/common/stats/stat_test_utility.h" #include "test/integration/utility.h" #include "test/test_common/utility.h" @@ -122,101 +123,117 @@ std::string ContentType(const BufferingStreamDecoderPtr& response) { } // namespace TEST_P(IntegrationAdminTest, Admin) { + Stats::TestUtil::SymbolTableCreatorTestPeer::setUseFakeSymbolTables(false); initialize(); - BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "GET", "/notfound", "", downstreamProtocol(), version_); + auto request = [this](absl::string_view request, + absl::string_view method) -> BufferingStreamDecoderPtr { + return IntegrationUtil::makeSingleRequest(lookupPort("admin"), std::string(method), + std::string(request), "", downstreamProtocol(), + version_); + }; + + BufferingStreamDecoderPtr response = request("/notfound", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("404", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); EXPECT_NE(std::string::npos, response->body().find("invalid path. admin commands are:")) << response->body(); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/help", "", - downstreamProtocol(), version_); + response = request("/help", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); EXPECT_NE(std::string::npos, response->body().find("admin commands are:")) << response->body(); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/", "", - downstreamProtocol(), version_); + response = request("/", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/html; charset=UTF-8", ContentType(response)); EXPECT_NE(std::string::npos, response->body().find("Envoy Admin")) << response->body(); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/server_info", "", - downstreamProtocol(), version_); + response = request("/server_info", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/ready", "", - downstreamProtocol(), version_); + response = request("/ready", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats", "", - downstreamProtocol(), version_); + response = request("/stats", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats?usedonly", "", - downstreamProtocol(), version_); + // Our first attempt to get recent lookups will get the error message as they + // are off by default. + response = request("/stats/recentlookups", "GET"); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); + EXPECT_THAT(response->body(), testing::HasSubstr("Lookup tracking is not enabled")); + + // Now enable recent-lookups tracking and check that we get a count. + request("/stats/recentlookups/enable", "POST"); + response = request("/stats/recentlookups", "GET"); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); + EXPECT_TRUE(absl::StartsWith(response->body(), " Count Lookup\n")) << response->body(); + EXPECT_LT(30, response->body().size()); + + // Now disable recent-lookups tracking and check that we get the error again. + request("/stats/recentlookups/disable", "POST"); + response = request("/stats/recentlookups", "GET"); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); + EXPECT_THAT(response->body(), testing::HasSubstr("Lookup tracking is not enabled")); + + response = request("/stats?usedonly", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); // Testing a filter with no matches - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats?filter=foo", "", - downstreamProtocol(), version_); + response = request("/stats?filter=foo", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); // Testing a filter with matches - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats?filter=server", - "", downstreamProtocol(), version_); + response = request("/stats?filter=server", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", - "/stats?filter=server&usedonly", "", - downstreamProtocol(), version_); + response = request("/stats?filter=server&usedonly", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = - IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats?format=json&usedonly", - "", downstreamProtocol(), version_); + response = request("/stats?format=json&usedonly", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); validateStatsJson(response->body(), 0); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats?format=blah", - "", downstreamProtocol(), version_); + response = request("/stats?format=blah", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("404", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats?format=json", - "", downstreamProtocol(), version_); + response = request("/stats?format=json", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("application/json", ContentType(response)); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); validateStatsJson(response->body(), 1); // Filtering stats by a regex with one match should return just that match. - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", - "/stats?format=json&filter=^server\\.version$", "", - downstreamProtocol(), version_); + response = request("/stats?format=json&filter=^server\\.version$", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("application/json", ContentType(response)); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); @@ -225,9 +242,7 @@ TEST_P(IntegrationAdminTest, Admin) { testing::Eq("{\"stats\":[{\"name\":\"server.version\",\"value\":0}]}")); // Filtering stats by a non-full-string regex should also return just that match. - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", - "/stats?format=json&filter=server\\.version", "", - downstreamProtocol(), version_); + response = request("/stats?format=json&filter=server\\.version", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("application/json", ContentType(response)); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); @@ -237,17 +252,14 @@ TEST_P(IntegrationAdminTest, Admin) { // Filtering stats by a regex with no matches (".*not_intended_to_appear.*") should return a // valid, empty, stats array. - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", - "/stats?format=json&filter=not_intended_to_appear", - "", downstreamProtocol(), version_); + response = request("/stats?format=json&filter=not_intended_to_appear", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("application/json", ContentType(response)); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); validateStatsJson(response->body(), 0); EXPECT_THAT(response->body(), testing::Eq("{\"stats\":[]}")); - response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "GET", "/stats?format=prometheus", "", downstreamProtocol(), version_); + response = request("/stats?format=prometheus", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_THAT(response->body(), @@ -265,8 +277,7 @@ TEST_P(IntegrationAdminTest, Admin) { response->body(), testing::HasSubstr("envoy_cluster_upstream_cx_active{envoy_cluster_name=\"cluster_0\"} 0\n")); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/stats/prometheus", "", - downstreamProtocol(), version_); + response = request("/stats/prometheus", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_THAT(response->body(), @@ -284,58 +295,84 @@ TEST_P(IntegrationAdminTest, Admin) { response->body(), testing::HasSubstr("envoy_cluster_upstream_cx_active{envoy_cluster_name=\"cluster_0\"} 0\n")); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/clusters", "", - downstreamProtocol(), version_); + response = request("/clusters", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_THAT(response->body(), testing::HasSubstr("added_via_api")); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/clusters?format=json", - "", downstreamProtocol(), version_); + response = request("/clusters?format=json", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); EXPECT_NO_THROW(Json::Factory::loadFromString(response->body())); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/cpuprofiler", "", - downstreamProtocol(), version_); + response = request("/cpuprofiler", "POST"); EXPECT_TRUE(response->complete()); EXPECT_EQ("400", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/hot_restart_version", - "", downstreamProtocol(), version_); + response = request("/hot_restart_version", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "POST", "/reset_counters", "", - downstreamProtocol(), version_); + response = request("/reset_counters", "POST"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/certs", "", - downstreamProtocol(), version_); + request("/stats/recentlookups/enable", "POST"); + request("/stats/recentlookups/clear", "POST"); + response = request("/stats/recentlookups", "GET"); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); + + // TODO(#8324): "http1.metadata_not_supported_error" should not still be in + // the 'recent lookups' output after reset_counters. + switch (GetParam().downstream_protocol) { + case Http::CodecClient::Type::HTTP1: + EXPECT_EQ(" Count Lookup\n" + " 1 http1.metadata_not_supported_error\n" + "\n" + "total: 1\n", + response->body()); + break; + case Http::CodecClient::Type::HTTP2: + EXPECT_EQ(" Count Lookup\n" + " 1 http2.header_overflow\n" + " 1 http2.headers_cb_no_stream\n" + " 1 http2.inbound_empty_frames_flood\n" + " 1 http2.inbound_priority_frames_flood\n" + " 1 http2.inbound_window_update_frames_flood\n" + " 1 http2.outbound_control_flood\n" + " 1 http2.outbound_flood\n" + " 1 http2.rx_messaging_error\n" + " 1 http2.rx_reset\n" + " 1 http2.too_many_header_frames\n" + " 1 http2.trailers\n" + " 1 http2.tx_reset\n" + "\n" + "total: 12\n", + response->body()); + } + + response = request("/certs", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/runtime", "", - downstreamProtocol(), version_); + response = request("/runtime", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); - response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "POST", "/runtime_modify", "foo=bar&foo1=bar1", downstreamProtocol(), - version_, "host", Http::Headers::get().ContentTypeValues.FormUrlEncoded); + response = request("/runtime_modify?foo=bar&foo1=bar1", "POST"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/runtime?format=json", - "", downstreamProtocol(), version_); + response = request("/runtime?format=json", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); @@ -347,8 +384,7 @@ TEST_P(IntegrationAdminTest, Admin) { auto foo1_obj = entries->getObject("foo1"); EXPECT_EQ("bar1", foo1_obj->getString("final_value")); - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/listeners", "", - downstreamProtocol(), version_); + response = request("/listeners", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("text/plain; charset=UTF-8", ContentType(response)); @@ -360,8 +396,7 @@ TEST_P(IntegrationAdminTest, Admin) { listener_it->get().socket().localAddress()->asString()))); } - response = IntegrationUtil::makeSingleRequest( - lookupPort("admin"), "GET", "/listeners?format=json", "", downstreamProtocol(), version_); + response = request("/listeners?format=json", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); @@ -381,8 +416,7 @@ TEST_P(IntegrationAdminTest, Admin) { socket_address->getInteger("port_value")); } - response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/config_dump", "", - downstreamProtocol(), version_); + response = request("/config_dump", "GET"); EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ("application/json", ContentType(response)); diff --git a/test/server/BUILD b/test/server/BUILD index e07c8826f69d..2b1e2f03de4c 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -318,9 +318,11 @@ envoy_cc_test( "//source/extensions/stat_sinks/statsd:config", "//source/extensions/tracers/zipkin:config", "//source/server:server_lib", + "//test/common/stats:stat_test_utility_lib", "//test/integration:integration_lib", "//test/mocks/server:server_mocks", "//test/mocks/stats:stats_mocks", + "//test/test_common:simulated_time_system_lib", "//test/test_common:test_time_lib", "//test/test_common:utility_lib", ], diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index e1e2ed47c9db..a67b422bf7c3 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1400,6 +1400,20 @@ TEST_P(AdminInstanceTest, GetRequestJson) { HasSubstr("application/json")); } +TEST_P(AdminInstanceTest, RecentLookups) { + Http::HeaderMapImpl response_headers; + std::string body; + + // Recent lookup tracking is disabled by default. + EXPECT_EQ(Http::Code::OK, admin_.request("/stats/recentlookups", "GET", response_headers, body)); + EXPECT_THAT(body, HasSubstr("Lookup tracking is not enabled")); + EXPECT_THAT(std::string(response_headers.ContentType()->value().getStringView()), + HasSubstr("text/plain")); + + // We can't test RecentLookups in admin unit tests as it doesn't work with a + // fake symbol table. However we cover this solidly in integration tests. +} + TEST_P(AdminInstanceTest, PostRequest) { Http::HeaderMapImpl response_headers; std::string body; diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 1abb1ba977b8..e103a9aff4e1 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -10,10 +10,12 @@ #include "server/process_context_impl.h" #include "server/server.h" +#include "test/common/stats/stat_test_utility.h" #include "test/integration/server.h" #include "test/mocks/server/mocks.h" #include "test/mocks/stats/mocks.h" #include "test/test_common/environment.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/test_time.h" #include "test/test_common/utility.h" @@ -159,7 +161,7 @@ class ServerInstanceImplTestBase { : std::make_unique("Server"); server_ = std::make_unique( - *init_manager_, options_, test_time_.timeSystem(), + *init_manager_, options_, time_system_, Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")), hooks_, restart_, stats_store_, fakelock_, component_factory_, std::make_unique>(), *thread_local_, @@ -178,7 +180,7 @@ class ServerInstanceImplTestBase { thread_local_ = std::make_unique(); init_manager_ = std::make_unique("Server"); server_ = std::make_unique( - *init_manager_, options_, test_time_.timeSystem(), + *init_manager_, options_, time_system_, Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")), hooks_, restart_, stats_store_, fakelock_, component_factory_, std::make_unique>(), *thread_local_, @@ -224,7 +226,7 @@ class ServerInstanceImplTestBase { Stats::TestIsolatedStoreImpl stats_store_; Thread::MutexBasicLockable fakelock_; TestComponentFactory component_factory_; - DangerousDeprecatedTestTime test_time_; + Event::GlobalTimeSystem time_system_; ProcessObject* process_object_ = nullptr; std::unique_ptr process_context_; std::unique_ptr init_manager_; @@ -281,7 +283,7 @@ TEST_P(ServerInstanceImplTest, StatsFlushWhenServerIsStillInitializing) { auto server_thread = startTestServer("test/server/stats_sink_bootstrap.yaml", true); // Wait till stats are flushed to custom sink and validate that the actual flush happens. - TestUtility::waitForCounterEq(stats_store_, "stats.flushed", 1, test_time_.timeSystem()); + TestUtility::waitForCounterEq(stats_store_, "stats.flushed", 1, time_system_); EXPECT_EQ(3L, TestUtility::findGauge(stats_store_, "server.state")->value()); EXPECT_EQ(Init::Manager::State::Initializing, server_->initManager().state()); @@ -442,6 +444,51 @@ TEST_P(ServerInstanceImplTest, Stats) { #endif } +class TestWithSimTimeAndRealSymbolTables : public Event::TestUsingSimulatedTime { +protected: + TestWithSimTimeAndRealSymbolTables() { + Stats::TestUtil::SymbolTableCreatorTestPeer::setUseFakeSymbolTables(false); + } +}; + +class ServerStatsTest : public TestWithSimTimeAndRealSymbolTables, public ServerInstanceImplTest { +protected: + void flushStats() { + // Default flush interval is 5 seconds. + simTime().sleep(std::chrono::seconds(6)); + server_->dispatcher().run(Event::Dispatcher::RunType::Block); + } +}; + +TEST_P(ServerStatsTest, FlushStats) { + initialize("test/server/empty_bootstrap.yaml"); + Stats::Gauge& recent_lookups = + stats_store_.gauge("server.stats_recent_lookups", Stats::Gauge::ImportMode::NeverImport); + EXPECT_EQ(0, recent_lookups.value()); + flushStats(); + uint64_t strobed_recent_lookups = recent_lookups.value(); + EXPECT_LT(100, strobed_recent_lookups); // Recently this was 319 but exact value not important. + Stats::StatNameSetPtr test_set = stats_store_.symbolTable().makeSet("test"); + + // When we remember a StatNameSet builtin, we charge only for the SymbolTable + // lookup, which requires a lock. + test_set->rememberBuiltin("a.b"); + flushStats(); + EXPECT_EQ(1, recent_lookups.value() - strobed_recent_lookups); + strobed_recent_lookups = recent_lookups.value(); + + // When we use a StatNameSet dynamic, we charge for the SymbolTable lookup and + // for the lookup in the StatNameSet as well, as that requires a lock for its + // dynamic hash_map. + test_set->getDynamic("c.d"); + flushStats(); + EXPECT_EQ(2, recent_lookups.value() - strobed_recent_lookups); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, ServerStatsTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + // Default validation mode TEST_P(ServerInstanceImplTest, ValidationDefault) { options_.service_cluster_name_ = "some_cluster_name"; @@ -757,7 +804,7 @@ TEST_P(ServerInstanceImplTest, NoOptionsPassed) { init_manager_ = std::make_unique("Server"); EXPECT_THROW_WITH_MESSAGE( server_.reset(new InstanceImpl( - *init_manager_, options_, test_time_.timeSystem(), + *init_manager_, options_, time_system_, Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("127.0.0.1")), hooks_, restart_, stats_store_, fakelock_, component_factory_, std::make_unique>(), *thread_local_, diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 5f06b3af1656..cb0aadb49f16 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -315,6 +315,7 @@ getaddrinfo sendto ssize upcasts +vectorize vip xDSes XFCC