diff --git a/api/envoy/config/bootstrap/v3/bootstrap.proto b/api/envoy/config/bootstrap/v3/bootstrap.proto index bf65f3df45c47..28b1eba6680ef 100644 --- a/api/envoy/config/bootstrap/v3/bootstrap.proto +++ b/api/envoy/config/bootstrap/v3/bootstrap.proto @@ -41,7 +41,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // ` for more detail. // Bootstrap :ref:`configuration overview `. -// [#next-free-field: 42] +// [#next-free-field: 43] message Bootstrap { option (udpa.annotations.versioning).previous_message_type = "envoy.config.bootstrap.v2.Bootstrap"; @@ -230,6 +230,14 @@ message Bootstrap { bool stats_flush_on_admin = 29 [(validate.rules).bool = {const: true}]; } + oneof stats_eviction { + // Optional duration to perform metric eviction. At every interval, during the stats flush + // the unused metrics are removed from the worker caches and the used metrics + // are marked as unused. Must be a multiple of the ``stats_flush_interval``. + google.protobuf.Duration stats_eviction_interval = 42 + [(validate.rules).duration = {gte {nanos: 1000000}}]; + } + // Optional watchdog configuration. // This is for a single watchdog configuration for the entire system. // Deprecated in favor of ``watchdogs`` which has finer granularity. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 65d03b41dc06c..04fe673be0e92 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -79,6 +79,12 @@ removed_config_or_runtime: Removed runtime guard ``envoy.reloadable_features.proxy_104`` and legacy code paths. new_features: +- area: stats + change: | + Added support to remove unused metrics from memory for extensions that + support evictable metrics. This is done :ref:`periodically + ` + during the metric flush. - area: quic change: | Added new option to support :ref:`base64 encoded server ID diff --git a/envoy/server/configuration.h b/envoy/server/configuration.h index 4c18b49054270..9da04d1ac7012 100644 --- a/envoy/server/configuration.h +++ b/envoy/server/configuration.h @@ -89,6 +89,11 @@ class StatsConfig { * @return true if deferred creation of stats is enabled. */ virtual bool enableDeferredCreationStats() const PURE; + + /** + * @return uint32_t a multiple of the flush interval to perform stats eviction, or 0 if disabled. + */ + virtual uint32_t evictOnFlush() const PURE; }; /** diff --git a/envoy/stats/scope.h b/envoy/stats/scope.h index 1116281d93026..e76582b34c430 100644 --- a/envoy/stats/scope.h +++ b/envoy/stats/scope.h @@ -71,8 +71,10 @@ class Scope : public std::enable_shared_from_this { * See also scopeFromStatName, which is preferred. * * @param name supplies the scope's namespace prefix. + * @param evictable whether unused metrics can be deleted from the scope caches. This requires + * that the metrics are not stored by reference. */ - virtual ScopeSharedPtr createScope(const std::string& name) PURE; + virtual ScopeSharedPtr createScope(const std::string& name, bool evictable = false) PURE; /** * Allocate a new scope. NOTE: The implementation should correctly handle overlapping scopes @@ -80,8 +82,10 @@ class Scope : public std::enable_shared_from_this { * gracefully swapped in while an old scope with the same name is being destroyed. * * @param name supplies the scope's namespace prefix. + * @param evictable whether unused metrics can be deleted from the scope caches. This requires + * that the metrics are not stored by reference. */ - virtual ScopeSharedPtr scopeFromStatName(StatName name) PURE; + virtual ScopeSharedPtr scopeFromStatName(StatName name, bool evictable = false) PURE; /** * Creates a Counter from the stat name. Tag extraction will be performed on the name. diff --git a/envoy/stats/stats.h b/envoy/stats/stats.h index 5d5b1f58c80cb..a8307baf836dd 100644 --- a/envoy/stats/stats.h +++ b/envoy/stats/stats.h @@ -93,6 +93,11 @@ class Metric : public RefcountInterface { */ virtual bool used() const PURE; + /** + * Clear any indicator on whether this metric has been updated. + */ + virtual void markUnused() PURE; + /** * Indicates whether this metric is hidden. */ diff --git a/envoy/stats/store.h b/envoy/stats/store.h index ff1c6a08c53e0..306cef4c5a9b0 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -117,6 +117,11 @@ class Store { virtual void forEachHistogram(SizeFn f_size, StatFn f_stat) const PURE; virtual void forEachScope(SizeFn f_size, StatFn f_stat) const PURE; + /** + * Delete unused metrics from all the evictable scope caches, and mark the rest as unused. + */ + virtual void evictUnused() PURE; + /** * @return a null counter that will ignore increments and always return 0. */ @@ -172,7 +177,9 @@ class Store { /** * @return a scope of the given name. */ - ScopeSharedPtr createScope(const std::string& name) { return rootScope()->createScope(name); } + ScopeSharedPtr createScope(const std::string& name, bool evictable = false) { + return rootScope()->createScope(name, evictable); + } /** * Extracts tags from the name and appends them to the provided StatNameTagVector. diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index c8c6905f4f481..963279a96ca9e 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -85,6 +85,7 @@ template class StatsSharedImpl : public MetricImpl // Metric SymbolTable& symbolTable() final { return alloc_.symbolTable(); } bool used() const override { return flags_ & Metric::Flags::Used; } + void markUnused() override { flags_ &= ~Metric::Flags::Used; } bool hidden() const override { return flags_ & Metric::Flags::Hidden; } // RefcountInterface diff --git a/source/common/stats/histogram_impl.h b/source/common/stats/histogram_impl.h index 3b30f1b289df7..80718294b7856 100644 --- a/source/common/stats/histogram_impl.h +++ b/source/common/stats/histogram_impl.h @@ -111,6 +111,7 @@ class HistogramImpl : public HistogramImplHelper { void recordValue(uint64_t value) override { parent_.deliverHistogramToSinks(*this, value); } bool used() const override { return true; } + void markUnused() override {} bool hidden() const override { return false; } SymbolTable& symbolTable() final { return parent_.symbolTable(); } @@ -132,6 +133,7 @@ class NullHistogramImpl : public HistogramImplHelper { ~NullHistogramImpl() override { MetricImpl::clear(symbol_table_); } bool used() const override { return false; } + void markUnused() override {} bool hidden() const override { return false; } SymbolTable& symbolTable() override { return symbol_table_; } diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index 9b4097971eef5..d070a996a1f39 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -63,12 +63,12 @@ ConstScopeSharedPtr IsolatedStoreImpl::constRootScope() const { IsolatedStoreImpl::~IsolatedStoreImpl() = default; -ScopeSharedPtr IsolatedScopeImpl::createScope(const std::string& name) { +ScopeSharedPtr IsolatedScopeImpl::createScope(const std::string& name, bool) { StatNameManagedStorage stat_name_storage(Utility::sanitizeStatsName(name), symbolTable()); - return scopeFromStatName(stat_name_storage.statName()); + return scopeFromStatName(stat_name_storage.statName(), false); } -ScopeSharedPtr IsolatedScopeImpl::scopeFromStatName(StatName name) { +ScopeSharedPtr IsolatedScopeImpl::scopeFromStatName(StatName name, bool) { SymbolTable::StoragePtr prefix_name_storage = symbolTable().join({prefix(), name}); ScopeSharedPtr scope = store_.makeScope(StatName(prefix_name_storage.get())); addScopeToStore(scope); diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index 84ea935102b98..76c3d364bf77b 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -203,6 +203,10 @@ class IsolatedStoreImpl : public Store { } } + void evictUnused() override { + // Do nothing. Eviction is only supported on the thread local stores. + } + void forEachSinkedCounter(SizeFn f_size, StatFn f_stat) const override { forEachCounter(f_size, f_stat); } @@ -295,8 +299,8 @@ class IsolatedScopeImpl : public Scope { StatNameTagVectorOptConstRef tags) override { return store_.counters_.get(prefix(), name, tags, symbolTable()); } - ScopeSharedPtr createScope(const std::string& name) override; - ScopeSharedPtr scopeFromStatName(StatName name) override; + ScopeSharedPtr createScope(const std::string& name, bool evictable) override; + ScopeSharedPtr scopeFromStatName(StatName name, bool evictable) override; Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags, Gauge::ImportMode import_mode) override { Gauge& gauge = store_.gauges_.get(prefix(), name, tags, symbolTable(), import_mode); diff --git a/source/common/stats/null_counter.h b/source/common/stats/null_counter.h index ca576310f0d79..f8b755c58c57d 100644 --- a/source/common/stats/null_counter.h +++ b/source/common/stats/null_counter.h @@ -31,6 +31,7 @@ class NullCounterImpl : public MetricImpl { // Metric bool used() const override { return false; } + void markUnused() override {} bool hidden() const override { return false; } SymbolTable& symbolTable() override { return symbol_table_; } diff --git a/source/common/stats/null_gauge.h b/source/common/stats/null_gauge.h index 5af09aa5999ee..642950de18d88 100644 --- a/source/common/stats/null_gauge.h +++ b/source/common/stats/null_gauge.h @@ -35,6 +35,7 @@ class NullGaugeImpl : public MetricImpl { // Metric bool used() const override { return false; } + void markUnused() override {} bool hidden() const override { return false; } SymbolTable& symbolTable() override { return symbol_table_; } diff --git a/source/common/stats/null_text_readout.h b/source/common/stats/null_text_readout.h index 3073fa8182b48..40a155b7ee66c 100644 --- a/source/common/stats/null_text_readout.h +++ b/source/common/stats/null_text_readout.h @@ -28,6 +28,7 @@ class NullTextReadoutImpl : public MetricImpl { // Metric bool used() const override { return false; } + void markUnused() override {} bool hidden() const override { return false; } SymbolTable& symbolTable() override { return symbol_table_; } diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 175d7d473e3cb..6bc34f71a086b 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -38,7 +38,7 @@ ThreadLocalStoreImpl::ThreadLocalStoreImpl(Allocator& alloc) well_known_tags_->rememberBuiltin(desc.name_); } StatNameManagedStorage empty("", alloc.symbolTable()); - auto new_scope = std::make_shared(*this, StatName(empty.statName())); + auto new_scope = std::make_shared(*this, StatName(empty.statName()), false); addScope(new_scope); default_scope_ = new_scope; } @@ -154,14 +154,15 @@ std::vector ThreadLocalStoreImpl::counters() const { return ret; } -ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::createScope(const std::string& name) { +ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::createScope(const std::string& name, + bool evictable) { StatNameManagedStorage stat_name_storage(Utility::sanitizeStatsName(name), symbolTable()); - return scopeFromStatName(stat_name_storage.statName()); + return scopeFromStatName(stat_name_storage.statName(), evictable); } -ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::scopeFromStatName(StatName name) { +ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::scopeFromStatName(StatName name, bool evictable) { SymbolTable::StoragePtr joined = symbolTable().join({prefix_.statName(), name}); - auto new_scope = std::make_shared(parent_, StatName(joined.get())); + auto new_scope = std::make_shared(parent_, StatName(joined.get()), evictable); parent_.addScope(new_scope); return new_scope; } @@ -394,8 +395,9 @@ void ThreadLocalStoreImpl::clearHistogramsFromCaches() { } } -ThreadLocalStoreImpl::ScopeImpl::ScopeImpl(ThreadLocalStoreImpl& parent, StatName prefix) - : scope_id_(parent.next_scope_id_++), parent_(parent), +ThreadLocalStoreImpl::ScopeImpl::ScopeImpl(ThreadLocalStoreImpl& parent, StatName prefix, + bool evictable) + : scope_id_(parent.next_scope_id_++), parent_(parent), evictable_(evictable), prefix_(prefix, parent.alloc_.symbolTable()), central_cache_(new CentralCacheEntry(parent.alloc_.symbolTable())) {} @@ -910,6 +912,14 @@ bool ParentHistogramImpl::used() const { return merged_; } +void ParentHistogramImpl::markUnused() { + merged_ = false; + Thread::LockGuard lock(merge_lock_); + for (const TlsHistogramSharedPtr& tls_histogram : tls_histograms_) { + tls_histogram->markUnused(); + } +} + bool ParentHistogramImpl::hidden() const { return false; } void ParentHistogramImpl::merge() { @@ -1030,6 +1040,109 @@ void ThreadLocalStoreImpl::forEachScope(std::function f_size, } } +namespace { +struct MetricBag { + explicit MetricBag(uint64_t scope_id) : scope_id_(scope_id) {} + const uint64_t scope_id_; + StatNameHashMap counters_; + StatNameHashMap gauges_; + StatNameHashMap histograms_; + StatNameHashMap text_readouts_; + bool empty() const { + return counters_.empty() && gauges_.empty() && histograms_.empty() && text_readouts_.empty(); + } +}; + +} // namespace + +void ThreadLocalStoreImpl::evictUnused() { + ASSERT_IS_MAIN_OR_TEST_THREAD(); + + // If we are shutting down, we no longer perform eviction as workers may be shutting down + // and not able to complete their work. + if (shutting_down_ || !tls_cache_) { + return; + } + + auto evicted_metrics = std::make_shared>(); + { + Thread::LockGuard lock(lock_); + iterateScopesLockHeld([evicted_metrics](const ScopeImplSharedPtr& scope) -> bool { + if (scope->evictable_) { + MetricBag metrics(scope->scope_id_); + CentralCacheEntrySharedPtr& central_cache = scope->centralCacheMutableNoThreadAnalysis(); + auto filter_unused = [](StatNameHashMap& unused_metrics) { + return [&unused_metrics](std::pair kv) { + const auto& [name, metric] = kv; + if (metric->used()) { + metric->markUnused(); + return false; + } else { + unused_metrics.try_emplace(name, metric); + return true; + } + }; + }; + absl::erase_if(central_cache->counters_, filter_unused(metrics.counters_)); + absl::erase_if(central_cache->gauges_, filter_unused(metrics.gauges_)); + absl::erase_if(central_cache->text_readouts_, filter_unused(metrics.text_readouts_)); + absl::erase_if(central_cache->histograms_, filter_unused(metrics.histograms_)); + if (!metrics.empty()) { + evicted_metrics->push_back(std::move(metrics)); + } + } + return true; + }); + } + + // At this point, central caches no longer return the evicted stats, but we + // need to keep the storage for the evicted stats until after the thread + // local caches are cleared. + if (!evicted_metrics->empty()) { + tls_cache_->runOnAllThreads( + [evicted_metrics](OptRef tls_cache) { + for (const auto& metrics : *evicted_metrics) { + TlsCacheEntry& entry = tls_cache->insertScope(metrics.scope_id_); + absl::erase_if(entry.counters_, + [&](std::pair> kv) { + return metrics.counters_.contains(kv.first); + }); + absl::erase_if(entry.gauges_, + [&](std::pair> kv) { + return metrics.gauges_.contains(kv.first); + }); + absl::erase_if(entry.text_readouts_, + [&](std::pair> kv) { + return metrics.text_readouts_.contains(kv.first); + }); + absl::erase_if(entry.parent_histograms_, + [&](std::pair kv) { + return metrics.histograms_.contains(kv.first); + }); + } + }, + [evicted_metrics]() { + // We want to delete stale stats on the main thread since stat + // destructors lock the stats allocator. Note that we might have + // received fresh values on the stale cache-local stats after deleting them from the + // central cache.. Eventually, we might also want to defer the deletion further in the + // allocator until the values are flushed to the sinks. + size_t scopes = 0, counters = 0, gauges = 0, readouts = 0, histograms = 0; + for (const auto& metrics : *evicted_metrics) { + scopes += 1; + counters += metrics.counters_.size(); + gauges += metrics.gauges_.size(); + readouts += metrics.text_readouts_.size(); + histograms += metrics.histograms_.size(); + } + ENVOY_LOG(debug, + "deleted stale {} counters, {} gauges, {} text readouts, {} histograms from " + "{} scopes", + counters, gauges, readouts, histograms, scopes); + }); + } +} + bool ThreadLocalStoreImpl::iterateScopesLockHeld( const std::function fn) const ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) { diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index b2d7f54990ea7..8a598b82713ce 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -60,6 +60,7 @@ class ThreadLocalHistogramImpl : public HistogramImplHelper { // Stats::Metric SymbolTable& symbolTable() final { return symbol_table_; } bool used() const override { return used_; } + void markUnused() override { used_ = false; } bool hidden() const override { return false; } private: @@ -116,6 +117,7 @@ class ParentHistogramImpl : public MetricImpl { // Stats::Metric SymbolTable& symbolTable() override; bool used() const override; + void markUnused() override; bool hidden() const override; // RefcountInterface @@ -184,6 +186,8 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void forEachHistogram(SizeFn f_size, StatFn f_stat) const override; void forEachScope(SizeFn f_size, StatFn f_stat) const override; + void evictUnused() override; + // Stats::StoreRoot void addSink(Sink& sink) override { timer_sinks_.push_back(sink); } void setTagProducer(TagProducerPtr&& tag_producer) override { @@ -277,7 +281,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo using CentralCacheEntrySharedPtr = RefcountPtr; struct ScopeImpl : public Scope { - ScopeImpl(ThreadLocalStoreImpl& parent, StatName prefix); + ScopeImpl(ThreadLocalStoreImpl& parent, StatName prefix, bool evictable); ~ScopeImpl() override; // Stats::Scope @@ -290,8 +294,8 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Histogram::Unit unit) override; TextReadout& textReadoutFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags) override; - ScopeSharedPtr createScope(const std::string& name) override; - ScopeSharedPtr scopeFromStatName(StatName name) override; + ScopeSharedPtr createScope(const std::string& name, bool evictale) override; + ScopeSharedPtr scopeFromStatName(StatName name, bool evictable) override; const SymbolTable& constSymbolTable() const final { return parent_.constSymbolTable(); } SymbolTable& symbolTable() final { return parent_.symbolTable(); } @@ -429,6 +433,11 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo return central_cache_; } + CentralCacheEntrySharedPtr& + centralCacheMutableNoThreadAnalysis() const ABSL_NO_THREAD_SAFETY_ANALYSIS { + return central_cache_; + } + // Returns the central cache, bypassing thread analysis. // // This is used only when passing references to maps held in the central @@ -441,6 +450,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo const uint64_t scope_id_; ThreadLocalStoreImpl& parent_; + const bool evictable_{}; private: StatNameStorage prefix_; diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index a1c9a438604cd..f341803f78c4e 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -99,6 +99,14 @@ StatsConfigImpl::StatsConfigImpl(const envoy::config::bootstrap::v3::Bootstrap& if (bootstrap.stats_flush_case() == envoy::config::bootstrap::v3::Bootstrap::kStatsFlushOnAdmin) { flush_on_admin_ = bootstrap.stats_flush_on_admin(); } + + const auto evict_interval_ms = PROTOBUF_GET_MS_OR_DEFAULT(bootstrap, stats_eviction_interval, 0); + if (evict_interval_ms % flush_interval_.count() != 0) { + status = absl::InvalidArgumentError( + "stats_eviction_interval must be a multiple of stats_flush_interval"); + return; + } + evict_on_flush_ = evict_interval_ms / flush_interval_.count(); } absl::Status MainImpl::initialize(const envoy::config::bootstrap::v3::Bootstrap& bootstrap, diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 2165239456f1b..932ffab806b24 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -56,6 +56,7 @@ class StatsConfigImpl : public StatsConfig { const std::list& sinks() const override { return sinks_; } std::chrono::milliseconds flushInterval() const override { return flush_interval_; } bool flushOnAdmin() const override { return flush_on_admin_; } + uint32_t evictOnFlush() const override { return evict_on_flush_; } void addSink(Stats::SinkPtr sink) { sinks_.emplace_back(std::move(sink)); } bool enableDeferredCreationStats() const override { @@ -67,6 +68,7 @@ class StatsConfigImpl : public StatsConfig { std::chrono::milliseconds flush_interval_; bool flush_on_admin_{false}; const envoy::config::bootstrap::v3::Bootstrap::DeferredStatOptions deferred_stat_options_; + uint32_t evict_on_flush_{0}; }; /** diff --git a/source/server/server.cc b/source/server/server.cc index 24e233bf35da3..cd89ef64c6a9c 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -292,6 +292,12 @@ void InstanceBase::flushStatsInternal() { auto& stats_config = config_.statsConfig(); InstanceUtil::flushMetricsToSinks(stats_config.sinks(), stats_store_, clusterManager(), timeSource()); + if (const auto evict_on_flush = stats_config.evictOnFlush(); evict_on_flush > 0) { + stats_eviction_counter_ = (stats_eviction_counter_ + 1) % evict_on_flush; + if (stats_eviction_counter_ == 0) { + stats_store_.evictUnused(); + } + } // TODO(ramaraochavali): consider adding different flush interval for histograms. if (stat_flush_timer_ != nullptr) { stat_flush_timer_->enableTimer(stats_config.flushInterval()); diff --git a/source/server/server.h b/source/server/server.h index 88ffac5cdd404..b59fce228f034 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -453,6 +453,8 @@ class InstanceBase : Logger::Loggable, : RaiiListElement(callbacks, callback) {} }; + uint32_t stats_eviction_counter_{0}; + #ifdef ENVOY_PERFETTO std::unique_ptr tracing_session_{}; os_fd_t tracing_fd_{INVALID_HANDLE}; @@ -468,6 +470,8 @@ class InstanceBase : Logger::Loggable, // copying and probably be a cleaner API in general. class MetricSnapshotImpl : public Stats::MetricSnapshot { public: + // MetricSnapshotImpl captures a snapshot of metrics by latching the delta usage, and optionally + // marking the stats as used. explicit MetricSnapshotImpl(Stats::Store& store, Upstream::ClusterManager& cluster_manager, TimeSource& time_source); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index e21ad17d3b4f0..b689fce5aa0e8 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -606,6 +606,86 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { tls_.shutdownThread(); } +TEST_F(StatsThreadLocalStoreTest, Eviction) { + InSequence s; + store_->initializeThreading(main_thread_dispatcher_, tls_); + + ScopeSharedPtr scope = store_->createScope("scope.", true); + ScopeSharedPtr scope1 = store_->createScope("scope.", true); + // References will become invalid, so we create a lexical scope. + { + Counter& c1 = scope->counterFromString("c1"); + EXPECT_EQ(&c1, &scope1->counterFromString("c1")); + c1.add(1); + EXPECT_TRUE(c1.used()); + + Gauge& g1 = scope->gaugeFromString("g1", Gauge::ImportMode::Accumulate); + g1.set(5); + EXPECT_TRUE(g1.used()); + + TextReadout& t1 = scope->textReadoutFromString("t1"); + t1.set("hello"); + EXPECT_TRUE(t1.used()); + + Histogram& h1 = scope->histogramFromString("h1", Histogram::Unit::Unspecified); + EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 1)); + h1.recordValue(1); + store_->mergeHistograms([]() -> void {}); + + // Eviction only marks unused but does not remove the counters. + store_->evictUnused(); + + EXPECT_EQ(&c1, &scope->counterFromString("c1")); + EXPECT_FALSE(c1.used()); + EXPECT_EQ(1, c1.value()); + EXPECT_EQ(1UL, store_->counters().size()); + + EXPECT_EQ(&g1, &scope->gaugeFromString("g1", Gauge::ImportMode::Accumulate)); + EXPECT_EQ(&g1, &scope1->gaugeFromString("g1", Gauge::ImportMode::Accumulate)); + EXPECT_FALSE(g1.used()); + EXPECT_EQ(5, g1.value()); + EXPECT_EQ(1UL, store_->gauges().size()); + + EXPECT_EQ(&t1, &scope->textReadoutFromString("t1")); + EXPECT_EQ(&t1, &scope1->textReadoutFromString("t1")); + EXPECT_FALSE(t1.used()); + EXPECT_EQ("hello", t1.value()); + EXPECT_EQ(1UL, store_->textReadouts().size()); + + EXPECT_EQ(&h1, &scope->histogramFromString("h1", Histogram::Unit::Unspecified)); + EXPECT_EQ(&h1, &scope1->histogramFromString("h1", Histogram::Unit::Unspecified)); + EXPECT_FALSE(h1.used()); + EXPECT_EQ(1UL, store_->histograms().size()); + } + + // Eviction removes here. + EXPECT_CALL(tls_, runOnAllThreads(_, _)).Times(testing::AtLeast(1)); + store_->evictUnused(); + EXPECT_EQ(0UL, store_->counters().size()); + EXPECT_EQ(0UL, store_->gauges().size()); + EXPECT_EQ(0UL, store_->textReadouts().size()); + EXPECT_EQ(0UL, store_->histograms().size()); + + // Make sure no dangling data is on caches and it is safe to use the same metrics. + { + scope->counterFromString("c1").add(1); + scope1->counterFromString("c1").add(1); + scope->gaugeFromString("g1", Gauge::ImportMode::Accumulate).set(5); + scope1->gaugeFromString("g1", Gauge::ImportMode::Accumulate).set(5); + scope->textReadoutFromString("t1").set("hello"); + scope1->textReadoutFromString("t1").set("hello"); + Histogram& h1 = scope->histogramFromString("h1", Histogram::Unit::Unspecified); + EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 1)); + h1.recordValue(1); + Histogram& h2 = scope1->histogramFromString("h1", Histogram::Unit::Unspecified); + EXPECT_EQ(&h1, &h2); + } + + tls_.shutdownGlobalThreading(); + store_->shutdownThreading(); + tls_.shutdownThread(); +} + TEST_F(StatsThreadLocalStoreTest, NestedScopes) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); diff --git a/test/integration/server.h b/test/integration/server.h index 335d55fa4431b..a78a8b4eb4ef2 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -77,12 +77,12 @@ class TestScopeWrapper : public Scope { TestScopeWrapper(Thread::MutexBasicLockable& lock, ScopeSharedPtr wrapped_scope, Store& store) : lock_(lock), wrapped_scope_(wrapped_scope), store_(store) {} - ScopeSharedPtr createScope(const std::string& name) override { + ScopeSharedPtr createScope(const std::string& name, bool) override { Thread::LockGuard lock(lock_); return std::make_shared(lock_, wrapped_scope_->createScope(name), store_); } - ScopeSharedPtr scopeFromStatName(StatName name) override { + ScopeSharedPtr scopeFromStatName(StatName name, bool) override { Thread::LockGuard lock(lock_); return std::make_shared(lock_, wrapped_scope_->scopeFromStatName(name), store_); @@ -196,6 +196,7 @@ class NotifyingCounter : public Stats::Counter { uint32_t use_count() const override { return counter_->use_count(); } StatName tagExtractedStatName() const override { return counter_->tagExtractedStatName(); } bool used() const override { return counter_->used(); } + void markUnused() override { counter_->markUnused(); } bool hidden() const override { return counter_->hidden(); } SymbolTable& symbolTable() override { return counter_->symbolTable(); } const SymbolTable& constSymbolTable() const override { return counter_->constSymbolTable(); } @@ -365,6 +366,16 @@ class TestIsolatedStoreImpl : public StoreRoot { void extractAndAppendTags(absl::string_view, StatNamePool&, StatNameTagVector&) override {}; const Stats::TagVector& fixedTags() override { CONSTRUCT_ON_FIRST_USE(Stats::TagVector); } + void evictUnused() override { + Thread::LockGuard lock(lock_); + eviction_count_++; + } + + uint32_t evictionCount() const { + Thread::LockGuard lock(lock_); + return eviction_count_; + } + // Stats::StoreRoot void addSink(Sink&) override {} void setTagProducer(TagProducerPtr&&) override {} @@ -381,6 +392,7 @@ class TestIsolatedStoreImpl : public StoreRoot { IsolatedStoreImpl store_; PostMergeCb merge_cb_; ScopeSharedPtr lazy_default_scope_; + uint32_t eviction_count_{0}; }; } // namespace Stats diff --git a/test/mocks/server/server_factory_context.h b/test/mocks/server/server_factory_context.h index 7221103be6359..059a50aaa9032 100644 --- a/test/mocks/server/server_factory_context.h +++ b/test/mocks/server/server_factory_context.h @@ -51,6 +51,7 @@ class MockStatsConfig : public virtual StatsConfig { MOCK_METHOD(bool, flushOnAdmin, (), (const)); MOCK_METHOD(const Stats::SinkPredicates*, sinkPredicates, (), (const)); MOCK_METHOD(bool, enableDeferredCreationStats, (), (const)); + MOCK_METHOD(uint32_t, evictOnFlush, (), (const)); }; class MockServerFactoryContext : public virtual ServerFactoryContext { diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 2b0d25435ca9c..e476b6bf75776 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -142,6 +142,7 @@ class MockCounter : public MockStatWithRefcount { MOCK_METHOD(uint64_t, latch, ()); MOCK_METHOD(void, reset, ()); MOCK_METHOD(bool, used, (), (const)); + MOCK_METHOD(void, markUnused, ()); MOCK_METHOD(bool, hidden, (), (const)); MOCK_METHOD(uint64_t, value, (), (const)); @@ -164,6 +165,7 @@ class MockGauge : public MockStatWithRefcount { MOCK_METHOD(void, sub, (uint64_t amount)); MOCK_METHOD(void, mergeImportMode, (ImportMode)); MOCK_METHOD(bool, used, (), (const)); + MOCK_METHOD(void, markUnused, ()); MOCK_METHOD(bool, hidden, (), (const)); MOCK_METHOD(uint64_t, value, (), (const)); MOCK_METHOD(absl::optional, cachedShouldImport, (), (const)); @@ -181,6 +183,7 @@ class MockHistogram : public MockMetric { ~MockHistogram() override; MOCK_METHOD(bool, used, (), (const)); + MOCK_METHOD(void, markUnused, ()); MOCK_METHOD(bool, hidden, (), (const)); MOCK_METHOD(Histogram::Unit, unit, (), (const)); MOCK_METHOD(void, recordValue, (uint64_t value)); @@ -206,6 +209,7 @@ class MockParentHistogram : public MockMetric { std::string quantileSummary() const override { return ""; }; std::string bucketSummary() const override { return ""; }; MOCK_METHOD(bool, used, (), (const)); + MOCK_METHOD(void, markUnused, ()); MOCK_METHOD(bool, hidden, (), (const)); MOCK_METHOD(Histogram::Unit, unit, (), (const)); MOCK_METHOD(void, recordValue, (uint64_t value)); @@ -292,10 +296,10 @@ class MockScope : public TestUtil::TestScope { public: MockScope(StatName prefix, MockStore& store); - ScopeSharedPtr createScope(const std::string& name) override { + ScopeSharedPtr createScope(const std::string& name, bool) override { return ScopeSharedPtr(createScope_(name)); } - ScopeSharedPtr scopeFromStatName(StatName name) override { + ScopeSharedPtr scopeFromStatName(StatName name, bool) override { return createScope_(symbolTable().toString(name)); } diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index 5ce86dc719cea..c916ebd031643 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -94,6 +94,7 @@ TEST_F(ConfigurationImplTest, DefaultStatsFlushInterval) { EXPECT_EQ(std::chrono::milliseconds(5000), config.statsConfig().flushInterval()); EXPECT_FALSE(config.statsConfig().flushOnAdmin()); + EXPECT_EQ(0, config.statsConfig().evictOnFlush()); } TEST_F(ConfigurationImplTest, CustomStatsFlushInterval) { @@ -224,6 +225,34 @@ TEST_F(ConfigurationImplTest, IntervalAndAdminFlush) { "Only one of stats_flush_interval or stats_flush_on_admin should be set!"); } +TEST_F(ConfigurationImplTest, Eviction) { + std::string json = R"EOF( + { + "stats_flush_interval": "0.500s", + "stats_eviction_interval": "1.5s" + } + )EOF"; + + auto bootstrap = Upstream::parseBootstrapFromV3Json(json); + MainImpl config; + EXPECT_TRUE(config.initialize(bootstrap, server_, cluster_manager_factory_).ok()); + EXPECT_EQ(3, config.statsConfig().evictOnFlush()); +} + +TEST_F(ConfigurationImplTest, EvictionNotMultiple) { + std::string json = R"EOF( + { + "stats_flush_interval": "0.500s", + "stats_eviction_interval": "0.750s" + } + )EOF"; + + auto bootstrap = Upstream::parseBootstrapFromV3Json(json); + MainImpl config; + EXPECT_THAT(config.initialize(bootstrap, server_, cluster_manager_factory_).message(), + testing::HasSubstr("must be a multiple")); +} + TEST_F(ConfigurationImplTest, SetUpstreamClusterPerConnectionBufferLimit) { const std::string json = R"EOF( { diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 9b69532160bc4..d90b608ccdd8d 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -964,6 +964,38 @@ TEST_P(ServerInstanceImplTest, FlushStatsOnAdmin) { server_thread->join(); } +TEST_P(ServerInstanceImplTest, EvictStats) { + CustomStatsSinkFactory factory; + Registry::InjectFactory registered(factory); + auto server_thread = + startTestServer("test/server/test_data/server/stats_evict_bootstrap.yaml", true); + EXPECT_EQ(2, server_->statsConfig().evictOnFlush()); + EXPECT_EQ(std::chrono::seconds(5), server_->statsConfig().flushInterval()); + + auto counter = TestUtility::findCounter(stats_store_, "stats.flushed"); + + time_system_.advanceTimeWait(std::chrono::seconds(6)); + EXPECT_EQ(1L, counter->value()); + EXPECT_EQ(0, stats_store_.evictionCount()); + + // Eviction applied here: side-effect is that c1 is now marked as unused. + time_system_.advanceTimeWait(std::chrono::seconds(6)); + EXPECT_EQ(2L, counter->value()); + EXPECT_EQ(1, stats_store_.evictionCount()); + + time_system_.advanceTimeWait(std::chrono::seconds(6)); + EXPECT_EQ(3L, counter->value()); + EXPECT_EQ(1, stats_store_.evictionCount()); + + // Second pass of eviction deletes the counter. + time_system_.advanceTimeWait(std::chrono::seconds(6)); + EXPECT_EQ(4L, counter->value()); + EXPECT_EQ(2, stats_store_.evictionCount()); + + server_->dispatcher().post([&] { server_->shutdown(); }); + server_thread->join(); +} + TEST_P(ServerInstanceImplTest, ConcurrentFlushes) { CustomStatsSinkFactory factory; Registry::InjectFactory registered(factory); diff --git a/test/server/test_data/server/stats_evict_bootstrap.yaml b/test/server/test_data/server/stats_evict_bootstrap.yaml new file mode 100644 index 0000000000000..b295c2c0c471c --- /dev/null +++ b/test/server/test_data/server/stats_evict_bootstrap.yaml @@ -0,0 +1,11 @@ +node: + id: bootstrap_id + cluster: bootstrap_cluster + locality: + zone: bootstrap_zone + sub_zone: bootstrap_sub_zone +stats_sinks: +- name: envoy.custom_stats_sink + typed_config: + "@type": type.googleapis.com/google.protobuf.Struct +stats_eviction_interval: 10s