-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Changes from 55 commits
f0a7faa
1a35b0e
5b66b74
8052ae7
5d30b85
a0bf03c
dede1fd
e744253
0aeac15
c200974
dd79e35
9dcddf6
5e8169a
f35083c
08cc211
a107b0f
dadba03
e41d7ef
9da426e
92620e5
6e6253a
6ca87c4
fa8f646
2b5772f
1032ffc
8266868
a041a3e
c9fef4b
29646d0
df348b2
68039f2
1b75b93
d5d4125
16003e7
05d00a2
9189b9e
1837956
8b1c7ab
2894da8
1e1f494
5ae99d3
7d3844e
5ccad60
339674c
33da1eb
61eb8bb
60f79e6
532a59b
aa5eda4
72d8850
cd0851f
85f5967
c010914
135d6f8
369877a
97daa92
edf4a52
b204bff
e6669b2
64d8c2a
86f447d
af7aacd
e9547f6
60428d3
9ee5b0a
b46f01c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
#include <chrono> | ||
#include <cstdint> | ||
#include <functional> | ||
#include <list> | ||
#include <memory> | ||
#include <string> | ||
|
@@ -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; | ||
}; | ||
|
||
/** | ||
|
@@ -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; | ||
}; | ||
|
||
|
@@ -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 | ||
|
@@ -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; | ||
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
*/ | ||
|
@@ -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. | ||
|
@@ -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. | ||
*/ | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -273,5 +273,34 @@ 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}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to not call refresh here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to save on |
||
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) { | ||
for (size_t i = 0; i < computed_quantiles_.size(); ++i) { | ||
computed_quantiles_[i] = 0; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fine; you could also use FYI in practice what you had earlier with clear() had two potential problems with it:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have changed it to use the way you suggested. All the merge tests execute this logic. I think even though clear does not set it to 0, the |
||
hist_approx_quantile(new_histogram_ptr, supported_quantiles_.data(), supported_quantiles_.size(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually I'd put: |
||
computed_quantiles_.data()); | ||
} | ||
|
||
} // namespace Stats | ||
} // namespace Envoy |
There was a problem hiding this comment.
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 thelibcircl
to stats.h interface definition. Otherwise I have to usedynamic_cast
in theParentHistogramImpl
merge method. LMK if there are any other better solutions.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 instats.h
as it is not used by others in the system. I moved it to thread_local_store.h. Also I removed theaddTlsHistogram
method inParentHistogram
as that is an implementation detail. Now I think it looks clean. PTAL.