diff --git a/include/envoy/http/codes.h b/include/envoy/http/codes.h index 8fdf547e9db8..c0d80c64078f 100644 --- a/include/envoy/http/codes.h +++ b/include/envoy/http/codes.h @@ -3,6 +3,7 @@ #include #include "envoy/stats/scope.h" +#include "envoy/stats/symbol_table.h" namespace Envoy { namespace Http { @@ -86,36 +87,13 @@ class CodeStats { public: virtual ~CodeStats() = default; - struct ResponseStatInfo { - Stats::Scope& global_scope_; - Stats::Scope& cluster_scope_; - const std::string& prefix_; - uint64_t response_status_code_; - bool internal_request_; - const std::string& request_vhost_name_; - const std::string& request_vcluster_name_; - const std::string& from_zone_; - const std::string& to_zone_; - bool upstream_canary_; - }; - - struct ResponseTimingInfo { - Stats::Scope& global_scope_; - Stats::Scope& cluster_scope_; - const std::string& prefix_; - std::chrono::milliseconds response_time_; - bool upstream_canary_; - bool internal_request_; - const std::string& request_vhost_name_; - const std::string& request_vcluster_name_; - const std::string& from_zone_; - const std::string& to_zone_; - }; + struct ResponseStatInfo; + struct ResponseTimingInfo; /** * Charge a simple response stat to an upstream. */ - virtual void chargeBasicResponseStat(Stats::Scope& scope, const std::string& prefix, + virtual void chargeBasicResponseStat(Stats::Scope& scope, Stats::StatName prefix, Code response_code) const PURE; /** diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index d2652df8d466..103e34506167 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -332,9 +332,9 @@ class VirtualCluster { virtual ~VirtualCluster() {} /** - * @return the name of the virtual cluster. + * @return the stat-name of the virtual cluster. */ - virtual const std::string& name() const PURE; + virtual Stats::StatName statName() const PURE; }; class RateLimitPolicy; @@ -364,9 +364,9 @@ class VirtualHost { virtual const CorsPolicy* corsPolicy() const PURE; /** - * @return const std::string& the name of the virtual host. + * @return the stat-name of the virtual host. */ - virtual const std::string& name() const PURE; + virtual Stats::StatName statName() const PURE; /** * @return const RateLimitPolicy& the rate limit policy for the virtual host. diff --git a/include/envoy/upstream/host_description.h b/include/envoy/upstream/host_description.h index 1fabc6686946..8250b8582292 100644 --- a/include/envoy/upstream/host_description.h +++ b/include/envoy/upstream/host_description.h @@ -104,6 +104,11 @@ class HostDescription { */ virtual const envoy::api::v2::core::Locality& locality() const PURE; + /** + * @return the human readable name of the host's locality zone as a StatName. + */ + virtual Stats::StatName localityZoneStatName() const PURE; + /** * @return the address used to health check the host. */ diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 030c1bd95f1a..0cabd8d2b18f 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -183,7 +183,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, struct NullVirtualHost : public Router::VirtualHost { // Router::VirtualHost - const std::string& name() const override { return EMPTY_STRING; } + Stats::StatName statName() const override { return Stats::StatName(); } const Router::RateLimitPolicy& rateLimitPolicy() const override { return rate_limit_policy_; } const Router::CorsPolicy* corsPolicy() const override { return nullptr; } const Router::Config& routeConfig() const override { return route_configuration_; } diff --git a/source/common/http/codes.cc b/source/common/http/codes.cc index b302b53e0ce3..ebe4819b077c 100644 --- a/source/common/http/codes.cc +++ b/source/common/http/codes.cc @@ -18,111 +18,140 @@ namespace Envoy { namespace Http { -void CodeStatsImpl::chargeBasicResponseStat(Stats::Scope& scope, const std::string& prefix, +CodeStatsImpl::CodeStatsImpl(Stats::SymbolTable& symbol_table) + : stat_name_pool_(symbol_table), symbol_table_(symbol_table), + canary_(stat_name_pool_.add("canary")), external_(stat_name_pool_.add("external")), + internal_(stat_name_pool_.add("internal")), + upstream_rq_1xx_(stat_name_pool_.add("upstream_rq_1xx")), + upstream_rq_2xx_(stat_name_pool_.add("upstream_rq_2xx")), + upstream_rq_3xx_(stat_name_pool_.add("upstream_rq_3xx")), + upstream_rq_4xx_(stat_name_pool_.add("upstream_rq_4xx")), + upstream_rq_5xx_(stat_name_pool_.add("upstream_rq_5xx")), + upstream_rq_unknown_(stat_name_pool_.add("upstream_rq_unknown")), // Covers invalid http + // response codes e.g. 600. + upstream_rq_completed_(stat_name_pool_.add("upstream_rq_completed")), + upstream_rq_time_(stat_name_pool_.add("upstream_rq_time")), + vcluster_(stat_name_pool_.add("vcluster")), vhost_(stat_name_pool_.add("vhost")), + zone_(stat_name_pool_.add("zone")) { + + for (uint32_t i = 0; i < NumHttpCodes; ++i) { + rc_stat_names_[i] = nullptr; + } + + // Pre-allocate response codes 200, 404, and 503, as those seem quite likely. + // We don't pre-allocate all the HTTP codes because the first 127 allocations + // are likely to be encoded in one byte, and we would rather spend those on + // common components of stat-names that appear frequently. + upstreamRqStatName(Code::OK); + upstreamRqStatName(Code::NotFound); + upstreamRqStatName(Code::ServiceUnavailable); +} + +void CodeStatsImpl::incCounter(Stats::Scope& scope, + const std::vector& names) const { + const Stats::SymbolTable::StoragePtr stat_name_storage = symbol_table_.join(names); + scope.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc(); +} + +void CodeStatsImpl::incCounter(Stats::Scope& scope, Stats::StatName a, Stats::StatName b) const { + const Stats::SymbolTable::StoragePtr stat_name_storage = symbol_table_.join({a, b}); + scope.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc(); +} + +void CodeStatsImpl::recordHistogram(Stats::Scope& scope, const std::vector& names, + uint64_t count) const { + const Stats::SymbolTable::StoragePtr stat_name_storage = symbol_table_.join(names); + scope.histogramFromStatName(Stats::StatName(stat_name_storage.get())).recordValue(count); +} + +void CodeStatsImpl::chargeBasicResponseStat(Stats::Scope& scope, Stats::StatName prefix, Code response_code) const { + ASSERT(&symbol_table_ == &scope.symbolTable()); + // Build a dynamic stat for the response code and increment it. - scope.counter(absl::StrCat(prefix, upstream_rq_completed_)).inc(); - scope - .counter(absl::StrCat(prefix, upstream_rq_, - CodeUtility::groupStringForResponseCode(response_code))) - .inc(); - scope.counter(absl::StrCat(prefix, upstream_rq_, enumToInt(response_code))).inc(); + incCounter(scope, prefix, upstream_rq_completed_); + const Stats::StatName rq_group = upstreamRqGroup(response_code); + if (!rq_group.empty()) { + incCounter(scope, prefix, rq_group); + } + incCounter(scope, prefix, upstreamRqStatName(response_code)); } void CodeStatsImpl::chargeResponseStat(const ResponseStatInfo& info) const { - const uint64_t response_code = info.response_status_code_; - chargeBasicResponseStat(info.cluster_scope_, info.prefix_, static_cast(response_code)); + const Code code = static_cast(info.response_status_code_); + + ASSERT(&info.cluster_scope_.symbolTable() == &symbol_table_); + chargeBasicResponseStat(info.cluster_scope_, info.prefix_, code); - std::string group_string = - CodeUtility::groupStringForResponseCode(static_cast(response_code)); + const Stats::StatName rq_group = upstreamRqGroup(code); + const Stats::StatName rq_code = upstreamRqStatName(code); // If the response is from a canary, also create canary stats. if (info.upstream_canary_) { - info.cluster_scope_.counter(absl::StrCat(info.prefix_, canary_upstream_rq_completed_)).inc(); - info.cluster_scope_.counter(absl::StrCat(info.prefix_, canary_upstream_rq_, group_string)) - .inc(); - info.cluster_scope_.counter(absl::StrCat(info.prefix_, canary_upstream_rq_, response_code)) - .inc(); + writeCategory(info, rq_group, rq_code, canary_); } // Split stats into external vs. internal. if (info.internal_request_) { - info.cluster_scope_.counter(absl::StrCat(info.prefix_, internal_upstream_rq_completed_)).inc(); - info.cluster_scope_.counter(absl::StrCat(info.prefix_, internal_upstream_rq_, group_string)) - .inc(); - info.cluster_scope_.counter(absl::StrCat(info.prefix_, internal_upstream_rq_, response_code)) - .inc(); + writeCategory(info, rq_group, rq_code, internal_); } else { - info.cluster_scope_.counter(absl::StrCat(info.prefix_, external_upstream_rq_completed_)).inc(); - info.cluster_scope_.counter(absl::StrCat(info.prefix_, external_upstream_rq_, group_string)) - .inc(); - info.cluster_scope_.counter(absl::StrCat(info.prefix_, external_upstream_rq_, response_code)) - .inc(); + writeCategory(info, rq_group, rq_code, external_); } // Handle request virtual cluster. if (!info.request_vcluster_name_.empty()) { - info.global_scope_ - .counter(join({vhost_, info.request_vhost_name_, vcluster_, info.request_vcluster_name_, - upstream_rq_completed_})) - .inc(); - info.global_scope_ - .counter(join({vhost_, info.request_vhost_name_, vcluster_, info.request_vcluster_name_, - absl::StrCat(upstream_rq_, group_string)})) - .inc(); - info.global_scope_ - .counter(join({vhost_, info.request_vhost_name_, vcluster_, info.request_vcluster_name_, - absl::StrCat(upstream_rq_, response_code)})) - .inc(); + incCounter(info.global_scope_, {vhost_, info.request_vhost_name_, vcluster_, + info.request_vcluster_name_, upstream_rq_completed_}); + incCounter(info.global_scope_, {vhost_, info.request_vhost_name_, vcluster_, + info.request_vcluster_name_, rq_group}); + incCounter(info.global_scope_, + {vhost_, info.request_vhost_name_, vcluster_, info.request_vcluster_name_, rq_code}); } // Handle per zone stats. if (!info.from_zone_.empty() && !info.to_zone_.empty()) { - absl::string_view prefix_without_trailing_dot = stripTrailingDot(info.prefix_); - info.cluster_scope_ - .counter(join({prefix_without_trailing_dot, zone_, info.from_zone_, info.to_zone_, - upstream_rq_completed_})) - .inc(); - info.cluster_scope_ - .counter(join({prefix_without_trailing_dot, zone_, info.from_zone_, info.to_zone_, - absl::StrCat(upstream_rq_, group_string)})) - .inc(); - info.cluster_scope_ - .counter(join({prefix_without_trailing_dot, zone_, info.from_zone_, info.to_zone_, - absl::StrCat(upstream_rq_, response_code)})) - .inc(); + incCounter(info.cluster_scope_, + {info.prefix_, zone_, info.from_zone_, info.to_zone_, upstream_rq_completed_}); + incCounter(info.cluster_scope_, + {info.prefix_, zone_, info.from_zone_, info.to_zone_, rq_group}); + incCounter(info.cluster_scope_, {info.prefix_, zone_, info.from_zone_, info.to_zone_, rq_code}); + } +} + +void CodeStatsImpl::writeCategory(const ResponseStatInfo& info, Stats::StatName rq_group, + Stats::StatName rq_code, Stats::StatName category) const { + incCounter(info.cluster_scope_, {info.prefix_, category, upstream_rq_completed_}); + if (!rq_group.empty()) { + incCounter(info.cluster_scope_, {info.prefix_, category, rq_group}); } + incCounter(info.cluster_scope_, {info.prefix_, category, rq_code}); } void CodeStatsImpl::chargeResponseTiming(const ResponseTimingInfo& info) const { - info.cluster_scope_.histogram(absl::StrCat(info.prefix_, upstream_rq_time_)) - .recordValue(info.response_time_.count()); + const uint64_t count = info.response_time_.count(); + recordHistogram(info.cluster_scope_, {info.prefix_, upstream_rq_time_}, count); if (info.upstream_canary_) { - info.cluster_scope_.histogram(absl::StrCat(info.prefix_, canary_upstream_rq_time_)) - .recordValue(info.response_time_.count()); + recordHistogram(info.cluster_scope_, {info.prefix_, canary_, upstream_rq_time_}, count); } if (info.internal_request_) { - info.cluster_scope_.histogram(absl::StrCat(info.prefix_, internal_upstream_rq_time_)) - .recordValue(info.response_time_.count()); + recordHistogram(info.cluster_scope_, {info.prefix_, internal_, upstream_rq_time_}, count); } else { - info.cluster_scope_.histogram(absl::StrCat(info.prefix_, external_upstream_rq_time_)) - .recordValue(info.response_time_.count()); + recordHistogram(info.cluster_scope_, {info.prefix_, external_, upstream_rq_time_}, count); } if (!info.request_vcluster_name_.empty()) { - info.global_scope_ - .histogram(join({vhost_, info.request_vhost_name_, vcluster_, info.request_vcluster_name_, - upstream_rq_time_})) - .recordValue(info.response_time_.count()); + recordHistogram(info.global_scope_, + {vhost_, info.request_vhost_name_, vcluster_, info.request_vcluster_name_, + upstream_rq_time_}, + count); } // Handle per zone stats. if (!info.from_zone_.empty() && !info.to_zone_.empty()) { - info.cluster_scope_ - .histogram(join({stripTrailingDot(info.prefix_), zone_, info.from_zone_, info.to_zone_, - upstream_rq_time_})) - .recordValue(info.response_time_.count()); + recordHistogram(info.cluster_scope_, + {info.prefix_, zone_, info.from_zone_, info.to_zone_, upstream_rq_time_}, + count); } } @@ -133,19 +162,48 @@ absl::string_view CodeStatsImpl::stripTrailingDot(absl::string_view str) { return str; } -std::string CodeStatsImpl::join(const std::vector& v) { - if (v.empty()) { - return ""; +Stats::StatName CodeStatsImpl::upstreamRqGroup(Code response_code) const { + switch (enumToInt(response_code) / 100) { + case 1: + return upstream_rq_1xx_; + case 2: + return upstream_rq_2xx_; + case 3: + return upstream_rq_3xx_; + case 4: + return upstream_rq_4xx_; + case 5: + return upstream_rq_5xx_; } - auto iter = v.begin(); - if (iter->empty()) { - ++iter; // Skip any initial empty prefix. + return empty_; // Unknown codes do not go into a group. +} + +Stats::StatName CodeStatsImpl::upstreamRqStatName(Code response_code) const { + // Take a lock only if we've never seen this response-code before. + const uint32_t rc_index = static_cast(response_code) - HttpCodeOffset; + if (rc_index >= NumHttpCodes) { + return upstream_rq_unknown_; + } + std::atomic& atomic_ref = rc_stat_names_[rc_index]; + if (atomic_ref.load() == nullptr) { + absl::MutexLock lock(&mutex_); + + // Check again under lock as two threads might have raced to add a StatName + // for the same code. + if (atomic_ref.load() == nullptr) { + atomic_ref = stat_name_pool_.addReturningStorage( + absl::StrCat("upstream_rq_", enumToInt(response_code))); + } } - return absl::StrJoin(iter, v.end(), "."); + return Stats::StatName(atomic_ref.load()); } std::string CodeUtility::groupStringForResponseCode(Code response_code) { - if (CodeUtility::is2xx(enumToInt(response_code))) { + // Note: this is only used in the unit test and in dynamo_filter.cc, which + // needs the same sort of symbolization treatment we are doing here. + if (CodeUtility::is1xx(enumToInt(response_code))) { + return "1xx"; + } else if (CodeUtility::is2xx(enumToInt(response_code))) { return "2xx"; } else if (CodeUtility::is3xx(enumToInt(response_code))) { return "3xx"; diff --git a/source/common/http/codes.h b/source/common/http/codes.h index b7b50ee9fa08..66e4f5019a53 100644 --- a/source/common/http/codes.h +++ b/source/common/http/codes.h @@ -8,15 +8,43 @@ #include "envoy/http/header_map.h" #include "envoy/stats/scope.h" +#include "common/stats/symbol_table_impl.h" + namespace Envoy { namespace Http { +struct CodeStats::ResponseStatInfo { + Stats::Scope& global_scope_; + Stats::Scope& cluster_scope_; + Stats::StatName prefix_; + uint64_t response_status_code_; + bool internal_request_; + Stats::StatName request_vhost_name_; + Stats::StatName request_vcluster_name_; + Stats::StatName from_zone_; + Stats::StatName to_zone_; + bool upstream_canary_; +}; + +struct CodeStats::ResponseTimingInfo { + Stats::Scope& global_scope_; + Stats::Scope& cluster_scope_; + Stats::StatName prefix_; + std::chrono::milliseconds response_time_; + bool upstream_canary_; + bool internal_request_; + Stats::StatName request_vhost_name_; + Stats::StatName request_vcluster_name_; + Stats::StatName from_zone_; + Stats::StatName to_zone_; +}; + class CodeStatsImpl : public CodeStats { public: - ~CodeStatsImpl() override = default; + explicit CodeStatsImpl(Stats::SymbolTable& symbol_table); // CodeStats - void chargeBasicResponseStat(Stats::Scope& scope, const std::string& prefix, + void chargeBasicResponseStat(Stats::Scope& scope, Stats::StatName prefix, Code response_code) const override; void chargeResponseStat(const ResponseStatInfo& info) const override; void chargeResponseTiming(const ResponseTimingInfo& info) const override; @@ -24,46 +52,65 @@ class CodeStatsImpl : public CodeStats { private: friend class CodeStatsTest; - /** - * Strips any trailing "." from a prefix. This is handy as most prefixes - * are specified as a literal like "http.", or an empty-string "". We - * are going to be passing these, as well as other tokens, to join() below, - * which will add "." between each token. - * - * TODO(jmarantz): remove all the trailing dots in all stat prefix string - * literals. Then this function can be removed. - */ + void writeCategory(const ResponseStatInfo& info, Stats::StatName rq_group, + Stats::StatName rq_code, Stats::StatName category) const; + void incCounter(Stats::Scope& scope, const std::vector& names) const; + void incCounter(Stats::Scope& scope, Stats::StatName a, Stats::StatName b) const; + void recordHistogram(Stats::Scope& scope, const std::vector& names, + uint64_t count) const; + static absl::string_view stripTrailingDot(absl::string_view prefix); + Stats::StatName upstreamRqGroup(Code response_code) const; + Stats::StatName upstreamRqStatName(Code response_code) const; - /** - * Joins a string-view vector with "." between each token. If there's an - * initial blank token it is skipped. Leading blank tokens occur due to empty - * prefixes, which are fairly common. - * - * Note: this layer probably should be called something other than join(), - * like joinSkippingLeadingEmptyToken but I thought the decrease in - * readability at all the call-sites would not be worth it. - */ - static std::string join(const std::vector& v); - - // Predeclared tokens used for combining with join(). - const absl::string_view canary_upstream_rq_completed_{"canary.upstream_rq_completed"}; - const absl::string_view canary_upstream_rq_time_{"canary.upstream_rq_time"}; - const absl::string_view canary_upstream_rq_{"canary.upstream_rq_"}; - const absl::string_view external_rq_time_{"external.upstream_rq_time"}; - const absl::string_view external_upstream_rq_completed_{"external.upstream_rq_completed"}; - const absl::string_view external_upstream_rq_time_{"external.upstream_rq_time"}; - const absl::string_view external_upstream_rq_{"external.upstream_rq_"}; - const absl::string_view internal_rq_time_{"internal.upstream_rq_time"}; - const absl::string_view internal_upstream_rq_completed_{"internal.upstream_rq_completed"}; - const absl::string_view internal_upstream_rq_time_{"internal.upstream_rq_time"}; - const absl::string_view internal_upstream_rq_{"internal.upstream_rq_"}; - const absl::string_view upstream_rq_completed_{"upstream_rq_completed"}; - const absl::string_view upstream_rq_time_{"upstream_rq_time"}; - const absl::string_view upstream_rq_{"upstream_rq_"}; - const absl::string_view vcluster_{"vcluster"}; - const absl::string_view vhost_{"vhost"}; - const absl::string_view zone_{"zone"}; + mutable Stats::StatNamePool stat_name_pool_ GUARDED_BY(mutex_); + mutable absl::Mutex mutex_; + Stats::SymbolTable& symbol_table_; + + const Stats::StatName canary_; + const Stats::StatName empty_; // Used for the group-name for invalid http codes. + const Stats::StatName external_; + const Stats::StatName internal_; + const Stats::StatName upstream_; + const Stats::StatName upstream_rq_1xx_; + const Stats::StatName upstream_rq_2xx_; + const Stats::StatName upstream_rq_3xx_; + const Stats::StatName upstream_rq_4xx_; + const Stats::StatName upstream_rq_5xx_; + const Stats::StatName upstream_rq_unknown_; + const Stats::StatName upstream_rq_completed_; + const Stats::StatName upstream_rq_time; + const Stats::StatName upstream_rq_time_; + const Stats::StatName vcluster_; + const Stats::StatName vhost_; + const Stats::StatName zone_; + + // Use an array of atomic pointers to hold StatNameStorage objects for + // every conceivable HTTP response code. In the hot-path we'll reference + // these with a null-check, and if we need to allocate a symbol for a + // new code, we'll take a mutex to avoid duplicate allocations and + // subsequent leaks. This is similar in principle to a ReaderMutexLock, + // but should be faster, as ReaderMutexLocks appear to be too expensive for + // fine-grained controls. Another option would be to use a lock per + // stat-name, which might have similar performance to atomics with default + // barrier policy. + // + // We don't allocate these all up front during construction because + // SymbolTable greedily encodes the first 128 names it discovers in one + // byte. We don't want those high-value single-byte codes to go to fully + // enumerating the 4 prefixes combined with HTTP codes that are seldom used, + // so we allocate these on demand. + // + // There can be multiple symbol tables in a server. The one passed into the + // Codes constructor should be the same as the one passed to + // Stats::ThreadLocalStore. Note that additional symbol tables can be created + // from IsolatedStoreImpl's default constructor. + // + // The Codes object is global to the server. + + static constexpr uint32_t NumHttpCodes = 500; + static constexpr uint32_t HttpCodeOffset = 100; // code 100 is at index 0. + mutable std::atomic rc_stat_names_[NumHttpCodes]; }; /** diff --git a/source/common/http/context_impl.cc b/source/common/http/context_impl.cc index 0fc4791f0256..1a185f572dee 100644 --- a/source/common/http/context_impl.cc +++ b/source/common/http/context_impl.cc @@ -3,7 +3,8 @@ namespace Envoy { namespace Http { -ContextImpl::ContextImpl() : tracer_(&null_tracer_) {} +ContextImpl::ContextImpl(Stats::SymbolTable& symbol_table) + : tracer_(&null_tracer_), code_stats_(symbol_table) {} } // namespace Http } // namespace Envoy diff --git a/source/common/http/context_impl.h b/source/common/http/context_impl.h index a499af6ceda3..b1b093365d20 100644 --- a/source/common/http/context_impl.h +++ b/source/common/http/context_impl.h @@ -13,7 +13,7 @@ namespace Http { */ class ContextImpl : public Context { public: - ContextImpl(); + explicit ContextImpl(Stats::SymbolTable& symbol_table); ~ContextImpl() override = default; Tracing::HttpTracer& tracer() override { return *tracer_; } diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 35a3caa1e9f4..182c6c804f55 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -876,15 +876,18 @@ VirtualHostImpl::VirtualHostImpl(const envoy::api::v2::route::VirtualHost& virtu const ConfigImpl& global_route_config, Server::Configuration::FactoryContext& factory_context, bool validate_clusters) - : name_(virtual_host.name()), rate_limit_policy_(virtual_host.rate_limits()), - global_route_config_(global_route_config), + : stat_name_pool_(factory_context.scope().symbolTable()), + stat_name_(stat_name_pool_.add(virtual_host.name())), + rate_limit_policy_(virtual_host.rate_limits()), global_route_config_(global_route_config), request_headers_parser_(HeaderParser::configure(virtual_host.request_headers_to_add(), virtual_host.request_headers_to_remove())), response_headers_parser_(HeaderParser::configure(virtual_host.response_headers_to_add(), virtual_host.response_headers_to_remove())), per_filter_configs_(virtual_host.typed_per_filter_config(), virtual_host.per_filter_config(), factory_context), - include_attempt_count_(virtual_host.include_request_attempt_count()) { + include_attempt_count_(virtual_host.include_request_attempt_count()), + virtual_cluster_catch_all_(stat_name_pool_) { + switch (virtual_host.require_tls()) { case envoy::api::v2::route::VirtualHost::NONE: ssl_requirements_ = SslRequirements::NONE; @@ -935,7 +938,7 @@ VirtualHostImpl::VirtualHostImpl(const envoy::api::v2::route::VirtualHost& virtu } for (const auto& virtual_cluster : virtual_host.virtual_clusters()) { - virtual_clusters_.push_back(VirtualClusterEntry(virtual_cluster)); + virtual_clusters_.push_back(VirtualClusterEntry(virtual_cluster, stat_name_pool_)); } if (virtual_host.has_cors()) { @@ -944,13 +947,12 @@ VirtualHostImpl::VirtualHostImpl(const envoy::api::v2::route::VirtualHost& virtu } VirtualHostImpl::VirtualClusterEntry::VirtualClusterEntry( - const envoy::api::v2::route::VirtualCluster& virtual_cluster) { + const envoy::api::v2::route::VirtualCluster& virtual_cluster, Stats::StatNamePool& pool) + : pattern_(RegexUtil::parseRegex(virtual_cluster.pattern())), + stat_name_(pool.add(virtual_cluster.name())) { if (virtual_cluster.method() != envoy::api::v2::core::RequestMethod::METHOD_UNSPECIFIED) { method_ = envoy::api::v2::core::RequestMethod_Name(virtual_cluster.method()); } - - pattern_ = RegexUtil::parseRegex(virtual_cluster.pattern()); - name_ = virtual_cluster.name(); } const Config& VirtualHostImpl::routeConfig() const { return global_route_config_; } @@ -1087,7 +1089,6 @@ RouteConstSharedPtr RouteMatcher::route(const Http::HeaderMap& headers, } } -const VirtualHostImpl::CatchAllVirtualCluster VirtualHostImpl::VIRTUAL_CLUSTER_CATCH_ALL; const SslRedirector SslRedirectRoute::SSL_REDIRECTOR; const std::shared_ptr VirtualHostImpl::SSL_REDIRECT_ROUTE{ new SslRedirectRoute()}; @@ -1105,7 +1106,7 @@ VirtualHostImpl::virtualClusterFromEntries(const Http::HeaderMap& headers) const } if (!virtual_clusters_.empty()) { - return &VIRTUAL_CLUSTER_CATCH_ALL; + return &virtual_cluster_catch_all_; } return nullptr; @@ -1114,7 +1115,8 @@ VirtualHostImpl::virtualClusterFromEntries(const Http::HeaderMap& headers) const ConfigImpl::ConfigImpl(const envoy::api::v2::RouteConfiguration& config, Server::Configuration::FactoryContext& factory_context, bool validate_clusters_default) - : name_(config.name()), uses_vhds_(config.has_vhds()) { + : name_(config.name()), symbol_table_(factory_context.scope().symbolTable()), + uses_vhds_(config.has_vhds()) { route_matcher_ = std::make_unique( config, *this, factory_context, PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, validate_clusters, validate_clusters_default)); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index 286afc9135ab..9e7266b9a976 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -24,6 +24,7 @@ #include "common/router/header_parser.h" #include "common/router/metadatamatchcriteria_impl.h" #include "common/router/router_ratelimit.h" +#include "common/stats/symbol_table_impl.h" #include "absl/types/optional.h" @@ -149,12 +150,12 @@ class VirtualHostImpl : public VirtualHost { uint64_t random_value) const; const VirtualCluster* virtualClusterFromEntries(const Http::HeaderMap& headers) const; const ConfigImpl& globalRouteConfig() const { return global_route_config_; } - const HeaderParser& requestHeaderParser() const { return *request_headers_parser_; }; - const HeaderParser& responseHeaderParser() const { return *response_headers_parser_; }; + const HeaderParser& requestHeaderParser() const { return *request_headers_parser_; } + const HeaderParser& responseHeaderParser() const { return *response_headers_parser_; } // Router::VirtualHost const CorsPolicy* corsPolicy() const override { return cors_policy_.get(); } - const std::string& name() const override { return name_; } + Stats::StatName statName() const override { return stat_name_; } const RateLimitPolicy& rateLimitPolicy() const override { return rate_limit_policy_; } const Config& routeConfig() const override; const RouteSpecificFilterConfig* perFilterConfig(const std::string&) const override; @@ -170,27 +171,32 @@ class VirtualHostImpl : public VirtualHost { enum class SslRequirements { NONE, EXTERNAL_ONLY, ALL }; struct VirtualClusterEntry : public VirtualCluster { - VirtualClusterEntry(const envoy::api::v2::route::VirtualCluster& virtual_cluster); + VirtualClusterEntry(const envoy::api::v2::route::VirtualCluster& virtual_cluster, + Stats::StatNamePool& pool); // Router::VirtualCluster - const std::string& name() const override { return name_; } + Stats::StatName statName() const override { return stat_name_; } - std::regex pattern_; + const std::regex pattern_; absl::optional method_; - std::string name_; + const Stats::StatName stat_name_; }; - struct CatchAllVirtualCluster : public VirtualCluster { + class CatchAllVirtualCluster : public VirtualCluster { + public: + explicit CatchAllVirtualCluster(Stats::StatNamePool& pool) : stat_name_(pool.add("other")) {} + // Router::VirtualCluster - const std::string& name() const override { return name_; } + Stats::StatName statName() const override { return stat_name_; } - std::string name_{"other"}; + private: + const Stats::StatName stat_name_; }; - static const CatchAllVirtualCluster VIRTUAL_CLUSTER_CATCH_ALL; static const std::shared_ptr SSL_REDIRECT_ROUTE; - const std::string name_; + Stats::StatNamePool stat_name_pool_; + const Stats::StatName stat_name_; std::vector routes_; std::vector virtual_clusters_; SslRequirements ssl_requirements_; @@ -204,6 +210,7 @@ class VirtualHostImpl : public VirtualHost { const bool include_attempt_count_; absl::optional retry_policy_; absl::optional hedge_policy_; + const CatchAllVirtualCluster virtual_cluster_catch_all_; }; typedef std::shared_ptr VirtualHostSharedPtr; @@ -787,6 +794,7 @@ class ConfigImpl : public Config { HeaderParserPtr request_headers_parser_; HeaderParserPtr response_headers_parser_; const std::string name_; + Stats::SymbolTable& symbol_table_; const bool uses_vhds_; }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4f698379a4c5..f8ac10974a69 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -195,8 +195,8 @@ Filter::~Filter() { ASSERT(!retry_state_); } -const std::string& Filter::upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host) { - return upstream_host ? upstream_host->locality().zone() : EMPTY_STRING; +Stats::StatName Filter::upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host) { + return upstream_host ? upstream_host->localityZoneStatName() : config_.empty_stat_name_; } void Filter::chargeUpstreamCode(uint64_t response_status_code, @@ -215,34 +215,34 @@ void Filter::chargeUpstreamCode(uint64_t response_status_code, const bool internal_request = internal_request_header && internal_request_header->value() == "true"; + Stats::StatName upstream_zone = upstreamZone(upstream_host); Http::CodeStats::ResponseStatInfo info{config_.scope_, cluster_->statsScope(), - EMPTY_STRING, + config_.empty_stat_name_, response_status_code, internal_request, - route_entry_->virtualHost().name(), - request_vcluster_ ? request_vcluster_->name() - : EMPTY_STRING, - config_.local_info_.zoneName(), - upstreamZone(upstream_host), + route_entry_->virtualHost().statName(), + request_vcluster_ ? request_vcluster_->statName() + : config_.empty_stat_name_, + config_.zone_name_, + upstream_zone, is_canary}; Http::CodeStats& code_stats = httpContext().codeStats(); code_stats.chargeResponseStat(info); - if (!alt_stat_prefix_.empty()) { - Http::CodeStats::ResponseStatInfo info{config_.scope_, - cluster_->statsScope(), - alt_stat_prefix_, - response_status_code, - internal_request, - EMPTY_STRING, - EMPTY_STRING, - config_.local_info_.zoneName(), - upstreamZone(upstream_host), - is_canary}; - - code_stats.chargeResponseStat(info); + if (alt_stat_prefix_ != nullptr) { + Http::CodeStats::ResponseStatInfo alt_info{config_.scope_, + cluster_->statsScope(), + alt_stat_prefix_->statName(), + response_status_code, + internal_request, + config_.empty_stat_name_, + config_.empty_stat_name_, + config_.zone_name_, + upstream_zone, + is_canary}; + code_stats.chargeResponseStat(alt_info); } if (dropped) { @@ -333,7 +333,12 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e const Http::HeaderEntry* request_alt_name = headers.EnvoyUpstreamAltStatName(); if (request_alt_name) { - alt_stat_prefix_ = std::string(request_alt_name->value().getStringView()) + "."; + // TODO(#7003): converting this header value into a StatName requires + // taking a global symbol-table lock. This is not a frequently used feature, + // but may not be the only occurrence of this pattern, where it's difficult + // or impossible to pre-compute a StatName for a component of a stat name. + alt_stat_prefix_ = std::make_unique( + request_alt_name->value().getStringView(), config_.scope_.symbolTable()); headers.removeEnvoyUpstreamAltStatName(); } @@ -770,7 +775,7 @@ void Filter::onUpstreamHeaders(uint64_t response_code, Http::HeaderMapPtr&& head retry_state_->shouldRetryHeaders(*headers, [this]() -> void { doRetry(); }); if (retry_status == RetryStatus::Yes && setupRetry(end_stream)) { Http::CodeStats& code_stats = httpContext().codeStats(); - code_stats.chargeBasicResponseStat(cluster_->statsScope(), "retry.", + code_stats.chargeBasicResponseStat(cluster_->statsScope(), config_.retry_, static_cast(response_code)); upstream_host->stats().rq_error_.inc(); return; @@ -887,28 +892,28 @@ void Filter::onUpstreamComplete(UpstreamRequest& upstream_request) { Http::CodeStats& code_stats = httpContext().codeStats(); Http::CodeStats::ResponseTimingInfo info{config_.scope_, cluster_->statsScope(), - EMPTY_STRING, + config_.empty_stat_name_, response_time, upstream_request.upstream_canary_, internal_request, - route_entry_->virtualHost().name(), - request_vcluster_ ? request_vcluster_->name() - : EMPTY_STRING, - config_.local_info_.zoneName(), + route_entry_->virtualHost().statName(), + request_vcluster_ ? request_vcluster_->statName() + : config_.empty_stat_name_, + config_.zone_name_, upstreamZone(upstream_request.upstream_host_)}; code_stats.chargeResponseTiming(info); - if (!alt_stat_prefix_.empty()) { + if (alt_stat_prefix_ != nullptr) { Http::CodeStats::ResponseTimingInfo info{config_.scope_, cluster_->statsScope(), - alt_stat_prefix_, + alt_stat_prefix_->statName(), response_time, upstream_request.upstream_canary_, internal_request, - EMPTY_STRING, - EMPTY_STRING, - config_.local_info_.zoneName(), + config_.empty_stat_name_, + config_.empty_stat_name_, + config_.zone_name_, upstreamZone(upstream_request.upstream_host_)}; code_stats.chargeResponseTiming(info); diff --git a/source/common/router/router.h b/source/common/router/router.h index 61f0c6b02f9d..c8c786baec45 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -26,6 +26,7 @@ #include "common/config/well_known_names.h" #include "common/http/utility.h" #include "common/router/config_impl.h" +#include "common/stats/symbol_table_impl.h" #include "common/stream_info/stream_info_impl.h" #include "common/upstream/load_balancer_impl.h" @@ -105,7 +106,10 @@ class FilterConfig { random_(random), stats_{ALL_ROUTER_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix))}, emit_dynamic_stats_(emit_dynamic_stats), start_child_span_(start_child_span), suppress_envoy_headers_(suppress_envoy_headers), http_context_(http_context), - shadow_writer_(std::move(shadow_writer)), time_source_(time_source) {} + stat_name_pool_(scope_.symbolTable()), retry_(stat_name_pool_.add("retry")), + zone_name_(stat_name_pool_.add(local_info_.zoneName())), + empty_stat_name_(stat_name_pool_.add("")), shadow_writer_(std::move(shadow_writer)), + time_source_(time_source) {} FilterConfig(const std::string& stat_prefix, Server::Configuration::FactoryContext& context, ShadowWriterPtr&& shadow_writer, @@ -134,6 +138,10 @@ class FilterConfig { const bool suppress_envoy_headers_; std::list upstream_logs_; Http::Context& http_context_; + Stats::StatNamePool stat_name_pool_; + Stats::StatName retry_; + Stats::StatName zone_name_; + Stats::StatName empty_stat_name_; private: ShadowWriterPtr shadow_writer_; @@ -370,7 +378,7 @@ class Filter : Logger::Loggable, StreamInfo::ResponseFlag streamResetReasonToResponseFlag(Http::StreamResetReason reset_reason); - static const std::string& upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host); + Stats::StatName upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host); void chargeUpstreamCode(uint64_t response_status_code, const Http::HeaderMap& response_headers, Upstream::HostDescriptionConstSharedPtr upstream_host, bool dropped); void chargeUpstreamCode(Http::Code code, Upstream::HostDescriptionConstSharedPtr upstream_host, @@ -422,7 +430,7 @@ class Filter : Logger::Loggable, RouteConstSharedPtr route_; const RouteEntry* route_entry_{}; Upstream::ClusterInfoConstSharedPtr cluster_; - std::string alt_stat_prefix_; + std::unique_ptr alt_stat_prefix_; const VirtualCluster* request_vcluster_; Event::TimerPtr response_timeout_; FilterUtility::TimeoutData timeout_; diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index 9b85687a66bc..9a330cabf08b 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -25,12 +25,12 @@ void StatName::debugPrint() { if (size_and_data_ == nullptr) { ENVOY_LOG_MISC(info, "Null StatName"); } else { - uint64_t nbytes = dataSize(); + const uint64_t nbytes = dataSize(); std::string msg = absl::StrCat("dataSize=", nbytes, ":"); for (uint64_t i = 0; i < nbytes; ++i) { absl::StrAppend(&msg, " ", static_cast(data()[i])); } - SymbolVec encoding = SymbolTableImpl::Encoding::decodeSymbols(data(), dataSize()); + const SymbolVec encoding = SymbolTableImpl::Encoding::decodeSymbols(data(), dataSize()); absl::StrAppend(&msg, ", numSymbols=", encoding.size(), ":"); for (Symbol symbol : encoding) { absl::StrAppend(&msg, " ", symbol); @@ -87,7 +87,7 @@ SymbolVec SymbolTableImpl::Encoding::decodeSymbols(const SymbolTable::Storage ar } uint64_t SymbolTableImpl::Encoding::moveToStorage(SymbolTable::Storage symbol_array) { - uint64_t sz = dataBytesRequired(); + const uint64_t sz = dataBytesRequired(); symbol_array = writeLengthReturningNext(sz, symbol_array); if (sz != 0) { memcpy(symbol_array, vec_.data(), sz * sizeof(uint8_t)); @@ -118,7 +118,7 @@ void SymbolTableImpl::addTokensToEncoding(const absl::string_view name, Encoding // We want to hold the lock for the minimum amount of time, so we do the // string-splitting and prepare a temp vector of Symbol first. - std::vector tokens = absl::StrSplit(name, '.'); + const std::vector tokens = absl::StrSplit(name, '.'); std::vector symbols; symbols.reserve(tokens.size()); @@ -167,7 +167,7 @@ std::string SymbolTableImpl::decodeSymbolVec(const SymbolVec& symbols) const { void SymbolTableImpl::incRefCount(const StatName& stat_name) { // Before taking the lock, decode the array of symbols from the SymbolTable::Storage. - SymbolVec symbols = Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()); + const SymbolVec symbols = Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()); Thread::LockGuard lock(lock_); for (Symbol symbol : symbols) { @@ -183,7 +183,7 @@ void SymbolTableImpl::incRefCount(const StatName& stat_name) { void SymbolTableImpl::free(const StatName& stat_name) { // Before taking the lock, decode the array of symbols from the SymbolTable::Storage. - SymbolVec symbols = Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()); + const SymbolVec symbols = Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()); Thread::LockGuard lock(lock_); for (Symbol symbol : symbols) { @@ -255,8 +255,8 @@ bool SymbolTableImpl::lessThan(const StatName& a, const StatName& b) const { // If this becomes a performance bottleneck (e.g. during sorting), we could // provide an iterator-like interface for incrementally decoding the symbols // without allocating memory. - SymbolVec av = Encoding::decodeSymbols(a.data(), a.dataSize()); - SymbolVec bv = Encoding::decodeSymbols(b.data(), b.dataSize()); + const SymbolVec av = Encoding::decodeSymbols(a.data(), a.dataSize()); + const SymbolVec bv = Encoding::decodeSymbols(b.data(), b.dataSize()); // Calling fromSymbol requires holding the lock, as it needs read-access to // the maps that are written when adding new symbols. @@ -279,7 +279,7 @@ void SymbolTableImpl::debugPrint() const { } std::sort(symbols.begin(), symbols.end()); for (Symbol symbol : symbols) { - std::string& token = *decode_map_.find(symbol)->second; + const std::string& token = *decode_map_.find(symbol)->second; const SharedSymbol& shared_symbol = encode_map_.find(token)->second; ENVOY_LOG_MISC(info, "{}: '{}' ({})", symbol, token, shared_symbol.ref_count_); } @@ -298,7 +298,7 @@ StatNameStorage::StatNameStorage(absl::string_view name, SymbolTable& table) : bytes_(table.encode(name)) {} StatNameStorage::StatNameStorage(StatName src, SymbolTable& table) { - uint64_t size = src.size(); + const uint64_t size = src.size(); bytes_ = std::make_unique(size); src.copyToStorage(bytes_.get()); table.incRefCount(statName()); @@ -324,11 +324,13 @@ void StatNamePool::clear() { storage_vector_.clear(); } -StatName StatNamePool::add(absl::string_view str) { +uint8_t* StatNamePool::addReturningStorage(absl::string_view str) { storage_vector_.push_back(Stats::StatNameStorage(str, symbol_table_)); - return StatName(storage_vector_.back().bytes()); + return storage_vector_.back().bytes(); } +StatName StatNamePool::add(absl::string_view str) { return StatName(addReturningStorage(str)); } + StatNameStorageSet::~StatNameStorageSet() { // free() must be called before destructing StatNameStorageSet to decrement // references to all symbols. @@ -367,9 +369,9 @@ SymbolTable::StoragePtr SymbolTableImpl::join(const std::vector& stat_ auto bytes = std::make_unique(num_bytes + StatNameSizeEncodingBytes); uint8_t* p = writeLengthReturningNext(num_bytes, bytes.get()); for (StatName stat_name : stat_names) { - num_bytes = stat_name.dataSize(); - memcpy(p, stat_name.data(), num_bytes); - p += num_bytes; + const uint64_t stat_name_bytes = stat_name.dataSize(); + memcpy(p, stat_name.data(), stat_name_bytes); + p += stat_name_bytes; } return bytes; } @@ -408,10 +410,10 @@ void SymbolTableImpl::populateList(const absl::string_view* names, uint32_t num_ StatNameList::~StatNameList() { ASSERT(!populated()); } void StatNameList::iterate(const std::function& f) const { - uint8_t* p = &storage_[0]; - uint32_t num_elements = *p++; + const uint8_t* p = &storage_[0]; + const uint32_t num_elements = *p++; for (uint32_t i = 0; i < num_elements; ++i) { - StatName stat_name(p); + const StatName stat_name(p); p += stat_name.size(); if (!f(stat_name)) { break; diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index 366963f790c7..b3405298f1a6 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -417,6 +417,8 @@ class StatNameManagedStorage : public StatNameStorage { * StatNamePool pool(symbol_table); * StatName name1 = pool.add("name1"); * StatName name2 = pool.add("name2"); + * uint8_t* storage = pool.addReturningStorage("name3"); + * StatName name3(storage); */ class StatNamePool { public: @@ -434,6 +436,24 @@ class StatNamePool { */ StatName add(absl::string_view name); + /** + * Does essentially the same thing as add(), but returns the storage as a + * pointer which can later be used to create a StatName. This can be used + * to accumulate a vector of uint8_t* which can later be used to create + * StatName objects on demand. + * + * The use-case for this is in source/common/http/codes.cc, where we have a + * fixed sized array of atomic pointers, indexed by HTTP code. This API + * enables storing the allocated stat-name in that array of atomics, which + * enables content-avoidance when finding StatNames for frequently used HTTP + * codes. + * + * @param name the name to add the container. + * @return a pointer to the bytes held in the container for this name, suitable for + * using to construct a StatName. + */ + uint8_t* addReturningStorage(absl::string_view name); + private: // We keep the stat names in a vector of StatNameStorage, storing the // SymbolTable reference separately. This saves 8 bytes per StatName, diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index 11c6e1e8549b..fa3bf93e2b88 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -138,9 +138,9 @@ Upstream::Host::CreateConnectionData LogicalDnsCluster::LogicalHost::createConne ASSERT(data.current_resolved_address_); return {HostImpl::createConnection(dispatcher, *parent_.info_, data.current_resolved_address_, options, transport_socket_options), - HostDescriptionConstSharedPtr{ - new RealHostDescription(data.current_resolved_address_, parent_.localityLbEndpoint(), - parent_.lbEndpoint(), shared_from_this())}}; + HostDescriptionConstSharedPtr{new RealHostDescription( + data.current_resolved_address_, parent_.localityLbEndpoint(), parent_.lbEndpoint(), + shared_from_this(), parent_.symbolTable())}}; } ClusterImplBaseSharedPtr LogicalDnsClusterFactory::createClusterImpl( diff --git a/source/common/upstream/logical_dns_cluster.h b/source/common/upstream/logical_dns_cluster.h index 79e60b9698c5..19fd052b3a38 100644 --- a/source/common/upstream/logical_dns_cluster.h +++ b/source/common/upstream/logical_dns_cluster.h @@ -76,7 +76,7 @@ class LogicalDnsCluster : public ClusterImplBase { RealHostDescription(Network::Address::InstanceConstSharedPtr address, const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint, const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint, - HostConstSharedPtr logical_host) + HostConstSharedPtr logical_host, Stats::SymbolTable& symbol_table) : address_(address), logical_host_(logical_host), metadata_(std::make_shared(lb_endpoint.metadata())), health_check_address_( @@ -84,7 +84,8 @@ class LogicalDnsCluster : public ClusterImplBase { ? address : Network::Utility::getAddressWithPort( *address, lb_endpoint.endpoint().health_check_config().port_value())), - locality_lb_endpoint_(locality_lb_endpoint), lb_endpoint_(lb_endpoint) {} + locality_lb_endpoint_(locality_lb_endpoint), lb_endpoint_(lb_endpoint), + locality_zone_stat_name_(locality().zone(), symbol_table) {} // Upstream:HostDescription bool canary() const override { return false; } @@ -107,6 +108,9 @@ class LogicalDnsCluster : public ClusterImplBase { const envoy::api::v2::core::Locality& locality() const override { return locality_lb_endpoint_.locality(); } + Stats::StatName localityZoneStatName() const override { + return locality_zone_stat_name_.statName(); + } Network::Address::InstanceConstSharedPtr healthCheckAddress() const override { return health_check_address_; } @@ -120,6 +124,7 @@ class LogicalDnsCluster : public ClusterImplBase { Network::Address::InstanceConstSharedPtr health_check_address_; envoy::api::v2::endpoint::LocalityLbEndpoints locality_lb_endpoint_; const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint_; + Stats::StatNameManagedStorage locality_zone_stat_name_; }; struct PerThreadCurrentHostData : public ThreadLocal::ThreadLocalObject { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 518218c27731..a1a6c9455cb5 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -669,7 +669,8 @@ ClusterImplBase::ClusterImplBase( Server::Configuration::TransportSocketFactoryContext& factory_context, Stats::ScopePtr&& stats_scope, bool added_via_api) : init_manager_(fmt::format("Cluster {}", cluster.name())), - init_watcher_("ClusterImplBase", [this]() { onInitDone(); }), runtime_(runtime) { + init_watcher_("ClusterImplBase", [this]() { onInitDone(); }), runtime_(runtime), + symbol_table_(stats_scope->symbolTable()) { factory_context.setInitManager(init_manager_); auto socket_factory = createTransportSocketFactory(cluster, factory_context); info_ = std::make_unique(cluster, factory_context.clusterManager().bindConfig(), diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index bd49dc9d676d..e9ddbb171a0b 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -71,9 +71,9 @@ class HostDescriptionImpl : virtual public HostDescription { canary_(Config::Metadata::metadataValue(metadata, Config::MetadataFilters::get().ENVOY_LB, Config::MetadataEnvoyLbKeys::get().CANARY) .bool_value()), - metadata_(std::make_shared(metadata)), - locality_(locality), stats_{ALL_HOST_STATS(POOL_COUNTER(stats_store_), - POOL_GAUGE(stats_store_))}, + metadata_(std::make_shared(metadata)), locality_(locality), + locality_zone_stat_name_(locality.zone(), cluster->statsScope().symbolTable()), + stats_{ALL_HOST_STATS(POOL_COUNTER(stats_store_), POOL_GAUGE(stats_store_))}, priority_(priority) { if (health_check_config.port_value() != 0 && dest_address->type() != Network::Address::Type::Ip) { @@ -138,6 +138,9 @@ class HostDescriptionImpl : virtual public HostDescription { // Setting health check address is usually done at initialization. This is NOP by default. void setHealthCheckAddress(Network::Address::InstanceConstSharedPtr) override {} const envoy::api::v2::core::Locality& locality() const override { return locality_; } + Stats::StatName localityZoneStatName() const override { + return locality_zone_stat_name_.statName(); + } uint32_t priority() const override { return priority_; } void priority(uint32_t priority) override { priority_ = priority; } @@ -150,6 +153,7 @@ class HostDescriptionImpl : virtual public HostDescription { mutable absl::Mutex metadata_mutex_; std::shared_ptr metadata_ GUARDED_BY(metadata_mutex_); const envoy::api::v2::core::Locality locality_; + Stats::StatNameManagedStorage locality_zone_stat_name_; Stats::IsolatedStoreImpl stats_store_; HostStats stats_; Outlier::DetectorHostMonitorPtr outlier_detector_; @@ -668,6 +672,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable partitionHostsPerLocality(const HostsPerLocality& hosts); + Stats::SymbolTable& symbolTable() { return symbol_table_; } // Upstream::Cluster HealthChecker* healthChecker() override { return health_checker_.get(); } @@ -727,6 +732,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable initialization_complete_callback_; uint64_t pending_initialize_health_checks_{}; + Stats::SymbolTable& symbol_table_; }; using ClusterImplBaseSharedPtr = std::shared_ptr; diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index c7b4bb98779a..9c4e87af2df5 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -138,6 +138,7 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { ASSERT(cluster_); state_ = State::Complete; using Filters::Common::ExtAuthz::CheckStatus; + Stats::StatName empty_stat_name; switch (response->status) { case CheckStatus::OK: @@ -150,13 +151,13 @@ void Filter::onComplete(Filters::Common::ExtAuthz::ResponsePtr&& response) { cluster_->statsScope().counter("ext_authz.denied").inc(); Http::CodeStats::ResponseStatInfo info{config_->scope(), cluster_->statsScope(), - EMPTY_STRING, + empty_stat_name, enumToInt(response->status_code), true, - EMPTY_STRING, - EMPTY_STRING, - EMPTY_STRING, - EMPTY_STRING, + empty_stat_name, + empty_stat_name, + empty_stat_name, + empty_stat_name, false}; config_->httpContext().codeStats().chargeResponseStat(info); break; diff --git a/source/extensions/filters/http/ratelimit/ratelimit.cc b/source/extensions/filters/http/ratelimit/ratelimit.cc index a23d3037676d..ed267a1fd3e2 100644 --- a/source/extensions/filters/http/ratelimit/ratelimit.cc +++ b/source/extensions/filters/http/ratelimit/ratelimit.cc @@ -129,6 +129,7 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, Http::HeaderMapPtr&& headers) { state_ = State::Complete; headers_to_add_ = std::move(headers); + Stats::StatName empty_stat_name; switch (status) { case Filters::Common::RateLimit::LimitStatus::OK: @@ -141,13 +142,13 @@ void Filter::complete(Filters::Common::RateLimit::LimitStatus status, cluster_->statsScope().counter("ratelimit.over_limit").inc(); Http::CodeStats::ResponseStatInfo info{config_->scope(), cluster_->statsScope(), - EMPTY_STRING, + empty_stat_name, enumToInt(Http::Code::TooManyRequests), true, - EMPTY_STRING, - EMPTY_STRING, - EMPTY_STRING, - EMPTY_STRING, + empty_stat_name, + empty_stat_name, + empty_stat_name, + empty_stat_name, false}; httpContext().codeStats().chargeResponseStat(info); headers_to_add_->insertEnvoyRateLimited().value( diff --git a/source/extensions/filters/network/redis_proxy/config.cc b/source/extensions/filters/network/redis_proxy/config.cc index 9838c2cc5ebf..137886868cbf 100644 --- a/source/extensions/filters/network/redis_proxy/config.cc +++ b/source/extensions/filters/network/redis_proxy/config.cc @@ -52,7 +52,8 @@ Network::FilterFactoryCb RedisProxyFilterConfigFactory::createFilterFactoryFromP upstreams.emplace(cluster, std::make_shared( cluster, context.clusterManager(), Common::Redis::Client::ClientFactoryImpl::instance_, - context.threadLocal(), proto_config.settings())); + context.threadLocal(), proto_config.settings(), + context.scope().symbolTable())); } auto router = std::make_unique(prefix_routes, std::move(upstreams)); diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc index 018d1135231a..be62ad9cdfac 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc @@ -16,8 +16,10 @@ namespace ConnPool { InstanceImpl::InstanceImpl( const std::string& cluster_name, Upstream::ClusterManager& cm, Common::Redis::Client::ClientFactory& client_factory, ThreadLocal::SlotAllocator& tls, - const envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings& config) - : cm_(cm), client_factory_(client_factory), tls_(tls.allocateSlot()), config_(config) { + const envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings& config, + Stats::SymbolTable& symbol_table) + : cm_(cm), client_factory_(client_factory), tls_(tls.allocateSlot()), config_(config), + symbol_table_(symbol_table) { tls_->set([this, cluster_name]( Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(*this, dispatcher, cluster_name); diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h index 8ed565ac50b9..2825cfe6a5ff 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h @@ -39,7 +39,8 @@ class InstanceImpl : public Instance { InstanceImpl( const std::string& cluster_name, Upstream::ClusterManager& cm, Common::Redis::Client::ClientFactory& client_factory, ThreadLocal::SlotAllocator& tls, - const envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings& config); + const envoy::config::filter::network::redis_proxy::v2::RedisProxy::ConnPoolSettings& config, + Stats::SymbolTable& symbol_table); // RedisProxy::ConnPool::Instance Common::Redis::Client::PoolRequest* makeRequest(const std::string& key, const Common::Redis::RespValue& request, @@ -48,6 +49,8 @@ class InstanceImpl : public Instance { makeRequestToHost(const std::string& host_address, const Common::Redis::RespValue& request, Common::Redis::Client::PoolCallbacks& callbacks) override; + Stats::SymbolTable& symbolTable() { return symbol_table_; } + private: struct ThreadLocalPool; @@ -110,6 +113,7 @@ class InstanceImpl : public Instance { Common::Redis::Client::ClientFactory& client_factory_; ThreadLocal::SlotPtr tls_; Common::Redis::Client::ConfigImpl config_; + Stats::SymbolTable& symbol_table_; }; } // namespace ConnPool diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 8c0b41e66f69..37c0903bcfca 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -48,7 +48,7 @@ ValidationInstance::ValidationInstance(const Options& options, Event::TimeSystem singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory().currentThreadId())), access_log_manager_(options.fileFlushIntervalMsec(), *api_, *dispatcher_, access_log_lock, store), - mutex_tracer_(nullptr), time_system_(time_system) { + mutex_tracer_(nullptr), http_context_(stats_store_.symbolTable()), time_system_(time_system) { try { initialize(options, local_address, component_factory); } catch (const EnvoyException& e) { diff --git a/source/server/server.cc b/source/server/server.cc index 0a039883d0f0..53d017d9fa0c 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -70,7 +70,7 @@ InstanceImpl::InstanceImpl(const Options& options, Event::TimeSystem& time_syste terminated_(false), mutex_tracer_(options.mutexTracingEnabled() ? &Envoy::MutexTracerImpl::getOrCreateTracer() : nullptr), - main_thread_id_(std::this_thread::get_id()) { + http_context_(store.symbolTable()), main_thread_id_(std::this_thread::get_id()) { try { if (!options.logPath().empty()) { try { diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 3b57f66cf323..d32253299715 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -102,7 +102,7 @@ TEST_F(AccessLogImplTest, DownstreamDisconnect) { EXPECT_CALL(*file_, write(_)); - std::shared_ptr cluster{new Upstream::MockClusterInfo()}; + auto cluster = std::make_shared>(); stream_info_.upstream_host_ = Upstream::makeTestHostDescription(cluster, "tcp://10.0.0.5:1234"); stream_info_.response_flags_ = StreamInfo::ResponseFlag::DownstreamConnectionTermination; @@ -147,7 +147,7 @@ TEST_F(AccessLogImplTest, NoFilter) { } TEST_F(AccessLogImplTest, UpstreamHost) { - std::shared_ptr cluster{new Upstream::MockClusterInfo()}; + auto cluster = std::make_shared>(); stream_info_.upstream_host_ = Upstream::makeTestHostDescription(cluster, "tcp://10.0.0.5:1234"); const std::string json = R"EOF( diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index 4d4a94e288e7..89bf21c9958b 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -222,7 +222,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest { GrpcClientIntegrationTest() : method_descriptor_(helloworld::Greeter::descriptor()->FindMethodByName("SayHello")), api_(Api::createApiForTest(*stats_store_, test_time_.timeSystem())), - dispatcher_(api_->allocateDispatcher()) {} + dispatcher_(api_->allocateDispatcher()), http_context_(stats_store_->symbolTable()) {} virtual void initialize() { if (fake_upstream_ == nullptr) { diff --git a/test/common/http/BUILD b/test/common/http/BUILD index aaf0f6d92024..a3dde8c5c747 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -110,7 +110,6 @@ envoy_cc_test_binary( "benchmark", ], deps = [ - "//source/common/common:empty_string", "//source/common/http:codes_lib", "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index c14523bcfa07..47584c65a34a 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -37,7 +37,8 @@ namespace { class AsyncClientImplTest : public testing::Test { public: AsyncClientImplTest() - : client_(cm_.thread_local_cluster_.cluster_.info_, stats_store_, dispatcher_, local_info_, + : http_context_(stats_store_.symbolTable()), + client_(cm_.thread_local_cluster_.cluster_.info_, stats_store_, dispatcher_, local_info_, cm_, runtime_, random_, Router::ShadowWriterPtr{new NiceMock()}, http_context_) { message_->headers().insertMethod().value(std::string("GET")); diff --git a/test/common/http/codes_speed_test.cc b/test/common/http/codes_speed_test.cc index 8ec80973600f..7883b97ac4cc 100644 --- a/test/common/http/codes_speed_test.cc +++ b/test/common/http/codes_speed_test.cc @@ -8,7 +8,6 @@ #include "envoy/stats/stats.h" -#include "common/common/empty_string.h" #include "common/http/codes.h" #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/isolated_store_impl.h" @@ -20,16 +19,18 @@ namespace Http { class CodeUtilitySpeedTest { public: - CodeUtilitySpeedTest() : global_store_(symbol_table_), cluster_scope_(symbol_table_) {} + CodeUtilitySpeedTest() + : global_store_(symbol_table_), cluster_scope_(symbol_table_), code_stats_(symbol_table_), + pool_(symbol_table_), prefix_(pool_.add("prefix")) {} void addResponse(uint64_t code, bool canary, bool internal_request, - const std::string& request_vhost_name = EMPTY_STRING, - const std::string& request_vcluster_name = EMPTY_STRING, - const std::string& from_az = EMPTY_STRING, - const std::string& to_az = EMPTY_STRING) { + Stats::StatName request_vhost_name = Stats::StatName(), + Stats::StatName request_vcluster_name = Stats::StatName(), + Stats::StatName from_az = Stats::StatName(), + Stats::StatName to_az = Stats::StatName()) { Http::CodeStats::ResponseStatInfo info{ - global_store_, cluster_scope_, "prefix.", code, internal_request, - request_vhost_name, request_vcluster_name, from_az, to_az, canary}; + global_store_, cluster_scope_, prefix_, code, internal_request, + request_vhost_name, request_vcluster_name, from_az, to_az, canary}; code_stats_.chargeResponseStat(info); } @@ -41,23 +42,33 @@ class CodeUtilitySpeedTest { addResponse(501, false, true); addResponse(200, true, true); addResponse(300, false, false); + Stats::StatName empty_stat_name; addResponse(500, true, false); - addResponse(200, false, false, "test-vhost", "test-cluster"); - addResponse(200, false, false, "", "", "from_az", "to_az"); + addResponse(200, false, false, pool_.add("test-vhost"), pool_.add("test-cluster")); + addResponse(200, false, false, empty_stat_name, empty_stat_name, pool_.add("from_az"), + pool_.add("to_az")); } void responseTiming() { - Http::CodeStats::ResponseTimingInfo info{ - global_store_, cluster_scope_, "prefix.", std::chrono::milliseconds(5), - true, true, "vhost_name", "req_vcluster_name", - "from_az", "to_az"}; + Http::CodeStats::ResponseTimingInfo info{global_store_, + cluster_scope_, + prefix_, + std::chrono::milliseconds(5), + true, + true, + pool_.add("vhost_name"), + pool_.add("req_vcluster_name"), + pool_.add("from_az"), + pool_.add("to_az")}; code_stats_.chargeResponseTiming(info); } - Stats::FakeSymbolTableImpl symbol_table_; + Stats::SymbolTableImpl symbol_table_; Stats::IsolatedStoreImpl global_store_; Stats::IsolatedStoreImpl cluster_scope_; Http::CodeStatsImpl code_stats_; + Stats::StatNamePool pool_; + Stats::StatName prefix_; }; } // namespace Http diff --git a/test/common/http/codes_test.cc b/test/common/http/codes_test.cc index b209d6ce74e3..1bdd1f3202e4 100644 --- a/test/common/http/codes_test.cc +++ b/test/common/http/codes_test.cc @@ -24,21 +24,32 @@ namespace Http { class CodeUtilityTest : public testing::Test { public: + CodeUtilityTest() + : global_store_(*symbol_table_), cluster_scope_(*symbol_table_), code_stats_(*symbol_table_), + pool_(*symbol_table_) {} + void addResponse(uint64_t code, bool canary, bool internal_request, const std::string& request_vhost_name = EMPTY_STRING, const std::string& request_vcluster_name = EMPTY_STRING, const std::string& from_az = EMPTY_STRING, const std::string& to_az = EMPTY_STRING) { + Stats::StatName prefix = pool_.add("prefix"); + Stats::StatName from_zone = pool_.add(from_az); + Stats::StatName to_zone = pool_.add(to_az); + Stats::StatName vhost_name = pool_.add(request_vhost_name); + Stats::StatName vcluster_name = pool_.add(request_vcluster_name); Http::CodeStats::ResponseStatInfo info{ - global_store_, cluster_scope_, "prefix.", code, internal_request, - request_vhost_name, request_vcluster_name, from_az, to_az, canary}; + global_store_, cluster_scope_, prefix, code, internal_request, + vhost_name, vcluster_name, from_zone, to_zone, canary}; code_stats_.chargeResponseStat(info); } + Envoy::Test::Global symbol_table_; Stats::IsolatedStoreImpl global_store_; Stats::IsolatedStoreImpl cluster_scope_; Http::CodeStatsImpl code_stats_; + Stats::StatNamePool pool_; }; TEST_F(CodeUtilityTest, GroupStrings) { @@ -80,10 +91,18 @@ TEST_F(CodeUtilityTest, NoCanary) { } TEST_F(CodeUtilityTest, Canary) { + addResponse(100, true, true); addResponse(200, true, true); addResponse(300, false, false); addResponse(500, true, false); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.upstream_rq_1xx").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.upstream_rq_100").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.internal.upstream_rq_1xx").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.internal.upstream_rq_100").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.canary.upstream_rq_1xx").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.canary.upstream_rq_100").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.upstream_rq_2xx").value()); EXPECT_EQ(1U, cluster_scope_.counter("prefix.upstream_rq_200").value()); EXPECT_EQ(1U, cluster_scope_.counter("prefix.internal.upstream_rq_2xx").value()); @@ -101,12 +120,25 @@ TEST_F(CodeUtilityTest, Canary) { EXPECT_EQ(1U, cluster_scope_.counter("prefix.canary.upstream_rq_5xx").value()); EXPECT_EQ(1U, cluster_scope_.counter("prefix.canary.upstream_rq_500").value()); - EXPECT_EQ(3U, cluster_scope_.counter("prefix.upstream_rq_completed").value()); + EXPECT_EQ(4U, cluster_scope_.counter("prefix.upstream_rq_completed").value()); EXPECT_EQ(2U, cluster_scope_.counter("prefix.external.upstream_rq_completed").value()); - EXPECT_EQ(1U, cluster_scope_.counter("prefix.internal.upstream_rq_completed").value()); - EXPECT_EQ(2U, cluster_scope_.counter("prefix.canary.upstream_rq_completed").value()); + EXPECT_EQ(2U, cluster_scope_.counter("prefix.internal.upstream_rq_completed").value()); + EXPECT_EQ(3U, cluster_scope_.counter("prefix.canary.upstream_rq_completed").value()); + + EXPECT_EQ(26U, cluster_scope_.counters().size()); +} - EXPECT_EQ(20U, cluster_scope_.counters().size()); +TEST_F(CodeUtilityTest, UnknownResponseCodes) { + addResponse(23, true, true); + addResponse(600, false, false); + addResponse(1000000, false, true); + + EXPECT_EQ(3U, cluster_scope_.counter("prefix.upstream_rq_unknown").value()); + EXPECT_EQ(2U, cluster_scope_.counter("prefix.internal.upstream_rq_unknown").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.canary.upstream_rq_unknown").value()); + EXPECT_EQ(1U, cluster_scope_.counter("prefix.external.upstream_rq_unknown").value()); + + EXPECT_EQ(8U, cluster_scope_.counters().size()); } TEST_F(CodeUtilityTest, All) { @@ -197,14 +229,21 @@ TEST_F(CodeUtilityTest, PerZoneStats) { EXPECT_EQ(1U, cluster_scope_.counter("prefix.zone.from_az.to_az.upstream_rq_2xx").value()); } -TEST(CodeUtilityResponseTimingTest, All) { +TEST_F(CodeUtilityTest, ResponseTimingTest) { Stats::MockStore global_store; Stats::MockStore cluster_scope; - Http::CodeStats::ResponseTimingInfo info{ - global_store, cluster_scope, "prefix.", std::chrono::milliseconds(5), - true, true, "vhost_name", "req_vcluster_name", - "from_az", "to_az"}; + Stats::StatNameManagedStorage prefix("prefix", *symbol_table_); + Http::CodeStats::ResponseTimingInfo info{global_store, + cluster_scope, + pool_.add("prefix"), + std::chrono::milliseconds(5), + true, + true, + pool_.add("vhost_name"), + pool_.add("req_vcluster_name"), + pool_.add("from_az"), + pool_.add("to_az")}; EXPECT_CALL(cluster_scope, histogram("prefix.upstream_rq_time")); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( @@ -231,18 +270,19 @@ TEST(CodeUtilityResponseTimingTest, All) { EXPECT_CALL(cluster_scope, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.zone.from_az.to_az.upstream_rq_time"), 5)); - Http::CodeStatsImpl code_stats; + Http::CodeStatsImpl code_stats(*symbol_table_); code_stats.chargeResponseTiming(info); } class CodeStatsTest : public testing::Test { protected: + CodeStatsTest() : code_stats_(symbol_table_) {} + absl::string_view stripTrailingDot(absl::string_view prefix) { return CodeStatsImpl::stripTrailingDot(prefix); } - std::string join(const std::vector& v) { return CodeStatsImpl::join(v); } - + Stats::FakeSymbolTableImpl symbol_table_; CodeStatsImpl code_stats_; }; @@ -253,13 +293,5 @@ TEST_F(CodeStatsTest, StripTrailingDot) { EXPECT_EQ("foo.", stripTrailingDot("foo..")); // only one dot gets stripped. } -TEST_F(CodeStatsTest, Join) { - EXPECT_EQ("hello.world", join({"hello", "world"})); - EXPECT_EQ("hello.world", join({"", "hello", "world"})); // leading empty token ignored. - EXPECT_EQ("hello.", join({"hello", ""})); // trailing empty token not ignored. - EXPECT_EQ("hello", join({"hello"})); - EXPECT_EQ("", join({""})); -} - } // namespace Http } // namespace Envoy diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 9711f6929d13..056b1096b467 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -388,7 +388,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { NiceMock drain_close; NiceMock random; Stats::FakeSymbolTableImpl symbol_table; - Http::ContextImpl http_context; + Http::ContextImpl http_context(symbol_table); NiceMock runtime; NiceMock local_info; NiceMock cluster_manager; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index d8c8d3121aaa..eba68e39a433 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -77,7 +77,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan }; HttpConnectionManagerImplTest() - : route_config_provider_(test_time_.timeSystem()), access_log_path_("dummy_path"), + : route_config_provider_(test_time_.timeSystem()), http_context_(fake_stats_.symbolTable()), + access_log_path_("dummy_path"), access_logs_{ AccessLog::InstanceSharedPtr{new Extensions::AccessLoggers::File::FileAccessLog( access_log_path_, {}, AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(), diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index b774faa39c40..cfc5b55adb9f 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -99,6 +99,17 @@ class ConfigImplTestBase { ON_CALL(factory_context_, api()).WillByDefault(ReturnRef(*api_)); } + std::string virtualHostName(const RouteEntry* route) { + Stats::StatName name = route->virtualHost().statName(); + return factory_context_.scope().symbolTable().toString(name); + } + + std::string virtualClusterName(const RouteEntry* route, Http::TestHeaderMapImpl& headers) { + Stats::StatName name = route->virtualCluster(headers)->statName(); + return factory_context_.scope().symbolTable().toString(name); + } + + Test::Global symbol_table_; Api::ApiPtr api_; NiceMock factory_context_; }; @@ -366,7 +377,7 @@ TEST_F(RouteMatcherTest, TestRoutes) { Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/new_endpoint/foo", "GET"); const RouteEntry* route = config.route(headers, 0)->routeEntry(); EXPECT_EQ("www2", route->clusterName()); - EXPECT_EQ("www2", route->virtualHost().name()); + EXPECT_EQ("www2", virtualHostName(route)); route->finalizeRequestHeaders(headers, stream_info, true); EXPECT_EQ("/api/new_endpoint/foo", headers.get_(Http::Headers::get().Path)); EXPECT_EQ("/new_endpoint/foo", headers.get_(Http::Headers::get().EnvoyOriginalPath)); @@ -377,7 +388,7 @@ TEST_F(RouteMatcherTest, TestRoutes) { Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/new_endpoint/foo", "GET"); const RouteEntry* route = config.route(headers, 0)->routeEntry(); EXPECT_EQ("www2", route->clusterName()); - EXPECT_EQ("www2", route->virtualHost().name()); + EXPECT_EQ("www2", virtualHostName(route)); route->finalizeRequestHeaders(headers, stream_info, false); EXPECT_EQ("/api/new_endpoint/foo", headers.get_(Http::Headers::get().Path)); EXPECT_FALSE(headers.has(Http::Headers::get().EnvoyOriginalPath)); @@ -487,62 +498,59 @@ TEST_F(RouteMatcherTest, TestRoutes) { // Virtual cluster testing. { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/rides", "GET"); - EXPECT_EQ("other", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("other", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/rides/blah", "POST"); - EXPECT_EQ("other", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("other", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/rides", "POST"); - EXPECT_EQ("ride_request", - config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("ride_request", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/rides/123", "PUT"); - EXPECT_EQ("update_ride", - config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("update_ride", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/rides/123/456", "POST"); - EXPECT_EQ("other", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("other", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/users/123/chargeaccounts", "POST"); - EXPECT_EQ("cc_add", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("cc_add", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/users/123/chargeaccounts/hello123", "PUT"); - EXPECT_EQ("cc_add", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("cc_add", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/users/123/chargeaccounts/validate", "PUT"); - EXPECT_EQ("other", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("other", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/foo/bar", "PUT"); - EXPECT_EQ("other", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("other", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/users", "POST"); EXPECT_EQ("create_user_login", - config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/users/123", "PUT"); - EXPECT_EQ("update_user", - config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("update_user", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/users/123/location", "POST"); - EXPECT_EQ("ulu", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("ulu", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } { Http::TestHeaderMapImpl headers = genHeaders("api.lyft.com", "/something/else", "GET"); - EXPECT_EQ("other", config.route(headers, 0)->routeEntry()->virtualCluster(headers)->name()); + EXPECT_EQ("other", virtualClusterName(config.route(headers, 0)->routeEntry(), headers)); } } @@ -4767,7 +4775,7 @@ name: foo EXPECT_NE(nullptr, typed_metadata.get(baz_factory.name())); EXPECT_EQ("bluh", typed_metadata.get(baz_factory.name())->name); - EXPECT_EQ("bar", route_entry->virtualHost().name()); + EXPECT_EQ("bar", symbol_table_->toString(route_entry->virtualHost().statName())); EXPECT_EQ("foo", route_entry->virtualHost().routeConfig().name()); } diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index 525eab03727b..c9c3c6c9c7b6 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -92,8 +92,8 @@ class RateLimitConfiguration : public testing::Test { config_ = std::make_unique(route_config, factory_context_, true); } - std::unique_ptr config_; NiceMock factory_context_; + std::unique_ptr config_; Http::TestHeaderMapImpl header_; const RouteEntry* route_; Network::Address::Ipv4Instance default_remote_address_{"10.0.0.1"}; diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 8b0c171a9e70..496a1e7f48a5 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -79,7 +79,7 @@ class TestFilter : public Filter { class RouterTestBase : public testing::Test { public: RouterTestBase(bool start_child_span, bool suppress_envoy_headers) - : shadow_writer_(new MockShadowWriter()), + : http_context_(stats_store_.symbolTable()), shadow_writer_(new MockShadowWriter()), config_("test.", local_info_, stats_store_, cm_, runtime_, random_, ShadowWriterPtr{shadow_writer_}, true, start_child_span, suppress_envoy_headers, test_time_.timeSystem(), http_context_), diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index fc2c2c772017..1ee2c4cf7887 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -196,7 +196,8 @@ const ClusterManager::ClusterWarmingCallback dummyWarmingCb = [](auto, auto) {}; class ClusterManagerImplTest : public testing::Test { public: - ClusterManagerImplTest() : api_(Api::createApiForTest()) {} + ClusterManagerImplTest() + : api_(Api::createApiForTest()), http_context_(factory_.stats_.symbolTable()) {} void create(const envoy::config::bootstrap::v2::Bootstrap& bootstrap) { cluster_manager_ = std::make_unique( diff --git a/test/common/upstream/hds_test.cc b/test/common/upstream/hds_test.cc index 85d40a3f430c..889723044422 100644 --- a/test/common/upstream/hds_test.cc +++ b/test/common/upstream/hds_test.cc @@ -167,7 +167,7 @@ TEST_F(HdsTest, TestProcessMessageEndpoints) { } // Process message - EXPECT_CALL(test_factory_, createClusterInfo(_)).Times(2); + EXPECT_CALL(test_factory_, createClusterInfo(_)).Times(2).WillRepeatedly(Return(cluster_info_)); hds_delegate_friend_.processPrivateMessage(*hds_delegate_, std::move(message)); // Check Correctness diff --git a/test/common/upstream/host_utility_test.cc b/test/common/upstream/host_utility_test.cc index 9fc5d3d4511c..d59419c3ba20 100644 --- a/test/common/upstream/host_utility_test.cc +++ b/test/common/upstream/host_utility_test.cc @@ -12,7 +12,7 @@ namespace Upstream { namespace { TEST(HostUtilityTest, All) { - ClusterInfoConstSharedPtr cluster{new MockClusterInfo()}; + auto cluster = std::make_shared>(); HostSharedPtr host = makeTestHost(cluster, "tcp://127.0.0.1:80"); EXPECT_EQ("healthy", HostUtility::healthFlagsToString(*host)); diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index e011340c5a16..1094996cddc2 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -48,6 +48,8 @@ namespace { template class HttpFilterTestBase : public T { public: + HttpFilterTestBase() : http_context_(stats_store_.symbolTable()) {} + void initialize(std::string&& yaml) { envoy::config::filter::http::ext_authz::v2::ExtAuthz proto_config{}; if (!yaml.empty()) { @@ -68,7 +70,7 @@ template class HttpFilterTestBase : public T { Filters::Common::ExtAuthz::RequestCallbacks* request_callbacks_; Http::TestHeaderMapImpl request_headers_; Buffer::OwnedImpl data_; - Stats::IsolatedStoreImpl stats_store_; + NiceMock stats_store_; NiceMock runtime_; NiceMock cm_; NiceMock local_info_; @@ -83,7 +85,10 @@ template class HttpFilterTestBase : public T { } }; -class HttpFilterTest : public HttpFilterTestBase {}; +class HttpFilterTest : public HttpFilterTestBase { +public: + HttpFilterTest() = default; +}; using CreateFilterConfigFunc = envoy::config::filter::http::ext_authz::v2::ExtAuthz(); diff --git a/test/extensions/filters/http/ratelimit/ratelimit_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_test.cc index a5d373f3bdd2..5a44fa7560cf 100644 --- a/test/extensions/filters/http/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/http/ratelimit/ratelimit_test.cc @@ -39,7 +39,7 @@ namespace { class HttpRateLimitFilterTest : public testing::Test { public: - HttpRateLimitFilterTest() { + HttpRateLimitFilterTest() : http_context_(stats_store_.symbolTable()) { ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enabled", 100)) .WillByDefault(Return(true)); ON_CALL(runtime_.snapshot_, featureEnabled("ratelimit.http_filter_enforcing", 100)) diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index 197cd910b001..c002c19836e3 100644 --- a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc @@ -2,6 +2,7 @@ #include #include "common/network/utility.h" +#include "common/stats/fake_symbol_table_impl.h" #include "common/upstream/upstream_impl.h" #include "extensions/filters/network/redis_proxy/conn_pool_impl.h" @@ -12,6 +13,7 @@ #include "test/mocks/network/mocks.h" #include "test/mocks/thread_local/mocks.h" #include "test/mocks/upstream/mocks.h" +#include "test/test_common/global.h" #include "test/test_common/printers.h" #include "gmock/gmock.h" @@ -46,7 +48,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client conn_pool_ = std::make_unique( cluster_name_, cm_, *this, tls_, - Common::Redis::Client::createConnPoolSettings(20, hashtagging, true)); + Common::Redis::Client::createConnPoolSettings(20, hashtagging, true), *symbol_table_); test_address_ = Network::Utility::resolveUrl("tcp://127.0.0.1:3000"); } @@ -77,6 +79,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client MOCK_METHOD1(create_, Common::Redis::Client::Client*(Upstream::HostConstSharedPtr host)); const std::string cluster_name_{"fake_cluster"}; + Envoy::Test::Global symbol_table_; NiceMock cm_; NiceMock tls_; InstanceSharedPtr conn_pool_; diff --git a/test/extensions/stats_sinks/common/statsd/statsd_test.cc b/test/extensions/stats_sinks/common/statsd/statsd_test.cc index 95ee5bf6ecf7..b23cac0f9ebe 100644 --- a/test/extensions/stats_sinks/common/statsd/statsd_test.cc +++ b/test/extensions/stats_sinks/common/statsd/statsd_test.cc @@ -43,7 +43,7 @@ class TcpStatsdSinkTest : public testing::Test { Upstream::MockHost::MockCreateConnectionData conn_info; conn_info.connection_ = connection_; conn_info.host_description_ = Upstream::makeTestHost( - Upstream::ClusterInfoConstSharedPtr{new Upstream::MockClusterInfo}, "tcp://127.0.0.1:80"); + std::make_unique>(), "tcp://127.0.0.1:80"); EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)) .WillOnce(Return(conn_info)); diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 6fdd74e70856..3442a8264869 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -211,8 +211,9 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithStats) { // 2019/04/23 6659 59512 Reintroduce dispatcher stats... // 2019/04/24 6161 49415 Pack tags and tag extracted names // 2019/05/07 6794 49957 Stats for excluded hosts in cluster + // 2019/04/27 6733 50213 Use SymbolTable API for HTTP codes - EXPECT_EQ(m_per_cluster, 49957); + EXPECT_EQ(m_per_cluster, 50213); } } // namespace diff --git a/test/mocks/router/BUILD b/test/mocks/router/BUILD index 76e2c77be171..e5a05ad7d075 100644 --- a/test/mocks/router/BUILD +++ b/test/mocks/router/BUILD @@ -24,6 +24,7 @@ envoy_cc_mock( "//include/envoy/stats:stats_interface", "//include/envoy/thread_local:thread_local_interface", "//include/envoy/upstream:cluster_manager_interface", + "//source/common/stats:fake_symbol_table_lib", "//test/mocks:common_lib", ], ) diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index c1bd440ef0de..00ca69b219b9 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -21,6 +21,10 @@ #include "envoy/thread_local/thread_local.h" #include "envoy/upstream/cluster_manager.h" +#include "common/stats/fake_symbol_table_impl.h" + +#include "test/test_common/global.h" + #include "gmock/gmock.h" namespace Envoy { @@ -188,9 +192,10 @@ class MockShadowWriter : public ShadowWriter { class TestVirtualCluster : public VirtualCluster { public: // Router::VirtualCluster - const std::string& name() const override { return name_; } + Stats::StatName statName() const override { return stat_name_.statName(); } - std::string name_{"fake_virtual_cluster"}; + Test::Global symbol_table_; + Stats::StatNameManagedStorage stat_name_{"fake_virtual_cluster", *symbol_table_}; }; class MockVirtualHost : public VirtualHost { @@ -208,7 +213,14 @@ class MockVirtualHost : public VirtualHost { MOCK_METHOD0(retryPriority, Upstream::RetryPrioritySharedPtr()); MOCK_METHOD0(retryHostPredicate, Upstream::RetryHostPredicateSharedPtr()); + Stats::StatName statName() const override { + stat_name_ = std::make_unique(name(), *symbol_table_); + return stat_name_->statName(); + } + + mutable Test::Global symbol_table_; std::string name_{"fake_vhost"}; + mutable std::unique_ptr stat_name_; testing::NiceMock rate_limit_policy_; TestCorsPolicy cors_policy_; }; diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 3f8397a68b50..53bdbf777882 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -126,7 +126,8 @@ MockWorker::~MockWorker() = default; MockInstance::MockInstance() : secret_manager_(new Secret::SecretManagerImpl()), cluster_manager_(timeSource()), ssl_context_manager_(timeSource()), singleton_manager_(new Singleton::ManagerImpl( - Thread::threadFactoryForTest().currentThreadId())) { + Thread::threadFactoryForTest().currentThreadId())), + http_context_(stats_store_.symbolTable()) { ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_)); ON_CALL(*this, stats()).WillByDefault(ReturnRef(stats_store_)); ON_CALL(*this, httpContext()).WillByDefault(ReturnRef(http_context_)); @@ -167,7 +168,8 @@ MockMain::~MockMain() = default; MockFactoryContext::MockFactoryContext() : singleton_manager_( - new Singleton::ManagerImpl(Thread::threadFactoryForTest().currentThreadId())) { + new Singleton::ManagerImpl(Thread::threadFactoryForTest().currentThreadId())), + http_context_(scope_.symbolTable()) { ON_CALL(*this, accessLogManager()).WillByDefault(ReturnRef(access_log_manager_)); ON_CALL(*this, clusterManager()).WillByDefault(ReturnRef(cluster_manager_)); ON_CALL(*this, dispatcher()).WillByDefault(ReturnRef(dispatcher_)); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 93ccbc954df5..435c96f2034e 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -36,6 +36,7 @@ #include "test/mocks/router/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/secret/mocks.h" +#include "test/mocks/stats/mocks.h" #include "test/mocks/thread_local/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" @@ -371,7 +372,7 @@ class MockInstance : public Instance { std::unique_ptr secret_manager_; testing::NiceMock thread_local_; - Stats::IsolatedStoreImpl stats_store_; + NiceMock stats_store_; std::shared_ptr> dns_resolver_{ new testing::NiceMock()}; testing::NiceMock api_; diff --git a/test/mocks/upstream/host.h b/test/mocks/upstream/host.h index e2f6a0614021..86b21e1a48b2 100644 --- a/test/mocks/upstream/host.h +++ b/test/mocks/upstream/host.h @@ -8,7 +8,10 @@ #include "envoy/api/v2/cluster/outlier_detection.pb.h" #include "envoy/upstream/upstream.h" +#include "common/stats/fake_symbol_table_impl.h" + #include "test/mocks/upstream/cluster_info.h" +#include "test/test_common/global.h" #include "gmock/gmock.h" @@ -90,6 +93,12 @@ class MockHostDescription : public HostDescription { MOCK_CONST_METHOD0(locality, const envoy::api::v2::core::Locality&()); MOCK_CONST_METHOD0(priority, uint32_t()); MOCK_METHOD1(priority, void(uint32_t)); + Stats::StatName localityZoneStatName() const override { + Stats::SymbolTable& symbol_table = *symbol_table_; + locality_zone_stat_name_ = + std::make_unique(locality().zone(), symbol_table); + return locality_zone_stat_name_->statName(); + } std::string hostname_; Network::Address::InstanceConstSharedPtr address_; @@ -98,6 +107,8 @@ class MockHostDescription : public HostDescription { testing::NiceMock cluster_; testing::NiceMock stats_store_; HostStats stats_{ALL_HOST_STATS(POOL_COUNTER(stats_store_), POOL_GAUGE(stats_store_))}; + mutable Test::Global symbol_table_; + mutable std::unique_ptr locality_zone_stat_name_; }; class MockHost : public Host { @@ -130,6 +141,12 @@ class MockHost : public Host { setOutlierDetector_(outlier_detector); } + Stats::StatName localityZoneStatName() const override { + locality_zone_stat_name_ = + std::make_unique(locality().zone(), *symbol_table_); + return locality_zone_stat_name_->statName(); + } + MOCK_CONST_METHOD0(address, Network::Address::InstanceConstSharedPtr()); MOCK_CONST_METHOD0(healthCheckAddress, Network::Address::InstanceConstSharedPtr()); MOCK_METHOD1(setHealthCheckAddress, void(Network::Address::InstanceConstSharedPtr)); @@ -169,6 +186,8 @@ class MockHost : public Host { testing::NiceMock outlier_detector_; NiceMock stats_store_; HostStats stats_{ALL_HOST_STATS(POOL_COUNTER(stats_store_), POOL_GAUGE(stats_store_))}; + mutable Test::Global symbol_table_; + mutable std::unique_ptr locality_zone_stat_name_; }; } // namespace Upstream diff --git a/test/server/config_validation/cluster_manager_test.cc b/test/server/config_validation/cluster_manager_test.cc index b64e43b0b83d..a247dcfa656e 100644 --- a/test/server/config_validation/cluster_manager_test.cc +++ b/test/server/config_validation/cluster_manager_test.cc @@ -39,7 +39,7 @@ TEST(ValidationClusterManagerTest, MockedMethods) { NiceMock dispatcher; LocalInfo::MockLocalInfo local_info; NiceMock admin; - Http::ContextImpl http_context; + Http::ContextImpl http_context(stats_store.symbolTable()); AccessLog::MockAccessLogManager log_manager; Singleton::ManagerImpl singleton_manager{Thread::threadFactoryForTest().currentThreadId()}; diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc index 043fcbe55480..eb7d21c5660b 100644 --- a/test/tools/router_check/router.cc +++ b/test/tools/router_check/router.cc @@ -144,7 +144,9 @@ bool RouterCheckTool::compareVirtualCluster(ToolConfig& tool_config, const std:: if (tool_config.route_->routeEntry() != nullptr && tool_config.route_->routeEntry()->virtualCluster(*tool_config.headers_) != nullptr) { - actual = tool_config.route_->routeEntry()->virtualCluster(*tool_config.headers_)->name(); + Stats::StatName stat_name = + tool_config.route_->routeEntry()->virtualCluster(*tool_config.headers_)->statName(); + actual = tool_config.symbolTable().toString(stat_name); } return compareResults(actual, expected, "virtual_cluster_name"); } @@ -152,7 +154,8 @@ bool RouterCheckTool::compareVirtualCluster(ToolConfig& tool_config, const std:: bool RouterCheckTool::compareVirtualHost(ToolConfig& tool_config, const std::string& expected) { std::string actual = ""; if (tool_config.route_->routeEntry() != nullptr) { - actual = tool_config.route_->routeEntry()->virtualHost().name(); + Stats::StatName stat_name = tool_config.route_->routeEntry()->virtualHost().statName(); + actual = tool_config.symbolTable().toString(stat_name); } return compareResults(actual, expected, "virtual_host_name"); } diff --git a/test/tools/router_check/router.h b/test/tools/router_check/router.h index 64283e11cd57..effd9347a016 100644 --- a/test/tools/router_check/router.h +++ b/test/tools/router_check/router.h @@ -9,8 +9,10 @@ #include "common/http/headers.h" #include "common/json/json_loader.h" #include "common/router/config_impl.h" +#include "common/stats/fake_symbol_table_impl.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/global.h" #include "test/test_common/printers.h" #include "test/test_common/utility.h" #include "test/tools/router_check/json/tool_config_schemas.h" @@ -30,12 +32,15 @@ struct ToolConfig { */ static ToolConfig create(const Json::ObjectSharedPtr check_config); + Stats::SymbolTable& symbolTable() { return *symbol_table_; } + std::unique_ptr headers_; Router::RouteConstSharedPtr route_; int random_value_; private: ToolConfig(std::unique_ptr headers, int random_value); + Test::Global symbol_table_; }; /**