stats: add unit support to histogram (envoyproxy#8484)

Signed-off-by: Taras Roshko <[email protected]>

.gitignore

@@ -11,6 +11,7 @@ BROWSE
 compile_commands.json
 cscope.*
 .deps
+.devcontainer.json
 /docs/landing_source/.bundle
 /generated
 .idea/

docs/root/intro/version_history.rst

@@ -82,6 +82,7 @@ Version history
 * server: added :ref:`per-handler listener stats <config_listener_stats_per_handler>` and
   :ref:`per-worker watchdog stats <operations_performance_watchdog>` 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.

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
 
 /**

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(

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;
 };

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.

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, "")

include/envoy/stats/timespan.h

@@ -3,9 +3,7 @@
 #include <chrono>
 #include <memory>
 
-#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 TimeUnit> 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<TimeUnit>(time_source_.monotonicTime() - start_);
-  }
-
-private:
-  TimeSource& time_source_;
-  Histogram& histogram_;
-  const MonotonicTime start_;
+  virtual void complete() PURE;
 };
 
-using Timespan = TimespanWithUnit<std::chrono::milliseconds>;
+using Timespan = CompletableTimespan;
 using TimespanPtr = std::unique_ptr<Timespan>;
-using CompletableTimespanPtr = std::unique_ptr<CompletableTimespan>;
 
 } // namespace Stats
 } // namespace Envoy

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

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",
     ],
 )
 

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);
   }
 }
 

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;

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

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<Network::Socket::Options>()) {

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",
     ],
 )

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<Stats::Timespan>(
+  parent_.conn_connect_ms_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>(
       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<Stats::Timespan>(
+  conn_length_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>(
       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();

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",
     ],
 )