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 #3059

Closed
Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
66 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
339674c
addressed review comments
ramaraochavali Apr 12, 2018
33da1eb
formatted
ramaraochavali Apr 12, 2018
61eb8bb
corrected the test case wording
ramaraochavali Apr 12, 2018
60f79e6
remove ASSERT
ramaraochavali Apr 12, 2018
532a59b
added warn log for duplicate hist name
ramaraochavali Apr 13, 2018
aa5eda4
addressed some more review comments
ramaraochavali Apr 14, 2018
72d8850
fixed compilation issues
ramaraochavali Apr 14, 2018
cd0851f
make histogramptr const
ramaraochavali Apr 14, 2018
85f5967
addressed review comments
ramaraochavali Apr 15, 2018
c010914
revert unlock
ramaraochavali Apr 15, 2018
135d6f8
safe clear computed quantiles
ramaraochavali Apr 15, 2018
369877a
formatted
ramaraochavali Apr 15, 2018
97daa92
moved static initialization of supportedQuantiles to first access
ramaraochavali Apr 16, 2018
edf4a52
formatted
ramaraochavali Apr 16, 2018
b204bff
added setUp and tearDown methods
ramaraochavali Apr 16, 2018
e6669b2
moved ThreadLocalHistogram interface to thread_local_store
ramaraochavali Apr 16, 2018
64d8c2a
fixed test comments
ramaraochavali Apr 16, 2018
86f447d
fixed test cases
ramaraochavali Apr 17, 2018
af7aacd
address review comments
ramaraochavali Apr 17, 2018
e9547f6
remove unnecessary array during tls histogram merge
ramaraochavali Apr 17, 2018
60428d3
addressed some more test comments
ramaraochavali Apr 17, 2018
9ee5b0a
removed struct type def
ramaraochavali Apr 17, 2018
b46f01c
address review comments
ramaraochavali Apr 18, 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 = "97ef5e088fd01fa8ec5a86334a6308ac0d51ea6f", # 2018-04-07
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
108 changes: 106 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 this metric has been updated since the server was started.
*/
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 @@ -171,6 +198,60 @@ class Histogram : public virtual Metric {

typedef std::shared_ptr<Histogram> HistogramSharedPtr;

/**
* A histogram that is stored in TLS and used to record values per thread. This holds two
* histograms, one to collect the values and other as backup that is used for merge process. The
* swap happens during the merge process.
*/
class ThreadLocalHistogram : public virtual Histogram {
public:
virtual ~ThreadLocalHistogram() {}

/**
* Called in the beginning of merge process. Swaps the histogram used for collection so that we do
* not have to lock the histogram in high throughput TLS writes.
*/
virtual void beginMerge() PURE;
};

class ThreadLocalHistogramImpl;
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 had to forward declare this because with out this, I have to explicitly add merge(histogram_t* target) to ThreadLocalHistogram interface which means we have to expose the libcircl to stats.h interface definition. Otherwise I have to use dynamic_cast in the ParentHistogramImpl merge method. LMK if there are any other better solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be exposing libcircl to stats.h interface. Nor do I think we should be exposing ThreadLocalHistogramImpl. Can't you put the methods you need into the interface and use abstract classes except in the impl itself?

Scanning through stats.h, there are actually a couple of references to 'ThreadLocal' in StoreRoot, but otherwise there shouldn't really be references to a particular implementation from an interface. There's some kind of abstraction leak here, IMO.

Copy link
Contributor Author

@ramaraochavali ramaraochavali Apr 16, 2018

Choose a reason for hiding this comment

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

ok. Thinking further, I think agree with you that ThreadLocalHistogram should not be in stats.h as it is not used by others in the system. I moved it to thread_local_store.h. Also I removed the addTlsHistogram method in ParentHistogram as that is an implementation detail. Now I think it looks clean. PTAL.


typedef std::shared_ptr<ThreadLocalHistogramImpl> TlsHistogramSharedPtr;

/**
* A histogram that is stored in main thread, manages all thread local histograms and provides
* summary view of the histogram.
*/
class ParentHistogram : public virtual Metric {
public:
virtual ~ParentHistogram() {}

/**
* This method is called during the main stats flush process for each of the histogram. This
* method iterates through the Tls histograms and collects the histogram data of all of them
* in to "interval_histogram_". Then the collected "interval_histogram_" is merged to a
* "cumulative_histogram".
*/
virtual void merge() PURE;

/**
* This is used to keep track of all worker thread local histograms.
*/
virtual void addTlsHistogram(TlsHistogramSharedPtr hist_ptr) PURE;
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about making this a HistogramSharedPtr -- which references only the interface.


/**
* 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.
*/
Expand All @@ -194,6 +275,11 @@ 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.
Expand Down Expand Up @@ -263,10 +349,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<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.
*/
Expand Down Expand Up @@ -294,6 +390,14 @@ 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.
*/
virtual void mergeHistograms(PostMergeCb merge_complete_cb) PURE;
};

typedef std::unique_ptr<StoreRoot> StoreRootPtr;
Expand Down
9 changes: 9 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,15 @@ 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 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
Expand Down
4 changes: 3 additions & 1 deletion source/common/common/logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ namespace Logger {
FUNCTION(testing) \
FUNCTION(tracing) \
FUNCTION(upstream) \
FUNCTION(grpc)
FUNCTION(grpc) \
FUNCTION(stats)


enum class Id {
ALL_LOGGER_IDS(GENERATE_ENUM)
Expand Down
5 changes: 5 additions & 0 deletions source/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,17 @@ 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",
Expand Down
27 changes: 27 additions & 0 deletions source/common/stats/stats_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -273,5 +273,32 @@ void RawStatData::initialize(absl::string_view key) {
name_[xfer_size] = '\0';
}

const std::vector<double> HistogramStatisticsImpl::supported_quantiles_ = {
0, 0.25, 0.5, 0.75, 0.90, 0.95, 0.99, 0.999, 1};
Copy link
Contributor

Choose a reason for hiding this comment

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

See the comments in this PR about how to initialize std::vector, and https://github.com/envoyproxy/envoy/blob/master/STYLE.md

The Google C++ style guide points out that non-PoD static and global variables are forbidden. This includes types such as std::string. We encourage the use of the advice in the C++ FAQ on the static initialization fiasco for how to best handle this.


HistogramStatisticsImpl::HistogramStatisticsImpl(const histogram_t* histogram_ptr)
: computed_quantiles_(supported_quantiles_.size(), 0.0) {
hist_approx_quantile(histogram_ptr, supported_quantiles_.data(), supported_quantiles_.size(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not call refresh here?

Copy link
Contributor Author

@ramaraochavali ramaraochavali Apr 14, 2018

Choose a reason for hiding this comment

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

just to save on computed_quantiles_.clear(); call. It is just initialized and no need to clear immediately.

computed_quantiles_.data());
}

std::string HistogramStatisticsImpl::summary() const {
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], 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) {
computed_quantiles_.clear();
hist_approx_quantile(new_histogram_ptr, supported_quantiles_.data(), supported_quantiles_.size(),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this tested? because computed_quantiles_.size() will be zero here and I think the results will be undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is tested. but now changed it to set to zero explicitly with out calling clear. I think that should be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually I'd put:
ASSERT(supported_quantiles_.size() == computed_quantiles_.size());
as that is what's being assumed in this line, without it being locally obvious that it must be the case.

computed_quantiles_.data());
}

} // namespace Stats
} // namespace Envoy
Loading