Revert histograms (envoyproxy#3235)

This reverts commit fd687dd. This reverts commit 5455ed1. This reverts commit 9a1e49f. Signed-off-by: Matt Klein <[email protected]>
ramaraochavali · May 3, 2018 · 837c1d8 · 837c1d8
1 parent e757f03
commit 837c1d8
Show file tree

Hide file tree

Showing 31 changed files with 77 additions and 1,005 deletions.
diff --git a/bazel/external/libcircllhist.BUILD b/bazel/external/libcircllhist.BUILD
diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl
@@ -226,7 +226,6 @@ def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
     _boringssl()
     _com_google_absl()
     _com_github_bombela_backward()
-    _com_github_circonus_labs_libcircllhist()
     _com_github_cyan4973_xxhash()
     _com_github_eile_tclap()
     _com_github_fmtlib_fmt()
@@ -266,16 +265,6 @@ def _com_github_bombela_backward():
         actual = "@com_github_bombela_backward//:backward",
     )
 
-def _com_github_circonus_labs_libcircllhist():
-    _repository_impl(
-        name = "com_github_circonus_labs_libcircllhist",
-        build_file = "@envoy//bazel/external:libcircllhist.BUILD",
-    )
-    native.bind(
-        name = "libcircllhist",
-        actual = "@com_github_circonus_labs_libcircllhist//:libcircllhist",
-    )
-
 def _com_github_cyan4973_xxhash():
     _repository_impl(
         name = "com_github_cyan4973_xxhash",

diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl
@@ -12,10 +12,6 @@ REPOSITORY_LOCATIONS = dict(
         commit = "44ae9609e860e3428cd057f7052e505b4819eb84",  # 2018-02-06
         remote = "https://github.com/bombela/backward-cpp",
     ),
-    com_github_circonus_labs_libcircllhist = dict(
-        commit = "97ef5e088fd01fa8ec5a86334a6308ac0d51ea6f",  # 2018-04-07
-        remote = "https://github.com/circonus-labs/libcircllhist",
-    ),
     com_github_cyan4973_xxhash = dict(
         commit = "7cc9639699f64b750c0b82333dced9ea77e8436e",  # v0.6.5
         remote = "https://github.com/Cyan4973/xxHash",

diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -45,7 +45,6 @@ Version history
   :ref:`cluster specific <envoy_api_field_Cluster.upstream_bind_config>` options.
 * sockets: added `IP_TRANSPARENT` socket option support for :ref:`listeners
   <envoy_api_field_Listener.transparent>`.
-* stats: added support for histograms.
 * tracing: the sampling decision is now delegated to the tracers, allowing the tracer to decide when and if
   to use it. For example, if the :ref:`x-b3-sampled <config_http_conn_man_headers_x-b3-sampled>` header
   is supplied with the client request, its value will override any sampling decision made by the Envoy proxy.

diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst
@@ -181,12 +181,10 @@ The fields are:
 
 .. http:get:: /stats
 
-  Outputs all statistics on demand. This command is very useful for local debugging.
-  Histograms will output the computed quantiles i.e P0,P25,P50,P75,P90,P99,P99.9 and P100. 
-  The output for each quantile will be in the form of (interval,cumulative) where interval value 
-  represents the summary since last flush interval and cumulative value represents the 
-  summary since the start of envoy instance.
-  See :ref:`here <operations_stats>` for more information.
+  Outputs all statistics on demand. This includes only counters and gauges. Histograms are not
+  output as Envoy currently has no built in histogram support and relies on statsd for
+  aggregation. This command is very useful for local debugging. See :ref:`here <operations_stats>`
+  for more information.
 
   .. http:get:: /stats?format=json
 

diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h
@@ -2,7 +2,6 @@
 
 #include <chrono>
 #include <cstdint>
-#include <functional>
 #include <list>
 #include <memory>
 #include <string>
@@ -115,11 +114,6 @@ class Metric {
    * Returns the name of the Metric with the portions designated as tags removed.
    */
   virtual const std::string& tagExtractedName() const PURE;
-
-  /**
-   * Indicates whether this metric has been updated since the server was started.
-   */
-  virtual bool used() const PURE;
 };
 
 /**
@@ -134,6 +128,7 @@ class Counter : public virtual Metric {
   virtual void inc() PURE;
   virtual uint64_t latch() PURE;
   virtual void reset() PURE;
+  virtual bool used() const PURE;
   virtual uint64_t value() const PURE;
 };
 
@@ -151,34 +146,12 @@ class Gauge : public virtual Metric {
   virtual void inc() PURE;
   virtual void set(uint64_t value) PURE;
   virtual void sub(uint64_t amount) PURE;
+  virtual bool used() const PURE;
   virtual uint64_t value() const PURE;
 };
 
 typedef std::shared_ptr<Gauge> GaugeSharedPtr;
 
-/**
- * Holds the computed statistics for a histogram.
- */
-class HistogramStatistics {
-public:
-  virtual ~HistogramStatistics() {}
-
-  /**
-   * Returns summary representation of the histogram.
-   */
-  virtual std::string summary() const PURE;
-
-  /**
-   * Returns supported quantiles.
-   */
-  virtual const std::vector<double>& supportedQuantiles() const PURE;
-
-  /**
-   * Returns computed quantile values during the period.
-   */
-  virtual const std::vector<double>& computedQuantiles() const PURE;
-};
-
 /**
  * A histogram that records values one at a time.
  * Note: Histograms now incorporate what used to be timers because the only difference between the
@@ -198,32 +171,6 @@ class Histogram : public virtual Metric {
 
 typedef std::shared_ptr<Histogram> HistogramSharedPtr;
 
-/**
- * A histogram that is stored in main thread and provides summary view of the histogram.
- */
-class ParentHistogram : public virtual Histogram {
-public:
-  virtual ~ParentHistogram() {}
-
-  /**
-   * This method is called during the main stats flush process for each of the histograms and used
-   * to merge the histogram values.
-   */
-  virtual void merge() PURE;
-
-  /**
-   * Returns the interval histogram summary statistics for the flush interval.
-   */
-  virtual const HistogramStatistics& intervalStatistics() const PURE;
-
-  /**
-   * Returns the cumulative histogram summary statistics.
-   */
-  virtual const HistogramStatistics& cumulativeStatistics() const PURE;
-};
-
-typedef std::shared_ptr<ParentHistogram> ParentHistogramSharedPtr;
-
 /**
  * A sink for stats. Each sink is responsible for writing stats to a backing store.
  */
@@ -247,11 +194,6 @@ class Sink {
    */
   virtual void flushGauge(const Gauge& gauge, uint64_t value) PURE;
 
-  /**
-   * Flush a histogram.
-   */
-  virtual void flushHistogram(const ParentHistogram& histogram) PURE;
-
   /**
    * This will be called after beginFlush(), some number of flushCounter(), and some number of
    * flushGauge(). Sinks can use this to optimize writing if desired.
@@ -321,20 +263,10 @@ class Store : public Scope {
    * @return a list of all known gauges.
    */
   virtual std::list<GaugeSharedPtr> gauges() const PURE;
-
-  /**
-   * @return a list of all known histograms.
-   */
-  virtual std::list<ParentHistogramSharedPtr> histograms() const PURE;
 };
 
 typedef std::unique_ptr<Store> StorePtr;
 
-/**
- * Callback invoked when a store's mergeHistogram() runs.
- */
-typedef std::function<void()> PostMergeCb;
-
 /**
  * The root of the stat store.
  */
@@ -362,15 +294,6 @@ class StoreRoot : public Store {
    * down.
    */
   virtual void shutdownThreading() PURE;
-
-  /**
-   * Called during the flush process to merge all the thread local histograms. The passed in
-   * callback will be called on the main thread, but it will happen after the method returns
-   * which means that the actual flush process will happen on the main thread after this method
-   * returns. It is expected that only one merge runs at any time and concurrent calls to this
-   * method would be asserted.
-   */
-  virtual void mergeHistograms(PostMergeCb merge_complete_cb) PURE;
 };
 
 typedef std::unique_ptr<StoreRoot> StoreRootPtr;

diff --git a/include/envoy/thread_local/thread_local.h b/include/envoy/thread_local/thread_local.h
@@ -46,15 +46,6 @@ class Slot {
    */
   virtual void runOnAllThreads(Event::PostCb cb) PURE;
 
-  /**
-   * Run a callback on all registered threads with a barrier. A shutdown initiated during the
-   * running of the PostCBs may prevent all_threads_complete_cb from being called.
-   * @param cb supplies the callback to run on each thread.
-   * @param all_threads_complete_cb supplies the callback to run on main thread after cb has
-   * been run on all registered threads.
-   */
-  virtual void runOnAllThreads(Event::PostCb cb, Event::PostCb all_threads_complete_cb) PURE;
-
   /**
    * Set thread local data on all threads previously registered via registerThread().
    * @param initializeCb supplies the functor that will be called *on each thread*. The functor

diff --git a/source/common/common/logger.h b/source/common/common/logger.h
@@ -41,9 +41,7 @@ namespace Logger {
   FUNCTION(testing)              \
   FUNCTION(tracing)              \
   FUNCTION(upstream)             \
-  FUNCTION(grpc)                 \
-  FUNCTION(stats)
-
+  FUNCTION(grpc)
 
 enum class Id {
   ALL_LOGGER_IDS(GENERATE_ENUM)

diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD
@@ -12,17 +12,12 @@ envoy_cc_library(
     name = "stats_lib",
     srcs = ["stats_impl.cc"],
     hdrs = ["stats_impl.h"],
-    external_deps = [
-        "abseil_optional",
-        "libcircllhist",
-    ],
     deps = [
         "//include/envoy/common:time_interface",
         "//include/envoy/server:options_interface",
         "//include/envoy/stats:stats_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:hash_lib",
-        "//source/common/common:non_copyable",
         "//source/common/common:perf_annotation_lib",
         "//source/common/common:utility_lib",
         "//source/common/config:well_known_names",

diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc
@@ -277,38 +277,5 @@ void RawStatData::initialize(absl::string_view key) {
   name_[xfer_size] = '\0';
 }
 
-HistogramStatisticsImpl::HistogramStatisticsImpl(const histogram_t* histogram_ptr)
-    : computed_quantiles_(supportedQuantiles().size(), 0.0) {
-  hist_approx_quantile(histogram_ptr, supportedQuantiles().data(), supportedQuantiles().size(),
-                       computed_quantiles_.data());
-}
-
-const std::vector<double>& HistogramStatisticsImpl::supportedQuantiles() const {
-  static const std::vector<double> supported_quantiles = {0,    0.25, 0.5,   0.75, 0.90,
-                                                          0.95, 0.99, 0.999, 1};
-  return supported_quantiles;
-}
-
-std::string HistogramStatisticsImpl::summary() const {
-  std::vector<std::string> summary;
-  const std::vector<double>& supported_quantiles_ref = supportedQuantiles();
-  summary.reserve(supported_quantiles_ref.size());
-  for (size_t i = 0; i < supported_quantiles_ref.size(); ++i) {
-    summary.push_back(
-        fmt::format("P{}: {}", 100 * supported_quantiles_ref[i], computed_quantiles_[i]));
-  }
-  return absl::StrJoin(summary, ", ");
-}
-
-/**
- * Clears the old computed values and refreshes it with values computed from passed histogram.
- */
-void HistogramStatisticsImpl::refresh(const histogram_t* new_histogram_ptr) {
-  std::fill(computed_quantiles_.begin(), computed_quantiles_.end(), 0.0);
-  ASSERT(supportedQuantiles().size() == computed_quantiles_.size());
-  hist_approx_quantile(new_histogram_ptr, supportedQuantiles().data(), supportedQuantiles().size(),
-                       computed_quantiles_.data());
-}
-
 } // namespace Stats
 } // namespace Envoy