diff --git a/.gitignore b/.gitignore index ccfc0dab4086..c401e121e35c 100644 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,7 @@ BROWSE compile_commands.json cscope.* .deps +.devcontainer.json /docs/landing_source/.bundle /generated .idea/ diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 3733961064a2..e034e3eb36f0 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -82,6 +82,7 @@ Version history * server: added :ref:`per-handler listener stats ` and :ref:`per-worker watchdog stats ` to help diagnosing event loop imbalance and general performance issues. +* stats: added unit support to histogram. * thrift_proxy: fix crashing bug on invalid transport/protocol framing * tls: added verification of IP address SAN fields in certificates against configured SANs in the * tracing: added support to the Zipkin reporter for sending list of spans as Zipkin JSON v2 and protobuf message over HTTP. diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index ee15a8be42a8..b4c908bb4cbf 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -30,8 +30,8 @@ namespace Event { */ // clang-format off #define ALL_DISPATCHER_STATS(HISTOGRAM) \ - HISTOGRAM(loop_duration_us) \ - HISTOGRAM(poll_delay_us) + HISTOGRAM(loop_duration_us, Microseconds) \ + HISTOGRAM(poll_delay_us, Microseconds) // clang-format on /** diff --git a/include/envoy/stats/BUILD b/include/envoy/stats/BUILD index 2c77099d1644..fcba981cd242 100644 --- a/include/envoy/stats/BUILD +++ b/include/envoy/stats/BUILD @@ -56,12 +56,8 @@ envoy_cc_library( ) envoy_cc_library( - name = "timespan", + name = "timespan_interface", hdrs = ["timespan.h"], - deps = [ - ":stats_interface", - "//include/envoy/common:time_interface", - ], ) envoy_cc_library( diff --git a/include/envoy/stats/histogram.h b/include/envoy/stats/histogram.h index 43fb64044fa0..719a83fea6e2 100644 --- a/include/envoy/stats/histogram.h +++ b/include/envoy/stats/histogram.h @@ -67,17 +67,37 @@ class HistogramStatistics { /** * A histogram that records values one at a time. - * Note: Histograms now incorporate what used to be timers because the only difference between the - * two stat types was the units being represented. It is assumed that no downstream user of this - * class (Sinks, in particular) will need to explicitly differentiate between histograms - * representing durations and histograms representing other types of data. + * Note: Histograms now incorporate what used to be timers because the only + * difference between the two stat types was the units being represented. */ class Histogram : public Metric { public: + /** + * Histogram values represent scalar quantity like time, length, mass, + * distance, or in general anything which has only magnitude and no other + * characteristics. These are often accompanied by a unit of measurement. + * This enum defines units for commonly measured quantities. Base units + * are preferred unless they are not granular enough to be useful as an + * integer. + */ + enum class Unit { + Null, // The histogram has been rejected, i.e. it's a null histogram and is not recording + // anything. + Unspecified, // Measured quantity does not require a unit, e.g. "items". + Bytes, + Microseconds, + Milliseconds, + }; + ~Histogram() override = default; /** - * Records an unsigned value. If a timer, values are in units of milliseconds. + * @return the unit of measurement for values recorded by the histogram. + */ + virtual Unit unit() const PURE; + + /** + * Records an unsigned value in the unit specified during the construction. */ virtual void recordValue(uint64_t value) PURE; }; diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index 1d122ddf6544..41ae185ced92 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -81,16 +81,18 @@ class Scope { /** * @param name The name of the stat, obtained from the SymbolTable. + * @param unit The unit of measurement. * @return a histogram within the scope's namespace with a particular value type. */ - virtual Histogram& histogramFromStatName(StatName name) PURE; + virtual Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) PURE; /** * TODO(#6667): this variant is deprecated: use histogramFromStatName. * @param name The name, expressed as a string. + * @param unit The unit of measurement. * @return a histogram within the scope's namespace with a particular value type. */ - virtual Histogram& histogram(const std::string& name) PURE; + virtual Histogram& histogram(const std::string& name, Histogram::Unit unit) PURE; /** * @param The name of the stat, obtained from the SymbolTable. diff --git a/include/envoy/stats/stats_macros.h b/include/envoy/stats/stats_macros.h index fd8f925e8ceb..feabf19eb2be 100644 --- a/include/envoy/stats/stats_macros.h +++ b/include/envoy/stats/stats_macros.h @@ -17,7 +17,7 @@ namespace Envoy { * #define MY_COOL_STATS(COUNTER, GAUGE, HISTOGRAM) \ * COUNTER(counter1) \ * GAUGE(gauge1, mode) \ - * HISTOGRAM(histogram1) + * HISTOGRAM(histogram1, unit) * ... * * By convention, starting with #7083, we sort the lines of this macro block, so @@ -38,10 +38,11 @@ namespace Envoy { // Fully-qualified for use in external callsites. #define GENERATE_COUNTER_STRUCT(NAME) Envoy::Stats::Counter& NAME##_; #define GENERATE_GAUGE_STRUCT(NAME, MODE) Envoy::Stats::Gauge& NAME##_; -#define GENERATE_HISTOGRAM_STRUCT(NAME) Envoy::Stats::Histogram& NAME##_; +#define GENERATE_HISTOGRAM_STRUCT(NAME, UNIT) Envoy::Stats::Histogram& NAME##_; #define FINISH_STAT_DECL_(X) #X)), #define FINISH_STAT_DECL_MODE_(X, MODE) #X), Envoy::Stats::Gauge::ImportMode::MODE), +#define FINISH_STAT_DECL_UNIT_(X, UNIT) #X), Envoy::Stats::Histogram::Unit::UNIT), static inline std::string statPrefixJoin(absl::string_view prefix, absl::string_view token) { if (prefix.empty()) { @@ -55,7 +56,7 @@ static inline std::string statPrefixJoin(absl::string_view prefix, absl::string_ #define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_ #define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_MODE_ -#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_ +#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_UNIT_ #define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "") #define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "") diff --git a/include/envoy/stats/timespan.h b/include/envoy/stats/timespan.h index c0c7c1b18d24..f8959a72cf22 100644 --- a/include/envoy/stats/timespan.h +++ b/include/envoy/stats/timespan.h @@ -3,9 +3,7 @@ #include #include -#include "envoy/common/time.h" -#include "envoy/stats/histogram.h" -#include "envoy/stats/stats.h" +#include "envoy/common/pure.h" namespace Envoy { namespace Stats { @@ -18,43 +16,18 @@ class CompletableTimespan { virtual ~CompletableTimespan() = default; /** - * Complete the timespan. - */ - virtual void complete() PURE; -}; - -/** - * An individual timespan that flushes its measured value in time unit (e.g - * std::chrono::milliseconds). The initial time is captured on construction. A timespan must be - * completed via complete() for it to be stored. If the timespan is deleted this will be treated as - * a cancellation. - */ -template class TimespanWithUnit : public CompletableTimespan { -public: - TimespanWithUnit(Histogram& histogram, TimeSource& time_source) - : time_source_(time_source), histogram_(histogram), start_(time_source.monotonicTime()) {} - - /** - * Complete the timespan and send the time to the histogram. + * Time elapsed since the creation of the timespan. */ - void complete() override { histogram_.recordValue(getRawDuration().count()); } + virtual std::chrono::milliseconds elapsed() const PURE; /** - * Get duration in the time unit since the creation of the span. + * Complete the timespan. */ - TimeUnit getRawDuration() { - return std::chrono::duration_cast(time_source_.monotonicTime() - start_); - } - -private: - TimeSource& time_source_; - Histogram& histogram_; - const MonotonicTime start_; + virtual void complete() PURE; }; -using Timespan = TimespanWithUnit; +using Timespan = CompletableTimespan; using TimespanPtr = std::unique_ptr; -using CompletableTimespanPtr = std::unique_ptr; } // namespace Stats } // namespace Envoy diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 1ab3e62bd0b8..a98f1740f12d 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -586,8 +586,8 @@ class PrioritySet { GAUGE(upstream_rq_active, Accumulate) \ GAUGE(upstream_rq_pending_active, Accumulate) \ GAUGE(version, NeverImport) \ - HISTOGRAM(upstream_cx_connect_ms) \ - HISTOGRAM(upstream_cx_length_ms) + HISTOGRAM(upstream_cx_connect_ms, Milliseconds) \ + HISTOGRAM(upstream_cx_length_ms, Milliseconds) /** * All cluster load report stats. These are only use for EDS load reporting and not sent to the diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 7650f165a397..e8dc5a1e9abd 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -169,7 +169,7 @@ envoy_cc_library( "//include/envoy/ssl:connection_interface", "//include/envoy/stats:stats_interface", "//include/envoy/stats:stats_macros", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//include/envoy/upstream:upstream_interface", "//source/common/access_log:access_log_formatter_lib", "//source/common/buffer:buffer_lib", @@ -186,6 +186,7 @@ envoy_cc_library( "//source/common/network:utility_lib", "//source/common/router:config_lib", "//source/common/runtime:uuid_util_lib", + "//source/common/stats:timespan_lib", "//source/common/stream_info:stream_info_lib", "//source/common/tracing:http_tracer_lib", ], @@ -289,7 +290,7 @@ envoy_cc_library( "//include/envoy/network:connection_interface", "//include/envoy/stats:stats_interface", "//include/envoy/stats:stats_macros", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", ], ) diff --git a/source/common/http/codes.cc b/source/common/http/codes.cc index 1afa8c841659..9fdf0028b4bf 100644 --- a/source/common/http/codes.cc +++ b/source/common/http/codes.cc @@ -58,9 +58,9 @@ void CodeStatsImpl::incCounter(Stats::Scope& scope, Stats::StatName a, Stats::St } void CodeStatsImpl::recordHistogram(Stats::Scope& scope, const Stats::StatNameVec& names, - uint64_t count) const { + Stats::Histogram::Unit unit, uint64_t count) const { const Stats::SymbolTable::StoragePtr stat_name_storage = symbol_table_.join(names); - scope.histogramFromStatName(Stats::StatName(stat_name_storage.get())).recordValue(count); + scope.histogramFromStatName(Stats::StatName(stat_name_storage.get()), unit).recordValue(count); } void CodeStatsImpl::chargeBasicResponseStat(Stats::Scope& scope, Stats::StatName prefix, @@ -128,29 +128,33 @@ void CodeStatsImpl::writeCategory(const ResponseStatInfo& info, Stats::StatName void CodeStatsImpl::chargeResponseTiming(const ResponseTimingInfo& info) const { const uint64_t count = info.response_time_.count(); - recordHistogram(info.cluster_scope_, {info.prefix_, upstream_rq_time_}, count); + recordHistogram(info.cluster_scope_, {info.prefix_, upstream_rq_time_}, + Stats::Histogram::Unit::Milliseconds, count); if (info.upstream_canary_) { - recordHistogram(info.cluster_scope_, {info.prefix_, canary_, upstream_rq_time_}, count); + recordHistogram(info.cluster_scope_, {info.prefix_, canary_, upstream_rq_time_}, + Stats::Histogram::Unit::Milliseconds, count); } if (info.internal_request_) { - recordHistogram(info.cluster_scope_, {info.prefix_, internal_, upstream_rq_time_}, count); + recordHistogram(info.cluster_scope_, {info.prefix_, internal_, upstream_rq_time_}, + Stats::Histogram::Unit::Milliseconds, count); } else { - recordHistogram(info.cluster_scope_, {info.prefix_, external_, upstream_rq_time_}, count); + recordHistogram(info.cluster_scope_, {info.prefix_, external_, upstream_rq_time_}, + Stats::Histogram::Unit::Milliseconds, count); } if (!info.request_vcluster_name_.empty()) { recordHistogram(info.global_scope_, {vhost_, info.request_vhost_name_, vcluster_, info.request_vcluster_name_, upstream_rq_time_}, - count); + Stats::Histogram::Unit::Milliseconds, count); } // Handle per zone stats. if (!info.from_zone_.empty() && !info.to_zone_.empty()) { recordHistogram(info.cluster_scope_, {info.prefix_, zone_, info.from_zone_, info.to_zone_, upstream_rq_time_}, - count); + Stats::Histogram::Unit::Milliseconds, count); } } diff --git a/source/common/http/codes.h b/source/common/http/codes.h index 9cc8339b275f..3b8a2d6fb4ef 100644 --- a/source/common/http/codes.h +++ b/source/common/http/codes.h @@ -56,7 +56,8 @@ class CodeStatsImpl : public CodeStats { Stats::StatName rq_code, Stats::StatName category) const; void incCounter(Stats::Scope& scope, const Stats::StatNameVec& names) const; void incCounter(Stats::Scope& scope, Stats::StatName a, Stats::StatName b) const; - void recordHistogram(Stats::Scope& scope, const Stats::StatNameVec& names, uint64_t count) const; + void recordHistogram(Stats::Scope& scope, const Stats::StatNameVec& names, + Stats::Histogram::Unit unit, uint64_t count) const; static absl::string_view stripTrailingDot(absl::string_view prefix); Stats::StatName upstreamRqGroup(Code response_code) const; diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 31f6d819bcb8..86d528015742 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -66,8 +66,8 @@ namespace Http { GAUGE(downstream_cx_tx_bytes_buffered, Accumulate) \ GAUGE(downstream_cx_upgrades_active, Accumulate) \ GAUGE(downstream_rq_active, Accumulate) \ - HISTOGRAM(downstream_cx_length_ms) \ - HISTOGRAM(downstream_rq_time) + HISTOGRAM(downstream_cx_length_ms, Milliseconds) \ + HISTOGRAM(downstream_rq_time, Milliseconds) /** * Wrapper struct for connection manager stats. @see stats_macros.h diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 382e864a5ee2..073730d28b83 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -36,6 +36,7 @@ #include "common/network/utility.h" #include "common/router/config_impl.h" #include "common/runtime/runtime_impl.h" +#include "common/stats/timespan_impl.h" #include "absl/strings/escaping.h" #include "absl/strings/match.h" @@ -102,7 +103,8 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, Server::OverloadManager* overload_manager, TimeSource& time_source) : config_(config), stats_(config_.stats()), - conn_length_(new Stats::Timespan(stats_.named_.downstream_cx_length_ms_, time_source)), + conn_length_(new Stats::HistogramCompletableTimespanImpl( + stats_.named_.downstream_cx_length_ms_, time_source)), drain_close_(drain_close), random_generator_(random_generator), http_context_(http_context), runtime_(runtime), local_info_(local_info), cluster_manager_(cluster_manager), listener_stats_(config_.listenerStats()), @@ -457,7 +459,7 @@ void ConnectionManagerImpl::chargeTracingStats(const Tracing::Reason& tracing_re ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connection_manager) : connection_manager_(connection_manager), stream_id_(connection_manager.random_generator_.random()), - request_response_timespan_(new Stats::Timespan( + request_response_timespan_(new Stats::HistogramCompletableTimespanImpl( connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource()), upstream_options_(std::make_shared()) { diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index 9756586598c1..8fbdefcaa322 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -49,7 +49,7 @@ envoy_cc_library( "//include/envoy/http:header_map_interface", "//include/envoy/network:connection_interface", "//include/envoy/stats:stats_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//include/envoy/upstream:upstream_interface", "//source/common/common:linked_object", "//source/common/common:utility_lib", @@ -60,6 +60,7 @@ envoy_cc_library( "//source/common/http:headers_lib", "//source/common/network:utility_lib", "//source/common/runtime:runtime_lib", + "//source/common/stats:timespan_lib", "//source/common/upstream:upstream_lib", ], ) diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 44245f777dd0..68fc967cf708 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -15,6 +15,7 @@ #include "common/http/headers.h" #include "common/network/utility.h" #include "common/runtime/runtime_impl.h" +#include "common/stats/timespan_impl.h" #include "common/upstream/upstream_impl.h" #include "absl/strings/match.h" @@ -302,7 +303,7 @@ ConnPoolImpl::ActiveClient::ActiveClient(ConnPoolImpl& parent) connect_timer_(parent_.dispatcher_.createTimer([this]() -> void { onConnectTimeout(); })), remaining_requests_(parent_.host_->cluster().maxRequestsPerConnection()) { - parent_.conn_connect_ms_ = std::make_unique( + parent_.conn_connect_ms_ = std::make_unique( parent_.host_->cluster().stats().upstream_cx_connect_ms_, parent_.dispatcher_.timeSource()); Upstream::Host::CreateConnectionData data = parent_.host_->createConnection( parent_.dispatcher_, parent_.socket_options_, parent_.transport_socket_options_); @@ -315,7 +316,7 @@ ConnPoolImpl::ActiveClient::ActiveClient(ConnPoolImpl& parent) parent_.host_->cluster().stats().upstream_cx_http1_total_.inc(); parent_.host_->stats().cx_total_.inc(); parent_.host_->stats().cx_active_.inc(); - conn_length_ = std::make_unique( + conn_length_ = std::make_unique( parent_.host_->cluster().stats().upstream_cx_length_ms_, parent_.dispatcher_.timeSource()); connect_timer_->enableTimer(parent_.host_->cluster().connectTimeout()); parent_.host_->cluster().resourceManager(parent_.priority_).connections().inc(); diff --git a/source/common/http/http2/BUILD b/source/common/http/http2/BUILD index 2133d6238e67..ba48dc67903f 100644 --- a/source/common/http/http2/BUILD +++ b/source/common/http/http2/BUILD @@ -65,11 +65,12 @@ envoy_cc_library( "//include/envoy/event:timer_interface", "//include/envoy/http:conn_pool_interface", "//include/envoy/network:connection_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//include/envoy/upstream:upstream_interface", "//source/common/http:codec_client_lib", "//source/common/http:conn_pool_base_lib", "//source/common/network:utility_lib", + "//source/common/stats:timespan_lib", "//source/common/upstream:upstream_lib", ], ) diff --git a/source/common/http/http2/conn_pool.cc b/source/common/http/http2/conn_pool.cc index ceb125d5e567..feac31813d75 100644 --- a/source/common/http/http2/conn_pool.cc +++ b/source/common/http/http2/conn_pool.cc @@ -9,6 +9,7 @@ #include "common/http/http2/codec_impl.h" #include "common/network/utility.h" +#include "common/stats/timespan_impl.h" #include "common/upstream/upstream_impl.h" namespace Envoy { @@ -272,7 +273,7 @@ void ConnPoolImpl::onUpstreamReady() { ConnPoolImpl::ActiveClient::ActiveClient(ConnPoolImpl& parent) : parent_(parent), connect_timer_(parent_.dispatcher_.createTimer([this]() -> void { onConnectTimeout(); })) { - parent_.conn_connect_ms_ = std::make_unique( + parent_.conn_connect_ms_ = std::make_unique( parent_.host_->cluster().stats().upstream_cx_connect_ms_, parent_.dispatcher_.timeSource()); Upstream::Host::CreateConnectionData data = parent_.host_->createConnection( parent_.dispatcher_, parent_.socket_options_, parent_.transport_socket_options_); @@ -288,7 +289,7 @@ ConnPoolImpl::ActiveClient::ActiveClient(ConnPoolImpl& parent) parent_.host_->cluster().stats().upstream_cx_total_.inc(); parent_.host_->cluster().stats().upstream_cx_active_.inc(); parent_.host_->cluster().stats().upstream_cx_http2_total_.inc(); - conn_length_ = std::make_unique( + conn_length_ = std::make_unique( parent_.host_->cluster().stats().upstream_cx_length_ms_, parent_.dispatcher_.timeSource()); client_->setConnectionStats({parent_.host_->cluster().stats().upstream_cx_rx_bytes_total_, diff --git a/source/common/http/user_agent.cc b/source/common/http/user_agent.cc index fb6725c4e13b..8910a331f8f0 100644 --- a/source/common/http/user_agent.cc +++ b/source/common/http/user_agent.cc @@ -21,7 +21,9 @@ void UserAgent::completeConnectionLength(Stats::Timespan& span) { return; } - scope_->histogram(prefix_ + "downstream_cx_length_ms").recordValue(span.getRawDuration().count()); + // TODO(jmarantz): use stat-names here. + scope_->histogram(prefix_ + "downstream_cx_length_ms", Stats::Histogram::Unit::Milliseconds) + .recordValue(span.elapsed().count()); } void UserAgent::initializeFromHeaders(const HeaderMap& headers, const std::string& prefix, diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 19dcbf43d257..80bde943e334 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -236,6 +236,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "timespan_lib", + srcs = ["timespan_impl.cc"], + hdrs = ["timespan_impl.h"], + deps = [ + "//include/envoy/common:time_interface", + "//include/envoy/stats:stats_interface", + "//include/envoy/stats:timespan_interface", + ], +) + envoy_cc_library( name = "utility_lib", srcs = ["utility.cc"], diff --git a/source/common/stats/histogram_impl.h b/source/common/stats/histogram_impl.h index 0177d9f3317e..b3d330c48383 100644 --- a/source/common/stats/histogram_impl.h +++ b/source/common/stats/histogram_impl.h @@ -68,10 +68,10 @@ class HistogramImplHelper : public MetricImpl { */ class HistogramImpl : public HistogramImplHelper { public: - HistogramImpl(StatName name, Store& parent, const std::string& tag_extracted_name, + HistogramImpl(StatName name, Unit unit, Store& parent, const std::string& tag_extracted_name, const std::vector& tags) - : HistogramImplHelper(name, tag_extracted_name, tags, parent.symbolTable()), parent_(parent) { - } + : HistogramImplHelper(name, tag_extracted_name, tags, parent.symbolTable()), unit_(unit), + parent_(parent) {} ~HistogramImpl() override { // We must explicitly free the StatName here in order to supply the // SymbolTable reference. An RAII alternative would be to store a @@ -81,12 +81,15 @@ class HistogramImpl : public HistogramImplHelper { } // Stats::Histogram + Unit unit() const override { return unit_; }; void recordValue(uint64_t value) override { parent_.deliverHistogramToSinks(*this, value); } bool used() const override { return true; } SymbolTable& symbolTable() override { return parent_.symbolTable(); } private: + Unit unit_; + // This is used for delivering the histogram data to sinks. Store& parent_; }; @@ -104,6 +107,7 @@ class NullHistogramImpl : public HistogramImplHelper { bool used() const override { return false; } SymbolTable& symbolTable() override { return symbol_table_; } + Unit unit() const override { return Unit::Null; }; void recordValue(uint64_t) override {} private: diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index aa58676ae59a..002504171ddb 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -30,9 +30,9 @@ IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table) return alloc_.makeGauge(name, alloc_.symbolTable().toString(name), std::vector(), import_mode); }), - histograms_([this](StatName name) -> HistogramSharedPtr { + histograms_([this](StatName name, Histogram::Unit unit) -> HistogramSharedPtr { return HistogramSharedPtr(new HistogramImpl( - name, *this, alloc_.symbolTable().toString(name), std::vector())); + name, unit, *this, alloc_.symbolTable().toString(name), std::vector())); }), null_counter_(new NullCounterImpl(symbol_table)), null_gauge_(new NullGaugeImpl(symbol_table)) {} diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index cce741211df1..8b3c58e84f04 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -25,11 +25,13 @@ namespace Stats { */ template class IsolatedStatsCache { public: - using Allocator = std::function(StatName name)>; - using AllocatorImportMode = std::function(StatName, Gauge::ImportMode)>; + using CounterAllocator = std::function(StatName name)>; + using GaugeAllocator = std::function(StatName, Gauge::ImportMode)>; + using HistogramAllocator = std::function(StatName, Histogram::Unit)>; - IsolatedStatsCache(Allocator alloc) : alloc_(alloc) {} - IsolatedStatsCache(AllocatorImportMode alloc) : alloc_import_(alloc) {} + IsolatedStatsCache(CounterAllocator alloc) : counter_alloc_(alloc) {} + IsolatedStatsCache(GaugeAllocator alloc) : gauge_alloc_(alloc) {} + IsolatedStatsCache(HistogramAllocator alloc) : histogram_alloc_(alloc) {} Base& get(StatName name) { auto stat = stats_.find(name); @@ -37,7 +39,7 @@ template class IsolatedStatsCache { return *stat->second; } - RefcountPtr new_stat = alloc_(name); + RefcountPtr new_stat = counter_alloc_(name); stats_.emplace(new_stat->statName(), new_stat); return *new_stat; } @@ -48,7 +50,18 @@ template class IsolatedStatsCache { return *stat->second; } - RefcountPtr new_stat = alloc_import_(name, import_mode); + RefcountPtr new_stat = gauge_alloc_(name, import_mode); + stats_.emplace(new_stat->statName(), new_stat); + return *new_stat; + } + + Base& get(StatName name, Histogram::Unit unit) { + auto stat = stats_.find(name); + if (stat != stats_.end()) { + return *stat->second; + } + + RefcountPtr new_stat = histogram_alloc_(name, unit); stats_.emplace(new_stat->statName(), new_stat); return *new_stat; } @@ -75,8 +88,9 @@ template class IsolatedStatsCache { } StatNameHashMap> stats_; - Allocator alloc_; - AllocatorImportMode alloc_import_; + CounterAllocator counter_alloc_; + GaugeAllocator gauge_alloc_; + HistogramAllocator histogram_alloc_; }; class IsolatedStoreImpl : public StoreImpl { @@ -95,8 +109,8 @@ class IsolatedStoreImpl : public StoreImpl { } NullCounterImpl& nullCounter() { return *null_counter_; } NullGaugeImpl& nullGauge(const std::string&) override { return *null_gauge_; } - Histogram& histogramFromStatName(StatName name) override { - Histogram& histogram = histograms_.get(name); + Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { + Histogram& histogram = histograms_.get(name, unit); return histogram; } OptionalCounter findCounter(StatName name) const override { return counters_.find(name); } @@ -125,9 +139,9 @@ class IsolatedStoreImpl : public StoreImpl { StatNameManagedStorage storage(name, symbolTable()); return gaugeFromStatName(storage.statName(), import_mode); } - Histogram& histogram(const std::string& name) override { + Histogram& histogram(const std::string& name, Histogram::Unit unit) override { StatNameManagedStorage storage(name, symbolTable()); - return histogramFromStatName(storage.statName()); + return histogramFromStatName(storage.statName(), unit); } private: diff --git a/source/common/stats/scope_prefixer.cc b/source/common/stats/scope_prefixer.cc index f83265b0511e..e2a79b77a0dc 100644 --- a/source/common/stats/scope_prefixer.cc +++ b/source/common/stats/scope_prefixer.cc @@ -38,10 +38,10 @@ Gauge& ScopePrefixer::gaugeFromStatName(StatName name, Gauge::ImportMode import_ return scope_.gaugeFromStatName(StatName(stat_name_storage.get()), import_mode); } -Histogram& ScopePrefixer::histogramFromStatName(StatName name) { +Histogram& ScopePrefixer::histogramFromStatName(StatName name, Histogram::Unit unit) { Stats::SymbolTable::StoragePtr stat_name_storage = scope_.symbolTable().join({prefix_.statName(), name}); - return scope_.histogramFromStatName(StatName(stat_name_storage.get())); + return scope_.histogramFromStatName(StatName(stat_name_storage.get()), unit); } OptionalCounter ScopePrefixer::findCounter(StatName name) const { return scope_.findCounter(name); } diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h index f85cfc3ff098..0e86b470e5c4 100644 --- a/source/common/stats/scope_prefixer.h +++ b/source/common/stats/scope_prefixer.h @@ -19,7 +19,7 @@ class ScopePrefixer : public Scope { ScopePtr createScope(const std::string& name) override; Counter& counterFromStatName(StatName name) override; Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override; - Histogram& histogramFromStatName(StatName name) override; + Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override; void deliverHistogramToSinks(const Histogram& histograms, uint64_t val) override; Counter& counter(const std::string& name) override { @@ -30,9 +30,9 @@ class ScopePrefixer : public Scope { StatNameManagedStorage storage(name, symbolTable()); return gaugeFromStatName(storage.statName(), import_mode); } - Histogram& histogram(const std::string& name) override { + Histogram& histogram(const std::string& name, Histogram::Unit unit) override { StatNameManagedStorage storage(name, symbolTable()); - return histogramFromStatName(storage.statName()); + return histogramFromStatName(storage.statName(), unit); } OptionalCounter findCounter(StatName name) const override; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 9ae746cb2c04..1d45a960cd92 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -459,7 +459,8 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatName(StatName name, return gauge; } -Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name) { +Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name, + Histogram::Unit unit) { if (parent_.rejectsAll()) { return parent_.null_histogram_; } @@ -502,7 +503,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name) TagExtraction extraction(parent_, final_stat_name); RefcountPtr stat(new ParentHistogramImpl( - final_stat_name, parent_, *this, extraction.tagExtractedName(), extraction.tags())); + final_stat_name, unit, parent_, *this, extraction.tagExtractedName(), extraction.tags())); central_ref = ¢ral_cache_.histograms_[stat->statName()]; *central_ref = stat; } @@ -552,7 +553,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name, std::string tag_extracted_name = parent_.tagProducer().produceTags(symbolTable().toString(name), tags); TlsHistogramSharedPtr hist_tls_ptr( - new ThreadLocalHistogramImpl(name, tag_extracted_name, tags, symbolTable())); + new ThreadLocalHistogramImpl(name, parent.unit(), tag_extracted_name, tags, symbolTable())); parent.addTlsHistogram(hist_tls_ptr); @@ -562,12 +563,13 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name, return *hist_tls_ptr; } -ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(StatName name, +ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(StatName name, Histogram::Unit unit, const std::string& tag_extracted_name, const std::vector& tags, SymbolTable& symbol_table) - : HistogramImplHelper(name, tag_extracted_name, tags, symbol_table), current_active_(0), - used_(false), created_thread_id_(std::this_thread::get_id()), symbol_table_(symbol_table) { + : HistogramImplHelper(name, tag_extracted_name, tags, symbol_table), unit_(unit), + current_active_(0), used_(false), created_thread_id_(std::this_thread::get_id()), + symbol_table_(symbol_table) { histograms_[0] = hist_alloc(); histograms_[1] = hist_alloc(); } @@ -590,13 +592,13 @@ void ThreadLocalHistogramImpl::merge(histogram_t* target) { hist_clear(*other_histogram); } -ParentHistogramImpl::ParentHistogramImpl(StatName name, Store& parent, TlsScope& tls_scope, - absl::string_view tag_extracted_name, +ParentHistogramImpl::ParentHistogramImpl(StatName name, Histogram::Unit unit, Store& parent, + TlsScope& tls_scope, absl::string_view tag_extracted_name, const std::vector& tags) - : MetricImpl(name, tag_extracted_name, tags, parent.symbolTable()), parent_(parent), - tls_scope_(tls_scope), interval_histogram_(hist_alloc()), cumulative_histogram_(hist_alloc()), - interval_statistics_(interval_histogram_), cumulative_statistics_(cumulative_histogram_), - merged_(false) {} + : MetricImpl(name, tag_extracted_name, tags, parent.symbolTable()), unit_(unit), + parent_(parent), tls_scope_(tls_scope), interval_histogram_(hist_alloc()), + cumulative_histogram_(hist_alloc()), interval_statistics_(interval_histogram_), + cumulative_statistics_(cumulative_histogram_), merged_(false) {} ParentHistogramImpl::~ParentHistogramImpl() { MetricImpl::clear(symbolTable()); @@ -604,6 +606,8 @@ ParentHistogramImpl::~ParentHistogramImpl() { hist_free(cumulative_histogram_); } +Histogram::Unit ParentHistogramImpl::unit() const { return unit_; } + void ParentHistogramImpl::recordValue(uint64_t value) { Histogram& tls_histogram = tls_scope_.tlsHistogram(statName(), *this); tls_histogram.recordValue(value); diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index a82c5bfa8365..9d129e6b7543 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -30,8 +30,9 @@ namespace Stats { */ class ThreadLocalHistogramImpl : public HistogramImplHelper { public: - ThreadLocalHistogramImpl(StatName name, const std::string& tag_extracted_name, - const std::vector& tags, SymbolTable& symbol_table); + ThreadLocalHistogramImpl(StatName name, Histogram::Unit unit, + const std::string& tag_extracted_name, const std::vector& tags, + SymbolTable& symbol_table); ~ThreadLocalHistogramImpl() override; void merge(histogram_t* target); @@ -47,13 +48,19 @@ class ThreadLocalHistogramImpl : public HistogramImplHelper { } // Stats::Histogram + Histogram::Unit unit() const override { + // If at some point ThreadLocalHistogramImpl will hold a pointer to its parent we can just + // return parent's unit here and not store it separately. + return unit_; + } void recordValue(uint64_t value) override; - bool used() const override { return used_; } // Stats::Metric SymbolTable& symbolTable() override { return symbol_table_; } + bool used() const override { return used_; } private: + Histogram::Unit unit_; uint64_t otherHistogramIndex() const { return 1 - current_active_; } uint64_t current_active_; histogram_t* histograms_[2]; @@ -71,11 +78,14 @@ class TlsScope; */ class ParentHistogramImpl : public MetricImpl { public: - ParentHistogramImpl(StatName name, Store& parent, TlsScope& tls_scope, + ParentHistogramImpl(StatName name, Histogram::Unit unit, Store& parent, TlsScope& tls_scope, absl::string_view tag_extracted_name, const std::vector& tags); ~ParentHistogramImpl() override; void addTlsHistogram(const TlsHistogramSharedPtr& hist_ptr); + + // Stats::Histogram + Histogram::Unit unit() const override; void recordValue(uint64_t value) override; /** @@ -105,6 +115,7 @@ class ParentHistogramImpl : public MetricImpl { private: bool usedLockHeld() const EXCLUSIVE_LOCKS_REQUIRED(merge_lock_); + Histogram::Unit unit_; Store& parent_; TlsScope& tls_scope_; histogram_t* interval_histogram_; @@ -158,10 +169,12 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Gauge& gauge(const std::string& name, Gauge::ImportMode import_mode) override { return default_scope_->gauge(name, import_mode); } - Histogram& histogramFromStatName(StatName name) override { - return default_scope_->histogramFromStatName(name); + Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { + return default_scope_->histogramFromStatName(name, unit); + } + Histogram& histogram(const std::string& name, Histogram::Unit unit) override { + return default_scope_->histogram(name, unit); } - Histogram& histogram(const std::string& name) override { return default_scope_->histogram(name); } NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } const SymbolTable& constSymbolTable() const override { return alloc_.constSymbolTable(); } SymbolTable& symbolTable() override { return alloc_.symbolTable(); } @@ -248,7 +261,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Counter& counterFromStatName(StatName name) override; void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) override; Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override; - Histogram& histogramFromStatName(StatName name) override; + Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override; Histogram& tlsHistogram(StatName name, ParentHistogramImpl& parent) override; ScopePtr createScope(const std::string& name) override { return parent_.createScope(symbolTable().toString(prefix_.statName()) + "." + name); @@ -264,9 +277,9 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo StatNameManagedStorage storage(name, symbolTable()); return gaugeFromStatName(storage.statName(), import_mode); } - Histogram& histogram(const std::string& name) override { + Histogram& histogram(const std::string& name, Histogram::Unit unit) override { StatNameManagedStorage storage(name, symbolTable()); - return histogramFromStatName(storage.statName()); + return histogramFromStatName(storage.statName(), unit); } NullGaugeImpl& nullGauge(const std::string&) override { return parent_.null_gauge_; } diff --git a/source/common/stats/timespan_impl.cc b/source/common/stats/timespan_impl.cc new file mode 100644 index 000000000000..0ba71eca416c --- /dev/null +++ b/source/common/stats/timespan_impl.cc @@ -0,0 +1,56 @@ +#include "common/stats/timespan_impl.h" + +#include "common/common/fmt.h" + +namespace Envoy { +namespace Stats { + +HistogramCompletableTimespanImpl::HistogramCompletableTimespanImpl(Histogram& histogram, + TimeSource& time_source) + : time_source_(time_source), histogram_(histogram), start_(time_source.monotonicTime()) { + ensureTimeHistogram(histogram); +} + +std::chrono::milliseconds HistogramCompletableTimespanImpl::elapsed() const { + return HistogramCompletableTimespanImpl::elapsedDuration(); +} + +void HistogramCompletableTimespanImpl::complete() { histogram_.recordValue(tickCount()); } + +void HistogramCompletableTimespanImpl::ensureTimeHistogram(const Histogram& histogram) const { + switch (histogram.unit()) { + case Histogram::Unit::Null: + case Histogram::Unit::Microseconds: + case Histogram::Unit::Milliseconds: + return; + case Histogram::Unit::Unspecified: + case Histogram::Unit::Bytes: + RELEASE_ASSERT( + false, + fmt::format("Cannot create a timespan flushing the duration to histogram '{}' because " + "it does not measure time. This is a programming error, either pass a " + "histogram measuring time or fix the unit of the passed histogram.", + histogram.name())); + } + + NOT_REACHED_GCOVR_EXCL_LINE; +} + +uint64_t HistogramCompletableTimespanImpl::tickCount() const { + switch (histogram_.unit()) { + case Histogram::Unit::Null: + return 0; + case Histogram::Unit::Microseconds: + return HistogramCompletableTimespanImpl::elapsedDuration().count(); + case Histogram::Unit::Milliseconds: + return HistogramCompletableTimespanImpl::elapsedDuration().count(); + case Histogram::Unit::Unspecified: + case Histogram::Unit::Bytes: + NOT_REACHED_GCOVR_EXCL_LINE; + } + + NOT_REACHED_GCOVR_EXCL_LINE; +} + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/timespan_impl.h b/source/common/stats/timespan_impl.h new file mode 100644 index 000000000000..baca0e5174d2 --- /dev/null +++ b/source/common/stats/timespan_impl.h @@ -0,0 +1,38 @@ +#pragma once + +#include "envoy/common/time.h" +#include "envoy/stats/histogram.h" +#include "envoy/stats/stats.h" +#include "envoy/stats/timespan.h" + +namespace Envoy { +namespace Stats { + +/** + * An individual timespan that flushes its measured value to the histogram on completion. + * The start time is captured on construction. The timespan must be + * completed via complete() for it to be stored. If the timespan is deleted this will be treated as + * a cancellation. The target histogram must represent a quantity of time. + */ +class HistogramCompletableTimespanImpl : public CompletableTimespan { +public: + HistogramCompletableTimespanImpl(Histogram& histogram, TimeSource& time_source); + + // Stats::CompletableTimespan + std::chrono::milliseconds elapsed() const override; + void complete() override; + +private: + void ensureTimeHistogram(const Histogram& histogram) const; + template TimeUnit elapsedDuration() const { + return std::chrono::duration_cast(time_source_.monotonicTime() - start_); + } + uint64_t tickCount() const; + + TimeSource& time_source_; + Histogram& histogram_; + const MonotonicTime start_; +}; + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/tcp/BUILD b/source/common/tcp/BUILD index a201fb01ebec..a9e3b948a1b2 100644 --- a/source/common/tcp/BUILD +++ b/source/common/tcp/BUILD @@ -19,13 +19,14 @@ envoy_cc_library( "//include/envoy/event:timer_interface", "//include/envoy/network:connection_interface", "//include/envoy/stats:stats_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//include/envoy/tcp:conn_pool_interface", "//include/envoy/upstream:upstream_interface", "//source/common/common:linked_object", "//source/common/common:utility_lib", "//source/common/network:filter_lib", "//source/common/network:utility_lib", + "//source/common/stats:timespan_lib", "//source/common/upstream:upstream_lib", ], ) diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc index f7b7467f8982..76f6d453be78 100644 --- a/source/common/tcp/conn_pool.cc +++ b/source/common/tcp/conn_pool.cc @@ -6,6 +6,7 @@ #include "envoy/event/timer.h" #include "envoy/upstream/upstream.h" +#include "common/stats/timespan_impl.h" #include "common/upstream/upstream_impl.h" namespace Envoy { @@ -347,7 +348,7 @@ ConnPoolImpl::ActiveConn::ActiveConn(ConnPoolImpl& parent) connect_timer_(parent_.dispatcher_.createTimer([this]() -> void { onConnectTimeout(); })), remaining_requests_(parent_.host_->cluster().maxRequestsPerConnection()), timed_out_(false) { - parent_.conn_connect_ms_ = std::make_unique( + parent_.conn_connect_ms_ = std::make_unique( parent_.host_->cluster().stats().upstream_cx_connect_ms_, parent_.dispatcher_.timeSource()); Upstream::Host::CreateConnectionData data = parent_.host_->createConnection( @@ -367,7 +368,7 @@ ConnPoolImpl::ActiveConn::ActiveConn(ConnPoolImpl& parent) parent_.host_->cluster().stats().upstream_cx_active_.inc(); parent_.host_->stats().cx_total_.inc(); parent_.host_->stats().cx_active_.inc(); - conn_length_ = std::make_unique( + conn_length_ = std::make_unique( parent_.host_->cluster().stats().upstream_cx_length_ms_, parent_.dispatcher_.timeSource()); connect_timer_->enableTimer(parent_.host_->cluster().connectTimeout()); parent_.host_->cluster().resourceManager(parent_.priority_).connections().inc(); diff --git a/source/common/tcp_proxy/BUILD b/source/common/tcp_proxy/BUILD index 29e8f2824900..faa924befffa 100644 --- a/source/common/tcp_proxy/BUILD +++ b/source/common/tcp_proxy/BUILD @@ -23,7 +23,7 @@ envoy_cc_library( "//include/envoy/server:filter_config_interface", "//include/envoy/stats:stats_interface", "//include/envoy/stats:stats_macros", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//include/envoy/stream_info:filter_state_interface", "//include/envoy/tcp:conn_pool_interface", "//include/envoy/upstream:cluster_manager_interface", diff --git a/source/extensions/filters/http/dynamo/dynamo_filter.cc b/source/extensions/filters/http/dynamo/dynamo_filter.cc index 8312d768f9fe..966f42837c15 100644 --- a/source/extensions/filters/http/dynamo/dynamo_filter.cc +++ b/source/extensions/filters/http/dynamo/dynamo_filter.cc @@ -187,11 +187,17 @@ void DynamoFilter::chargeStatsPerEntity(const std::string& entity, const std::st stats_->counter({entity_type_name, entity_name, total_group}).inc(); stats_->counter({entity_type_name, entity_name, total_name}).inc(); - stats_->histogram({entity_type_name, entity_name, stats_->upstream_rq_time_}) + stats_ + ->histogram({entity_type_name, entity_name, stats_->upstream_rq_time_}, + Stats::Histogram::Unit::Milliseconds) .recordValue(latency.count()); const Stats::StatName time_group = stats_->upstream_rq_time_groups_[group_index]; - stats_->histogram({entity_type_name, entity_name, time_group}).recordValue(latency.count()); - stats_->histogram({entity_type_name, entity_name, time_name}).recordValue(latency.count()); + stats_ + ->histogram({entity_type_name, entity_name, time_group}, Stats::Histogram::Unit::Milliseconds) + .recordValue(latency.count()); + stats_ + ->histogram({entity_type_name, entity_name, time_name}, Stats::Histogram::Unit::Milliseconds) + .recordValue(latency.count()); } void DynamoFilter::chargeUnProcessedKeysStats(const Json::Object& json_body) { diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.cc b/source/extensions/filters/http/dynamo/dynamo_stats.cc index 29e2ef1d00bc..bf30a27a0f22 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.cc +++ b/source/extensions/filters/http/dynamo/dynamo_stats.cc @@ -59,9 +59,10 @@ Stats::Counter& DynamoStats::counter(const Stats::StatNameVec& names) { return scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())); } -Stats::Histogram& DynamoStats::histogram(const Stats::StatNameVec& names) { +Stats::Histogram& DynamoStats::histogram(const Stats::StatNameVec& names, + Stats::Histogram::Unit unit) { const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); - return scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get())); + return scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get()), unit); } Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_name, diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.h b/source/extensions/filters/http/dynamo/dynamo_stats.h index 8b802592877e..628c0c476fba 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.h +++ b/source/extensions/filters/http/dynamo/dynamo_stats.h @@ -17,7 +17,7 @@ class DynamoStats { DynamoStats(Stats::Scope& scope, const std::string& prefix); Stats::Counter& counter(const Stats::StatNameVec& names); - Stats::Histogram& histogram(const Stats::StatNameVec& names); + Stats::Histogram& histogram(const Stats::StatNameVec& names, Stats::Histogram::Unit unit); /** * Creates the partition id stats string. The stats format is diff --git a/source/extensions/filters/network/common/redis/BUILD b/source/extensions/filters/network/common/redis/BUILD index b50d4ebfa9f8..052759278ec2 100644 --- a/source/extensions/filters/network/common/redis/BUILD +++ b/source/extensions/filters/network/common/redis/BUILD @@ -60,7 +60,7 @@ envoy_cc_library( ":codec_lib", ":utility_lib", "//include/envoy/router:router_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//include/envoy/thread_local:thread_local_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/buffer:buffer_lib", @@ -90,9 +90,10 @@ envoy_cc_library( ":codec_interface", ":supported_commands_lib", "//include/envoy/stats:stats_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//source/common/common:to_lower_table_lib", "//source/common/common:utility_lib", "//source/common/stats:symbol_table_lib", + "//source/common/stats:timespan_lib", ], ) diff --git a/source/extensions/filters/network/common/redis/client_impl.h b/source/extensions/filters/network/common/redis/client_impl.h index be0d1f8119be..1b6a4a978c28 100644 --- a/source/extensions/filters/network/common/redis/client_impl.h +++ b/source/extensions/filters/network/common/redis/client_impl.h @@ -114,8 +114,8 @@ class ClientImpl : public Client, public DecoderCallbacks, public Network::Conne PoolCallbacks& callbacks_; Stats::StatName command_; bool canceled_{}; - Stats::CompletableTimespanPtr aggregate_request_timer_; - Stats::CompletableTimespanPtr command_request_timer_; + Stats::TimespanPtr aggregate_request_timer_; + Stats::TimespanPtr command_request_timer_; }; void onConnectOrOpTimeout(); diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.cc b/source/extensions/filters/network/common/redis/redis_command_stats.cc index 6178f363c6d2..71a2f46ce36a 100644 --- a/source/extensions/filters/network/common/redis/redis_command_stats.cc +++ b/source/extensions/filters/network/common/redis/redis_command_stats.cc @@ -1,5 +1,7 @@ #include "extensions/filters/network/common/redis/redis_command_stats.h" +#include "common/stats/timespan_impl.h" + #include "extensions/filters/network/common/redis/supported_commands.h" namespace Envoy { @@ -38,23 +40,26 @@ Stats::Counter& RedisCommandStats::counter(Stats::Scope& scope, } Stats::Histogram& RedisCommandStats::histogram(Stats::Scope& scope, - const Stats::StatNameVec& stat_names) { + const Stats::StatNameVec& stat_names, + Stats::Histogram::Unit unit) { const Stats::SymbolTable::StoragePtr storage_ptr = symbol_table_.join(stat_names); Stats::StatName full_stat_name = Stats::StatName(storage_ptr.get()); - return scope.histogramFromStatName(full_stat_name); + return scope.histogramFromStatName(full_stat_name, unit); } -Stats::CompletableTimespanPtr -RedisCommandStats::createCommandTimer(Stats::Scope& scope, Stats::StatName command, - Envoy::TimeSource& time_source) { - return std::make_unique>( - histogram(scope, {prefix_, command, latency_}), time_source); +Stats::TimespanPtr RedisCommandStats::createCommandTimer(Stats::Scope& scope, + Stats::StatName command, + Envoy::TimeSource& time_source) { + return std::make_unique( + histogram(scope, {prefix_, command, latency_}, Stats::Histogram::Unit::Microseconds), + time_source); } -Stats::CompletableTimespanPtr -RedisCommandStats::createAggregateTimer(Stats::Scope& scope, Envoy::TimeSource& time_source) { - return std::make_unique>( - histogram(scope, {prefix_, upstream_rq_time_}), time_source); +Stats::TimespanPtr RedisCommandStats::createAggregateTimer(Stats::Scope& scope, + Envoy::TimeSource& time_source) { + return std::make_unique( + histogram(scope, {prefix_, upstream_rq_time_}, Stats::Histogram::Unit::Microseconds), + time_source); } Stats::StatName RedisCommandStats::getCommandFromRequest(const RespValue& request) { diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.h b/source/extensions/filters/network/common/redis/redis_command_stats.h index 556cc00e0150..a6602b682cb2 100644 --- a/source/extensions/filters/network/common/redis/redis_command_stats.h +++ b/source/extensions/filters/network/common/redis/redis_command_stats.h @@ -30,11 +30,11 @@ class RedisCommandStats { } Stats::Counter& counter(Stats::Scope& scope, const Stats::StatNameVec& stat_names); - Stats::Histogram& histogram(Stats::Scope& scope, const Stats::StatNameVec& stat_names); - Stats::CompletableTimespanPtr createCommandTimer(Stats::Scope& scope, Stats::StatName command, - Envoy::TimeSource& time_source); - Stats::CompletableTimespanPtr createAggregateTimer(Stats::Scope& scope, - Envoy::TimeSource& time_source); + Stats::Histogram& histogram(Stats::Scope& scope, const Stats::StatNameVec& stat_names, + Stats::Histogram::Unit unit); + Stats::TimespanPtr createCommandTimer(Stats::Scope& scope, Stats::StatName command, + Envoy::TimeSource& time_source); + Stats::TimespanPtr createAggregateTimer(Stats::Scope& scope, Envoy::TimeSource& time_source); Stats::StatName getCommandFromRequest(const RespValue& request); void updateStatsTotal(Stats::Scope& scope, Stats::StatName command); void updateStats(Stats::Scope& scope, Stats::StatName command, const bool success); diff --git a/source/extensions/filters/network/dubbo_proxy/BUILD b/source/extensions/filters/network/dubbo_proxy/BUILD index 27805df27a8d..252e16de4b9f 100644 --- a/source/extensions/filters/network/dubbo_proxy/BUILD +++ b/source/extensions/filters/network/dubbo_proxy/BUILD @@ -215,13 +215,14 @@ envoy_cc_library( "//include/envoy/network:connection_interface", "//include/envoy/network:filter_interface", "//include/envoy/stats:stats_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//source/common/buffer:buffer_lib", "//source/common/buffer:watermark_buffer_lib", "//source/common/common:assert_lib", "//source/common/common:linked_object", "//source/common/common:logger_lib", "//source/common/network:filter_lib", + "//source/common/stats:timespan_lib", "//source/common/stream_info:stream_info_lib", "//source/extensions/filters/network/dubbo_proxy/filters:filter_interface", "//source/extensions/filters/network/dubbo_proxy/router:router_interface", diff --git a/source/extensions/filters/network/dubbo_proxy/active_message.cc b/source/extensions/filters/network/dubbo_proxy/active_message.cc index 8cb8a378b3ff..e672ef343716 100644 --- a/source/extensions/filters/network/dubbo_proxy/active_message.cc +++ b/source/extensions/filters/network/dubbo_proxy/active_message.cc @@ -1,5 +1,7 @@ #include "extensions/filters/network/dubbo_proxy/active_message.h" +#include "common/stats/timespan_impl.h" + #include "extensions/filters/network/dubbo_proxy/app_exception.h" #include "extensions/filters/network/dubbo_proxy/conn_manager.h" @@ -182,7 +184,7 @@ void ActiveMessageEncoderFilter::continueEncoding() { // class ActiveMessage ActiveMessage::ActiveMessage(ConnectionManager& parent) - : parent_(parent), request_timer_(std::make_unique( + : parent_(parent), request_timer_(std::make_unique( parent_.stats().request_time_ms_, parent.time_system())), request_id_(-1), stream_id_(parent.random_generator().random()), stream_info_(parent.time_system()), pending_stream_decoded_(false), diff --git a/source/extensions/filters/network/dubbo_proxy/stats.h b/source/extensions/filters/network/dubbo_proxy/stats.h index b6084025d3d1..19093b86724d 100644 --- a/source/extensions/filters/network/dubbo_proxy/stats.h +++ b/source/extensions/filters/network/dubbo_proxy/stats.h @@ -33,7 +33,7 @@ namespace DubboProxy { COUNTER(response_error_caused_connection_close) \ COUNTER(response_success) \ GAUGE(request_active, Accumulate) \ - HISTOGRAM(request_time_ms) + HISTOGRAM(request_time_ms, Milliseconds) /** * Struct definition for all dubbo proxy stats. @see stats_macros.h diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc index d569a73e1bad..bf9e90ce105c 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc @@ -44,9 +44,10 @@ void MongoStats::incCounter(const std::vector& names) { scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc(); } -void MongoStats::recordHistogram(const std::vector& names, uint64_t sample) { +void MongoStats::recordHistogram(const std::vector& names, + Stats::Histogram::Unit unit, uint64_t sample) { const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); - scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get())).recordValue(sample); + scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get()), unit).recordValue(sample); } } // namespace MongoProxy diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.h b/source/extensions/filters/network/mongo_proxy/mongo_stats.h index 57373693daa9..a1a53e80067f 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.h +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.h @@ -18,7 +18,8 @@ class MongoStats { MongoStats(Stats::Scope& scope, absl::string_view prefix); void incCounter(const std::vector& names); - void recordHistogram(const std::vector& names, uint64_t sample); + void recordHistogram(const std::vector& names, Stats::Histogram::Unit unit, + uint64_t sample); /** * Finds or creates a StatName by string, taking a global lock if needed. diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc index 81618c35880d..f8ae0c5f747f 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.cc +++ b/source/extensions/filters/network/mongo_proxy/proxy.cc @@ -302,13 +302,15 @@ void ProxyFilter::chargeReplyStats(ActiveQuery& active_query, Stats::StatNameVec // names to its original state upon return. const size_t orig_size = names.size(); names.push_back(mongo_stats_->reply_num_docs_); - mongo_stats_->recordHistogram(names, message.documents().size()); + mongo_stats_->recordHistogram(names, Stats::Histogram::Unit::Unspecified, + message.documents().size()); names[orig_size] = mongo_stats_->reply_size_; - mongo_stats_->recordHistogram(names, reply_documents_byte_size); + mongo_stats_->recordHistogram(names, Stats::Histogram::Unit::Bytes, reply_documents_byte_size); names[orig_size] = mongo_stats_->reply_time_ms_; - mongo_stats_->recordHistogram(names, std::chrono::duration_cast( - time_source_.monotonicTime() - active_query.start_time_) - .count()); + mongo_stats_->recordHistogram(names, Stats::Histogram::Unit::Milliseconds, + std::chrono::duration_cast( + time_source_.monotonicTime() - active_query.start_time_) + .count()); names.resize(orig_size); } diff --git a/source/extensions/filters/network/redis_proxy/BUILD b/source/extensions/filters/network/redis_proxy/BUILD index 700acf9c616c..d277405a43f1 100644 --- a/source/extensions/filters/network/redis_proxy/BUILD +++ b/source/extensions/filters/network/redis_proxy/BUILD @@ -58,11 +58,12 @@ envoy_cc_library( ":command_splitter_interface", ":router_interface", "//include/envoy/stats:stats_macros", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", "//source/common/common:to_lower_table_lib", "//source/common/common:utility_lib", + "//source/common/stats:timespan_lib", "//source/extensions/filters/network/common/redis:client_lib", "//source/extensions/filters/network/common/redis:supported_commands_lib", ], diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc index c148825a7909..c8dc37f0319e 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -157,9 +157,9 @@ void SingleServerRequest::cancel() { SplitRequestPtr SimpleRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros) { + TimeSource& time_source) { std::unique_ptr request_ptr{ - new SimpleRequest(callbacks, command_stats, time_source, latency_in_micros)}; + new SimpleRequest(callbacks, command_stats, time_source)}; const auto route = router.upstreamPool(incoming_request->asArray()[1].asString()); if (route) { @@ -180,7 +180,7 @@ SplitRequestPtr SimpleRequest::create(Router& router, SplitRequestPtr EvalRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros) { + TimeSource& time_source) { // EVAL looks like: EVAL script numkeys key [key ...] arg [arg ...] // Ensure there are at least three args to the command or it cannot be hashed. if (incoming_request->asArray().size() < 4) { @@ -189,8 +189,7 @@ SplitRequestPtr EvalRequest::create(Router& router, Common::Redis::RespValuePtr& return nullptr; } - std::unique_ptr request_ptr{ - new EvalRequest(callbacks, command_stats, time_source, latency_in_micros)}; + std::unique_ptr request_ptr{new EvalRequest(callbacks, command_stats, time_source)}; const auto route = router.upstreamPool(incoming_request->asArray()[3].asString()); if (route) { @@ -233,9 +232,8 @@ void FragmentedRequest::onChildFailure(uint32_t index) { SplitRequestPtr MGETRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros) { - std::unique_ptr request_ptr{ - new MGETRequest(callbacks, command_stats, time_source, latency_in_micros)}; + TimeSource& time_source) { + std::unique_ptr request_ptr{new MGETRequest(callbacks, command_stats, time_source)}; request_ptr->num_pending_responses_ = incoming_request->asArray().size() - 1; request_ptr->pending_requests_.reserve(request_ptr->num_pending_responses_); @@ -358,14 +356,13 @@ void MGETRequest::recreate(Common::Redis::RespValue& request, uint32_t index) { SplitRequestPtr MSETRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros) { + TimeSource& time_source) { if ((incoming_request->asArray().size() - 1) % 2 != 0) { onWrongNumberOfArguments(callbacks, *incoming_request); command_stats.error_.inc(); return nullptr; } - std::unique_ptr request_ptr{ - new MSETRequest(callbacks, command_stats, time_source, latency_in_micros)}; + std::unique_ptr request_ptr{new MSETRequest(callbacks, command_stats, time_source)}; request_ptr->num_pending_responses_ = (incoming_request->asArray().size() - 1) / 2; request_ptr->pending_requests_.reserve(request_ptr->num_pending_responses_); @@ -460,9 +457,9 @@ SplitRequestPtr SplitKeysSumResultRequest::create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros) { + TimeSource& time_source) { std::unique_ptr request_ptr{ - new SplitKeysSumResultRequest(callbacks, command_stats, time_source, latency_in_micros)}; + new SplitKeysSumResultRequest(callbacks, command_stats, time_source)}; request_ptr->num_pending_responses_ = incoming_request->asArray().size() - 1; request_ptr->pending_requests_.reserve(request_ptr->num_pending_responses_); @@ -555,22 +552,25 @@ InstanceImpl::InstanceImpl(RouterPtr&& router, Stats::Scope& scope, const std::s eval_command_handler_(*router_), mget_handler_(*router_), mset_handler_(*router_), split_keys_sum_result_handler_(*router_), stats_{ALL_COMMAND_SPLITTER_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix + "splitter."))}, - latency_in_micros_(latency_in_micros), time_source_(time_source) { + time_source_(time_source) { for (const std::string& command : Common::Redis::SupportedCommands::simpleCommands()) { - addHandler(scope, stat_prefix, command, simple_command_handler_); + addHandler(scope, stat_prefix, command, latency_in_micros, simple_command_handler_); } for (const std::string& command : Common::Redis::SupportedCommands::evalCommands()) { - addHandler(scope, stat_prefix, command, eval_command_handler_); + addHandler(scope, stat_prefix, command, latency_in_micros, eval_command_handler_); } for (const std::string& command : Common::Redis::SupportedCommands::hashMultipleSumResultCommands()) { - addHandler(scope, stat_prefix, command, split_keys_sum_result_handler_); + addHandler(scope, stat_prefix, command, latency_in_micros, split_keys_sum_result_handler_); } - addHandler(scope, stat_prefix, Common::Redis::SupportedCommands::mget(), mget_handler_); - addHandler(scope, stat_prefix, Common::Redis::SupportedCommands::mset(), mset_handler_); + addHandler(scope, stat_prefix, Common::Redis::SupportedCommands::mget(), latency_in_micros, + mget_handler_); + + addHandler(scope, stat_prefix, Common::Redis::SupportedCommands::mset(), latency_in_micros, + mset_handler_); } SplitRequestPtr InstanceImpl::makeRequest(Common::Redis::RespValuePtr&& request, @@ -629,7 +629,7 @@ SplitRequestPtr InstanceImpl::makeRequest(Common::Redis::RespValuePtr&& request, ENVOY_LOG(debug, "redis: splitting '{}'", request->toString()); handler->command_stats_.total_.inc(); SplitRequestPtr request_ptr = handler->handler_.get().startRequest( - std::move(request), callbacks, handler->command_stats_, time_source_, latency_in_micros_); + std::move(request), callbacks, handler->command_stats_, time_source_); return request_ptr; } @@ -639,15 +639,21 @@ void InstanceImpl::onInvalidRequest(SplitCallbacks& callbacks) { } void InstanceImpl::addHandler(Stats::Scope& scope, const std::string& stat_prefix, - const std::string& name, CommandHandler& handler) { + const std::string& name, bool latency_in_micros, + CommandHandler& handler) { std::string to_lower_name(name); to_lower_table_.toLowerCase(to_lower_name); const std::string command_stat_prefix = fmt::format("{}command.{}.", stat_prefix, to_lower_name); + Stats::StatNameManagedStorage storage{command_stat_prefix + std::string("latency"), + scope.symbolTable()}; handler_lookup_table_.add( to_lower_name.c_str(), std::make_shared(HandlerData{ - CommandStats{ALL_COMMAND_STATS(POOL_COUNTER_PREFIX(scope, command_stat_prefix), - POOL_HISTOGRAM_PREFIX(scope, command_stat_prefix))}, + CommandStats{ALL_COMMAND_STATS(POOL_COUNTER_PREFIX(scope, command_stat_prefix)) + scope.histogramFromStatName(storage.statName(), + latency_in_micros + ? Stats::Histogram::Unit::Microseconds + : Stats::Histogram::Unit::Milliseconds)}, handler})); } diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h index f4ee4c6f79d4..f4c5f1255cc5 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.h @@ -13,6 +13,7 @@ #include "common/common/to_lower_table.h" #include "common/common/utility.h" #include "common/singleton/const_singleton.h" +#include "common/stats/timespan_impl.h" #include "extensions/filters/network/common/redis/client_impl.h" #include "extensions/filters/network/redis_proxy/command_splitter.h" @@ -45,18 +46,18 @@ class Utility { * All command level stats. @see stats_macros.h */ // clang-format off -#define ALL_COMMAND_STATS(COUNTER, HISTOGRAM) \ +#define ALL_COMMAND_STATS(COUNTER) \ COUNTER(total) \ COUNTER(success) \ - COUNTER(error) \ - HISTOGRAM(latency) + COUNTER(error) // clang-format on /** * Struct definition for all command stats. @see stats_macros.h */ struct CommandStats { - ALL_COMMAND_STATS(GENERATE_COUNTER_STRUCT, GENERATE_HISTOGRAM_STRUCT) + ALL_COMMAND_STATS(GENERATE_COUNTER_STRUCT) + Envoy::Stats::Histogram& latency_; }; class CommandHandler { @@ -65,7 +66,7 @@ class CommandHandler { virtual SplitRequestPtr startRequest(Common::Redis::RespValuePtr&& request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros) PURE; + TimeSource& time_source) PURE; }; class CommandHandlerBase { @@ -81,18 +82,13 @@ class SplitRequestBase : public SplitRequest { const Common::Redis::RespValue& request); void updateStats(const bool success); - SplitRequestBase(CommandStats& command_stats, TimeSource& time_source, bool latency_in_micros) + SplitRequestBase(CommandStats& command_stats, TimeSource& time_source) : command_stats_(command_stats) { - if (latency_in_micros) { - command_latency_ = std::make_unique>( - command_stats_.latency_, time_source); - } else { - command_latency_ = std::make_unique>( - command_stats_.latency_, time_source); - } + command_latency_ = std::make_unique( + command_stats_.latency_, time_source); } CommandStats& command_stats_; - Stats::CompletableTimespanPtr command_latency_; + Stats::TimespanPtr command_latency_; }; /** @@ -112,8 +108,8 @@ class SingleServerRequest : public SplitRequestBase, public Common::Redis::Clien protected: SingleServerRequest(SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros) - : SplitRequestBase(command_stats, time_source, latency_in_micros), callbacks_(callbacks) {} + TimeSource& time_source) + : SplitRequestBase(command_stats, time_source), callbacks_(callbacks) {} SplitCallbacks& callbacks_; ConnPool::InstanceSharedPtr conn_pool_; @@ -128,12 +124,11 @@ class SimpleRequest : public SingleServerRequest { public: static SplitRequestPtr create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros); + TimeSource& time_source); private: - SimpleRequest(SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, - bool latency_in_micros) - : SingleServerRequest(callbacks, command_stats, time_source, latency_in_micros) {} + SimpleRequest(SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source) + : SingleServerRequest(callbacks, command_stats, time_source) {} }; /** @@ -143,12 +138,11 @@ class EvalRequest : public SingleServerRequest { public: static SplitRequestPtr create(Router& router, Common::Redis::RespValuePtr&& incoming_request, SplitCallbacks& callbacks, CommandStats& command_stats, - TimeSource& time_source, bool latency_in_micros); + TimeSource& time_source); private: - EvalRequest(SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, - bool latency_in_micros) - : SingleServerRequest(callbacks, command_stats, time_source, latency_in_micros) {} + EvalRequest(SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source) + : SingleServerRequest(callbacks, command_stats, time_source) {} }; /** @@ -164,9 +158,8 @@ class FragmentedRequest : public SplitRequestBase { void cancel() override; protected: - FragmentedRequest(SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source, - bool latency_in_micros) - : SplitRequestBase(command_stats, time_source, latency_in_micros), callbacks_(callbacks) {} + FragmentedRequest(SplitCallbacks& callbacks, CommandStats& command_stats, TimeSource& time_source) + : SplitRequestBase(command_stats, time_source), callbacks_(callbacks) {} struct PendingRequest : public Common::Redis::Client::PoolCallbacks { PendingRequest(FragmentedRequest& parent, uint32_t index) : parent_(parent), index_(index) {} @@ -210,12 +203,11 @@ class MGETRequest : public FragmentedRequest, Logger::Loggable { using HandlerDataPtr = std::shared_ptr; void addHandler(Stats::Scope& scope, const std::string& stat_prefix, const std::string& name, - CommandHandler& handler); + bool latency_in_micros, CommandHandler& handler); void onInvalidRequest(SplitCallbacks& callbacks); RouterPtr router_; @@ -329,7 +318,6 @@ class InstanceImpl : public Instance, Logger::Loggable { TrieLookupTable handler_lookup_table_; InstanceStats stats_; const ToLowerTable to_lower_table_; - const bool latency_in_micros_; TimeSource& time_source_; }; diff --git a/source/extensions/filters/network/thrift_proxy/BUILD b/source/extensions/filters/network/thrift_proxy/BUILD index 31988a845f78..7ed10d835209 100644 --- a/source/extensions/filters/network/thrift_proxy/BUILD +++ b/source/extensions/filters/network/thrift_proxy/BUILD @@ -75,12 +75,13 @@ envoy_cc_library( "//include/envoy/network:connection_interface", "//include/envoy/network:filter_interface", "//include/envoy/stats:stats_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//source/common/buffer:buffer_lib", "//source/common/common:assert_lib", "//source/common/common:linked_object", "//source/common/common:logger_lib", "//source/common/network:filter_lib", + "//source/common/stats:timespan_lib", "//source/common/stream_info:stream_info_lib", "//source/extensions/filters/network/thrift_proxy/router:router_interface", ], diff --git a/source/extensions/filters/network/thrift_proxy/conn_manager.h b/source/extensions/filters/network/thrift_proxy/conn_manager.h index 2188ad7ee9eb..54331d020b38 100644 --- a/source/extensions/filters/network/thrift_proxy/conn_manager.h +++ b/source/extensions/filters/network/thrift_proxy/conn_manager.h @@ -10,6 +10,7 @@ #include "common/buffer/buffer_impl.h" #include "common/common/linked_object.h" #include "common/common/logger.h" +#include "common/stats/timespan_impl.h" #include "common/stream_info/stream_info_impl.h" #include "extensions/filters/network/thrift_proxy/decoder.h" @@ -153,8 +154,8 @@ class ConnectionManager : public Network::ReadFilter, public ThriftFilters::DecoderFilterCallbacks, public ThriftFilters::FilterChainFactoryCallbacks { ActiveRpc(ConnectionManager& parent) - : parent_(parent), request_timer_(new Stats::Timespan(parent_.stats_.request_time_ms_, - parent_.time_source_)), + : parent_(parent), request_timer_(new Stats::HistogramCompletableTimespanImpl( + parent_.stats_.request_time_ms_, parent_.time_source_)), stream_id_(parent_.random_generator_.random()), stream_info_(parent_.time_source_), local_response_sent_{false}, pending_transport_end_{ false} { diff --git a/source/extensions/filters/network/thrift_proxy/stats.h b/source/extensions/filters/network/thrift_proxy/stats.h index 7bc42d8eef71..583c02882f86 100644 --- a/source/extensions/filters/network/thrift_proxy/stats.h +++ b/source/extensions/filters/network/thrift_proxy/stats.h @@ -29,7 +29,7 @@ namespace ThriftProxy { COUNTER(response_reply) \ COUNTER(response_success) \ GAUGE(request_active, Accumulate) \ - HISTOGRAM(request_time_ms) + HISTOGRAM(request_time_ms, Milliseconds) /** * Struct definition for all mongo proxy stats. @see stats_macros.h diff --git a/source/extensions/filters/network/zookeeper_proxy/filter.cc b/source/extensions/filters/network/zookeeper_proxy/filter.cc index e84f7c2e57fe..c9b694f0922a 100644 --- a/source/extensions/filters/network/zookeeper_proxy/filter.cc +++ b/source/extensions/filters/network/zookeeper_proxy/filter.cc @@ -290,7 +290,8 @@ void ZooKeeperFilter::onConnectResponse(const int32_t proto_version, const int32 Stats::SymbolTable::StoragePtr storage = config_->scope_.symbolTable().join({config_->stat_prefix_, config_->connect_latency_}); - config_->scope_.histogramFromStatName(Stats::StatName(storage.get())) + config_->scope_ + .histogramFromStatName(Stats::StatName(storage.get()), Stats::Histogram::Unit::Milliseconds) .recordValue(latency.count()); setDynamicMetadata({{"opname", "connect_response"}, @@ -312,7 +313,8 @@ void ZooKeeperFilter::onResponse(const OpCodes opcode, const int32_t xid, const } Stats::SymbolTable::StoragePtr storage = config_->scope_.symbolTable().join({config_->stat_prefix_, opcode_latency}); - config_->scope_.histogramFromStatName(Stats::StatName(storage.get())) + config_->scope_ + .histogramFromStatName(Stats::StatName(storage.get()), Stats::Histogram::Unit::Milliseconds) .recordValue(latency.count()); setDynamicMetadata({{"opname", opname}, diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 8ffcbe05f898..028835aa5562 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -67,7 +67,12 @@ void UdpStatsdSink::flush(Stats::MetricSnapshot& snapshot) { } void UdpStatsdSink::onHistogramComplete(const Stats::Histogram& histogram, uint64_t value) { - // For statsd histograms are all timers. + // For statsd histograms are all timers in milliseconds, Envoy histograms are however + // not necessarily timers in milliseconds, for Envoy histograms suffixed with their corresponding + // SI unit symbol this is acceptable, but for histograms without a suffix, especially those which + // are timers but record in units other than milliseconds, it may make sense to scale the value to + // milliseconds here and potentially suffix the names accordingly (minus the pre-existing ones for + // backwards compatibility). const std::string message(fmt::format("{}.{}:{}|ms{}", prefix_, getName(histogram), std::chrono::milliseconds(value).count(), buildTagStr(histogram.tags()))); @@ -206,6 +211,7 @@ void TcpStatsdSink::TlsSink::onTimespanComplete(const std::string& name, std::chrono::milliseconds ms) { // Ultimately it would be nice to perf optimize this path also, but it's not very frequent. It's // also currently not possible that this interleaves with any counter/gauge flushing. + // See the comment at UdpStatsdSink::onHistogramComplete with respect to unit suffixes. ASSERT(current_slice_mem_ == nullptr); Buffer::OwnedImpl buffer( fmt::format("{}.{}:{}|ms\n", parent_.getPrefix().c_str(), name, ms.count())); diff --git a/source/server/BUILD b/source/server/BUILD index 459058f6c08a..92b30e0d46be 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -69,10 +69,11 @@ envoy_cc_library( "//include/envoy/network:listener_interface", "//include/envoy/server:active_udp_listener_config_interface", "//include/envoy/server:listener_manager_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//source/common/common:linked_object", "//source/common/common:non_copyable", "//source/common/network:connection_lib", + "//source/common/stats:timespan_lib", "//source/extensions/transport_sockets:well_known_names", ], ) diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 356b85bfc743..365e12aa6dac 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -8,6 +8,7 @@ #include "common/network/connection_impl.h" #include "common/network/utility.h" +#include "common/stats/timespan_impl.h" #include "extensions/transport_sockets/well_known_names.h" @@ -370,7 +371,8 @@ void ConnectionHandlerImpl::ActiveTcpListener::post(Network::ConnectionSocketPtr ConnectionHandlerImpl::ActiveTcpConnection::ActiveTcpConnection( ActiveTcpListener& listener, Network::ConnectionPtr&& new_connection, TimeSource& time_source) : listener_(listener), connection_(std::move(new_connection)), - conn_length_(new Stats::Timespan(listener_.stats_.downstream_cx_length_ms_, time_source)) { + conn_length_(new Stats::HistogramCompletableTimespanImpl( + listener_.stats_.downstream_cx_length_ms_, time_source)) { // We just universally set no delay on connections. Theoretically we might at some point want // to make this configurable. connection_->noDelay(true); diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 3e2ec490d21c..a9355f32efd8 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -32,7 +32,7 @@ namespace Server { COUNTER(no_filter_chain_match) \ GAUGE(downstream_cx_active, Accumulate) \ GAUGE(downstream_pre_cx_active, Accumulate) \ - HISTOGRAM(downstream_cx_length_ms) + HISTOGRAM(downstream_cx_length_ms, Milliseconds) /** * Wrapper struct for listener stats. @see stats_macros.h diff --git a/source/server/server.cc b/source/server/server.cc index 3700c2c30586..cf693c39e118 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -36,6 +36,7 @@ #include "common/runtime/runtime_impl.h" #include "common/singleton/manager_impl.h" #include "common/stats/thread_local_store.h" +#include "common/stats/timespan_impl.h" #include "common/upstream/cluster_manager_impl.h" #include "server/configuration_impl.h" @@ -300,8 +301,8 @@ void InstanceImpl::initialize(const Options& options, validation_context_.dynamic_warning_validation_visitor().setCounter( server_stats_->dynamic_unknown_fields_); - initialization_timer_ = - std::make_unique(server_stats_->initialization_time_ms_, timeSource()); + initialization_timer_ = std::make_unique( + server_stats_->initialization_time_ms_, timeSource()); server_stats_->concurrency_.set(options_.concurrency()); server_stats_->hot_restart_epoch_.set(options_.restartEpoch()); diff --git a/source/server/server.h b/source/server/server.h index 9f3c2ad6ffd8..6e87c18faad2 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -64,7 +64,7 @@ namespace Server { GAUGE(total_connections, Accumulate) \ GAUGE(uptime, Accumulate) \ GAUGE(version, NeverImport) \ - HISTOGRAM(initialization_time_ms) + HISTOGRAM(initialization_time_ms, Milliseconds) struct ServerStats { ALL_SERVER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index 58e750b29375..05ad4d9a4e2d 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -96,8 +96,10 @@ class DispatcherImplTest : public testing::Test { // TODO(mergeconflict): We also need integration testing to validate that the expected histograms // are written when `enable_dispatcher_stats` is true. See issue #6582. TEST_F(DispatcherImplTest, InitializeStats) { - EXPECT_CALL(scope_, histogram("test.dispatcher.loop_duration_us")); - EXPECT_CALL(scope_, histogram("test.dispatcher.poll_delay_us")); + EXPECT_CALL(scope_, + histogram("test.dispatcher.loop_duration_us", Stats::Histogram::Unit::Microseconds)); + EXPECT_CALL(scope_, + histogram("test.dispatcher.poll_delay_us", Stats::Histogram::Unit::Microseconds)); dispatcher_->initializeStats(scope_, "test."); } diff --git a/test/common/http/codes_test.cc b/test/common/http/codes_test.cc index 4f7766fc2d49..ff501bfe26f5 100644 --- a/test/common/http/codes_test.cc +++ b/test/common/http/codes_test.cc @@ -245,28 +245,33 @@ TEST_F(CodeUtilityTest, ResponseTimingTest) { pool_.add("from_az"), pool_.add("to_az")}; - EXPECT_CALL(cluster_scope, histogram("prefix.upstream_rq_time")); + EXPECT_CALL(cluster_scope, + histogram("prefix.upstream_rq_time", Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.upstream_rq_time"), 5)); - EXPECT_CALL(cluster_scope, histogram("prefix.canary.upstream_rq_time")); + EXPECT_CALL(cluster_scope, + histogram("prefix.canary.upstream_rq_time", Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL( cluster_scope, deliverHistogramToSinks(Property(&Stats::Metric::name, "prefix.canary.upstream_rq_time"), 5)); - EXPECT_CALL(cluster_scope, histogram("prefix.internal.upstream_rq_time")); + EXPECT_CALL(cluster_scope, + histogram("prefix.internal.upstream_rq_time", Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.internal.upstream_rq_time"), 5)); EXPECT_CALL(global_store, - histogram("vhost.vhost_name.vcluster.req_vcluster_name.upstream_rq_time")); + histogram("vhost.vhost_name.vcluster.req_vcluster_name.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(global_store, deliverHistogramToSinks( Property(&Stats::Metric::name, "vhost.vhost_name.vcluster.req_vcluster_name.upstream_rq_time"), 5)); - EXPECT_CALL(cluster_scope, histogram("prefix.zone.from_az.to_az.upstream_rq_time")); + EXPECT_CALL(cluster_scope, histogram("prefix.zone.from_az.to_az.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.zone.from_az.to_az.upstream_rq_time"), 5)); diff --git a/test/common/http/user_agent_test.cc b/test/common/http/user_agent_test.cc index 173f37ee8b81..7a6e0be8cd06 100644 --- a/test/common/http/user_agent_test.cc +++ b/test/common/http/user_agent_test.cc @@ -20,14 +20,16 @@ namespace { TEST(UserAgentTest, All) { Stats::MockStore stat_store; NiceMock original_histogram; + original_histogram.unit_ = Stats::Histogram::Unit::Milliseconds; Event::SimulatedTimeSystem time_system; - Stats::Timespan span(original_histogram, time_system); + Stats::HistogramCompletableTimespanImpl span(original_histogram, time_system); EXPECT_CALL(stat_store.counter_, inc()).Times(5); EXPECT_CALL(stat_store, counter("test.user_agent.ios.downstream_cx_total")); EXPECT_CALL(stat_store, counter("test.user_agent.ios.downstream_rq_total")); EXPECT_CALL(stat_store, counter("test.user_agent.ios.downstream_cx_destroy_remote_active_rq")); - EXPECT_CALL(stat_store, histogram("test.user_agent.ios.downstream_cx_length_ms")); + EXPECT_CALL(stat_store, histogram("test.user_agent.ios.downstream_cx_length_ms", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL( stat_store, deliverHistogramToSinks( @@ -45,7 +47,8 @@ TEST(UserAgentTest, All) { EXPECT_CALL(stat_store, counter("test.user_agent.android.downstream_rq_total")); EXPECT_CALL(stat_store, counter("test.user_agent.android.downstream_cx_destroy_remote_active_rq")); - EXPECT_CALL(stat_store, histogram("test.user_agent.android.downstream_cx_length_ms")); + EXPECT_CALL(stat_store, histogram("test.user_agent.android.downstream_cx_length_ms", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL( stat_store, deliverHistogramToSinks( diff --git a/test/common/stats/isolated_store_impl_test.cc b/test/common/stats/isolated_store_impl_test.cc index b74b9fb1a840..68d892a6f033 100644 --- a/test/common/stats/isolated_store_impl_test.cc +++ b/test/common/stats/isolated_store_impl_test.cc @@ -65,9 +65,9 @@ TEST_F(StatsIsolatedStoreImplTest, All) { g1.set(0); EXPECT_EQ(0, found_gauge->get().value()); - Histogram& h1 = store_.histogram("h1"); + Histogram& h1 = store_.histogram("h1", Stats::Histogram::Unit::Unspecified); EXPECT_TRUE(h1.used()); // hardcoded in impl to be true always. - Histogram& h2 = scope1->histogram("h2"); + Histogram& h2 = scope1->histogram("h2", Stats::Histogram::Unit::Unspecified); scope1->deliverHistogramToSinks(h2, 0); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); @@ -119,8 +119,10 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) { EXPECT_EQ(0, g1.tags().size()); EXPECT_EQ(0, g1.tags().size()); - Histogram& h1 = store_.histogramFromStatName(makeStatName("h1")); - Histogram& h2 = scope1->histogramFromStatName(makeStatName("h2")); + Histogram& h1 = + store_.histogramFromStatName(makeStatName("h1"), Stats::Histogram::Unit::Unspecified); + Histogram& h2 = + scope1->histogramFromStatName(makeStatName("h2"), Stats::Histogram::Unit::Unspecified); scope1->deliverHistogramToSinks(h2, 0); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); @@ -164,7 +166,7 @@ TEST_F(StatsIsolatedStoreImplTest, LongStatName) { #define ALL_TEST_STATS(COUNTER, GAUGE, HISTOGRAM) \ COUNTER(test_counter) \ GAUGE(test_gauge, Accumulate) \ - HISTOGRAM(test_histogram) + HISTOGRAM(test_histogram, Microseconds) struct TestStats { ALL_TEST_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) @@ -183,6 +185,7 @@ TEST_F(StatsIsolatedStoreImplTest, StatsMacros) { Histogram& histogram = test_stats.test_histogram_; EXPECT_EQ("test.test_histogram", histogram.name()); + EXPECT_EQ(Histogram::Unit::Microseconds, histogram.unit()); } TEST_F(StatsIsolatedStoreImplTest, NullImplCoverage) { diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index baba626c31a6..e589ff850a4b 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -202,8 +202,8 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { g1.set(0); EXPECT_EQ(0, found_gauge->get().value()); - Histogram& h1 = store_->histogram("h1"); - EXPECT_EQ(&h1, &store_->histogram("h1")); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + EXPECT_EQ(&h1, &store_->histogram("h1", Stats::Histogram::Unit::Unspecified)); StatNameManagedStorage h1_name("h1", *symbol_table_); auto found_histogram = store_->findHistogram(h1_name.statName()); ASSERT_TRUE(found_histogram.has_value()); @@ -250,8 +250,8 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { g1.set(0); EXPECT_EQ(0, found_gauge->get().value()); - Histogram& h1 = store_->histogram("h1"); - EXPECT_EQ(&h1, &store_->histogram("h1")); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + EXPECT_EQ(&h1, &store_->histogram("h1", Stats::Histogram::Unit::Unspecified)); StatNameManagedStorage h1_name("h1", *symbol_table_); auto found_histogram = store_->findHistogram(h1_name.statName()); ASSERT_TRUE(found_histogram.has_value()); @@ -306,8 +306,8 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { ASSERT_TRUE(found_gauge2.has_value()); EXPECT_EQ(&g2, &found_gauge2->get()); - Histogram& h1 = store_->histogram("h1"); - Histogram& h2 = scope1->histogram("h2"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h2 = scope1->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 100)); @@ -488,8 +488,10 @@ TEST_F(LookupWithStatNameTest, All) { EXPECT_EQ(0, g1.tags().size()); EXPECT_EQ(0, g1.tags().size()); - Histogram& h1 = store_.histogramFromStatName(makeStatName("h1")); - Histogram& h2 = scope1->histogramFromStatName(makeStatName("h2")); + Histogram& h1 = + store_.histogramFromStatName(makeStatName("h1"), Stats::Histogram::Unit::Unspecified); + Histogram& h2 = + scope1->histogramFromStatName(makeStatName("h2"), Stats::Histogram::Unit::Unspecified); scope1->deliverHistogramToSinks(h2, 0); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); @@ -564,9 +566,11 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { EXPECT_EQ(&noop_gauge, &noop_gauge_2); // Histogram - Histogram& noop_histogram = store_->histogram("noop_histogram"); + Histogram& noop_histogram = + store_->histogram("noop_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(noop_histogram.name(), ""); - Histogram& noop_histogram_2 = store_->histogram("noop_histogram_2"); + Histogram& noop_histogram_2 = + store_->histogram("noop_histogram_2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(&noop_histogram, &noop_histogram_2); store_->shutdownThreading(); @@ -589,7 +593,8 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { EXPECT_EQ(lowercase_counter.name(), "lowercase_counter"); Gauge& lowercase_gauge = store_->gauge("lowercase_gauge", Gauge::ImportMode::Accumulate); EXPECT_EQ(lowercase_gauge.name(), "lowercase_gauge"); - Histogram& lowercase_histogram = store_->histogram("lowercase_histogram"); + Histogram& lowercase_histogram = + store_->histogram("lowercase_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(lowercase_histogram.name(), "lowercase_histogram"); // And the creation of counters/gauges/histograms which have uppercase letters should fail. @@ -609,7 +614,8 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { // 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"); + Histogram& uppercase_histogram = + store_->histogram("upperCASE_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(uppercase_histogram.name(), ""); // Adding another exclusion rule -- now we reject not just uppercase stats but those starting with @@ -644,13 +650,16 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { invalid_gauge_2.inc(); EXPECT_EQ(invalid_gauge_2.value(), 0); - Histogram& valid_histogram = store_->histogram("valid_histogram"); + Histogram& valid_histogram = + store_->histogram("valid_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(valid_histogram.name(), "valid_histogram"); - Histogram& invalid_histogram_1 = store_->histogram("invalid_histogram"); + Histogram& invalid_histogram_1 = + store_->histogram("invalid_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(invalid_histogram_1.name(), ""); - Histogram& invalid_histogram_2 = store_->histogram("also_INVALID_histogram"); + Histogram& invalid_histogram_2 = + store_->histogram("also_INVALID_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(invalid_histogram_2.name(), ""); // Expected to free lowercase_counter, lowercase_gauge, valid_counter, valid_gauge @@ -754,7 +763,7 @@ class RememberStatsMatcherTest : public testing::TestWithParam { LookupStatFn lookupHistogramFn() { return [this](const std::string& stat_name) -> std::string { - return scope_->histogram(stat_name).name(); + return scope_->histogram(stat_name, Stats::Histogram::Unit::Unspecified).name(); }; } @@ -803,7 +812,7 @@ TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { store_->initializeThreading(main_thread_dispatcher_, tls_); Counter& counter = store_->counter("c1"); Gauge& gauge = store_->gauge("g1", Gauge::ImportMode::Accumulate); - Histogram& histogram = store_->histogram("h1"); + Histogram& histogram = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); ASSERT_EQ(1, store_->counters().size()); // "c1". EXPECT_TRUE(&counter == store_->counters()[0].get() || &counter == store_->counters()[1].get()); // counters() order is non-deterministic. @@ -953,7 +962,7 @@ TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - Histogram& h1 = store_->histogram("h1"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 1)); @@ -985,7 +994,7 @@ TEST(ThreadLocalStoreThreadTest, ConstructDestruct) { // Histogram tests TEST_F(HistogramTest, BasicSingleHistogramMerge) { - Histogram& h1 = store_->histogram("h1"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); expectCallAndAccumulate(h1, 0); @@ -1001,8 +1010,8 @@ TEST_F(HistogramTest, BasicSingleHistogramMerge) { } TEST_F(HistogramTest, BasicMultiHistogramMerge) { - Histogram& h1 = store_->histogram("h1"); - Histogram& h2 = store_->histogram("h2"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h2 = store_->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("h2", h2.name()); @@ -1014,8 +1023,8 @@ TEST_F(HistogramTest, BasicMultiHistogramMerge) { } TEST_F(HistogramTest, MultiHistogramMultipleMerges) { - Histogram& h1 = store_->histogram("h1"); - Histogram& h2 = store_->histogram("h2"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h2 = store_->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("h2", h2.name()); @@ -1045,8 +1054,8 @@ TEST_F(HistogramTest, MultiHistogramMultipleMerges) { TEST_F(HistogramTest, BasicScopeHistogramMerge) { ScopePtr scope1 = store_->createScope("scope1."); - Histogram& h1 = store_->histogram("h1"); - Histogram& h2 = scope1->histogram("h2"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h2 = scope1->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); @@ -1056,8 +1065,8 @@ TEST_F(HistogramTest, BasicScopeHistogramMerge) { } TEST_F(HistogramTest, BasicHistogramSummaryValidate) { - Histogram& h1 = store_->histogram("h1"); - Histogram& h2 = store_->histogram("h2"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h2 = store_->histogram("h2", Stats::Histogram::Unit::Unspecified); expectCallAndAccumulate(h1, 1); @@ -1096,7 +1105,7 @@ TEST_F(HistogramTest, BasicHistogramSummaryValidate) { // Validates the summary after known value merge in to same histogram. TEST_F(HistogramTest, BasicHistogramMergeSummary) { - Histogram& h1 = store_->histogram("h1"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); for (size_t i = 0; i < 50; ++i) { expectCallAndAccumulate(h1, i); @@ -1124,8 +1133,8 @@ TEST_F(HistogramTest, BasicHistogramMergeSummary) { TEST_F(HistogramTest, BasicHistogramUsed) { ScopePtr scope1 = store_->createScope("scope1."); - Histogram& h1 = store_->histogram("h1"); - Histogram& h2 = scope1->histogram("h2"); + Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h2 = scope1->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); diff --git a/test/extensions/filters/http/dynamo/dynamo_filter_test.cc b/test/extensions/filters/http/dynamo/dynamo_filter_test.cc index ce8638ccbe14..c6db226c64e3 100644 --- a/test/extensions/filters/http/dynamo/dynamo_filter_test.cc +++ b/test/extensions/filters/http/dynamo/dynamo_filter_test.cc @@ -74,9 +74,12 @@ TEST_F(DynamoFilterTest, operatorPresent) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.Get.upstream_rq_total_200")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.Get.upstream_rq_total")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.Get.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.Get.upstream_rq_time_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.Get.upstream_rq_time")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.Get.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.Get.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.Get.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL( stats_, deliverHistogramToSinks( @@ -181,9 +184,12 @@ TEST_F(DynamoFilterTest, HandleErrorTypeTablePresent) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total_4xx")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total_400")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_4xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_400")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_4xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_400", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.GetItem.upstream_rq_time_4xx"), @@ -201,9 +207,12 @@ TEST_F(DynamoFilterTest, HandleErrorTypeTablePresent) { EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_400")); EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_4xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_400")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_4xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_400", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.table.locations.upstream_rq_time_4xx"), @@ -250,9 +259,12 @@ TEST_F(DynamoFilterTest, BatchMultipleTables) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx"), @@ -299,9 +311,12 @@ TEST_F(DynamoFilterTest, BatchMultipleTablesUnprocessedKeys) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx"), @@ -366,9 +381,12 @@ TEST_F(DynamoFilterTest, BatchMultipleTablesNoUnprocessedKeys) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx"), @@ -429,9 +447,12 @@ TEST_F(DynamoFilterTest, BatchMultipleTablesInvalidResponseBody) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx"), @@ -486,9 +507,12 @@ TEST_F(DynamoFilterTest, bothOperationAndTableCorrect) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total_200")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.GetItem.upstream_rq_time_2xx"), @@ -506,9 +530,12 @@ TEST_F(DynamoFilterTest, bothOperationAndTableCorrect) { EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_200")); EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.table.locations.upstream_rq_time_2xx"), @@ -561,9 +588,12 @@ TEST_F(DynamoFilterTest, PartitionIdStats) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total_200")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.GetItem.upstream_rq_total")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.GetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.GetItem.upstream_rq_time_2xx"), @@ -581,9 +611,12 @@ TEST_F(DynamoFilterTest, PartitionIdStats) { EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_200")); EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.table.locations.upstream_rq_time_2xx"), @@ -654,9 +687,12 @@ TEST_F(DynamoFilterTest, NoPartitionIdStatsForMultipleTables) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx"), @@ -728,9 +764,12 @@ TEST_F(DynamoFilterTest, PartitionIdStatsForSingleTableBatchOperation) { EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_2xx")); EXPECT_CALL(stats_, counter("prefix.dynamodb.operation.BatchGetItem.upstream_rq_total_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.operation.BatchGetItem.upstream_rq_time_2xx"), @@ -748,9 +787,12 @@ TEST_F(DynamoFilterTest, PartitionIdStatsForSingleTableBatchOperation) { EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total_200")); EXPECT_CALL(stats_, counter("prefix.dynamodb.table.locations.upstream_rq_total")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_2xx")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_200")); - EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time")); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_2xx", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time_200", + Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(stats_, histogram("prefix.dynamodb.table.locations.upstream_rq_time", + Stats::Histogram::Unit::Milliseconds)); EXPECT_CALL(stats_, deliverHistogramToSinks( Property(&Stats::Metric::name, "prefix.dynamodb.table.locations.upstream_rq_time_2xx"), diff --git a/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc b/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc index dcbe36bd4207..e190cb1aab8c 100644 --- a/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/command_splitter_impl_test.cc @@ -1492,6 +1492,7 @@ class RedisSingleServerRequestWithLatencyMicrosTest : public RedisSingleServerRe } ConnPool::MockInstance* conn_pool_{new ConnPool::MockInstance()}; + NiceMock store_; InstanceImpl splitter_{std::make_unique(ConnPool::InstanceSharedPtr{conn_pool_}), store_, "redis.foo.", time_system_, true}; }; diff --git a/test/extensions/stats_sinks/common/statsd/statsd_test.cc b/test/extensions/stats_sinks/common/statsd/statsd_test.cc index 12f7f7df2270..4a55d126ad0a 100644 --- a/test/extensions/stats_sinks/common/statsd/statsd_test.cc +++ b/test/extensions/stats_sinks/common/statsd/statsd_test.cc @@ -102,6 +102,42 @@ TEST_F(TcpStatsdSinkTest, BasicFlow) { tls_.shutdownThread(); } +TEST_F(TcpStatsdSinkTest, SiSuffix) { + InSequence s; + expectCreateConnection(); + + NiceMock items; + items.name_ = "items"; + items.unit_ = Stats::Histogram::Unit::Unspecified; + + EXPECT_CALL(*connection_, write(BufferStringEqual("envoy.items:1|ms\n"), _)); + sink_->onHistogramComplete(items, 1); + + NiceMock information; + information.name_ = "information"; + information.unit_ = Stats::Histogram::Unit::Bytes; + + EXPECT_CALL(*connection_, write(BufferStringEqual("envoy.information:2|ms\n"), _)); + sink_->onHistogramComplete(information, 2); + + NiceMock duration_micro; + duration_micro.name_ = "duration"; + duration_micro.unit_ = Stats::Histogram::Unit::Microseconds; + + EXPECT_CALL(*connection_, write(BufferStringEqual("envoy.duration:3|ms\n"), _)); + sink_->onHistogramComplete(duration_micro, 3); + + NiceMock duration_milli; + duration_milli.name_ = "duration"; + duration_milli.unit_ = Stats::Histogram::Unit::Milliseconds; + + EXPECT_CALL(*connection_, write(BufferStringEqual("envoy.duration:4|ms\n"), _)); + sink_->onHistogramComplete(duration_milli, 4); + + EXPECT_CALL(*connection_, close(Network::ConnectionCloseType::NoFlush)); + tls_.shutdownThread(); +} + // Verify that when there is no statsd host we correctly empty all output buffers so we don't // infinitely buffer. TEST_F(TcpStatsdSinkTest, NoHost) { 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 9e016e5df708..d82c47d6a312 100644 --- a/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc +++ b/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc @@ -178,6 +178,47 @@ TEST(UdpStatsdSinkTest, CheckActualStatsWithCustomPrefix) { tls_.shutdownThread(); } +TEST(UdpStatsdSinkTest, SiSuffix) { + NiceMock snapshot; + auto writer_ptr = std::make_shared>(); + NiceMock tls_; + UdpStatsdSink sink(tls_, writer_ptr, false); + + NiceMock items; + items.name_ = "items"; + items.unit_ = Stats::Histogram::Unit::Unspecified; + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.items:1|ms")); + sink.onHistogramComplete(items, 1); + + NiceMock information; + information.name_ = "information"; + information.unit_ = Stats::Histogram::Unit::Bytes; + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.information:2|ms")); + sink.onHistogramComplete(information, 2); + + NiceMock duration_micro; + duration_micro.name_ = "duration"; + duration_micro.unit_ = Stats::Histogram::Unit::Microseconds; + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.duration:3|ms")); + sink.onHistogramComplete(duration_micro, 3); + + NiceMock duration_milli; + duration_milli.name_ = "duration"; + duration_milli.unit_ = Stats::Histogram::Unit::Milliseconds; + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.duration:4|ms")); + sink.onHistogramComplete(duration_milli, 4); + + tls_.shutdownThread(); +} + TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { NiceMock snapshot; auto writer_ptr = std::make_shared>(); @@ -218,6 +259,53 @@ TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { tls_.shutdownThread(); } +TEST(UdpStatsdSinkWithTagsTest, SiSuffix) { + NiceMock snapshot; + auto writer_ptr = std::make_shared>(); + NiceMock tls_; + UdpStatsdSink sink(tls_, writer_ptr, true); + + std::vector tags = {Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}; + + NiceMock items; + items.name_ = "items"; + items.unit_ = Stats::Histogram::Unit::Unspecified; + items.setTags(tags); + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.items:1|ms|#key1:value1,key2:value2")); + sink.onHistogramComplete(items, 1); + + NiceMock information; + information.name_ = "information"; + information.unit_ = Stats::Histogram::Unit::Bytes; + information.setTags(tags); + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.information:2|ms|#key1:value1,key2:value2")); + sink.onHistogramComplete(information, 2); + + NiceMock duration_micro; + duration_micro.name_ = "duration"; + duration_micro.unit_ = Stats::Histogram::Unit::Microseconds; + duration_micro.setTags(tags); + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.duration:3|ms|#key1:value1,key2:value2")); + sink.onHistogramComplete(duration_micro, 3); + + NiceMock duration_milli; + duration_milli.name_ = "duration"; + duration_milli.unit_ = Stats::Histogram::Unit::Milliseconds; + duration_milli.setTags(tags); + + EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), + write("envoy.duration:4|ms|#key1:value1,key2:value2")); + sink.onHistogramComplete(duration_milli, 4); + + tls_.shutdownThread(); +} + } // namespace } // namespace Statsd } // namespace Common diff --git a/test/integration/server.h b/test/integration/server.h index d367b5acd00c..513b5c6d1a40 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -91,9 +91,9 @@ class TestScopeWrapper : public Scope { return wrapped_scope_->gaugeFromStatName(name, import_mode); } - Histogram& histogramFromStatName(StatName name) override { + Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { Thread::LockGuard lock(lock_); - return wrapped_scope_->histogramFromStatName(name); + return wrapped_scope_->histogramFromStatName(name, unit); } NullGaugeImpl& nullGauge(const std::string& str) override { return wrapped_scope_->nullGauge(str); @@ -107,9 +107,9 @@ class TestScopeWrapper : public Scope { StatNameManagedStorage storage(name, symbolTable()); return gaugeFromStatName(storage.statName(), import_mode); } - Histogram& histogram(const std::string& name) override { + Histogram& histogram(const std::string& name, Histogram::Unit unit) override { StatNameManagedStorage storage(name, symbolTable()); - return histogramFromStatName(storage.statName()); + return histogramFromStatName(storage.statName(), unit); } OptionalCounter findCounter(StatName name) const override { @@ -163,14 +163,14 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.gauge(name, import_mode); } - Histogram& histogramFromStatName(StatName name) override { + Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { Thread::LockGuard lock(lock_); - return store_.histogramFromStatName(name); + return store_.histogramFromStatName(name, unit); } NullGaugeImpl& nullGauge(const std::string& name) override { return store_.nullGauge(name); } - Histogram& histogram(const std::string& name) override { + Histogram& histogram(const std::string& name, Histogram::Unit unit) override { Thread::LockGuard lock(lock_); - return store_.histogram(name); + return store_.histogram(name, unit); } OptionalCounter findCounter(StatName name) const override { Thread::LockGuard lock(lock_); diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index bad62a24de7d..8e090671778b 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -256,6 +256,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2019/09/25 8226 43022 44000 dns: enable dns failure refresh rate configuration // 2019/09/30 8354 43310 44000 Implement transport socket match. // 2019/10/17 8537 43308 44000 add new enum value HTTP3 + // 2019/10/17 8484 43340 44000 stats: add unit support to histogram // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -265,7 +266,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // On a local clang8/libstdc++/linux flow, the memory usage was observed in // June 2019 to be 64 bytes higher than it is in CI/release. Your mileage may // vary. - EXPECT_MEMORY_EQ(m_per_cluster, 43308); // 104 bytes higher than a debug build. + EXPECT_MEMORY_EQ(m_per_cluster, 43340); // 104 bytes higher than a debug build. EXPECT_MEMORY_LE(m_per_cluster, 44000); } @@ -295,6 +296,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2019/09/25 8226 34777 35000 dns: enable dns failure refresh rate configuration // 2019/09/30 8354 34969 35000 Implement transport socket match. // 2019/10/17 8537 34966 35000 add new enum value HTTP3 + // 2019/10/17 8484 34998 35000 stats: add unit support to histogram // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -304,7 +306,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // On a local clang8/libstdc++/linux flow, the memory usage was observed in // June 2019 to be 64 bytes higher than it is in CI/release. Your mileage may // vary. - EXPECT_MEMORY_EQ(m_per_cluster, 34966); // 104 bytes higher than a debug build. + EXPECT_MEMORY_EQ(m_per_cluster, 34998); // 104 bytes higher than a debug build. EXPECT_MEMORY_LE(m_per_cluster, 36000); } diff --git a/test/mocks/stats/BUILD b/test/mocks/stats/BUILD index bf9048b25366..46ba1211048f 100644 --- a/test/mocks/stats/BUILD +++ b/test/mocks/stats/BUILD @@ -14,7 +14,7 @@ envoy_cc_mock( hdrs = ["mocks.h"], deps = [ "//include/envoy/stats:stats_interface", - "//include/envoy/stats:timespan", + "//include/envoy/stats:timespan_interface", "//include/envoy/thread_local:thread_local_interface", "//include/envoy/upstream:cluster_manager_interface", "//source/common/stats:fake_symbol_table_lib", @@ -23,6 +23,7 @@ envoy_cc_mock( "//source/common/stats:stats_lib", "//source/common/stats:store_impl_lib", "//source/common/stats:symbol_table_creator_lib", + "//source/common/stats:timespan_lib", "//test/mocks:common_lib", "//test/test_common:global_lib", ], diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index 8196b0a43b03..25d908b4d79e 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -31,6 +31,7 @@ MockGauge::MockGauge() : used_(false), value_(0), import_mode_(ImportMode::Accum MockGauge::~MockGauge() = default; MockHistogram::MockHistogram() { + ON_CALL(*this, unit()).WillByDefault(ReturnPointee(&unit_)); ON_CALL(*this, recordValue(_)).WillByDefault(Invoke([this](uint64_t value) { if (store_ != nullptr) { store_->deliverHistogramToSinks(*this, value); @@ -40,6 +41,8 @@ MockHistogram::MockHistogram() { MockHistogram::~MockHistogram() = default; MockParentHistogram::MockParentHistogram() { + ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); + ON_CALL(*this, unit()).WillByDefault(ReturnPointee(&unit_)); ON_CALL(*this, recordValue(_)).WillByDefault(Invoke([this](uint64_t value) { if (store_ != nullptr) { store_->deliverHistogramToSinks(*this, value); @@ -47,7 +50,6 @@ MockParentHistogram::MockParentHistogram() { })); ON_CALL(*this, intervalStatistics()).WillByDefault(ReturnRef(*histogram_stats_)); ON_CALL(*this, cumulativeStatistics()).WillByDefault(ReturnRef(*histogram_stats_)); - ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); } MockParentHistogram::~MockParentHistogram() = default; @@ -64,13 +66,15 @@ MockSink::~MockSink() = default; MockStore::MockStore() : StoreImpl(*global_symbol_table_) { ON_CALL(*this, counter(_)).WillByDefault(ReturnRef(counter_)); - ON_CALL(*this, histogram(_)).WillByDefault(Invoke([this](const std::string& name) -> Histogram& { - auto* histogram = new NiceMock(); // symbol_table_); - histogram->name_ = name; - histogram->store_ = this; - histograms_.emplace_back(histogram); - return *histogram; - })); + ON_CALL(*this, histogram(_, _)) + .WillByDefault(Invoke([this](const std::string& name, Histogram::Unit unit) -> Histogram& { + auto* histogram = new NiceMock(); // symbol_table_); + histogram->name_ = name; + histogram->unit_ = unit; + histogram->store_ = this; + histograms_.emplace_back(histogram); + return *histogram; + })); } MockStore::~MockStore() = default; diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index d5ad120ffcae..29c7307bdf02 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -19,6 +19,7 @@ #include "common/stats/isolated_store_impl.h" #include "common/stats/store_impl.h" #include "common/stats/symbol_table_creator.h" +#include "common/stats/timespan_impl.h" #include "test/test_common/global.h" @@ -204,14 +205,16 @@ class MockHistogram : public MockMetric { MockHistogram(); ~MockHistogram() override; - MOCK_METHOD1(recordValue, void(uint64_t value)); MOCK_CONST_METHOD0(used, bool()); + MOCK_CONST_METHOD0(unit, Histogram::Unit()); + MOCK_METHOD1(recordValue, void(uint64_t value)); // RefcountInterface void incRefCount() override { refcount_helper_.incRefCount(); } bool decRefCount() override { return refcount_helper_.decRefCount(); } uint32_t use_count() const override { return refcount_helper_.use_count(); } + Unit unit_{Histogram::Unit::Unspecified}; Store* store_; private: @@ -228,6 +231,7 @@ class MockParentHistogram : public MockMetric { const std::string bucketSummary() const override { return ""; }; MOCK_CONST_METHOD0(used, bool()); + MOCK_CONST_METHOD0(unit, Histogram::Unit()); MOCK_METHOD1(recordValue, void(uint64_t value)); MOCK_CONST_METHOD0(cumulativeStatistics, const HistogramStatistics&()); MOCK_CONST_METHOD0(intervalStatistics, const HistogramStatistics&()); @@ -238,6 +242,7 @@ class MockParentHistogram : public MockMetric { uint32_t use_count() const override { return refcount_helper_.use_count(); } bool used_; + Unit unit_{Histogram::Unit::Unspecified}; Store* store_; std::shared_ptr histogram_stats_ = std::make_shared(); @@ -288,7 +293,7 @@ class MockStore : public SymbolTableProvider, public StoreImpl { MOCK_METHOD2(gauge, Gauge&(const std::string&, Gauge::ImportMode)); MOCK_METHOD1(nullGauge, NullGaugeImpl&(const std::string&)); MOCK_CONST_METHOD0(gauges, std::vector()); - MOCK_METHOD1(histogram, Histogram&(const std::string& name)); + MOCK_METHOD2(histogram, Histogram&(const std::string&, Histogram::Unit)); MOCK_CONST_METHOD0(histograms, std::vector()); MOCK_CONST_METHOD1(findCounter, OptionalCounter(StatName)); @@ -301,8 +306,8 @@ class MockStore : public SymbolTableProvider, public StoreImpl { Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override { return gauge(symbol_table_->toString(name), import_mode); } - Histogram& histogramFromStatName(StatName name) override { - return histogram(symbol_table_->toString(name)); + Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { + return histogram(symbol_table_->toString(name), unit); } TestSymbolTable symbol_table_; diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index a67b422bf7c3..5f50af2290a6 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -111,8 +111,8 @@ TEST_P(AdminStatsTest, StatsAsJson) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - Stats::Histogram& h1 = store_->histogram("h1"); - Stats::Histogram& h2 = store_->histogram("h2"); + Stats::Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Stats::Histogram& h2 = store_->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 200)); h1.recordValue(200); @@ -257,8 +257,8 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJson) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - Stats::Histogram& h1 = store_->histogram("h1"); - Stats::Histogram& h2 = store_->histogram("h2"); + Stats::Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Stats::Histogram& h2 = store_->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("h2", h2.name()); @@ -355,8 +355,8 @@ TEST_P(AdminStatsTest, StatsAsJsonFilterString) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - Stats::Histogram& h1 = store_->histogram("h1"); - Stats::Histogram& h2 = store_->histogram("h2"); + Stats::Histogram& h1 = store_->histogram("h1", Stats::Histogram::Unit::Unspecified); + Stats::Histogram& h2 = store_->histogram("h2", Stats::Histogram::Unit::Unspecified); EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 200)); h1.recordValue(200); @@ -455,9 +455,12 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJsonFilterString) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - Stats::Histogram& h1 = store_->histogram("h1_matches"); // Will match, be used, and print - Stats::Histogram& h2 = store_->histogram("h2_matches"); // Will match but not be used - Stats::Histogram& h3 = store_->histogram("h3_not"); // Will be used but not match + Stats::Histogram& h1 = store_->histogram( + "h1_matches", Stats::Histogram::Unit::Unspecified); // Will match, be used, and print + Stats::Histogram& h2 = store_->histogram( + "h2_matches", Stats::Histogram::Unit::Unspecified); // Will match but not be used + Stats::Histogram& h3 = store_->histogram( + "h3_not", Stats::Histogram::Unit::Unspecified); // Will be used but not match EXPECT_EQ("h1_matches", h1.name()); EXPECT_EQ("h2_matches", h2.name()); @@ -1664,6 +1667,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithAllMetricTypes) { auto histogram1 = makeHistogram(); histogram1->name_ = "cluster.test_1.upstream_rq_time"; + histogram1->unit_ = Stats::Histogram::Unit::Milliseconds; histogram1->used_ = true; histogram1->setTags({Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}); addHistogram(histogram1); @@ -1724,6 +1728,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithUsedOnly) { auto histogram1 = makeHistogram(); histogram1->name_ = "cluster.test_1.upstream_rq_time"; + histogram1->unit_ = Stats::Histogram::Unit::Milliseconds; histogram1->used_ = true; histogram1->setTags({Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}); addHistogram(histogram1); @@ -1771,6 +1776,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithUsedOnlyHistogram) { auto histogram1 = makeHistogram(); histogram1->name_ = "cluster.test_1.upstream_rq_time"; + histogram1->unit_ = Stats::Histogram::Unit::Milliseconds; histogram1->used_ = false; histogram1->setTags({Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}); addHistogram(histogram1); @@ -1810,6 +1816,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithRegexp) { auto histogram1 = makeHistogram(); histogram1->name_ = "cluster.test_1.upstream_rq_time"; + histogram1->unit_ = Stats::Histogram::Unit::Milliseconds; histogram1->setTags({Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}); addHistogram(histogram1); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index e103a9aff4e1..91c150d5ca3f 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -43,7 +43,7 @@ TEST(ServerInstanceUtil, flushHelper) { Stats::Counter& c = store.counter("hello"); c.inc(); store.gauge("world", Stats::Gauge::ImportMode::Accumulate).set(5); - store.histogram("histogram"); + store.histogram("histogram", Stats::Histogram::Unit::Unspecified); std::list sinks; InstanceUtil::flushMetricsToSinks(sinks, store); @@ -306,7 +306,9 @@ TEST_P(ServerInstanceImplTest, EmptyShutdownLifecycleNotifications) { server_->dispatcher().post([&] { server_->shutdown(); }); server_thread->join(); // Validate that initialization_time histogram value has been set. - EXPECT_TRUE(stats_store_.histogram("server.initialization_time").used()); + EXPECT_TRUE( + stats_store_.histogram("server.initialization_time_ms", Stats::Histogram::Unit::Milliseconds) + .used()); EXPECT_EQ(0L, TestUtility::findGauge(stats_store_, "server.state")->value()); } diff --git a/tools/check_format.py b/tools/check_format.py index aee54da4e2ec..c0dde32b252f 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -51,6 +51,13 @@ # Files in these paths can use Protobuf::util::JsonStringToMessage JSON_STRING_TO_MESSAGE_WHITELIST = ("./source/common/protobuf/utility.cc") +# Histogram names which are allowed to be suffixed with the unit symbol, all of the pre-existing +# ones were grandfathered as part of PR #8484 for backwards compatibility. +HISTOGRAM_WITH_SI_SUFFIX_WHITELIST = ("downstream_cx_length_ms", "downstream_cx_length_ms", + "initialization_time_ms", "loop_duration_us", "poll_delay_us", + "request_time_ms", "upstream_cx_connect_ms", + "upstream_cx_length_ms") + # Files in these paths can use std::regex STD_REGEX_WHITELIST = ("./source/common/common/utility.cc", "./source/common/common/regex.h", "./source/common/common/regex.cc", @@ -262,6 +269,10 @@ def whitelistedForJsonStringToMessage(file_path): return file_path in JSON_STRING_TO_MESSAGE_WHITELIST +def whitelistedForHistogramSiSuffix(name): + return name in HISTOGRAM_WITH_SI_SUFFIX_WHITELIST + + def whitelistedForStdRegex(file_path): return file_path.startswith("./test") or file_path in STD_REGEX_WHITELIST @@ -520,6 +531,13 @@ def checkSourceLine(line, file_path, reportError): ('.counter(' in line or '.gauge(' in line or '.histogram(' in line): reportError("Don't lookup stats by name at runtime; use StatName saved during construction") + hist_m = re.search("(?<=HISTOGRAM\()[a-zA-Z0-9_]+_(b|kb|mb|ns|us|ms|s)(?=,)", line) + if hist_m and not whitelistedForHistogramSiSuffix(hist_m.group(0)): + reportError( + "Don't suffix histogram names with the unit symbol, " + "it's already part of the histogram object and unit-supporting sinks can use this information natively, " + "other sinks can add the suffix automatically on flush should they prefer to do so.") + if not whitelistedForStdRegex(file_path) and "std::regex" in line: reportError("Don't use std::regex in code that handles untrusted input. Use RegexMatcher") diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index cb0aadb49f16..ba1d6ab2310a 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -396,6 +396,7 @@ codecs codings combinatorial comparator +completable cond condvar conf diff --git a/tools/testdata/check_format/histogram_from_string.cc b/tools/testdata/check_format/histogram_from_string.cc index 3c16b433a1a9..e7668d3eeada 100644 --- a/tools/testdata/check_format/histogram_from_string.cc +++ b/tools/testdata/check_format/histogram_from_string.cc @@ -1,7 +1,5 @@ namespace Envoy { -void init(Stats::Scope& scope) { - scope.histogram("hello"); -} +void init(Stats::Scope& scope) { scope.histogram("hello", Stats::Histogram::Unit::Unspecified); } } // namespace Envoy