Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stats: add built-in log linear histogram support #2932

Closed
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
f0a7faa
per thread collection implementation
ramaraochavali Mar 28, 2018
1a35b0e
implemented merge logic
ramaraochavali Mar 29, 2018
5b66b74
fixed test cases
ramaraochavali Mar 30, 2018
8052ae7
formatted
ramaraochavali Mar 30, 2018
5d30b85
added support for flushHistograms and implemented for metrics sink
ramaraochavali Mar 30, 2018
a0bf03c
added runOnAllThreadsWithBarrier to TLS system
ramaraochavali Mar 31, 2018
dede1fd
addressed review comments
ramaraochavali Mar 31, 2018
e744253
handle hot restart case
ramaraochavali Mar 31, 2018
0aeac15
reacquire lock in mergeInternal
ramaraochavali Apr 1, 2018
c200974
new lock for merge process
ramaraochavali Apr 1, 2018
dd79e35
remove handle main thread
ramaraochavali Apr 1, 2018
9dcddf6
move worker_count to class level variable
ramaraochavali Apr 1, 2018
5e8169a
remove separate merge lock
ramaraochavali Apr 2, 2018
f35083c
use separate merge lock
ramaraochavali Apr 2, 2018
08cc211
renamed few methods, added used support for histograms
ramaraochavali Apr 2, 2018
a107b0f
removed exterc C usage and moved worker_count to shared ptr
ramaraochavali Apr 3, 2018
dadba03
added HistogramStatistics interface
ramaraochavali Apr 3, 2018
e41d7ef
moved histogram impl to threadlocal store along with merge_lock
ramaraochavali Apr 3, 2018
9da426e
fixed admin tsan violation
ramaraochavali Apr 3, 2018
92620e5
formatted
ramaraochavali Apr 3, 2018
6e6253a
compilation failure
ramaraochavali Apr 3, 2018
6ca87c4
calculate stats only once
ramaraochavali Apr 3, 2018
fa8f646
initialize stats
ramaraochavali Apr 3, 2018
2b5772f
corrected ref returns for other histograms
ramaraochavali Apr 3, 2018
1032ffc
merged master
ramaraochavali Apr 4, 2018
8266868
fixed compilation issues
ramaraochavali Apr 4, 2018
a041a3e
addressed review comments
ramaraochavali Apr 4, 2018
c9fef4b
added callback for mergeHistogram
ramaraochavali Apr 4, 2018
29646d0
addressed review comments and added basic test
ramaraochavali Apr 5, 2018
df348b2
added some tests
ramaraochavali Apr 5, 2018
68039f2
formatted
ramaraochavali Apr 5, 2018
1b75b93
formatted
ramaraochavali Apr 5, 2018
d5d4125
address review comments, added more tests
ramaraochavali Apr 5, 2018
16003e7
address review comments
ramaraochavali Apr 6, 2018
05d00a2
added more tests
ramaraochavali Apr 6, 2018
9189b9e
formatted
ramaraochavali Apr 6, 2018
1837956
updated the libcircl lib and added integration test
ramaraochavali Apr 7, 2018
8b1c7ab
added test case for runAllThreadsWithBarrier
ramaraochavali Apr 10, 2018
2894da8
removed unnecessary arg
ramaraochavali Apr 10, 2018
1e1f494
remove unnecessary code
ramaraochavali Apr 10, 2018
5ae99d3
addressed review comments, simiplified tests
ramaraochavali Apr 11, 2018
7d3844e
revert the time interval
ramaraochavali Apr 11, 2018
5ccad60
formatted
ramaraochavali Apr 11, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions bazel/external/libcircllhist.BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
cc_library(
name = "libcircllhist",
srcs = ["src/circllhist.c"],
hdrs = [
"src/circllhist.h",
],
includes = ["src"],
visibility = ["//visibility:public"],
)
11 changes: 11 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ 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()
Expand Down Expand Up @@ -215,6 +216,16 @@ 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",
Expand Down
4 changes: 4 additions & 0 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ REPOSITORY_LOCATIONS = dict(
commit = "44ae9609e860e3428cd057f7052e505b4819eb84", # 2018-02-06
remote = "https://github.com/bombela/backward-cpp",
),
com_github_circonus_labs_libcircllhist = dict(
commit = "eea8653b7e5abd8b5f2701e20e0334802f6d2dfb", # 2018-04-05
remote = "https://github.com/circonus-labs/libcircllhist",
),
com_github_cyan4973_xxhash = dict(
commit = "7caf8bd76440c75dfe1070d3acfbd7891aea8fca", # v0.6.4
remote = "https://github.com/Cyan4973/xxHash",
Expand Down
67 changes: 65 additions & 2 deletions include/envoy/stats/stats.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <chrono>
#include <cstdint>
#include <functional>
#include <list>
#include <memory>
#include <string>
Expand Down Expand Up @@ -114,6 +115,11 @@ class Metric {
* Returns the name of the Metric with the portions designated as tags removed.
*/
virtual const std::string& tagExtractedName() const PURE;

/**
* Indicates whether a metric has been used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has been used by what? What does 'used' mean here? From reading the code, does it mean non-empty? If so, could you call this empty() and invert it, per C++ conventions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually is a method used in other Metric as well. Used here signifies whether it has been updated/a value has been set because of some thing happening like connection closing etc. This is helpful in flushing only metrics whose value has been modified. I just moved it to the Metric interface now because histogram also uses it. Earlier Counter and Gauge has this method. I am not sure if it is equivalent of empty.

Copy link
Contributor

@jmarantz jmarantz Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK you shouldn't change the name as that would make the PR bigger.

But could you change to the comment something like:

  * Indicates whether the metric contains new data since the last flush().

@mattklein123 confirming that's accurate....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not since last flush, it indicates it has actually ever been set a value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No? Then what is it used for? Above you said "flushing only metrics whose value has been modified."

Over time, couldn't you wind up with every metric in this state, if the bit isn't cleared during flush?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it works like this. If a metric value has ever been set it is considered "used" and it would be pushed to stats sinks on subsequent flusher irrespective of whether it has been updated in that interval or not. I think it is reasonable because and would allow only metrics that are really used in that envoy instance to be pushed rather than all metrics. @mattklein123 may have better idea on the history. So I am not changing any thing here.

*/
virtual bool used() const PURE;
};

/**
Expand All @@ -128,7 +134,6 @@ 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;
};

Expand All @@ -146,12 +151,34 @@ 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
Expand All @@ -167,6 +194,21 @@ class Histogram : public virtual Metric {
* Records an unsigned value. If a timer, values are in units of milliseconds.
*/
virtual void recordValue(uint64_t value) PURE;

/**
* Merges the histogram values collected during the flush interval.
*/
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<Histogram> HistogramSharedPtr;
Expand Down Expand Up @@ -194,6 +236,11 @@ class Sink {
*/
virtual void flushGauge(const Gauge& gauge, uint64_t value) PURE;

/**
* Flush a histogram.
*/
virtual void flushHistogram(const Histogram& 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.
Expand Down Expand Up @@ -263,10 +310,20 @@ 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<HistogramSharedPtr> histograms() const PURE;
};

typedef std::unique_ptr<Store> StorePtr;

/**
* Callback invoked when a store's mergeHistogram() runs.
*/
typedef std::function<void()> PostMergeCb;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we can probably kill the typedef for this, just use std::function inline below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added it because, it is repeated in tests as well. if you are strongly think about removing it, I can remove it.


/**
* The root of the stat store.
*/
Expand Down Expand Up @@ -294,6 +351,12 @@ class StoreRoot : public Store {
* down.
*/
virtual void shutdownThreading() PURE;

/**
* Called during the flush process to merge all the thread local histograms. The passed in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this is going to complete on another thread. If so, it's worth calling out explicitly that mergeHistogram will return before it is complete.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback will be called on the main thread, but it will happen after the method returns. Please clarify. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please clarify in the comment when the flush will complete, and in what thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarified. lmk any changes are needed here.

* callback will be called on the main thread, but it will happen after the method returns.
*/
virtual void mergeHistograms(PostMergeCb merge_complete_cb) PURE;
};

typedef std::unique_ptr<StoreRoot> StoreRootPtr;
Expand Down
10 changes: 10 additions & 0 deletions include/envoy/thread_local/thread_local.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ 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 threads are
* done.
*/
virtual void runOnAllThreadsWithBarrier(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
Expand Down
4 changes: 4 additions & 0 deletions source/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ 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",
Expand Down
82 changes: 74 additions & 8 deletions source/common/stats/stats_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
#include "common/common/utility.h"
#include "common/protobuf/protobuf.h"

#include "absl/strings/str_join.h"
#include "absl/strings/string_view.h"
#include "circllhist.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a TODO here to get extern "C" in the headers for the library? (Or just do a PR over there). cc @postwait

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@postwait awesome. I updated the latest version and it is no longer needed.


namespace Envoy {
namespace Stats {
Expand Down Expand Up @@ -167,9 +169,6 @@ class Utility {
* RawStatData::size() instead.
*/
struct RawStatData {
struct Flags {
static const uint8_t Used = 0x1;
};

/**
* Due to the flexible-array-length of name_, c-style allocation
Expand Down Expand Up @@ -284,6 +283,14 @@ class MetricImpl : public virtual Metric {
const std::string& tagExtractedName() const override { return tag_extracted_name_; }
const std::vector<Tag>& tags() const override { return tags_; }

protected:
/**
* Flags used by all stats types to figure out whether they have been used.
*/
struct Flags {
static const uint8_t Used = 0x1;
};

private:
const std::string name_;
const std::string tag_extracted_name_;
Expand All @@ -305,13 +312,13 @@ class CounterImpl : public Counter, public MetricImpl {
void add(uint64_t amount) override {
data_.value_ += amount;
data_.pending_increment_ += amount;
data_.flags_ |= RawStatData::Flags::Used;
data_.flags_ |= Flags::Used;
}

void inc() override { add(1); }
uint64_t latch() override { return data_.pending_increment_.exchange(0); }
void reset() override { data_.value_ = 0; }
bool used() const override { return data_.flags_ & RawStatData::Flags::Used; }
bool used() const override { return data_.flags_ & Flags::Used; }
uint64_t value() const override { return data_.value_; }

private:
Expand All @@ -333,27 +340,71 @@ class GaugeImpl : public Gauge, public MetricImpl {
// Stats::Gauge
virtual void add(uint64_t amount) override {
data_.value_ += amount;
data_.flags_ |= RawStatData::Flags::Used;
data_.flags_ |= Flags::Used;
}
virtual void dec() override { sub(1); }
virtual void inc() override { add(1); }
virtual void set(uint64_t value) override {
data_.value_ = value;
data_.flags_ |= RawStatData::Flags::Used;
data_.flags_ |= Flags::Used;
}
virtual void sub(uint64_t amount) override {
ASSERT(data_.value_ >= amount);
ASSERT(used());
data_.value_ -= amount;
}
virtual uint64_t value() const override { return data_.value_; }
bool used() const override { return data_.flags_ & RawStatData::Flags::Used; }
bool used() const override { return data_.flags_ & Flags::Used; }

private:
RawStatData& data_;
RawStatDataAllocator& alloc_;
};

/**
* Implementation of HistogramStatistics for circllhist.
*/
class HistogramStatisticsImpl : public HistogramStatistics {
public:
HistogramStatisticsImpl() : computed_quantiles_(supported_quantiles_.size(), 0.0) {}

HistogramStatisticsImpl(histogram_t* histogram_ptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to cc file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

: computed_quantiles_(supported_quantiles_.size(), 0.0) {
hist_approx_quantile(histogram_ptr, supported_quantiles_.data(), supported_quantiles_.size(),
computed_quantiles_.data());
}

HistogramStatisticsImpl(const HistogramStatisticsImpl&) = delete;
HistogramStatisticsImpl& operator=(HistogramStatisticsImpl const&) = delete;

std::string summary() const override {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omve to cc file

std::vector<std::string> summary;
for (size_t i = 0; i < supported_quantiles_.size(); ++i) {
summary.push_back(
fmt::format("P{}: {}", 100 * supported_quantiles_[i],
std::isnan(computed_quantiles_[i]) ? 0 : computed_quantiles_[i]));
}
return absl::StrJoin(summary, ", ");
}

const std::vector<double>& supportedQuantiles() const override { return supported_quantiles_; }

const std::vector<double>& computedQuantiles() const override { return computed_quantiles_; }

/**
* Clears the old computed values and refreshes it with values computed from passed histogram.
*/
void refresh(histogram_t* new_histogram_ptr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to cc file

computed_quantiles_.clear();
hist_approx_quantile(new_histogram_ptr, supported_quantiles_.data(),
supported_quantiles_.size(), computed_quantiles_.data());
}

private:
const std::vector<double> supported_quantiles_ = {0, 0.25, 0.5, 0.75, 0.90, 0.95, 0.99, 0.999, 1};
std::vector<double> computed_quantiles_;
};

/**
* Histogram implementation for the heap.
*/
Expand All @@ -366,7 +417,21 @@ class HistogramImpl : public Histogram, public MetricImpl {
// Stats::Histogram
void recordValue(uint64_t value) override { parent_.deliverHistogramToSinks(*this, value); }

void merge() override {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this ever get called? If not I would do NOT_IMPLEMENTED. Same for used(). It would probably be better to actually split the histogram interfaces into two different interfaces to avoid this. (Basically some type of parent interface, and then a basic histogram interface). If you don't want to do that I would do NOT_IMPLEMENTED and put in a TODO.


bool used() const override { return true; }

const HistogramStatistics& intervalStatistics() const override { return interval_statistics_; }

const HistogramStatistics& cumulativeStatistics() const override {
return cumulative_statistics_;
}

Store& parent_;

private:
HistogramStatisticsImpl interval_statistics_;
HistogramStatisticsImpl cumulative_statistics_;
};

/**
Expand Down Expand Up @@ -446,6 +511,7 @@ class IsolatedStoreImpl : public Store {
// Stats::Store
std::list<CounterSharedPtr> counters() const override { return counters_.toList(); }
std::list<GaugeSharedPtr> gauges() const override { return gauges_.toList(); }
std::list<HistogramSharedPtr> histograms() const override { return histograms_.toList(); }

private:
struct ScopeImpl : public Scope {
Expand Down
Loading