diff --git a/docs/root/configuration/cluster_manager/cds.rst b/docs/root/configuration/cluster_manager/cds.rst index ebfe5008ac92a..89f2dbcd4b186 100644 --- a/docs/root/configuration/cluster_manager/cds.rst +++ b/docs/root/configuration/cluster_manager/cds.rst @@ -29,4 +29,4 @@ CDS has a statistics tree rooted at *cluster_manager.cds.* with the following st update_failure, Counter, Total API fetches that failed because of network errors update_rejected, Counter, Total API fetches that failed because of schema/validation errors version, Gauge, Hash of the contents from the last successful API fetch - control_plane.connected_state, BoolIndicator, Current connection state with management server + control_plane.connected_state, Gauge, A boolean (1 for connected and 0 for disconnected) that indicates the current connection state with management server diff --git a/docs/root/configuration/cluster_manager/cluster_stats.rst b/docs/root/configuration/cluster_manager/cluster_stats.rst index 38d0759aa2ca1..5135080e356e2 100644 --- a/docs/root/configuration/cluster_manager/cluster_stats.rst +++ b/docs/root/configuration/cluster_manager/cluster_stats.rst @@ -148,10 +148,10 @@ Circuit breakers statistics will be rooted at *cluster..circuit_breakers.< :header: Name, Type, Description :widths: 1, 1, 2 - cx_open, BoolIndicator, Whether the connection circuit breaker is closed (false) or open (true) - rq_pending_open, BoolIndicator, Whether the pending requests circuit breaker is closed (false) or open (true) - rq_open, BoolIndicator, Whether the requests circuit breaker is closed (false) or open (true) - rq_retry_open, BoolIndicator, Whether the retry circuit breaker is closed (false) or open (true) + cx_open, Gauge, Whether the connection circuit breaker is closed (0) or open (1) + rq_pending_open, Gauge, Whether the pending requests circuit breaker is closed (0) or open (1) + rq_open, Gauge, Whether the requests circuit breaker is closed (0) or open (1) + rq_retry_open, Gauge, Whether the retry circuit breaker is closed (0) or open (1) .. _config_cluster_manager_cluster_stats_dynamic_http: diff --git a/docs/root/configuration/listeners/lds.rst b/docs/root/configuration/listeners/lds.rst index a876087bb305a..a90fe84f6f11e 100644 --- a/docs/root/configuration/listeners/lds.rst +++ b/docs/root/configuration/listeners/lds.rst @@ -48,4 +48,4 @@ LDS has a statistics tree rooted at *listener_manager.lds.* with the following s update_failure, Counter, Total API fetches that failed because of network errors update_rejected, Counter, Total API fetches that failed because of schema/validation errors version, Gauge, Hash of the contents from the last successful API fetch - control_plane.connected_state, BoolIndicator, Current connection state with management server + control_plane.connected_state, Gauge, A boolean (1 for connected and 0 for disconnected) that indicates the current connection state with management server diff --git a/docs/root/configuration/overview/v2_overview.rst b/docs/root/configuration/overview/v2_overview.rst index c7c8426469e2d..be242f5bd3845 100644 --- a/docs/root/configuration/overview/v2_overview.rst +++ b/docs/root/configuration/overview/v2_overview.rst @@ -590,7 +590,7 @@ Management Server has a statistics tree rooted at *control_plane.* with the foll :header: Name, Type, Description :widths: 1, 1, 2 - connected_state, BoolIndicator, Current connection state with management server + connected_state, Gauge, A boolean (1 for connected and 0 for disconnected) that indicates the current connection state with management server rate_limit_enforced, Counter, Total number of times rate limit was enforced for management server requests pending_requests, Gauge, Total number of pending requests when the rate limit was enforced diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index c5a4066be763d..65f0c2fc0b0c2 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -53,7 +53,6 @@ Version history * stats: added support for histograms in prometheus * stats: added usedonly flag to prometheus stats to only output metrics which have been updated at least once. -* stats: added BoolIndicator stat type, converted the following 1-or-0 Gauges: control_plane.connected_state, cx_open, rq_pending_open, rq_open, rq_retry_open, runtime.admin_overrides_active, open_gauge, config.active, server.live. * tap: added new alpha :ref:`HTTP tap filter `. * tls: enabled TLS 1.3 on the server-side (non-FIPS builds). * upstream: add hash_function to specify the hash function for :ref:`ring hash` as either xxHash or `murmurHash2 `_. MurmurHash2 is compatible with std::hash in GNU libstdc++ 3.4.20 or above. This is typically the case when compiled on Linux and not macOS. diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index af568566e7edb..6872a62d875b6 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -13,9 +13,9 @@ namespace Config { * All control plane related stats. @see stats_macros.h */ // clang-format off -#define ALL_CONTROL_PLANE_STATS(BOOL_INDICATOR, COUNTER, GAUGE) \ +#define ALL_CONTROL_PLANE_STATS(COUNTER, GAUGE) \ COUNTER(rate_limit_enforced) \ - BOOL_INDICATOR(connected_state) \ + GAUGE(connected_state) \ GAUGE(pending_requests) \ // clang-format on @@ -23,7 +23,7 @@ namespace Config { * Struct definition for all control plane stats. @see stats_macros.h */ struct ControlPlaneStats { - ALL_CONTROL_PLANE_STATS(GENERATE_BOOL_INDICATOR_STRUCT,GENERATE_COUNTER_STRUCT,GENERATE_GAUGE_STRUCT) + ALL_CONTROL_PLANE_STATS(GENERATE_COUNTER_STRUCT,GENERATE_GAUGE_STRUCT) }; class GrpcMuxCallbacks { diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index ea0378bb752a9..ef913edea6e02 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -51,11 +51,6 @@ class Scope { */ virtual Gauge& gauge(const std::string& name) PURE; - /** - * @return a bool within the scope's namespace. - */ - virtual BoolIndicator& boolIndicator(const std::string& name) PURE; - /** * @return a histogram within the scope's namespace with a particular value type. */ diff --git a/include/envoy/stats/source.h b/include/envoy/stats/source.h index 149b8e7fc3c70..8dea65487a587 100644 --- a/include/envoy/stats/source.h +++ b/include/envoy/stats/source.h @@ -37,14 +37,6 @@ class Source { */ virtual const std::vector& cachedGauges() PURE; - /** - * Returns all known bools. Will use cached values if already accessed and clearCache() hasn't - * been called since. - * @return std::vector& all known bools. Note: reference may not be - * valid after clearCache() is called. - */ - virtual const std::vector& cachedBoolIndicators() PURE; - /** * Returns all known parent histograms. Will use cached values if already accessed and * clearCache() hasn't been called since. diff --git a/include/envoy/stats/stat_data_allocator.h b/include/envoy/stats/stat_data_allocator.h index 5696408e2f062..f0ea93e266d04 100644 --- a/include/envoy/stats/stat_data_allocator.h +++ b/include/envoy/stats/stat_data_allocator.h @@ -47,17 +47,6 @@ class StatDataAllocator { virtual GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) PURE; - /** - * @param name the full name of the stat. - * @param tag_extracted_name the name of the stat with tag-values stripped out. - * @param tags the extracted tag values. - * @return BoolIndicatorSharedPtr a bool, or nullptr if allocation failed, in which case - * tag_extracted_name and tags are not moved. - */ - virtual BoolIndicatorSharedPtr makeBoolIndicator(absl::string_view name, - std::string&& tag_extracted_name, - std::vector&& tags) PURE; - /** * Determines whether this stats allocator requires bounded stat-name size. */ diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 09e5842b4d041..6d191bf24f9fb 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -94,18 +94,5 @@ class Gauge : public virtual Metric { typedef std::shared_ptr GaugeSharedPtr; -/** - * A Boolean. - */ -class BoolIndicator : public virtual Metric { -public: - virtual ~BoolIndicator() {} - - virtual void set(bool value) PURE; - virtual bool value() const PURE; -}; - -typedef std::shared_ptr BoolIndicatorSharedPtr; - } // namespace Stats } // namespace Envoy diff --git a/include/envoy/stats/stats_macros.h b/include/envoy/stats/stats_macros.h index 99cab9aeac3ba..eb1c89557c664 100644 --- a/include/envoy/stats/stats_macros.h +++ b/include/envoy/stats/stats_macros.h @@ -29,18 +29,15 @@ namespace Envoy { #define GENERATE_COUNTER_STRUCT(NAME) Stats::Counter& NAME##_; #define GENERATE_GAUGE_STRUCT(NAME) Stats::Gauge& NAME##_; -#define GENERATE_BOOL_INDICATOR_STRUCT(NAME) Stats::BoolIndicator& NAME##_; #define GENERATE_HISTOGRAM_STRUCT(NAME) Stats::Histogram& NAME##_; #define FINISH_STAT_DECL_(X) + std::string(#X)), #define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(PREFIX FINISH_STAT_DECL_ #define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(PREFIX FINISH_STAT_DECL_ -#define POOL_BOOL_INDICATOR_PREFIX(POOL, PREFIX) (POOL).boolIndicator(PREFIX FINISH_STAT_DECL_ #define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(PREFIX FINISH_STAT_DECL_ #define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "") #define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "") -#define POOL_BOOL_INDICATOR(POOL) POOL_BOOL_INDICATOR_PREFIX(POOL, "") #define POOL_HISTOGRAM(POOL) POOL_HISTOGRAM_PREFIX(POOL, "") } // namespace Envoy diff --git a/include/envoy/stats/store.h b/include/envoy/stats/store.h index d22ff2b3eb67d..cd017f9ad8843 100644 --- a/include/envoy/stats/store.h +++ b/include/envoy/stats/store.h @@ -39,11 +39,6 @@ class Store : public Scope { */ virtual std::vector gauges() const PURE; - /** - * @return a list of all known bools. - */ - virtual std::vector boolIndicators() const PURE; - /** * @return a list of all known histograms. */ diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 12b1229c0a21e..79a54472fdd7d 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -524,11 +524,11 @@ class PrioritySet { * Cluster circuit breakers stats. */ // clang-format off -#define ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(BOOL_INDICATOR) \ - BOOL_INDICATOR (cx_open) \ - BOOL_INDICATOR (rq_pending_open) \ - BOOL_INDICATOR (rq_open) \ - BOOL_INDICATOR (rq_retry_open) +#define ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(GAUGE) \ + GAUGE (cx_open) \ + GAUGE (rq_pending_open) \ + GAUGE (rq_open) \ + GAUGE (rq_retry_open) // clang-format on /** @@ -549,7 +549,7 @@ struct ClusterLoadReportStats { * Struct definition for cluster circuit breakers stats. @see stats_macros.h */ struct ClusterCircuitBreakersStats { - ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(GENERATE_BOOL_INDICATOR_STRUCT) + ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(GENERATE_GAUGE_STRUCT) }; /** diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index 7329f47f841e6..0ef95427caf6f 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -66,7 +66,7 @@ class GrpcStream : public Grpc::TypedAsyncStreamCallbacks, setRetryTimer(); return; } - control_plane_stats_.connected_state_.set(true); + control_plane_stats_.connected_state_.set(1); handleStreamEstablished(); } @@ -100,7 +100,7 @@ class GrpcStream : public Grpc::TypedAsyncStreamCallbacks, void onRemoteClose(Grpc::Status::GrpcStatus status, const std::string& message) override { ENVOY_LOG(warn, "gRPC config stream closed: {}, {}", status, message); stream_ = nullptr; - control_plane_stats_.connected_state_.set(false); + control_plane_stats_.connected_state_.set(0); handleEstablishmentFailure(); setRetryTimer(); } @@ -142,8 +142,7 @@ class GrpcStream : public Grpc::TypedAsyncStreamCallbacks, ControlPlaneStats generateControlPlaneStats(Stats::Scope& scope) { const std::string control_plane_prefix = "control_plane."; - return {ALL_CONTROL_PLANE_STATS(POOL_BOOL_INDICATOR_PREFIX(scope, control_plane_prefix), - POOL_COUNTER_PREFIX(scope, control_plane_prefix), + return {ALL_CONTROL_PLANE_STATS(POOL_COUNTER_PREFIX(scope, control_plane_prefix), POOL_GAUGE_PREFIX(scope, control_plane_prefix))}; } diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index b8e79b07c24c3..a7b3ee2f48565 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -319,7 +319,7 @@ void AdminLayer::mergeValues(const std::unordered_map& values_.emplace(kv.first, SnapshotImpl::createEntry(kv.second)); } } - stats_.admin_overrides_active_.set(!values_.empty()); + stats_.admin_overrides_active_.set(values_.empty() ? 0 : 1); } DiskLayer::DiskLayer(const std::string& name, const std::string& path, Api::Api& api) @@ -429,9 +429,8 @@ DiskBackedLoaderImpl::DiskBackedLoaderImpl(Event::Dispatcher& dispatcher, RuntimeStats LoaderImpl::generateStats(Stats::Store& store) { std::string prefix = "runtime."; - RuntimeStats stats{ALL_RUNTIME_STATS(POOL_BOOL_INDICATOR_PREFIX(store, prefix), - POOL_COUNTER_PREFIX(store, prefix), - POOL_GAUGE_PREFIX(store, prefix))}; + RuntimeStats stats{ + ALL_RUNTIME_STATS(POOL_COUNTER_PREFIX(store, prefix), POOL_GAUGE_PREFIX(store, prefix))}; return stats; } diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index edbed0bbba4a3..d2e80127a23df 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -43,21 +43,21 @@ class RandomGeneratorImpl : public RandomGenerator { * All runtime stats. @see stats_macros.h */ // clang-format off -#define ALL_RUNTIME_STATS(BOOL_INDICATOR, COUNTER, GAUGE) \ - COUNTER (load_error) \ - COUNTER (override_dir_not_exists) \ - COUNTER (override_dir_exists) \ - COUNTER (load_success) \ - COUNTER (deprecated_feature_use) \ - GAUGE (num_keys) \ - BOOL_INDICATOR (admin_overrides_active) +#define ALL_RUNTIME_STATS(COUNTER, GAUGE) \ + COUNTER(load_error) \ + COUNTER(override_dir_not_exists) \ + COUNTER(override_dir_exists) \ + COUNTER(load_success) \ + COUNTER(deprecated_feature_use) \ + GAUGE (num_keys) \ + GAUGE (admin_overrides_active) // clang-format on /** * Struct definition for all runtime stats. @see stats_macros.h */ struct RuntimeStats { - ALL_RUNTIME_STATS(GENERATE_BOOL_INDICATOR_STRUCT, GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) + ALL_RUNTIME_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) }; /** diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index 8caa8561c5fad..d85d38ef03213 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -23,11 +23,6 @@ IsolatedStoreImpl::IsolatedStoreImpl() std::vector tags; return alloc_.makeGauge(name, std::move(tag_extracted_name), std::move(tags)); }), - bool_indicators_([this](const std::string& name) -> BoolIndicatorSharedPtr { - std::string tag_extracted_name = name; - std::vector tags; - return alloc_.makeBoolIndicator(name, std::move(tag_extracted_name), std::move(tags)); - }), histograms_([this](const std::string& name) -> HistogramSharedPtr { return std::make_shared(name, *this, std::string(name), std::vector()); }) {} @@ -43,9 +38,6 @@ struct IsolatedScopeImpl : public Scope { void deliverHistogramToSinks(const Histogram&, uint64_t) override {} Counter& counter(const std::string& name) override { return parent_.counter(prefix_ + name); } Gauge& gauge(const std::string& name) override { return parent_.gauge(prefix_ + name); } - BoolIndicator& boolIndicator(const std::string& name) override { - return parent_.boolIndicator(prefix_ + name); - } Histogram& histogram(const std::string& name) override { return parent_.histogram(prefix_ + name); } diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index 91b8d39d1c127..7765fd50e3e72 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -61,9 +61,6 @@ class IsolatedStoreImpl : public Store { ScopePtr createScope(const std::string& name) override; void deliverHistogramToSinks(const Histogram&, uint64_t) override {} Gauge& gauge(const std::string& name) override { return gauges_.get(name); } - BoolIndicator& boolIndicator(const std::string& name) override { - return bool_indicators_.get(name); - } Histogram& histogram(const std::string& name) override { Histogram& histogram = histograms_.get(name); return histogram; @@ -73,9 +70,6 @@ class IsolatedStoreImpl : public Store { // Stats::Store std::vector counters() const override { return counters_.toVector(); } std::vector gauges() const override { return gauges_.toVector(); } - std::vector boolIndicators() const override { - return bool_indicators_.toVector(); - } std::vector histograms() const override { return std::vector{}; } @@ -84,7 +78,6 @@ class IsolatedStoreImpl : public Store { HeapStatDataAllocator alloc_; IsolatedStatsCache counters_; IsolatedStatsCache gauges_; - IsolatedStatsCache bool_indicators_; IsolatedStatsCache histograms_; const StatsOptionsImpl stats_options_; }; diff --git a/source/common/stats/source_impl.cc b/source/common/stats/source_impl.cc index a0cbbe84ff0f5..e6102d5a608cd 100644 --- a/source/common/stats/source_impl.cc +++ b/source/common/stats/source_impl.cc @@ -17,12 +17,6 @@ std::vector& SourceImpl::cachedGauges() { } return *gauges_; } -std::vector& SourceImpl::cachedBoolIndicators() { - if (!bool_indicators_) { - bool_indicators_ = store_.boolIndicators(); - } - return *bool_indicators_; -} std::vector& SourceImpl::cachedHistograms() { if (!histograms_) { histograms_ = store_.histograms(); @@ -33,7 +27,6 @@ std::vector& SourceImpl::cachedHistograms() { void SourceImpl::clearCache() { counters_.reset(); gauges_.reset(); - bool_indicators_.reset(); histograms_.reset(); } diff --git a/source/common/stats/source_impl.h b/source/common/stats/source_impl.h index 5c85d8c980ce8..a3a79ba9a76ab 100644 --- a/source/common/stats/source_impl.h +++ b/source/common/stats/source_impl.h @@ -16,7 +16,6 @@ class SourceImpl : public Source { // Stats::Source std::vector& cachedCounters() override; std::vector& cachedGauges() override; - std::vector& cachedBoolIndicators() override; std::vector& cachedHistograms() override; void clearCache() override; @@ -24,7 +23,6 @@ class SourceImpl : public Source { Store& store_; absl::optional> counters_; absl::optional> gauges_; - absl::optional> bool_indicators_; absl::optional> histograms_; }; diff --git a/source/common/stats/stat_data_allocator_impl.h b/source/common/stats/stat_data_allocator_impl.h index 4b17527d3b8ce..a25b1e4501817 100644 --- a/source/common/stats/stat_data_allocator_impl.h +++ b/source/common/stats/stat_data_allocator_impl.h @@ -34,8 +34,6 @@ template class StatDataAllocatorImpl : public StatDataAllocator std::vector&& tags) override; GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) override; - BoolIndicatorSharedPtr makeBoolIndicator(absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) override; /** * @param name the full name of the stat. @@ -71,7 +69,6 @@ template class CounterImpl : public Counter, public MetricImpl // Stats::Metric std::string name() const override { return std::string(data_.name()); } const char* nameCStr() const override { return data_.name(); } - bool used() const override { return data_.flags_ & Flags::Used; } // Stats::Counter void add(uint64_t amount) override { @@ -83,6 +80,7 @@ template class CounterImpl : public Counter, public MetricImpl void inc() override { add(1); } uint64_t latch() override { return data_.pending_increment_.exchange(0); } void reset() override { data_.value_ = 0; } + bool used() const override { return data_.flags_ & Flags::Used; } uint64_t value() const override { return data_.value_; } private: @@ -123,7 +121,6 @@ template class GaugeImpl : public Gauge, public MetricImpl { // Stats::Metric std::string name() const override { return std::string(data_.name()); } const char* nameCStr() const override { return data_.name(); } - bool used() const override { return data_.flags_ & Flags::Used; } // Stats::Gauge virtual void add(uint64_t amount) override { @@ -142,6 +139,7 @@ template class GaugeImpl : public Gauge, public MetricImpl { data_.value_ -= amount; } virtual uint64_t value() const override { return data_.value_; } + bool used() const override { return data_.flags_ & Flags::Used; } private: StatData& data_; @@ -169,50 +167,6 @@ class NullGaugeImpl : public Gauge { uint64_t value() const override { return 0; } }; -/** - * BoolIndicator implementation that wraps a StatData. - */ -template class BoolIndicatorImpl : public BoolIndicator, public MetricImpl { -public: - BoolIndicatorImpl(StatData& data, StatDataAllocatorImpl& alloc, - std::string&& tag_extracted_name, std::vector&& tags) - : MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {} - ~BoolIndicatorImpl() { alloc_.free(data_); } - - // Stats::Metric - std::string name() const override { return std::string(data_.name()); } - const char* nameCStr() const override { return data_.name(); } - bool used() const override { return data_.flags_ & Flags::Used; } - - // Stats::BoolIndicator - virtual void set(bool value) override { - data_.value_ = value ? 1 : 0; - data_.flags_ |= Flags::Used; - } - virtual bool value() const override { return data_.value_; } - -private: - StatData& data_; - StatDataAllocatorImpl& alloc_; -}; - -/** - * Null bool implementation. - * No-ops on all calls and requires no underlying metric or data. - */ -class NullBoolIndicatorImpl : public BoolIndicator { -public: - NullBoolIndicatorImpl() {} - ~NullBoolIndicatorImpl() {} - std::string name() const override { return ""; } - const char* nameCStr() const override { return ""; } - const std::string& tagExtractedName() const override { CONSTRUCT_ON_FIRST_USE(std::string, ""); } - const std::vector& tags() const override { CONSTRUCT_ON_FIRST_USE(std::vector, {}); } - void set(bool) override {} - bool used() const override { return false; } - bool value() const override { return false; } -}; - template CounterSharedPtr StatDataAllocatorImpl::makeCounter(absl::string_view name, std::string&& tag_extracted_name, @@ -237,16 +191,5 @@ GaugeSharedPtr StatDataAllocatorImpl::makeGauge(absl::string_view name std::move(tags)); } -template -BoolIndicatorSharedPtr StatDataAllocatorImpl::makeBoolIndicator( - absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) { - StatData* data = alloc(name); - if (data == nullptr) { - return nullptr; - } - return std::make_shared>(*data, *this, std::move(tag_extracted_name), - std::move(tags)); -} - } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index d8d7567658734..2c72dbf94b983 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -45,7 +45,6 @@ void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { for (ScopeImpl* scope : scopes_) { removeRejectedStats(scope->central_cache_.counters_, deleted_counters_); removeRejectedStats(scope->central_cache_.gauges_, deleted_gauges_); - removeRejectedStats(scope->central_cache_.bool_indicators_, deleted_bool_indicators_); removeRejectedStats(scope->central_cache_.histograms_, deleted_histograms_); } } @@ -112,22 +111,6 @@ std::vector ThreadLocalStoreImpl::gauges() const { return ret; } -std::vector ThreadLocalStoreImpl::boolIndicators() const { - // Handle de-dup due to overlapping scopes. - std::vector ret; - CharStarHashSet names; - Thread::LockGuard lock(lock_); - for (ScopeImpl* scope : scopes_) { - for (auto& bool_indicator : scope->central_cache_.bool_indicators_) { - if (names.insert(bool_indicator.first).second) { - ret.push_back(bool_indicator.second); - } - } - } - - return ret; -} - std::vector ThreadLocalStoreImpl::histograms() const { std::vector ret; Thread::LockGuard lock(lock_); @@ -376,34 +359,6 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) { tls_cache); } -BoolIndicator& ThreadLocalStoreImpl::ScopeImpl::boolIndicator(const std::string& name) { - // See comments in counter(). There is no super clean way (via templates or otherwise) to - // share this code so I'm leaving it largely duplicated for now. - // - // Note that we can do map.find(final_name.c_str()), but we cannot do - // map[final_name.c_str()] as the char*-keyed maps would then save the pointer to - // a temporary, and address sanitization errors would follow. Instead we must - // do a find() first, using that if it succeeds. If it fails, then after we - // construct the stat we can insert it into the required maps. - std::string final_name = prefix_ + name; - if (parent_.rejects(final_name)) { - return null_bool_; - } - - StatMap* tls_cache = nullptr; - if (!parent_.shutting_down_ && parent_.tls_) { - tls_cache = &parent_.tls_->getTyped().scope_cache_[this->scope_id_].bool_indicators_; - } - - return safeMakeStat( - final_name, central_cache_.bool_indicators_, - [](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) -> BoolIndicatorSharedPtr { - return allocator.makeBoolIndicator(name, std::move(tag_extracted_name), std::move(tags)); - }, - tls_cache); -} - Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) { // See comments in counter(). There is no super clean way (via templates or otherwise) to // share this code so I'm leaving it largely duplicated for now. diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 292147f44d473..4f85c763567c2 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -145,9 +145,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo return default_scope_->deliverHistogramToSinks(histogram, value); } Gauge& gauge(const std::string& name) override { return default_scope_->gauge(name); } - BoolIndicator& boolIndicator(const std::string& name) override { - return default_scope_->boolIndicator(name); - } Histogram& histogram(const std::string& name) override { return default_scope_->histogram(name); }; @@ -155,7 +152,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo // Stats::Store std::vector counters() const override; std::vector gauges() const override; - std::vector boolIndicators() const override; std::vector histograms() const override; // Stats::StoreRoot @@ -180,7 +176,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo struct TlsCacheEntry { StatMap counters_; StatMap gauges_; - StatMap bool_indicators_; StatMap histograms_; StatMap parent_histograms_; }; @@ -188,7 +183,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo struct CentralCacheEntry { StatMap counters_; StatMap gauges_; - StatMap bool_indicators_; StatMap histograms_; }; @@ -205,7 +199,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo } void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) override; Gauge& gauge(const std::string& name) override; - BoolIndicator& boolIndicator(const std::string& name) override; Histogram& histogram(const std::string& name) override; Histogram& tlsHistogram(const std::string& name, ParentHistogramImpl& parent) override; const Stats::StatsOptions& statsOptions() const override { return parent_.statsOptions(); } @@ -241,7 +234,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo NullCounterImpl null_counter_; NullGaugeImpl null_gauge_; - NullBoolIndicatorImpl null_bool_; NullHistogramImpl null_histogram_; }; @@ -291,7 +283,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo // but that would be fairly complex to change. std::vector deleted_counters_; std::vector deleted_gauges_; - std::vector deleted_bool_indicators_; std::vector deleted_histograms_; }; diff --git a/source/common/upstream/resource_manager_impl.h b/source/common/upstream/resource_manager_impl.h index 3c564d4c8bb07..35cf4bf87b7ff 100644 --- a/source/common/upstream/resource_manager_impl.h +++ b/source/common/upstream/resource_manager_impl.h @@ -44,21 +44,20 @@ class ResourceManagerImpl : public ResourceManager { private: struct ResourceImpl : public Resource { ResourceImpl(uint64_t max, Runtime::Loader& runtime, const std::string& runtime_key, - Stats::BoolIndicator& circuit_breaker_open) - : max_(max), runtime_(runtime), runtime_key_(runtime_key), - circuit_breaker_open_(circuit_breaker_open) {} + Stats::Gauge& open_gauge) + : max_(max), runtime_(runtime), runtime_key_(runtime_key), open_gauge_(open_gauge) {} ~ResourceImpl() { ASSERT(current_ == 0); } // Upstream::Resource bool canCreate() override { return current_ < max(); } void inc() override { current_++; - circuit_breaker_open_.set(!canCreate()); + open_gauge_.set(canCreate() ? 0 : 1); } void dec() override { ASSERT(current_ > 0); current_--; - circuit_breaker_open_.set(!canCreate()); + open_gauge_.set(canCreate() ? 0 : 1); } uint64_t max() override { return runtime_.snapshot().getInteger(runtime_key_, max_); } @@ -68,10 +67,11 @@ class ResourceManagerImpl : public ResourceManager { const std::string runtime_key_; /** - * The live circuit breaker state: false when the circuit breaker is closed, - * true when open. + * A gauge to notify the live circuit breaker state. The gauge is set to 0 + * to notify that the circuit breaker is closed, or to 1 to notify that it + * is open. */ - Stats::BoolIndicator& circuit_breaker_open_; + Stats::Gauge& open_gauge_; }; ResourceImpl connections_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 825df6f4ad474..e959961e139b9 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -818,7 +818,7 @@ ClusterInfoImpl::ResourceManagers::ResourceManagers(const envoy::api::v2::Cluste ClusterCircuitBreakersStats ClusterInfoImpl::generateCircuitBreakersStats(Stats::Scope& scope, const std::string& stat_prefix) { std::string prefix(fmt::format("circuit_breakers.{}.", stat_prefix)); - return {ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(POOL_BOOL_INDICATOR_PREFIX(scope, prefix))}; + return {ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(POOL_GAUGE_PREFIX(scope, prefix))}; } ResourceManagerImplPtr diff --git a/source/server/overload_manager_impl.cc b/source/server/overload_manager_impl.cc index 6a9401208d001..364f739bfaa1a 100644 --- a/source/server/overload_manager_impl.cc +++ b/source/server/overload_manager_impl.cc @@ -41,7 +41,7 @@ std::string StatsName(const std::string& a, const std::string& b) { OverloadAction::OverloadAction(const envoy::config::overload::v2alpha::OverloadAction& config, Stats::Scope& stats_scope) - : active_indicator_(stats_scope.boolIndicator(StatsName(config.name(), "active"))) { + : active_gauge_(stats_scope.gauge(StatsName(config.name(), "active"))) { for (const auto& trigger_config : config.triggers()) { TriggerPtr trigger; @@ -59,7 +59,7 @@ OverloadAction::OverloadAction(const envoy::config::overload::v2alpha::OverloadA } } - active_indicator_.set(false); + active_gauge_.set(0); } bool OverloadAction::updateResourcePressure(const std::string& name, double pressure) { @@ -69,11 +69,11 @@ bool OverloadAction::updateResourcePressure(const std::string& name, double pres ASSERT(it != triggers_.end()); if (it->second->updateValue(pressure)) { if (it->second->isFired()) { - active_indicator_.set(true); + active_gauge_.set(1); const auto result = fired_triggers_.insert(name); ASSERT(result.second); } else { - active_indicator_.set(false); + active_gauge_.set(0); const auto result = fired_triggers_.erase(name); ASSERT(result == 1); } diff --git a/source/server/overload_manager_impl.h b/source/server/overload_manager_impl.h index a2519003211ca..9b7293ed231c9 100644 --- a/source/server/overload_manager_impl.h +++ b/source/server/overload_manager_impl.h @@ -46,7 +46,7 @@ class OverloadAction { private: std::unordered_map triggers_; std::unordered_set fired_triggers_; - Stats::BoolIndicator& active_indicator_; + Stats::Gauge& active_gauge_; }; class OverloadManagerImpl : Logger::Loggable, public OverloadManager { diff --git a/source/server/server.cc b/source/server/server.cc index b0d36829729a8..fcbdd04a88960 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -125,7 +125,10 @@ void InstanceImpl::drainListeners() { drain_manager_->startDrainSequence(nullptr); } -void InstanceImpl::failHealthcheck(bool fail) { server_stats_->live_.set(!fail); } +void InstanceImpl::failHealthcheck(bool fail) { + // We keep liveness state in shared memory so the parent process sees the same state. + server_stats_->live_.set(!fail); +} void InstanceUtil::flushMetricsToSinks(const std::list& sinks, Stats::Source& source) { @@ -165,7 +168,7 @@ void InstanceImpl::getParentStats(HotRestart::GetParentStatsInfo& info) { info.num_connections_ = numConnections(); } -bool InstanceImpl::healthCheckFailed() { return !server_stats_->live_.value(); } +bool InstanceImpl::healthCheckFailed() { return server_stats_->live_.value() == 0; } InstanceUtil::BootstrapVersion InstanceUtil::loadBootstrapConfig(envoy::config::bootstrap::v2::Bootstrap& bootstrap, @@ -238,8 +241,7 @@ void InstanceImpl::initialize(const Options& options, const std::string server_stats_prefix = "server."; server_stats_ = std::make_unique( - ServerStats{ALL_SERVER_STATS(POOL_BOOL_INDICATOR_PREFIX(stats_store_, server_stats_prefix), - POOL_COUNTER_PREFIX(stats_store_, server_stats_prefix), + ServerStats{ALL_SERVER_STATS(POOL_COUNTER_PREFIX(stats_store_, server_stats_prefix), POOL_GAUGE_PREFIX(stats_store_, server_stats_prefix))}); server_stats_->concurrency_.set(options_.concurrency()); diff --git a/source/server/server.h b/source/server/server.h index aa026fb65ce13..27b02a0722f43 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -46,22 +46,22 @@ namespace Server { * All server wide stats. @see stats_macros.h */ // clang-format off -#define ALL_SERVER_STATS(BOOL_INDICATOR, COUNTER, GAUGE) \ - BOOL_INDICATOR(live) \ - COUNTER(debug_assertion_failures) \ +#define ALL_SERVER_STATS(COUNTER, GAUGE) \ + GAUGE(uptime) \ GAUGE(concurrency) \ - GAUGE(days_until_first_cert_expiring) \ - GAUGE(hot_restart_epoch) \ GAUGE(memory_allocated) \ GAUGE(memory_heap_size) \ + GAUGE(live) \ GAUGE(parent_connections) \ GAUGE(total_connections) \ - GAUGE(uptime) \ - GAUGE(version) + GAUGE(version) \ + GAUGE(days_until_first_cert_expiring) \ + GAUGE(hot_restart_epoch) \ + COUNTER(debug_assertion_failures) // clang-format on struct ServerStats { - ALL_SERVER_STATS(GENERATE_BOOL_INDICATOR_STRUCT, GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) + ALL_SERVER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) }; /** diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 2b056dbf5666b..f8e9f62cb8169 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -108,7 +108,7 @@ TEST_F(GrpcMuxImplTest, MultipleTypeUrlStreams) { expectSendMessage("foo", {"x", "y"}, ""); expectSendMessage("bar", {}, ""); grpc_mux_->start(); - EXPECT_TRUE(stats_.boolIndicator("control_plane.connected_state").value()); + EXPECT_EQ(1, stats_.gauge("control_plane.connected_state").value()); expectSendMessage("bar", {"z"}, ""); auto bar_z_sub = grpc_mux_->subscribe("bar", {"z"}, callbacks_); expectSendMessage("bar", {"zz", "z"}, ""); @@ -146,7 +146,7 @@ TEST_F(GrpcMuxImplTest, ResetStream) { ASSERT_TRUE(timer != nullptr); // initialized from dispatcher mock. EXPECT_CALL(*timer, enableTimer(_)); grpc_mux_->onRemoteClose(Grpc::Status::GrpcStatus::Canceled, ""); - EXPECT_FALSE(stats_.boolIndicator("control_plane.connected_state").value()); + EXPECT_EQ(0, stats_.gauge("control_plane.connected_state").value()); EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); expectSendMessage("foo", {"x", "y"}, ""); expectSendMessage("bar", {}, ""); diff --git a/test/common/config/subscription_test_harness.h b/test/common/config/subscription_test_harness.h index 1b50774a256e0..0df305a00f581 100644 --- a/test/common/config/subscription_test_harness.h +++ b/test/common/config/subscription_test_harness.h @@ -58,8 +58,8 @@ class SubscriptionTestHarness { EXPECT_EQ(version, stats_.version_.value()); } - virtual void verifyControlPlaneStats(bool connected_state) { - EXPECT_EQ(connected_state, stats_store_.boolIndicator("control_plane.connected_state").value()); + virtual void verifyControlPlaneStats(uint32_t connected_state) { + EXPECT_EQ(connected_state, stats_store_.gauge("control_plane.connected_state").value()); } Stats::IsolatedStoreImpl stats_store_; diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index d9b27c5f22db2..4b29fb71b1fb5 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -295,7 +295,7 @@ TEST_F(Http1ConnPoolImplTest, MultipleRequestAndResponse) { TEST_F(Http1ConnPoolImplTest, MaxPendingRequests) { cluster_->resetResourceManager(1, 1, 1024, 1); - EXPECT_FALSE(cluster_->circuit_breakers_stats_.rq_pending_open_.value()); + EXPECT_EQ(0U, cluster_->circuit_breakers_stats_.rq_pending_open_.value()); NiceMock outer_decoder; ConnPoolCallbacks callbacks; @@ -309,7 +309,7 @@ TEST_F(Http1ConnPoolImplTest, MaxPendingRequests) { Http::ConnectionPool::Cancellable* handle2 = conn_pool_.newStream(outer_decoder2, callbacks2); EXPECT_EQ(nullptr, handle2); - EXPECT_TRUE(cluster_->circuit_breakers_stats_.rq_pending_open_.value()); + EXPECT_EQ(1U, cluster_->circuit_breakers_stats_.rq_pending_open_.value()); handle->cancel(); @@ -436,7 +436,7 @@ TEST_F(Http1ConnPoolImplTest, DisconnectWhileBound) { TEST_F(Http1ConnPoolImplTest, MaxConnections) { InSequence s; - EXPECT_FALSE(cluster_->circuit_breakers_stats_.cx_open_.value()); + EXPECT_EQ(0U, cluster_->circuit_breakers_stats_.cx_open_.value()); // Request 1 should kick off a new connection. NiceMock outer_decoder1; @@ -451,7 +451,7 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { ConnPoolCallbacks callbacks2; handle = conn_pool_.newStream(outer_decoder2, callbacks2); EXPECT_EQ(1U, cluster_->stats_.upstream_cx_overflow_.value()); - EXPECT_TRUE(cluster_->circuit_breakers_stats_.cx_open_.value()); + EXPECT_EQ(1U, cluster_->circuit_breakers_stats_.cx_open_.value()); EXPECT_NE(nullptr, handle); diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index 95917de1f1bad..75327b04e9684 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -440,7 +440,7 @@ TEST_F(Http2ConnPoolImplTest, LocalReset) { EXPECT_CALL(*this, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); EXPECT_EQ(1U, cluster_->stats_.upstream_rq_tx_reset_.value()); - EXPECT_FALSE(cluster_->circuit_breakers_stats_.rq_open_.value()); + EXPECT_EQ(0U, cluster_->circuit_breakers_stats_.rq_open_.value()); } TEST_F(Http2ConnPoolImplTest, RemoteReset) { @@ -581,7 +581,7 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimaryNoActiveRequest) { TEST_F(Http2ConnPoolImplTest, ConnectTimeout) { InSequence s; - EXPECT_FALSE(cluster_->circuit_breakers_stats_.rq_open_.value()); + EXPECT_EQ(0U, cluster_->circuit_breakers_stats_.rq_open_.value()); expectClientCreate(); ActiveTestRequest r1(*this, 0, false); @@ -591,7 +591,7 @@ TEST_F(Http2ConnPoolImplTest, ConnectTimeout) { EXPECT_CALL(*this, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); - EXPECT_FALSE(cluster_->circuit_breakers_stats_.rq_open_.value()); + EXPECT_EQ(0U, cluster_->circuit_breakers_stats_.rq_open_.value()); expectClientCreate(); ActiveTestRequest r2(*this, 1, false); diff --git a/test/common/router/retry_state_impl_test.cc b/test/common/router/retry_state_impl_test.cc index 309b6592dca69..b126f70cca3f1 100644 --- a/test/common/router/retry_state_impl_test.cc +++ b/test/common/router/retry_state_impl_test.cc @@ -459,7 +459,7 @@ TEST_F(RouterRetryStateImplTest, MaxRetriesHeader) { EXPECT_CALL(callback_ready_, ready()); retry_timer_->callback_(); - EXPECT_TRUE(cluster_.circuit_breakers_stats_.rq_retry_open_.value()); + EXPECT_EQ(1UL, cluster_.circuit_breakers_stats_.rq_retry_open_.value()); EXPECT_EQ(RetryStatus::NoRetryLimitExceeded, state_->shouldRetryReset(connect_failure_, callback_)); @@ -499,7 +499,7 @@ TEST_F(RouterRetryStateImplTest, Backoff) { EXPECT_EQ(3UL, cluster_.stats().upstream_rq_retry_.value()); EXPECT_EQ(1UL, cluster_.stats().upstream_rq_retry_success_.value()); - EXPECT_FALSE(cluster_.circuit_breakers_stats_.rq_retry_open_.value()); + EXPECT_EQ(0UL, cluster_.circuit_breakers_stats_.rq_retry_open_.value()); } TEST_F(RouterRetryStateImplTest, HostSelectionAttempts) { diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 6e26b4893b106..6cb66c3d17856 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -250,22 +250,22 @@ void testNewOverrides(Loader& loader, Stats::Store& store) { // New string loader.mergeValues({{"foo", "bar"}}); EXPECT_EQ("bar", loader.snapshot().get("foo")); - EXPECT_TRUE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove new string loader.mergeValues({{"foo", ""}}); EXPECT_EQ("", loader.snapshot().get("foo")); - EXPECT_FALSE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); // New integer loader.mergeValues({{"baz", "42"}}); EXPECT_EQ(42, loader.snapshot().getInteger("baz", 0)); - EXPECT_TRUE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove new integer loader.mergeValues({{"baz", ""}}); EXPECT_EQ(0, loader.snapshot().getInteger("baz", 0)); - EXPECT_FALSE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); } TEST_F(DiskBackedLoaderImplTest, mergeValues) { @@ -276,32 +276,32 @@ TEST_F(DiskBackedLoaderImplTest, mergeValues) { // Override string loader->mergeValues({{"file2", "new world"}}); EXPECT_EQ("new world", loader->snapshot().get("file2")); - EXPECT_TRUE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove overridden string loader->mergeValues({{"file2", ""}}); EXPECT_EQ("world", loader->snapshot().get("file2")); - EXPECT_FALSE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); // Override integer loader->mergeValues({{"file3", "42"}}); EXPECT_EQ(42, loader->snapshot().getInteger("file3", 1)); - EXPECT_TRUE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove overridden integer loader->mergeValues({{"file3", ""}}); EXPECT_EQ(2, loader->snapshot().getInteger("file3", 1)); - EXPECT_FALSE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); // Override override string loader->mergeValues({{"file1", "hello overridden override"}}); EXPECT_EQ("hello overridden override", loader->snapshot().get("file1")); - EXPECT_TRUE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove overridden override string loader->mergeValues({{"file1", ""}}); EXPECT_EQ("hello override", loader->snapshot().get("file1")); - EXPECT_FALSE(store.boolIndicator("runtime.admin_overrides_active").value()); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); } TEST(LoaderImplTest, All) { diff --git a/test/common/stats/isolated_store_impl_test.cc b/test/common/stats/isolated_store_impl_test.cc index a6401a063aa4a..b2aaa80693fdd 100644 --- a/test/common/stats/isolated_store_impl_test.cc +++ b/test/common/stats/isolated_store_impl_test.cc @@ -22,7 +22,7 @@ TEST(StatsIsolatedStoreImplTest, All) { EXPECT_EQ("c1", c1.tagExtractedName()); EXPECT_EQ("scope1.c2", c2.tagExtractedName()); EXPECT_EQ(0, c1.tags().size()); - EXPECT_EQ(0, c2.tags().size()); + EXPECT_EQ(0, c1.tags().size()); Gauge& g1 = store.gauge("g1"); Gauge& g2 = scope1->gauge("g2"); @@ -31,16 +31,7 @@ TEST(StatsIsolatedStoreImplTest, All) { EXPECT_EQ("g1", g1.tagExtractedName()); EXPECT_EQ("scope1.g2", g2.tagExtractedName()); EXPECT_EQ(0, g1.tags().size()); - EXPECT_EQ(0, g2.tags().size()); - - BoolIndicator& b1 = store.boolIndicator("b1"); - BoolIndicator& b2 = scope1->boolIndicator("b2"); - EXPECT_EQ("b1", b1.name()); - EXPECT_EQ("scope1.b2", b2.name()); - EXPECT_EQ("b1", b1.tagExtractedName()); - EXPECT_EQ("scope1.b2", b2.tagExtractedName()); - EXPECT_EQ(0, b1.tags().size()); - EXPECT_EQ(0, b2.tags().size()); + EXPECT_EQ(0, g1.tags().size()); Histogram& h1 = store.histogram("h1"); Histogram& h2 = scope1->histogram("h2"); @@ -63,7 +54,6 @@ TEST(StatsIsolatedStoreImplTest, All) { EXPECT_EQ(4UL, store.counters().size()); EXPECT_EQ(2UL, store.gauges().size()); - EXPECT_EQ(2UL, store.boolIndicators().size()); } TEST(StatsIsolatedStoreImplTest, LongStatName) { @@ -80,23 +70,20 @@ TEST(StatsIsolatedStoreImplTest, LongStatName) { * Test stats macros. @see stats_macros.h */ // clang-format off -#define ALL_TEST_STATS(COUNTER, GAUGE, BOOL_INDICATOR, HISTOGRAM) \ - COUNTER (test_counter) \ - GAUGE (test_gauge) \ - BOOL_INDICATOR(test_bool_indicator) \ - HISTOGRAM (test_histogram) +#define ALL_TEST_STATS(COUNTER, GAUGE, HISTOGRAM) \ + COUNTER (test_counter) \ + GAUGE (test_gauge) \ + HISTOGRAM(test_histogram) // clang-format on struct TestStats { - ALL_TEST_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_BOOL_INDICATOR_STRUCT, - GENERATE_HISTOGRAM_STRUCT) + ALL_TEST_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) }; TEST(StatsMacros, All) { IsolatedStoreImpl stats_store; TestStats test_stats{ALL_TEST_STATS(POOL_COUNTER_PREFIX(stats_store, "test."), POOL_GAUGE_PREFIX(stats_store, "test."), - POOL_BOOL_INDICATOR_PREFIX(stats_store, "test."), POOL_HISTOGRAM_PREFIX(stats_store, "test."))}; Counter& counter = test_stats.test_counter_; @@ -105,9 +92,6 @@ TEST(StatsMacros, All) { Gauge& gauge = test_stats.test_gauge_; EXPECT_EQ("test.test_gauge", gauge.name()); - BoolIndicator& boolIndicator = test_stats.test_bool_indicator_; - EXPECT_EQ("test.test_bool_indicator", boolIndicator.name()); - Histogram& histogram = test_stats.test_histogram_; EXPECT_EQ("test.test_histogram", histogram.name()); } diff --git a/test/common/stats/source_impl_test.cc b/test/common/stats/source_impl_test.cc index 76f86905c326b..bf5b15851d308 100644 --- a/test/common/stats/source_impl_test.cc +++ b/test/common/stats/source_impl_test.cc @@ -17,12 +17,10 @@ TEST(SourceImplTest, Caching) { NiceMock store; std::vector stored_counters; std::vector stored_gauges; - std::vector stored_bools; std::vector stored_histograms; ON_CALL(store, counters()).WillByDefault(ReturnPointee(&stored_counters)); ON_CALL(store, gauges()).WillByDefault(ReturnPointee(&stored_gauges)); - ON_CALL(store, boolIndicators()).WillByDefault(ReturnPointee(&stored_bools)); ON_CALL(store, histograms()).WillByDefault(ReturnPointee(&stored_histograms)); SourceImpl source(store); @@ -38,11 +36,6 @@ TEST(SourceImplTest, Caching) { stored_gauges.push_back(std::make_shared()); EXPECT_NE(source.cachedGauges(), stored_gauges); - stored_bools.push_back(std::make_shared()); - EXPECT_EQ(source.cachedBoolIndicators(), stored_bools); - stored_bools.push_back(std::make_shared()); - EXPECT_NE(source.cachedBoolIndicators(), stored_bools); - stored_histograms.push_back(std::make_shared()); EXPECT_EQ(source.cachedHistograms(), stored_histograms); stored_histograms.push_back(std::make_shared()); @@ -52,7 +45,6 @@ TEST(SourceImplTest, Caching) { source.clearCache(); EXPECT_EQ(source.cachedCounters(), stored_counters); EXPECT_EQ(source.cachedGauges(), stored_gauges); - EXPECT_EQ(source.cachedBoolIndicators(), stored_bools); EXPECT_EQ(source.cachedHistograms(), stored_histograms); } diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 1b47ff6128ce0..dddfea3e6146e 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -181,7 +181,7 @@ class HistogramTest : public testing::Test { TEST_F(StatsThreadLocalStoreTest, NoTls) { InSequence s; - EXPECT_CALL(*alloc_, alloc(_)).Times(3); + EXPECT_CALL(*alloc_, alloc(_)).Times(2); Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -189,9 +189,6 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { Gauge& g1 = store_->gauge("g1"); EXPECT_EQ(&g1, &store_->gauge("g1")); - BoolIndicator& b1 = store_->boolIndicator("b1"); - EXPECT_EQ(&b1, &store_->boolIndicator("b1")); - Histogram& h1 = store_->histogram("h1"); EXPECT_EQ(&h1, &store_->histogram("h1")); @@ -206,12 +203,9 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(2L, store_->gauges().front().use_count()); - EXPECT_EQ(1UL, store_->boolIndicators().size()); - EXPECT_EQ(&b1, store_->boolIndicators().front().get()); // front() ok when size()==1 - EXPECT_EQ(2L, store_->boolIndicators().front().use_count()); // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(4); + EXPECT_CALL(*alloc_, free(_)).Times(3); store_->shutdownThreading(); } @@ -220,7 +214,7 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*alloc_, alloc(_)).Times(3); + EXPECT_CALL(*alloc_, alloc(_)).Times(2); Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -228,9 +222,6 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { Gauge& g1 = store_->gauge("g1"); EXPECT_EQ(&g1, &store_->gauge("g1")); - BoolIndicator& b1 = store_->boolIndicator("b1"); - EXPECT_EQ(&b1, &store_->boolIndicator("b1")); - Histogram& h1 = store_->histogram("h1"); EXPECT_EQ(&h1, &store_->histogram("h1")); @@ -240,9 +231,6 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(3L, store_->gauges().front().use_count()); - EXPECT_EQ(1UL, store_->boolIndicators().size()); - EXPECT_EQ(&b1, store_->boolIndicators().front().get()); // front() ok when size()==1 - EXPECT_EQ(3L, store_->boolIndicators().front().use_count()); store_->shutdownThreading(); tls_.shutdownThread(); @@ -253,12 +241,9 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(2L, store_->gauges().front().use_count()); - EXPECT_EQ(1UL, store_->boolIndicators().size()); - EXPECT_EQ(&b1, store_->boolIndicators().front().get()); // front() ok when size()==1 - EXPECT_EQ(2L, store_->boolIndicators().front().use_count()); // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(4); + EXPECT_CALL(*alloc_, free(_)).Times(3); } TEST_F(StatsThreadLocalStoreTest, BasicScope) { @@ -266,7 +251,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(*alloc_, alloc(_)).Times(6); + EXPECT_CALL(*alloc_, alloc(_)).Times(4); Counter& c1 = store_->counter("c1"); Counter& c2 = scope1->counter("c2"); EXPECT_EQ("c1", c1.name()); @@ -277,11 +262,6 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { EXPECT_EQ("g1", g1.name()); EXPECT_EQ("scope1.g2", g2.name()); - BoolIndicator& b1 = store_->boolIndicator("b1"); - BoolIndicator& b2 = scope1->boolIndicator("b2"); - EXPECT_EQ("b1", b1.name()); - EXPECT_EQ("scope1.b2", b2.name()); - Histogram& h1 = store_->histogram("h1"); Histogram& h2 = scope1->histogram("h2"); EXPECT_EQ("h1", h1.name()); @@ -297,7 +277,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(7); + EXPECT_CALL(*alloc_, free(_)).Times(5); } // Validate that we sanitize away bad characters in the stats prefix. @@ -372,15 +352,11 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { Gauge& g1 = scope2->gauge("some_gauge"); EXPECT_EQ("scope1.foo.some_gauge", g1.name()); - EXPECT_CALL(*alloc_, alloc(_)); - BoolIndicator& b1 = scope2->boolIndicator("some_bool"); - EXPECT_EQ("scope1.foo.some_bool", b1.name()); - store_->shutdownThreading(); tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(5); + EXPECT_CALL(*alloc_, free(_)).Times(4); } TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { @@ -420,21 +396,8 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(1UL, g2.value()); EXPECT_EQ(1UL, store_->gauges().size()); - // Bools should work just like gauges. - EXPECT_CALL(*alloc_, alloc(_)).Times(2); - BoolIndicator& b1 = scope1->boolIndicator("b"); - BoolIndicator& b2 = scope2->boolIndicator("b"); - EXPECT_NE(&b1, &b2); - b1.set(true); - EXPECT_EQ(1, b1.value()); - EXPECT_EQ(1, b2.value()); - b2.set(false); - EXPECT_EQ(0, b1.value()); - EXPECT_EQ(0, b2.value()); - EXPECT_EQ(1UL, store_->boolIndicators().size()); - // Deleting scope 1 will call free but will be reference counted. It still leaves scope 2 valid. - EXPECT_CALL(*alloc_, free(_)).Times(7); + EXPECT_CALL(*alloc_, free(_)).Times(2); scope1.reset(); c2.inc(); EXPECT_EQ(3UL, c2.value()); @@ -442,47 +405,12 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { g2.set(10); EXPECT_EQ(10UL, g2.value()); EXPECT_EQ(1UL, store_->gauges().size()); - b2.set(false); - EXPECT_EQ(0, b2.value()); - EXPECT_EQ(1UL, store_->boolIndicators().size()); store_->shutdownThreading(); tls_.shutdownThread(); -} -// Demonstrates that counters, gauges, and indicators are all mixed together in -// the shared memory, and not separated by type; only the name matters. -// This test is only here to reassure us that PR #5813, in the context of the current -// state of the Envoy codebase it is being submitted into, will not break hot restart! -// It is not meant to enforce this behavior as a desirable feature that must be kept. -TEST_F(StatsThreadLocalStoreTest, SameNameDifferentType) { - InSequence s; - store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*alloc_, alloc(_)).Times(4); - - Counter& c1 = store_->counter("samename"); - EXPECT_EQ(&c1, &store_->counter("samename")); - Gauge& g1 = store_->gauge("samename"); - EXPECT_EQ(&g1, &store_->gauge("samename")); - c1.add(5); - EXPECT_EQ(5UL, c1.value()); - g1.add(3); - EXPECT_EQ(8UL, c1.value()); - - Gauge& g2 = store_->gauge("samename2"); - EXPECT_EQ(&g2, &store_->gauge("samename2")); - BoolIndicator& b1 = store_->boolIndicator("samename2"); - EXPECT_EQ(&b1, &store_->boolIndicator("samename2")); - g2.add(1); - EXPECT_EQ(1UL, g2.value()); - EXPECT_TRUE(b1.value()); - b1.set(false); - EXPECT_EQ(0UL, g2.value()); - - store_->shutdownThreading(); - tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(5); + EXPECT_CALL(*alloc_, free(_)).Times(3); } TEST_F(StatsThreadLocalStoreTest, AllocFailed) { @@ -593,21 +521,6 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { Gauge& noop_gauge_2 = store_->gauge("noop_gauge_2"); EXPECT_EQ(&noop_gauge, &noop_gauge_2); - // BoolIndicator - BoolIndicator& noop_bool = store_->boolIndicator("noop_bool"); - EXPECT_EQ(noop_bool.name(), ""); - EXPECT_EQ(0, noop_bool.value()); - noop_bool.set(true); - EXPECT_EQ(0, noop_bool.value()); - noop_bool.set(true); - EXPECT_EQ(0, noop_bool.value()); - noop_bool.set(false); - EXPECT_EQ(0, noop_bool.value()); - noop_bool.set(true); - EXPECT_EQ(0, noop_bool.value()); - BoolIndicator& noop_bool_2 = store_->boolIndicator("noop_bool_2"); - EXPECT_EQ(&noop_bool, &noop_bool_2); - // Histogram Histogram& noop_histogram = store_->histogram("noop_histogram"); EXPECT_EQ(noop_histogram.name(), ""); @@ -625,9 +538,8 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { InSequence s; - // Expected to alloc lowercase_counter, lowercase_gauge, lowercase_bool, - // valid_counter, valid_gauge, valid_bool - EXPECT_CALL(*alloc_, alloc(_)).Times(6); + // Expected to alloc lowercase_counter, lowercase_gauge, valid_counter, valid_gauge + EXPECT_CALL(*alloc_, alloc(_)).Times(4); // Will block all stats containing any capital alphanumeric letter. stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_regex( @@ -639,8 +551,6 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { EXPECT_EQ(lowercase_counter.name(), "lowercase_counter"); Gauge& lowercase_gauge = store_->gauge("lowercase_gauge"); EXPECT_EQ(lowercase_gauge.name(), "lowercase_gauge"); - BoolIndicator& lowercase_bool = store_->boolIndicator("lowercase_bool"); - EXPECT_EQ(lowercase_bool.name(), "lowercase_bool"); Histogram& lowercase_histogram = store_->histogram("lowercase_histogram"); EXPECT_EQ(lowercase_histogram.name(), "lowercase_histogram"); @@ -659,11 +569,6 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { uppercase_gauge.inc(); EXPECT_EQ(uppercase_gauge.value(), 0); - BoolIndicator& uppercase_bool = store_->boolIndicator("uppercase_BOOL"); - EXPECT_EQ(uppercase_bool.name(), ""); - uppercase_bool.set(true); - EXPECT_FALSE(uppercase_bool.value()); - // Histograms are harder to query and test, so we resort to testing that name() returns the empty // string. Histogram& uppercase_histogram = store_->histogram("upperCASE_histogram"); @@ -684,11 +589,11 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { EXPECT_EQ(invalid_counter.value(), 0); // But the old exclusion rule still holds. - Counter& invalid_counter_2 = store_->counter("also_INVLD_counter"); + Counter& invalid_counter_2 = store_->counter("also_INVALID_counter"); invalid_counter_2.inc(); EXPECT_EQ(invalid_counter_2.value(), 0); - // And we expect the same behavior from gauges, histograms, and bools. + // And we expect the same behavior from gauges and histograms. Gauge& valid_gauge = store_->gauge("valid_gauge"); valid_gauge.set(2); EXPECT_EQ(valid_gauge.value(), 2); @@ -697,22 +602,10 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { invalid_gauge_1.inc(); EXPECT_EQ(invalid_gauge_1.value(), 0); - Gauge& invalid_gauge_2 = store_->gauge("also_INVLD_gauge"); + Gauge& invalid_gauge_2 = store_->gauge("also_INVALID_gauge"); invalid_gauge_2.inc(); EXPECT_EQ(invalid_gauge_2.value(), 0); - BoolIndicator& valid_bool = store_->boolIndicator("valid_bool"); - valid_bool.set(true); - EXPECT_EQ(1, valid_bool.value()); - - BoolIndicator& invalid_bool_1 = store_->boolIndicator("invalid_bool"); - invalid_bool_1.set(true); - EXPECT_EQ(0, invalid_gauge_1.value()); - - BoolIndicator& invalid_bool_2 = store_->boolIndicator("also_INVLD_bool"); - invalid_bool_2.set(true); - EXPECT_EQ(0, invalid_bool_2.value()); - Histogram& valid_histogram = store_->histogram("valid_histogram"); EXPECT_EQ(valid_histogram.name(), "valid_histogram"); @@ -722,9 +615,9 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { Histogram& invalid_histogram_2 = store_->histogram("also_INVALID_histogram"); EXPECT_EQ(invalid_histogram_2.name(), ""); - // Expected to free lowercase_counter, lowercase_gauge, lowercase_bool, - // valid_counter, valid_gauge, valid_bool, overflow.stats - EXPECT_CALL(*alloc_, free(_)).Times(7); + // Expected to free lowercase_counter, lowercase_gauge, valid_counter, + // valid_gauge, overflow.stats + EXPECT_CALL(*alloc_, free(_)).Times(5); store_->shutdownThreading(); } @@ -748,15 +641,12 @@ class HeapStatsThreadLocalStoreTest : public StatsThreadLocalStoreTest { TEST_F(HeapStatsThreadLocalStoreTest, RemoveRejectedStats) { Counter& counter = store_->counter("c1"); Gauge& gauge = store_->gauge("g1"); - BoolIndicator& boolIndicator = store_->boolIndicator("b1"); Histogram& histogram = store_->histogram("h1"); ASSERT_EQ(2, store_->counters().size()); // "stats.overflow" and "c1". EXPECT_TRUE(&counter == store_->counters()[0].get() || &counter == store_->counters()[1].get()); // counters() order is non-deterministic. ASSERT_EQ(1, store_->gauges().size()); EXPECT_EQ("g1", store_->gauges()[0]->name()); - ASSERT_EQ(1, store_->boolIndicators().size()); - EXPECT_EQ("b1", store_->boolIndicators()[0]->name()); ASSERT_EQ(1, store_->histograms().size()); EXPECT_EQ("h1", store_->histograms()[0]->name()); @@ -769,13 +659,11 @@ TEST_F(HeapStatsThreadLocalStoreTest, RemoveRejectedStats) { // They can no longer be found. EXPECT_EQ(0, store_->counters().size()); EXPECT_EQ(0, store_->gauges().size()); - EXPECT_EQ(0, store_->boolIndicators().size()); EXPECT_EQ(0, store_->histograms().size()); // However, referencing the previously allocated stats will not crash. counter.inc(); gauge.inc(); - boolIndicator.set(true); EXPECT_CALL(sink_, onHistogramComplete(Ref(histogram), 42)); histogram.recordValue(42); } @@ -845,27 +733,23 @@ TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*alloc_, alloc(_)).Times(6); + EXPECT_CALL(*alloc_, alloc(_)).Times(4); store_->counter("c1"); store_->gauge("g1"); - store_->boolIndicator("b1"); store_->shutdownThreading(); store_->counter("c2"); store_->gauge("g2"); - store_->boolIndicator("b2"); - // c1, g1, b1 should have a thread local ref, but c2, g2, b2 should not. + // c1, g1 should have a thread local ref, but c2, g2 should not. EXPECT_EQ(3L, TestUtility::findCounter(*store_, "c1").use_count()); EXPECT_EQ(3L, TestUtility::findGauge(*store_, "g1").use_count()); - EXPECT_EQ(3L, TestUtility::findBoolIndicator(*store_, "b1").use_count()); EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c2").use_count()); EXPECT_EQ(2L, TestUtility::findGauge(*store_, "g2").use_count()); - EXPECT_EQ(2L, TestUtility::findBoolIndicator(*store_, "b2").use_count()); tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*alloc_, free(_)).Times(7); + EXPECT_CALL(*alloc_, free(_)).Times(5); } TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { @@ -1087,13 +971,6 @@ TEST_F(TruncatingAllocTest, GaugeNotTruncated) { }); } -TEST_F(TruncatingAllocTest, BoolNotTruncated) { - EXPECT_NO_LOGS({ - BoolIndicator& boolIndicator = store_->boolIndicator("simple"); - EXPECT_EQ(&boolIndicator, &store_->boolIndicator("simple")); - }); -} - TEST_F(TruncatingAllocTest, CounterTruncated) { Counter* counter = nullptr; EXPECT_LOG_CONTAINS("warning", "is too long with", { @@ -1112,15 +989,6 @@ TEST_F(TruncatingAllocTest, GaugeTruncated) { EXPECT_NO_LOGS(EXPECT_EQ(gauge, &store_->gauge(long_name_))); } -TEST_F(TruncatingAllocTest, BoolTruncated) { - BoolIndicator* boolIndicator = nullptr; - EXPECT_LOG_CONTAINS("warning", "is too long with", { - BoolIndicator& b = store_->boolIndicator(long_name_); - boolIndicator = &b; - }); - EXPECT_NO_LOGS(EXPECT_EQ(boolIndicator, &store_->boolIndicator(long_name_))); -} - TEST_F(TruncatingAllocTest, HistogramWithLongNameNotTruncated) { EXPECT_NO_LOGS({ Histogram& histogram = store_->histogram(long_name_); diff --git a/test/common/upstream/resource_manager_impl_test.cc b/test/common/upstream/resource_manager_impl_test.cc index 259023a00e577..ae1c7d356d628 100644 --- a/test/common/upstream/resource_manager_impl_test.cc +++ b/test/common/upstream/resource_manager_impl_test.cc @@ -19,14 +19,14 @@ namespace { TEST(ResourceManagerImplTest, RuntimeResourceManager) { NiceMock runtime; - NiceMock bool_indicator; + NiceMock gauge; NiceMock store; - ON_CALL(store, boolIndicator(_)).WillByDefault(ReturnRef(bool_indicator)); + ON_CALL(store, gauge(_)).WillByDefault(ReturnRef(gauge)); ResourceManagerImpl resource_manager( runtime, "circuit_breakers.runtime_resource_manager_test.default.", 0, 0, 0, 1, - ClusterCircuitBreakersStats{ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(POOL_BOOL_INDICATOR(store))}); + ClusterCircuitBreakersStats{ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(POOL_GAUGE(store))}); EXPECT_CALL( runtime.snapshot_, diff --git a/test/integration/overload_integration_test.cc b/test/integration/overload_integration_test.cc index 40f8c23756915..27c9818f4a949 100644 --- a/test/integration/overload_integration_test.cc +++ b/test/integration/overload_integration_test.cc @@ -63,8 +63,7 @@ TEST_P(OverloadIntegrationTest, CloseStreamsWhenOverloaded) { // Put envoy in overloaded state and check that it drops new requests. // Test both header-only and header+body requests since the code paths are slightly different. updateResource(0.9); - test_server_->waitForBoolIndicatorEq( - "overload.envoy.overload_actions.stop_accepting_requests.active", true); + test_server_->waitForGaugeEq("overload.envoy.overload_actions.stop_accepting_requests.active", 1); Http::TestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}}; @@ -88,8 +87,7 @@ TEST_P(OverloadIntegrationTest, CloseStreamsWhenOverloaded) { // Deactivate overload state and check that new requests are accepted. updateResource(0.8); - test_server_->waitForBoolIndicatorEq( - "overload.envoy.overload_actions.stop_accepting_requests.active", false); + test_server_->waitForGaugeEq("overload.envoy.overload_actions.stop_accepting_requests.active", 0); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); response = sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); @@ -111,8 +109,7 @@ TEST_P(OverloadIntegrationTest, DisableKeepaliveWhenOverloaded) { // Put envoy in overloaded state and check that it disables keepalive updateResource(0.8); - test_server_->waitForBoolIndicatorEq( - "overload.envoy.overload_actions.disable_http_keepalive.active", true); + test_server_->waitForGaugeEq("overload.envoy.overload_actions.disable_http_keepalive.active", 1); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); Http::TestHeaderMapImpl request_headers{ @@ -126,8 +123,7 @@ TEST_P(OverloadIntegrationTest, DisableKeepaliveWhenOverloaded) { // Deactivate overload state and check that keepalive is not disabled updateResource(0.7); - test_server_->waitForBoolIndicatorEq( - "overload.envoy.overload_actions.disable_http_keepalive.active", false); + test_server_->waitForGaugeEq("overload.envoy.overload_actions.disable_http_keepalive.active", 0); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); response = sendRequestAndWaitForResponse(request_headers, 1, default_response_headers_, 1); @@ -143,8 +139,8 @@ TEST_P(OverloadIntegrationTest, StopAcceptingConnectionsWhenOverloaded) { // Put envoy in overloaded state and check that it doesn't accept the new client connection. updateResource(0.95); - test_server_->waitForBoolIndicatorEq( - "overload.envoy.overload_actions.stop_accepting_connections.active", true); + test_server_->waitForGaugeEq("overload.envoy.overload_actions.stop_accepting_connections.active", + 1); codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); Http::TestHeaderMapImpl request_headers{ {":method", "GET"}, {":path", "/test/long/url"}, {":scheme", "http"}, {":authority", "host"}}; @@ -155,8 +151,8 @@ TEST_P(OverloadIntegrationTest, StopAcceptingConnectionsWhenOverloaded) { // Reduce load a little to allow the connection to be accepted but then immediately reject the // request. updateResource(0.9); - test_server_->waitForBoolIndicatorEq( - "overload.envoy.overload_actions.stop_accepting_connections.active", false); + test_server_->waitForGaugeEq("overload.envoy.overload_actions.stop_accepting_connections.active", + 0); response->waitForEndStream(); EXPECT_TRUE(response->complete()); diff --git a/test/integration/server.h b/test/integration/server.h index 6648f1f9c6fa3..3652119081244 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -85,11 +85,6 @@ class TestScopeWrapper : public Scope { return wrapped_scope_->gauge(name); } - BoolIndicator& boolIndicator(const std::string& name) override { - Thread::LockGuard lock(lock_); - return wrapped_scope_->boolIndicator(name); - } - Histogram& histogram(const std::string& name) override { Thread::LockGuard lock(lock_); return wrapped_scope_->histogram(name); @@ -124,10 +119,6 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.gauge(name); } - BoolIndicator& boolIndicator(const std::string& name) override { - Thread::LockGuard lock(lock_); - return store_.boolIndicator(name); - } Histogram& histogram(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.histogram(name); @@ -143,10 +134,7 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.gauges(); } - std::vector boolIndicators() const override { - Thread::LockGuard lock(lock_); - return store_.boolIndicators(); - } + std::vector histograms() const override { Thread::LockGuard lock(lock_); return store_.histograms(); @@ -206,12 +194,6 @@ class IntegrationTestServer : public Logger::Loggable, std::function on_server_init_function, bool deterministic, bool defer_listener_finalization); - void waitForBoolIndicatorEq(const std::string& name, uint64_t value) { - while (boolIndicator(name) == nullptr || boolIndicator(name)->value() != value) { - time_system_.sleep(std::chrono::milliseconds(10)); - } - } - void waitForCounterGe(const std::string& name, uint64_t value) override { while (counter(name) == nullptr || counter(name)->value() < value) { time_system_.sleep(std::chrono::milliseconds(10)); @@ -230,12 +212,6 @@ class IntegrationTestServer : public Logger::Loggable, } } - Stats::BoolIndicatorSharedPtr boolIndicator(const std::string& name) { - // When using the thread local store, only boolIndicators() is thread safe. This also allows us - // to test if an indicator exists at all versus just defaulting to false. - return TestUtility::findBoolIndicator(stat_store(), name); - } - Stats::CounterSharedPtr counter(const std::string& name) override { // When using the thread local store, only counters() is thread safe. This also allows us // to test if a counter exists at all versus just defaulting to zero. diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 63a2391a04714..4ee2971236d42 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -32,8 +32,8 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, StatsIntegrationTest, TEST_P(StatsIntegrationTest, WithDefaultConfig) { initialize(); - auto live = test_server_->boolIndicator("server.live"); - EXPECT_TRUE(live->value()); + auto live = test_server_->gauge("server.live"); + EXPECT_EQ(live->value(), 1); EXPECT_EQ(live->tags().size(), 0); auto counter = test_server_->counter("http.config_test.rq_total"); @@ -122,8 +122,8 @@ TEST_P(StatsIntegrationTest, WithTagSpecifierWithFixedValue) { }); initialize(); - auto live = test_server_->boolIndicator("server.live"); - EXPECT_TRUE(live->value()); + auto live = test_server_->gauge("server.live"); + EXPECT_EQ(live->value(), 1); EXPECT_EQ(live->tags().size(), 1); EXPECT_EQ(live->tags()[0].name_, "test.x"); EXPECT_EQ(live->tags()[0].value_, "xxx"); diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index ea078cf4ff7c4..5b1dbf98df98c 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -30,14 +30,6 @@ MockGauge::MockGauge() { } MockGauge::~MockGauge() {} -MockBoolIndicator::MockBoolIndicator() { - ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_)); - ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_)); - ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); - ON_CALL(*this, value()).WillByDefault(ReturnPointee(&value_)); -} -MockBoolIndicator::~MockBoolIndicator() {} - MockHistogram::MockHistogram() { ON_CALL(*this, recordValue(_)).WillByDefault(Invoke([this](uint64_t value) { if (store_ != nullptr) { diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index c4dacdaaee22f..c079e785be16b 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -74,28 +74,6 @@ class MockGauge : public Gauge { std::vector tags_; }; -class MockBoolIndicator : public BoolIndicator { -public: - MockBoolIndicator(); - ~MockBoolIndicator(); - - // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This - // creates a deadlock in gmock and is an unintended use of mock functions. - std::string name() const override { return name_; }; - const char* nameCStr() const override { return name_.c_str(); }; - - MOCK_CONST_METHOD0(tagExtractedName, const std::string&()); - MOCK_CONST_METHOD0(tags, const std::vector&()); - MOCK_METHOD1(set, void(bool value)); - MOCK_CONST_METHOD0(used, bool()); - MOCK_CONST_METHOD0(value, bool()); - - bool used_; - uint64_t value_; - std::string name_; - std::vector tags_; -}; - class MockHistogram : public Histogram { public: MockHistogram(); @@ -151,7 +129,6 @@ class MockSource : public Source { MOCK_METHOD0(cachedCounters, const std::vector&()); MOCK_METHOD0(cachedGauges, const std::vector&()); - MOCK_METHOD0(cachedBoolIndicators, const std::vector&()); MOCK_METHOD0(cachedHistograms, const std::vector&()); MOCK_METHOD0(clearCache, void()); @@ -182,8 +159,6 @@ class MockStore : public Store { MOCK_METHOD1(createScope_, Scope*(const std::string& name)); MOCK_METHOD1(gauge, Gauge&(const std::string&)); MOCK_CONST_METHOD0(gauges, std::vector()); - MOCK_METHOD1(boolIndicator, BoolIndicator&(const std::string&)); - MOCK_CONST_METHOD0(boolIndicators, std::vector()); MOCK_METHOD1(histogram, Histogram&(const std::string& name)); MOCK_CONST_METHOD0(histograms, std::vector()); MOCK_CONST_METHOD0(statsOptions, const StatsOptions&()); diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index 2b7ada09678ef..439103650548f 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -152,8 +152,7 @@ TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { [&](OverloadActionState) { EXPECT_TRUE(false); }); manager->start(); - Stats::BoolIndicator& active_indicator = - stats_.boolIndicator("overload.envoy.overload_actions.dummy_action.active"); + Stats::Gauge& active_gauge = stats_.gauge("overload.envoy.overload_actions.dummy_action.active"); Stats::Gauge& pressure_gauge1 = stats_.gauge("overload.envoy.resource_monitors.fake_resource1.pressure"); Stats::Gauge& pressure_gauge2 = @@ -166,7 +165,7 @@ TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { EXPECT_FALSE(is_active); EXPECT_EQ(action_state, OverloadActionState::Inactive); EXPECT_EQ(0, cb_count); - EXPECT_FALSE(active_indicator.value()); + EXPECT_EQ(0, active_gauge.value()); EXPECT_EQ(50, pressure_gauge1.value()); factory1_.monitor_->setPressure(0.95); @@ -174,7 +173,7 @@ TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { EXPECT_TRUE(is_active); EXPECT_EQ(action_state, OverloadActionState::Active); EXPECT_EQ(1, cb_count); - EXPECT_TRUE(active_indicator.value()); + EXPECT_EQ(1, active_gauge.value()); EXPECT_EQ(95, pressure_gauge1.value()); // Callback should not be invoked if action active state has not changed @@ -200,7 +199,7 @@ TEST_F(OverloadManagerImplTest, CallbackOnlyFiresWhenStateChanges) { EXPECT_FALSE(is_active); EXPECT_EQ(action_state, OverloadActionState::Inactive); EXPECT_EQ(2, cb_count); - EXPECT_FALSE(active_indicator.value()); + EXPECT_EQ(0, active_gauge.value()); EXPECT_EQ(40, pressure_gauge2.value()); manager->stop(); diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 2a901599656e3..263971a611c83 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -148,11 +148,6 @@ Stats::GaugeSharedPtr TestUtility::findGauge(Stats::Store& store, const std::str return findByName(store.gauges(), name); } -Stats::BoolIndicatorSharedPtr TestUtility::findBoolIndicator(Stats::Store& store, - const std::string& name) { - return findByName(store.boolIndicators(), name); -} - std::list TestUtility::makeDnsResponse(const std::list& addresses) { std::list ret; diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 06ec1e60e1a82..a91e68d74176b 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -153,7 +153,7 @@ class TestUtility { * Find a counter in a stats store. * @param store supplies the stats store. * @param name supplies the name to search for. - * @return Stats::CounterSharedPtr the counter, or nullptr if there is none. + * @return Stats::CounterSharedPtr the counter or nullptr if there is none. */ static Stats::CounterSharedPtr findCounter(Stats::Store& store, const std::string& name); @@ -161,19 +161,10 @@ class TestUtility { * Find a gauge in a stats store. * @param store supplies the stats store. * @param name supplies the name to search for. - * @return Stats::GaugeSharedPtr the gauge, or nullptr if there is none. + * @return Stats::GaugeSharedPtr the gauge or nullptr if there is none. */ static Stats::GaugeSharedPtr findGauge(Stats::Store& store, const std::string& name); - /** - * Find a bool in a stats store. - * @param store supplies the stats store. - * @param name supplies the name to search for. - * @return Stats::BoolIndicatorSharedPtr the bool, or nullptr if there is none. - */ - static Stats::BoolIndicatorSharedPtr findBoolIndicator(Stats::Store& store, - const std::string& name); - /** * Convert a string list of IP addresses into a list of network addresses usable for DNS * response testing.