diff --git a/include/envoy/stats/BUILD b/include/envoy/stats/BUILD index 420ad3883f52a..aeef2282b8233 100644 --- a/include/envoy/stats/BUILD +++ b/include/envoy/stats/BUILD @@ -15,7 +15,6 @@ envoy_cc_library( "histogram.h", "scope.h", "sink.h", - "source.h", "stat_data_allocator.h", "stats.h", "stats_matcher.h", diff --git a/include/envoy/stats/sink.h b/include/envoy/stats/sink.h index 7cb18223ba365..d58ea33220fa6 100644 --- a/include/envoy/stats/sink.h +++ b/include/envoy/stats/sink.h @@ -4,25 +4,51 @@ #include #include "envoy/common/pure.h" +#include "envoy/stats/histogram.h" +#include "envoy/stats/stats.h" namespace Envoy { namespace Stats { class Histogram; -class Source; + +class MetricSnapshot { +public: + struct CounterSnapshot { + uint64_t delta_; + std::reference_wrapper counter_; + }; + + virtual ~MetricSnapshot() = default; + + /** + * @return a snapshot of all counters with pre-latched deltas. + */ + virtual const std::vector& counters() PURE; + + /** + * @return a snapshot of all gauges. + */ + virtual const std::vector>& gauges() PURE; + + /** + * @return a snapshot of all histograms. + */ + virtual const std::vector>& histograms() PURE; +}; /** * A sink for stats. Each sink is responsible for writing stats to a backing store. */ class Sink { public: - virtual ~Sink() {} + virtual ~Sink() = default; /** * Periodic metric flush to the sink. - * @param source interface through which the sink can access all metrics being flushed. + * @param snapshot interface through which the sink can access all metrics being flushed. */ - virtual void flush(Source& source) PURE; + virtual void flush(MetricSnapshot& snapshot) PURE; /** * Flush a single histogram sample. Note: this call is called synchronously as a part of recording @@ -33,7 +59,7 @@ class Sink { virtual void onHistogramComplete(const Histogram& histogram, uint64_t value) PURE; }; -typedef std::unique_ptr SinkPtr; +using SinkPtr = std::unique_ptr; } // namespace Stats } // namespace Envoy diff --git a/include/envoy/stats/source.h b/include/envoy/stats/source.h deleted file mode 100644 index 8dea65487a587..0000000000000 --- a/include/envoy/stats/source.h +++ /dev/null @@ -1,55 +0,0 @@ -#pragma once - -#include -#include -#include -#include -#include -#include -#include - -#include "envoy/stats/histogram.h" -#include "envoy/stats/stats.h" - -namespace Envoy { -namespace Stats { - -/** - * Provides cached access to a particular store's stats. - */ -class Source { -public: - virtual ~Source() {} - - /** - * Returns all known counters. Will use cached values if already accessed and clearCache() hasn't - * been called since. - * @return std::vector& all known counters. Note: reference may not be valid - * after clearCache() is called. - */ - virtual const std::vector& cachedCounters() PURE; - - /** - * Returns all known gauges. Will use cached values if already accessed and clearCache() hasn't - * been called since. - * @return std::vector& all known gauges. Note: reference may not be valid after - * clearCache() is called. - */ - virtual const std::vector& cachedGauges() PURE; - - /** - * Returns all known parent histograms. Will use cached values if already accessed and - * clearCache() hasn't been called since. - * @return std::vector& all known histograms. Note: reference may not be - * valid after clearCache() is called. - */ - virtual const std::vector& cachedHistograms() PURE; - - /** - * Resets the cache so that any future calls to get cached metrics will refresh the set. - */ - virtual void clearCache() PURE; -}; - -} // namespace Stats -} // namespace Envoy diff --git a/include/envoy/stats/store.h b/include/envoy/stats/store.h index cd017f9ad8843..36f827467d8a4 100644 --- a/include/envoy/stats/store.h +++ b/include/envoy/stats/store.h @@ -22,7 +22,6 @@ class Instance; namespace Stats { class Sink; -class Source; /** * A store for all known counters, gauges, and timers. @@ -95,12 +94,6 @@ class StoreRoot : public Store { * method would be asserted. */ virtual void mergeHistograms(PostMergeCb merge_complete_cb) PURE; - - /** - * Returns the Source to provide cached metrics. - * @return Source& the source. - */ - virtual Source& source() PURE; }; typedef std::unique_ptr StoreRootPtr; diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index b766709c0d5b6..de5c79849394a 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -84,18 +84,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "source_impl_lib", - srcs = ["source_impl.cc"], - hdrs = ["source_impl.h"], - external_deps = [ - "abseil_optional", - ], - deps = [ - "//include/envoy/stats:stats_interface", - ], -) - envoy_cc_library( name = "stat_data_allocator_lib", hdrs = ["stat_data_allocator_impl.h"], @@ -122,7 +110,6 @@ envoy_cc_library( deps = [ ":histogram_lib", ":metric_impl_lib", - ":source_impl_lib", ":symbol_table_lib", ":tag_extractor_lib", ":utility_lib", diff --git a/source/common/stats/source_impl.cc b/source/common/stats/source_impl.cc deleted file mode 100644 index e6102d5a608cd..0000000000000 --- a/source/common/stats/source_impl.cc +++ /dev/null @@ -1,34 +0,0 @@ -#include "common/stats/source_impl.h" - -#include - -namespace Envoy { -namespace Stats { - -std::vector& SourceImpl::cachedCounters() { - if (!counters_) { - counters_ = store_.counters(); - } - return *counters_; -} -std::vector& SourceImpl::cachedGauges() { - if (!gauges_) { - gauges_ = store_.gauges(); - } - return *gauges_; -} -std::vector& SourceImpl::cachedHistograms() { - if (!histograms_) { - histograms_ = store_.histograms(); - } - return *histograms_; -} - -void SourceImpl::clearCache() { - counters_.reset(); - gauges_.reset(); - histograms_.reset(); -} - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/stats/source_impl.h b/source/common/stats/source_impl.h deleted file mode 100644 index a3a79ba9a76ab..0000000000000 --- a/source/common/stats/source_impl.h +++ /dev/null @@ -1,30 +0,0 @@ -#pragma once - -#include "envoy/stats/source.h" -#include "envoy/stats/stats.h" -#include "envoy/stats/store.h" - -#include "absl/types/optional.h" - -namespace Envoy { -namespace Stats { - -class SourceImpl : public Source { -public: - SourceImpl(Store& store) : store_(store){}; - - // Stats::Source - std::vector& cachedCounters() override; - std::vector& cachedGauges() override; - std::vector& cachedHistograms() override; - void clearCache() override; - -private: - Store& store_; - absl::optional> counters_; - absl::optional> gauges_; - absl::optional> histograms_; -}; - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 8a31348ca748a..d461e342f941b 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -25,7 +25,7 @@ ThreadLocalStoreImpl::ThreadLocalStoreImpl(StatDataAllocator& alloc) : alloc_(alloc), default_scope_(createScope("")), tag_producer_(std::make_unique()), stats_matcher_(std::make_unique()), heap_allocator_(alloc.symbolTable()), - source_(*this), null_counter_(alloc.symbolTable()), null_gauge_(alloc.symbolTable()), + null_counter_(alloc.symbolTable()), null_gauge_(alloc.symbolTable()), null_histogram_(alloc.symbolTable()) {} ThreadLocalStoreImpl::~ThreadLocalStoreImpl() { diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 8a062b2269574..3f53d23ac24a6 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -11,7 +11,6 @@ #include "common/common/hash.h" #include "common/stats/heap_stat_data.h" #include "common/stats/histogram_impl.h" -#include "common/stats/source_impl.h" #include "common/stats/symbol_table_impl.h" #include "common/stats/utility.h" @@ -211,11 +210,8 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void initializeThreading(Event::Dispatcher& main_thread_dispatcher, ThreadLocal::Instance& tls) override; void shutdownThreading() override; - void mergeHistograms(PostMergeCb mergeCb) override; - Source& source() override { return source_; } - private: template using StatMap = StatNameHashMap; @@ -362,7 +358,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo std::atomic shutting_down_{}; std::atomic merge_in_progress_{}; HeapStatDataAllocator heap_allocator_; - SourceImpl source_; NullCounterImpl null_counter_; NullGaugeImpl null_gauge_; diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index f40a8532ea48e..64d7238bba074 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -48,20 +48,19 @@ UdpStatsdSink::UdpStatsdSink(ThreadLocal::SlotAllocator& tls, }); } -void UdpStatsdSink::flush(Stats::Source& source) { +void UdpStatsdSink::flush(Stats::MetricSnapshot& snapshot) { Writer& writer = tls_->getTyped(); - for (const Stats::CounterSharedPtr& counter : source.cachedCounters()) { - if (counter->used()) { - uint64_t delta = counter->latch(); - writer.write(fmt::format("{}.{}:{}|c{}", prefix_, getName(*counter), delta, - buildTagStr(counter->tags()))); + for (const auto& counter : snapshot.counters()) { + if (counter.counter_.get().used()) { + writer.write(fmt::format("{}.{}:{}|c{}", prefix_, getName(counter.counter_.get()), + counter.delta_, buildTagStr(counter.counter_.get().tags()))); } } - for (const Stats::GaugeSharedPtr& gauge : source.cachedGauges()) { - if (gauge->used()) { - writer.write(fmt::format("{}.{}:{}|g{}", prefix_, getName(*gauge), gauge->value(), - buildTagStr(gauge->tags()))); + for (const auto& gauge : snapshot.gauges()) { + if (gauge.get().used()) { + writer.write(fmt::format("{}.{}:{}|g{}", prefix_, getName(gauge.get()), gauge.get().value(), + buildTagStr(gauge.get().tags()))); } } } @@ -110,18 +109,18 @@ TcpStatsdSink::TcpStatsdSink(const LocalInfo::LocalInfo& local_info, }); } -void TcpStatsdSink::flush(Stats::Source& source) { +void TcpStatsdSink::flush(Stats::MetricSnapshot& snapshot) { TlsSink& tls_sink = tls_->getTyped(); tls_sink.beginFlush(true); - for (const Stats::CounterSharedPtr& counter : source.cachedCounters()) { - if (counter->used()) { - tls_sink.flushCounter(counter->name(), counter->latch()); + for (const auto& counter : snapshot.counters()) { + if (counter.counter_.get().used()) { + tls_sink.flushCounter(counter.counter_.get().name(), counter.delta_); } } - for (const Stats::GaugeSharedPtr& gauge : source.cachedGauges()) { - if (gauge->used()) { - tls_sink.flushGauge(gauge->name(), gauge->value()); + for (const auto& gauge : snapshot.gauges()) { + if (gauge.get().used()) { + tls_sink.flushGauge(gauge.get().name(), gauge.get().value()); } } tls_sink.endFlush(true); diff --git a/source/extensions/stat_sinks/common/statsd/statsd.h b/source/extensions/stat_sinks/common/statsd/statsd.h index d80c4af3552ad..a13cd0a730bcb 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.h +++ b/source/extensions/stat_sinks/common/statsd/statsd.h @@ -5,7 +5,6 @@ #include "envoy/stats/histogram.h" #include "envoy/stats/scope.h" #include "envoy/stats/sink.h" -#include "envoy/stats/source.h" #include "envoy/stats/stats.h" #include "envoy/stats/tag.h" #include "envoy/thread_local/thread_local.h" @@ -58,7 +57,7 @@ class UdpStatsdSink : public Stats::Sink { } // Stats::Sink - void flush(Stats::Source& source) override; + void flush(Stats::MetricSnapshot& snapshot) override; void onHistogramComplete(const Stats::Histogram& histogram, uint64_t value) override; // Called in unit test to validate writer construction and address. @@ -87,7 +86,7 @@ class TcpStatsdSink : public Stats::Sink { Stats::Scope& scope, const std::string& prefix = getDefaultPrefix()); // Stats::Sink - void flush(Stats::Source& source) override; + void flush(Stats::MetricSnapshot& snapshot) override; void onHistogramComplete(const Stats::Histogram& histogram, uint64_t value) override { // For statsd histograms are all timers. tls_->getTyped().onTimespanComplete(histogram.name(), diff --git a/source/extensions/stat_sinks/hystrix/hystrix.cc b/source/extensions/stat_sinks/hystrix/hystrix.cc index ede5d6021c070..c14fcf8150ac9 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.cc +++ b/source/extensions/stat_sinks/hystrix/hystrix.cc @@ -320,7 +320,7 @@ Http::Code HystrixSink::handlerHystrixEventStream(absl::string_view, return Http::Code::OK; } -void HystrixSink::flush(Stats::Source& source) { +void HystrixSink::flush(Stats::MetricSnapshot& snapshot) { if (callbacks_list_.empty()) { return; } @@ -330,9 +330,10 @@ void HystrixSink::flush(Stats::Source& source) { // Save a map of the relevant histograms per cluster in a convenient format. std::unordered_map time_histograms; - for (const Stats::ParentHistogramSharedPtr& histogram : source.cachedHistograms()) { - if (histogram->tagExtractedStatName() == cluster_upstream_rq_time_) { - absl::optional value = Stats::Utility::findTag(*histogram, cluster_name_); + for (const auto& histogram : snapshot.histograms()) { + if (histogram.get().tagExtractedStatName() == cluster_upstream_rq_time_) { + absl::optional value = + Stats::Utility::findTag(histogram.get(), cluster_name_); // Make sure we found the cluster name tag ASSERT(value); std::string value_str = server_.stats().symbolTable().toString(*value); @@ -342,12 +343,12 @@ void HystrixSink::flush(Stats::Source& source) { QuantileLatencyMap& hist_map = it_bool_pair.first->second; const std::vector& supported_quantiles = - histogram->intervalStatistics().supportedQuantiles(); + histogram.get().intervalStatistics().supportedQuantiles(); for (size_t i = 0; i < supported_quantiles.size(); ++i) { // binary-search here is likely not worth it, as hystrix_quantiles has <10 elements. if (std::find(hystrix_quantiles.begin(), hystrix_quantiles.end(), supported_quantiles[i]) != hystrix_quantiles.end()) { - const double value = histogram->intervalStatistics().computedQuantiles()[i]; + const double value = histogram.get().intervalStatistics().computedQuantiles()[i]; if (!std::isnan(value)) { hist_map[supported_quantiles[i]] = value; } diff --git a/source/extensions/stat_sinks/hystrix/hystrix.h b/source/extensions/stat_sinks/hystrix/hystrix.h index f3801cb612e78..44234669f7782 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.h +++ b/source/extensions/stat_sinks/hystrix/hystrix.h @@ -8,7 +8,6 @@ #include "envoy/server/instance.h" #include "envoy/stats/histogram.h" #include "envoy/stats/sink.h" -#include "envoy/stats/source.h" #include "common/stats/symbol_table_impl.h" @@ -51,7 +50,7 @@ class HystrixSink : public Stats::Sink, public Logger::Loggablemutable_gauge(); gauage_metric->set_value(gauge.value()); } + void MetricsServiceSink::flushHistogram(const Stats::ParentHistogram& histogram) { io::prometheus::client::MetricFamily* metrics_family = message_.add_envoy_metrics(); metrics_family->set_type(io::prometheus::client::MetricType::SUMMARY); @@ -77,30 +77,29 @@ void MetricsServiceSink::flushHistogram(const Stats::ParentHistogram& histogram) } } -void MetricsServiceSink::flush(Stats::Source& source) { +void MetricsServiceSink::flush(Stats::MetricSnapshot& snapshot) { message_.clear_envoy_metrics(); - const std::vector& counters = source.cachedCounters(); - const std::vector& gauges = source.cachedGauges(); - const std::vector& histograms = source.cachedHistograms(); + // TODO(mrice32): there's probably some more sophisticated preallocation we can do here where we // actually preallocate the submessages and then pass ownership to the proto (rather than just // preallocating the pointer array). - message_.mutable_envoy_metrics()->Reserve(counters.size() + gauges.size() + histograms.size()); - for (const Stats::CounterSharedPtr& counter : counters) { - if (counter->used()) { - flushCounter(*counter); + message_.mutable_envoy_metrics()->Reserve(snapshot.counters().size() + snapshot.gauges().size() + + snapshot.histograms().size()); + for (const auto& counter : snapshot.counters()) { + if (counter.counter_.get().used()) { + flushCounter(counter.counter_.get()); } } - for (const Stats::GaugeSharedPtr& gauge : gauges) { - if (gauge->used()) { - flushGauge(*gauge); + for (const auto& gauge : snapshot.gauges()) { + if (gauge.get().used()) { + flushGauge(gauge.get()); } } - for (const Stats::ParentHistogramSharedPtr& histogram : histograms) { - if (histogram->used()) { - flushHistogram(*histogram); + for (const auto& histogram : snapshot.histograms()) { + if (histogram.get().used()) { + flushHistogram(histogram.get()); } } diff --git a/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h b/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h index 8881a0bd1ff76..0e8bed1d193d6 100644 --- a/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h +++ b/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h @@ -8,7 +8,6 @@ #include "envoy/singleton/instance.h" #include "envoy/stats/histogram.h" #include "envoy/stats/sink.h" -#include "envoy/stats/source.h" #include "envoy/stats/stats.h" #include "envoy/upstream/cluster_manager.h" @@ -73,7 +72,7 @@ class MetricsServiceSink : public Stats::Sink { // MetricsService::Sink MetricsServiceSink(const GrpcMetricsStreamerSharedPtr& grpc_metrics_streamer, TimeSource& time_system); - void flush(Stats::Source& source) override; + void flush(Stats::MetricSnapshot& snapshot) override; void onHistogramComplete(const Stats::Histogram&, uint64_t) override {} void flushCounter(const Stats::Counter& counter); diff --git a/source/server/server.cc b/source/server/server.cc index d67dd6be692e7..805d6bfa5bd44 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -128,19 +128,65 @@ void InstanceImpl::drainListeners() { drain_manager_->startDrainSequence(nullptr); } -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 InstanceImpl::failHealthcheck(bool fail) { server_stats_->live_.set(!fail); } + +// Local implementation of Stats::MetricSnapshot used to flush metrics to sinks. We could +// potentially have a single class instance held in a static and have a clear() method to avoid some +// vector constructions and reservations, but I'm not sure it's worth the extra complexity until it +// shows up in perf traces. +// TODO(mattklein123): One thing we probably want to do is switch from returning vectors of metrics +// to a lambda based callback iteration API. This would require less vector +// copying and probably be a cleaner API in general. +class MetricSnapshotImpl : public Stats::MetricSnapshot { +public: + MetricSnapshotImpl(Stats::Store& store) { + snapped_counters_ = store.counters(); + counters_.reserve(snapped_counters_.size()); + for (const auto& counter : snapped_counters_) { + counters_.push_back({counter->latch(), *counter}); + } + + snapped_gauges_ = store.gauges(); + gauges_.reserve(snapped_gauges_.size()); + for (const auto& gauge : snapped_gauges_) { + gauges_.push_back(*gauge); + } + + snapped_histograms_ = store.histograms(); + histograms_.reserve(snapped_histograms_.size()); + for (const auto& histogram : snapped_histograms_) { + histograms_.push_back(*histogram); + } + } + + // Stats::MetricSnapshot + const std::vector& counters() override { return counters_; } + const std::vector>& gauges() override { + return gauges_; + }; + const std::vector>& histograms() override { + return histograms_; + } + +private: + std::vector snapped_counters_; + std::vector counters_; + std::vector snapped_gauges_; + std::vector> gauges_; + std::vector snapped_histograms_; + std::vector> histograms_; +}; void InstanceUtil::flushMetricsToSinks(const std::list& sinks, - Stats::Source& source) { + Stats::Store& store) { + // Create a snapshot and flush to all sinks. + // NOTE: Even if there are no sinks, creating the snapshot has the important property that it + // latches all counters on a periodic basis. The hot restart code assumes this is being + // done so this should not be removed. + MetricSnapshotImpl snapshot(store); for (const auto& sink : sinks) { - sink->flush(source); + sink->flush(snapshot); } - // TODO(mrice32): this reset should be called by the StoreRoot on stat construction/destruction so - // that it doesn't need to be reset when the set of stats isn't changing. - source.clearCache(); } void InstanceImpl::flushStats() { @@ -160,7 +206,7 @@ void InstanceImpl::flushStats() { parent_stats.parent_connections_); server_stats_->days_until_first_cert_expiring_.set( sslContextManager().daysUntilFirstCertExpires()); - InstanceUtil::flushMetricsToSinks(config_.statsSinks(), stats_store_.source()); + InstanceUtil::flushMetricsToSinks(config_.statsSinks(), stats_store_); // TODO(ramaraochavali): consider adding different flush interval for histograms. if (stat_flush_timer_ != nullptr) { stat_flush_timer_->enableTimer(config_.statsFlushInterval()); @@ -535,9 +581,6 @@ void InstanceImpl::shutdown() { void InstanceImpl::shutdownAdmin() { ENVOY_LOG(warn, "shutting down admin due to child startup"); - // TODO(mattklein123): Since histograms are not shared between processes, this will also stop - // histogram flushing. In the future we can consider whether we want to - // somehow keep flushing histograms from the old process. stat_flush_timer_.reset(); handler_->stopListeners(); admin_->closeSocket(); diff --git a/source/server/server.h b/source/server/server.h index 822cbd7f8a8bf..aa31d22d27d1b 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -97,11 +97,11 @@ class InstanceUtil : Logger::Loggable { /** * Helper for flushing counters, gauges and histograms to sinks. This takes care of calling - * flush() on each sink and clearing the cache afterward. + * flush() on each sink. * @param sinks supplies the list of sinks. - * @param source provides the metrics being flushed. + * @param store provides the store being flushed. */ - static void flushMetricsToSinks(const std::list& sinks, Stats::Source& source); + static void flushMetricsToSinks(const std::list& sinks, Stats::Store& store); /** * Load a bootstrap config from either v1 or v2 and perform validation. diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index 0a73e14ad5fa9..83b6e6fcd3c76 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -39,15 +39,6 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "source_impl_test", - srcs = ["source_impl_test.cc"], - deps = [ - "//source/common/stats:source_impl_lib", - "//test/mocks/stats:stats_mocks", - ], -) - envoy_cc_test( name = "stat_merger_test", srcs = ["stat_merger_test.cc"], diff --git a/test/common/stats/source_impl_test.cc b/test/common/stats/source_impl_test.cc deleted file mode 100644 index bf5b15851d308..0000000000000 --- a/test/common/stats/source_impl_test.cc +++ /dev/null @@ -1,53 +0,0 @@ -#include - -#include "common/stats/source_impl.h" - -#include "test/mocks/stats/mocks.h" - -#include "gtest/gtest.h" - -using testing::NiceMock; -using testing::ReturnPointee; - -namespace Envoy { -namespace Stats { -namespace { - -TEST(SourceImplTest, Caching) { - NiceMock store; - std::vector stored_counters; - std::vector stored_gauges; - std::vector stored_histograms; - - ON_CALL(store, counters()).WillByDefault(ReturnPointee(&stored_counters)); - ON_CALL(store, gauges()).WillByDefault(ReturnPointee(&stored_gauges)); - ON_CALL(store, histograms()).WillByDefault(ReturnPointee(&stored_histograms)); - - SourceImpl source(store); - - // Once cached, new values should not be reflected by the return value. - stored_counters.push_back(std::make_shared()); - EXPECT_EQ(source.cachedCounters(), stored_counters); - stored_counters.push_back(std::make_shared()); - EXPECT_NE(source.cachedCounters(), stored_counters); - - stored_gauges.push_back(std::make_shared()); - EXPECT_EQ(source.cachedGauges(), stored_gauges); - stored_gauges.push_back(std::make_shared()); - EXPECT_NE(source.cachedGauges(), stored_gauges); - - stored_histograms.push_back(std::make_shared()); - EXPECT_EQ(source.cachedHistograms(), stored_histograms); - stored_histograms.push_back(std::make_shared()); - EXPECT_NE(source.cachedHistograms(), stored_histograms); - - // After clearing, the new values should be reflected in the cache. - source.clearCache(); - EXPECT_EQ(source.cachedCounters(), stored_counters); - EXPECT_EQ(source.cachedGauges(), stored_gauges); - EXPECT_EQ(source.cachedHistograms(), stored_histograms); -} - -} // namespace -} // namespace Stats -} // namespace Envoy diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 6954a28147f1e..f573565f441cc 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -351,15 +351,12 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { EXPECT_EQ(1UL, store_->counters().size()); CounterSharedPtr c1 = TestUtility::findCounter(*store_, "scope1.c1"); EXPECT_EQ("scope1.c1", c1->name()); - EXPECT_EQ(TestUtility::findByName(store_->source().cachedCounters(), "scope1.c1"), c1); + EXPECT_EQ(TestUtility::findByName(store_->counters(), "scope1.c1"), c1); EXPECT_CALL(main_thread_dispatcher_, post(_)); EXPECT_CALL(tls_, runOnAllThreads(_, _)); scope1.reset(); EXPECT_EQ(0UL, store_->counters().size()); - EXPECT_EQ(1UL, store_->source().cachedCounters().size()); - store_->source().clearCache(); - EXPECT_EQ(0UL, store_->source().cachedCounters().size()); EXPECT_EQ(1L, c1.use_count()); c1.reset(); diff --git a/test/extensions/stats_sinks/common/statsd/statsd_test.cc b/test/extensions/stats_sinks/common/statsd/statsd_test.cc index b23cac0f9ebea..12f7f7df22702 100644 --- a/test/extensions/stats_sinks/common/statsd/statsd_test.cc +++ b/test/extensions/stats_sinks/common/statsd/statsd_test.cc @@ -56,14 +56,14 @@ class TcpStatsdSinkTest : public testing::Test { std::unique_ptr sink_; NiceMock local_info_; Network::MockClientConnection* connection_{}; - NiceMock source_; + NiceMock snapshot_; }; TEST_F(TcpStatsdSinkTest, EmptyFlush) { InSequence s; expectCreateConnection(); EXPECT_CALL(*connection_, write(BufferStringEqual(""), _)); - sink_->flush(source_); + sink_->flush(snapshot_); } TEST_F(TcpStatsdSinkTest, BasicFlow) { @@ -72,18 +72,18 @@ TEST_F(TcpStatsdSinkTest, BasicFlow) { counter->name_ = "test_counter"; counter->latch_ = 1; counter->used_ = true; - source_.counters_.push_back(counter); + snapshot_.counters_.push_back({1, *counter}); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 2; gauge->used_ = true; - source_.gauges_.push_back(gauge); + snapshot_.gauges_.push_back(*gauge); expectCreateConnection(); EXPECT_CALL(*connection_, write(BufferStringEqual("envoy.test_counter:1|c\nenvoy.test_gauge:2|g\n"), _)); - sink_->flush(source_); + sink_->flush(snapshot_); connection_->runHighWatermarkCallbacks(); connection_->runLowWatermarkCallbacks(); @@ -110,16 +110,16 @@ TEST_F(TcpStatsdSinkTest, NoHost) { counter->name_ = "test_counter"; counter->latch_ = 1; counter->used_ = true; - source_.counters_.push_back(counter); + snapshot_.counters_.push_back({1, *counter}); Upstream::MockHost::MockCreateConnectionData conn_info; EXPECT_CALL(cluster_manager_, tcpConnForCluster_("fake_cluster", _)) .WillOnce(Return(conn_info)) .WillOnce(Return(conn_info)); - sink_->flush(source_); + sink_->flush(snapshot_); // Flush again to make sure we correctly drain the buffer and the output buffer is empty. - sink_->flush(source_); + sink_->flush(snapshot_); } TEST_F(TcpStatsdSinkTest, WithCustomPrefix) { @@ -131,11 +131,11 @@ TEST_F(TcpStatsdSinkTest, WithCustomPrefix) { counter->name_ = "test_counter"; counter->latch_ = 1; counter->used_ = true; - source_.counters_.push_back(counter); + snapshot_.counters_.push_back({1, *counter}); expectCreateConnection(); EXPECT_CALL(*connection_, write(BufferStringEqual("test_prefix.test_counter:1|c\n"), _)); - sink_->flush(source_); + sink_->flush(snapshot_); } TEST_F(TcpStatsdSinkTest, BufferReallocate) { @@ -146,7 +146,7 @@ TEST_F(TcpStatsdSinkTest, BufferReallocate) { counter->latch_ = 1; counter->used_ = true; - source_.counters_.resize(2000, counter); + snapshot_.counters_.resize(2000, {1, *counter}); expectCreateConnection(); EXPECT_CALL(*connection_, write(_, _)) @@ -158,7 +158,7 @@ TEST_F(TcpStatsdSinkTest, BufferReallocate) { EXPECT_EQ(compare, buffer.toString()); buffer.drain(buffer.length()); })); - sink_->flush(source_); + sink_->flush(snapshot_); } TEST_F(TcpStatsdSinkTest, Overflow) { @@ -168,25 +168,25 @@ TEST_F(TcpStatsdSinkTest, Overflow) { counter->name_ = "test_counter"; counter->latch_ = 1; counter->used_ = true; - source_.counters_.push_back(counter); + snapshot_.counters_.push_back({1, *counter}); // Synthetically set buffer above high watermark. Make sure we don't write anything. cluster_manager_.thread_local_cluster_.cluster_.info_->stats().upstream_cx_tx_bytes_buffered_.set( 1024 * 1024 * 17); - sink_->flush(source_); + sink_->flush(snapshot_); // Lower and make sure we write. cluster_manager_.thread_local_cluster_.cluster_.info_->stats().upstream_cx_tx_bytes_buffered_.set( 1024 * 1024 * 15); expectCreateConnection(); EXPECT_CALL(*connection_, write(BufferStringEqual("envoy.test_counter:1|c\n"), _)); - sink_->flush(source_); + sink_->flush(snapshot_); // Raise and make sure we don't write and kill connection. cluster_manager_.thread_local_cluster_.cluster_.info_->stats().upstream_cx_tx_bytes_buffered_.set( 1024 * 1024 * 17); EXPECT_CALL(*connection_, close(Network::ConnectionCloseType::NoFlush)); - sink_->flush(source_); + sink_->flush(snapshot_); EXPECT_EQ(2UL, cluster_manager_.thread_local_cluster_.cluster_.info_->stats_store_ .counter("statsd.cx_overflow") diff --git a/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc b/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc index d48999fc258ab..9e016e5df7083 100644 --- a/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc +++ b/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc @@ -36,7 +36,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, UdpStatsdSinkTest, TEST_P(UdpStatsdSinkTest, InitWithIpAddress) { NiceMock tls_; - NiceMock source; // UDP statsd server address. + NiceMock snapshot; // UDP statsd server address. Network::Address::InstanceConstSharedPtr server_address = Network::Utility::parseInternetAddressAndPort( fmt::format("{}:8125", Network::Test::getLoopbackAddressUrlString(GetParam()))); @@ -49,15 +49,15 @@ TEST_P(UdpStatsdSinkTest, InitWithIpAddress) { counter->name_ = "test_counter"; counter->used_ = true; counter->latch_ = 1; - source.counters_.push_back(counter); + snapshot.counters_.push_back({1, *counter}); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - source.gauges_.push_back(gauge); + snapshot.gauges_.push_back(*gauge); - sink.flush(source); + sink.flush(snapshot); NiceMock timer; timer.name_ = "test_timer"; @@ -80,7 +80,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, UdpStatsdSinkWithTagsTest, TEST_P(UdpStatsdSinkWithTagsTest, InitWithIpAddress) { NiceMock tls_; - NiceMock source; + NiceMock snapshot; // UDP statsd server address. Network::Address::InstanceConstSharedPtr server_address = Network::Utility::parseInternetAddressAndPort( @@ -96,16 +96,16 @@ TEST_P(UdpStatsdSinkWithTagsTest, InitWithIpAddress) { counter->used_ = true; counter->latch_ = 1; counter->setTags(tags); - source.counters_.push_back(counter); + snapshot.counters_.push_back({1, *counter}); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; gauge->setTags(tags); - source.gauges_.push_back(gauge); + snapshot.gauges_.push_back(*gauge); - sink.flush(source); + sink.flush(snapshot); NiceMock timer; timer.name_ = "test_timer"; @@ -123,7 +123,7 @@ TEST_P(UdpStatsdSinkWithTagsTest, InitWithIpAddress) { } TEST(UdpStatsdSinkTest, CheckActualStats) { - NiceMock source; + NiceMock snapshot; auto writer_ptr = std::make_shared>(); NiceMock tls_; UdpStatsdSink sink(tls_, writer_ptr, false); @@ -132,22 +132,22 @@ TEST(UdpStatsdSinkTest, CheckActualStats) { counter->name_ = "test_counter"; counter->used_ = true; counter->latch_ = 1; - source.counters_.push_back(counter); + snapshot.counters_.push_back({1, *counter}); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), write("envoy.test_counter:1|c")); - sink.flush(source); + sink.flush(snapshot); counter->used_ = false; auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - source.gauges_.push_back(gauge); + snapshot.gauges_.push_back(*gauge); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), write("envoy.test_gauge:1|g")); - sink.flush(source); + sink.flush(snapshot); NiceMock timer; timer.name_ = "test_timer"; @@ -159,7 +159,7 @@ TEST(UdpStatsdSinkTest, CheckActualStats) { } TEST(UdpStatsdSinkTest, CheckActualStatsWithCustomPrefix) { - NiceMock source; + NiceMock snapshot; auto writer_ptr = std::make_shared>(); NiceMock tls_; UdpStatsdSink sink(tls_, writer_ptr, false, "test_prefix"); @@ -168,18 +168,18 @@ TEST(UdpStatsdSinkTest, CheckActualStatsWithCustomPrefix) { counter->name_ = "test_counter"; counter->used_ = true; counter->latch_ = 1; - source.counters_.push_back(counter); + snapshot.counters_.push_back({1, *counter}); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), write("test_prefix.test_counter:1|c")); - sink.flush(source); + sink.flush(snapshot); counter->used_ = false; tls_.shutdownThread(); } TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { - NiceMock source; + NiceMock snapshot; auto writer_ptr = std::make_shared>(); NiceMock tls_; UdpStatsdSink sink(tls_, writer_ptr, true); @@ -190,11 +190,11 @@ TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { counter->used_ = true; counter->latch_ = 1; counter->setTags(tags); - source.counters_.push_back(counter); + snapshot.counters_.push_back({1, *counter}); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), write("envoy.test_counter:1|c|#key1:value1,key2:value2")); - sink.flush(source); + sink.flush(snapshot); counter->used_ = false; auto gauge = std::make_shared>(); @@ -202,11 +202,11 @@ TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { gauge->value_ = 1; gauge->used_ = true; gauge->setTags(tags); - source.gauges_.push_back(gauge); + snapshot.gauges_.push_back(*gauge); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), write("envoy.test_gauge:1|g|#key1:value1,key2:value2")); - sink.flush(source); + sink.flush(snapshot); NiceMock timer; timer.name_ = "test_timer"; diff --git a/test/extensions/stats_sinks/hystrix/hystrix_test.cc b/test/extensions/stats_sinks/hystrix/hystrix_test.cc index 0e4a140035d2c..bf0988003b9fa 100644 --- a/test/extensions/stats_sinks/hystrix/hystrix_test.cc +++ b/test/extensions/stats_sinks/hystrix/hystrix_test.cc @@ -170,7 +170,7 @@ class HystrixSinkTest : public testing::Test { buffer.drain(buffer.length()); cluster1_.setCounterReturnValues(i, success_step, error_step, 0, 0, 0, timeout_step, 0, 0); cluster2_.setCounterReturnValues(i, success_step2, error_step2, 0, 0, 0, timeout_step2, 0, 0); - sink_->flush(source_); + sink_->flush(snapshot_); } return buildClusterMap(buffer.toString()); @@ -179,7 +179,7 @@ class HystrixSinkTest : public testing::Test { void removeSecondClusterHelper(Buffer::OwnedImpl& buffer) { buffer.drain(buffer.length()); removeClusterFromMap(cluster2_name_); - sink_->flush(source_); + sink_->flush(snapshot_); } void validateResults(const std::string& data_message, uint64_t success_step, uint64_t error_step, @@ -246,7 +246,7 @@ class HystrixSinkTest : public testing::Test { Upstream::ClusterManager::ClusterInfoMap cluster_map_; std::unique_ptr sink_; - NiceMock source_; + NiceMock snapshot_; NiceMock cluster_manager_; }; @@ -255,7 +255,7 @@ TEST_F(HystrixSinkTest, EmptyFlush) { Buffer::OwnedImpl buffer = createClusterAndCallbacks(); // Register callback to sink. sink_->registerConnection(&callbacks_); - sink_->flush(source_); + sink_->flush(snapshot_); std::unordered_map cluster_message_map = buildClusterMap(buffer.toString()); validateResults(cluster_message_map[cluster1_name_], 0, 0, 0, 0, 0, window_size_); @@ -271,12 +271,12 @@ TEST_F(HystrixSinkTest, BasicFlow) { // Later in the test we'll "shortcut" by constant traffic uint64_t traffic_counter = 0; - sink_->flush(source_); // init window with 0 + sink_->flush(snapshot_); // init window with 0 for (uint64_t i = 0; i < (window_size_ - 1); i++) { buffer.drain(buffer.length()); traffic_counter += rand_.random() % 1000; ON_CALL(cluster1_.success_counter_, value()).WillByDefault(Return(traffic_counter)); - sink_->flush(source_); + sink_->flush(snapshot_); } std::unordered_map cluster_message_map = @@ -308,7 +308,7 @@ TEST_F(HystrixSinkTest, BasicFlow) { cluster1_.setCounterReturnValues(i, success_step, error_4xx_step, error_4xx_retry_step, error_5xx_step, error_5xx_retry_step, timeout_step, timeout_retry_step, rejected_step); - sink_->flush(source_); + sink_->flush(snapshot_); } std::string rolling_map = sink_->printRollingWindows(); @@ -325,7 +325,7 @@ TEST_F(HystrixSinkTest, BasicFlow) { // Check the values are reset. buffer.drain(buffer.length()); sink_->resetRollingWindow(); - sink_->flush(source_); + sink_->flush(snapshot_); cluster_message_map = buildClusterMap(buffer.toString()); validateResults(cluster_message_map[cluster1_name_], 0, 0, 0, 0, 0, window_size_); } @@ -335,7 +335,7 @@ TEST_F(HystrixSinkTest, Disconnect) { InSequence s; Buffer::OwnedImpl buffer = createClusterAndCallbacks(); - sink_->flush(source_); + sink_->flush(snapshot_); EXPECT_EQ(buffer.length(), 0); // Register callback to sink. @@ -347,7 +347,7 @@ TEST_F(HystrixSinkTest, Disconnect) { for (uint64_t i = 0; i < (window_size_ + 1); i++) { buffer.drain(buffer.length()); ON_CALL(cluster1_.success_counter_, value()).WillByDefault(Return((i + 1) * success_step)); - sink_->flush(source_); + sink_->flush(snapshot_); } EXPECT_NE(buffer.length(), 0); @@ -360,14 +360,14 @@ TEST_F(HystrixSinkTest, Disconnect) { // Disconnect. buffer.drain(buffer.length()); sink_->unregisterConnection(&callbacks_); - sink_->flush(source_); + sink_->flush(snapshot_); EXPECT_EQ(buffer.length(), 0); // Reconnect. buffer.drain(buffer.length()); sink_->registerConnection(&callbacks_); ON_CALL(cluster1_.success_counter_, value()).WillByDefault(Return(success_step)); - sink_->flush(source_); + sink_->flush(snapshot_); EXPECT_NE(buffer.length(), 0); cluster_message_map = buildClusterMap(buffer.toString()); json_buffer = Json::Factory::loadFromString(cluster_message_map[cluster1_name_]); @@ -442,7 +442,7 @@ TEST_F(HystrixSinkTest, AddAndRemoveClusters) { // Add cluster again and flush data to sink. addSecondClusterHelper(buffer); - sink_->flush(source_); + sink_->flush(snapshot_); // Check that add worked. cluster_message_map = buildClusterMap(buffer.toString()); @@ -457,7 +457,6 @@ TEST_F(HystrixSinkTest, AddAndRemoveClusters) { TEST_F(HystrixSinkTest, HistogramTest) { InSequence s; - std::vector stored_histograms; // Create histogram for the Hystrix sink to read. auto histogram = std::make_shared>(); @@ -478,14 +477,12 @@ TEST_F(HystrixSinkTest, HistogramTest) { Stats::HistogramStatisticsImpl h1_interval_statistics(hist1_interval.getHistogram()); ON_CALL(*histogram, intervalStatistics()) .WillByDefault(testing::ReturnRef(h1_interval_statistics)); - stored_histograms.push_back(histogram); - - ON_CALL(source_, cachedHistograms()).WillByDefault(ReturnPointee(&stored_histograms)); + snapshot_.histograms_.push_back(*histogram); Buffer::OwnedImpl buffer = createClusterAndCallbacks(); // Register callback to sink. sink_->registerConnection(&callbacks_); - sink_->flush(source_); + sink_->flush(snapshot_); std::unordered_map cluster_message_map = buildClusterMap(buffer.toString()); diff --git a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc index a8f06af6cefba..344de8f1bd8b6 100644 --- a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc +++ b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc @@ -99,7 +99,7 @@ class TestGrpcMetricsStreamer : public GrpcMetricsStreamer { class MetricsServiceSinkTest : public testing::Test {}; TEST(MetricsServiceSinkTest, CheckSendCall) { - NiceMock source; + NiceMock snapshot; Event::SimulatedTimeSystem time_system; std::shared_ptr streamer_{new MockGrpcMetricsStreamer()}; @@ -109,13 +109,13 @@ TEST(MetricsServiceSinkTest, CheckSendCall) { counter->name_ = "test_counter"; counter->latch_ = 1; counter->used_ = true; - source.counters_.push_back(counter); + snapshot.counters_.push_back({1, *counter}); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - source.gauges_.push_back(gauge); + snapshot.gauges_.push_back(*gauge); auto histogram = std::make_shared>(); histogram->name_ = "test_histogram"; @@ -123,11 +123,11 @@ TEST(MetricsServiceSinkTest, CheckSendCall) { EXPECT_CALL(*streamer_, send(_)); - sink.flush(source); + sink.flush(snapshot); } TEST(MetricsServiceSinkTest, CheckStatsCount) { - NiceMock source; + NiceMock snapshot; Event::SimulatedTimeSystem time_system; std::shared_ptr streamer_{new TestGrpcMetricsStreamer()}; @@ -137,20 +137,20 @@ TEST(MetricsServiceSinkTest, CheckStatsCount) { counter->name_ = "test_counter"; counter->latch_ = 1; counter->used_ = true; - source.counters_.push_back(counter); + snapshot.counters_.push_back({1, *counter}); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - source.gauges_.push_back(gauge); + snapshot.gauges_.push_back(*gauge); - sink.flush(source); + sink.flush(snapshot); EXPECT_EQ(2, (*streamer_).metric_count); // Verify only newly added metrics come after endFlush call. gauge->used_ = false; - sink.flush(source); + sink.flush(snapshot); EXPECT_EQ(1, (*streamer_).metric_count); } diff --git a/test/integration/server.h b/test/integration/server.h index aa1e3865913bb..2ae5492a463b8 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -13,7 +13,6 @@ #include "common/common/lock_guard.h" #include "common/common/logger.h" #include "common/common/thread.h" -#include "common/stats/source_impl.h" #include "server/listener_hooks.h" #include "server/options_impl.h" @@ -134,7 +133,6 @@ class TestScopeWrapper : public Scope { */ class TestIsolatedStoreImpl : public StoreRoot { public: - TestIsolatedStoreImpl() : source_(*this) {} // Stats::Scope Counter& counterFromStatName(StatName name) override { Thread::LockGuard lock(lock_); @@ -204,12 +202,10 @@ class TestIsolatedStoreImpl : public StoreRoot { void initializeThreading(Event::Dispatcher&, ThreadLocal::Instance&) override {} void shutdownThreading() override {} void mergeHistograms(PostMergeCb) override {} - Source& source() override { return source_; } private: mutable Thread::MutexBasicLockable lock_; IsolatedStoreImpl store_; - SourceImpl source_; }; } // namespace Stats diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index 18c7e1a08bc0f..350ba468d5291 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -102,13 +102,13 @@ MockParentHistogram::MockParentHistogram() { } MockParentHistogram::~MockParentHistogram() {} -MockSource::MockSource() { - ON_CALL(*this, cachedCounters()).WillByDefault(ReturnRef(counters_)); - ON_CALL(*this, cachedGauges()).WillByDefault(ReturnRef(gauges_)); - ON_CALL(*this, cachedHistograms()).WillByDefault(ReturnRef(histograms_)); +MockMetricSnapshot::MockMetricSnapshot() { + ON_CALL(*this, counters()).WillByDefault(ReturnRef(counters_)); + ON_CALL(*this, gauges()).WillByDefault(ReturnRef(gauges_)); + ON_CALL(*this, histograms()).WillByDefault(ReturnRef(histograms_)); } -MockSource::~MockSource() {} +MockMetricSnapshot::~MockMetricSnapshot() {} MockSink::MockSink() {} MockSink::~MockSink() {} diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 23d6dd2738cd6..e50975d8cff90 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -7,7 +7,6 @@ #include "envoy/stats/histogram.h" #include "envoy/stats/sink.h" -#include "envoy/stats/source.h" #include "envoy/stats/stats.h" #include "envoy/stats/stats_matcher.h" #include "envoy/stats/store.h" @@ -148,19 +147,18 @@ class MockParentHistogram : public ParentHistogram, public MockMetric { std::make_shared(); }; -class MockSource : public Source { +class MockMetricSnapshot : public MetricSnapshot { public: - MockSource(); - ~MockSource(); + MockMetricSnapshot(); + ~MockMetricSnapshot(); - MOCK_METHOD0(cachedCounters, const std::vector&()); - MOCK_METHOD0(cachedGauges, const std::vector&()); - MOCK_METHOD0(cachedHistograms, const std::vector&()); - MOCK_METHOD0(clearCache, void()); + MOCK_METHOD0(counters, const std::vector&()); + MOCK_METHOD0(gauges, const std::vector>&()); + MOCK_METHOD0(histograms, const std::vector>&()); - std::vector counters_; - std::vector gauges_; - std::vector histograms_; + std::vector counters_; + std::vector> gauges_; + std::vector> histograms_; }; class MockSink : public Sink { @@ -168,7 +166,7 @@ class MockSink : public Sink { MockSink(); ~MockSink(); - MOCK_METHOD1(flush, void(Source& source)); + MOCK_METHOD1(flush, void(MetricSnapshot& snapshot)); MOCK_METHOD2(onHistogramComplete, void(const Histogram& histogram, uint64_t value)); }; diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 9ebd5790ecbe7..303755bac2d8a 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -37,23 +37,42 @@ TEST(ServerInstanceUtil, flushHelper) { InSequence s; Stats::IsolatedStoreImpl store; - Stats::SourceImpl source(store); - store.counter("hello").inc(); + Stats::Counter& c = store.counter("hello"); + c.inc(); store.gauge("world").set(5); - std::unique_ptr sink(new StrictMock()); - EXPECT_CALL(*sink, flush(Ref(source))).WillOnce(Invoke([](Stats::Source& source) { - ASSERT_EQ(source.cachedCounters().size(), 1); - EXPECT_EQ(source.cachedCounters().front()->name(), "hello"); - EXPECT_EQ(source.cachedCounters().front()->latch(), 1); - - ASSERT_EQ(source.cachedGauges().size(), 1); - EXPECT_EQ(source.cachedGauges().front()->name(), "world"); - EXPECT_EQ(source.cachedGauges().front()->value(), 5); - })); + store.histogram("histogram"); std::list sinks; - sinks.emplace_back(std::move(sink)); - InstanceUtil::flushMetricsToSinks(sinks, source); + InstanceUtil::flushMetricsToSinks(sinks, store); + // Make sure that counters have been latched even if there are no sinks. + EXPECT_EQ(1UL, c.value()); + EXPECT_EQ(0, c.latch()); + + Stats::MockSink* sink = new StrictMock(); + sinks.emplace_back(sink); + EXPECT_CALL(*sink, flush(_)).WillOnce(Invoke([](Stats::MetricSnapshot& snapshot) { + ASSERT_EQ(snapshot.counters().size(), 1); + EXPECT_EQ(snapshot.counters()[0].counter_.get().name(), "hello"); + EXPECT_EQ(snapshot.counters()[0].delta_, 1); + + ASSERT_EQ(snapshot.gauges().size(), 1); + EXPECT_EQ(snapshot.gauges()[0].get().name(), "world"); + EXPECT_EQ(snapshot.gauges()[0].get().value(), 5); + })); + c.inc(); + InstanceUtil::flushMetricsToSinks(sinks, store); + + // Histograms don't currently work with the isolated store so test those with a mock store. + NiceMock mock_store; + Stats::ParentHistogramSharedPtr parent_histogram(new Stats::MockParentHistogram()); + std::vector parent_histograms = {parent_histogram}; + ON_CALL(mock_store, histograms).WillByDefault(Return(parent_histograms)); + EXPECT_CALL(*sink, flush(_)).WillOnce(Invoke([](Stats::MetricSnapshot& snapshot) { + EXPECT_TRUE(snapshot.counters().empty()); + EXPECT_TRUE(snapshot.gauges().empty()); + EXPECT_EQ(snapshot.histograms().size(), 1); + })); + InstanceUtil::flushMetricsToSinks(sinks, mock_store); } class RunHelperTest : public testing::Test {