-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changes from 15 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
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,17 @@ | ||
cc_library( | ||
name = "libcircllhist", | ||
srcs = ["src/circllhist.c"], | ||
hdrs = [ | ||
"src/circllhist.h", | ||
"src/circllhist_config.h", # Generated. | ||
], | ||
includes = ["src"], | ||
visibility = ["//visibility:public"], | ||
) | ||
|
||
genrule( | ||
name = "circllhist_config", | ||
srcs = ["src/circllhist_config.h.in"], | ||
outs = ["src/circllhist_config.h"], | ||
cmd = "cp $< $@", | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,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. | ||
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. 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? 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 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. 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. OK you shouldn't change the name as that would make the PR bigger. But could you change to the comment something like:
@mattklein123 confirming that's accurate.... 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. it is not since last flush, it indicates it has actually ever been set a value. 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. 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? 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. 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; | ||
}; | ||
|
||
/** | ||
|
@@ -128,7 +133,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 +150,13 @@ 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; | ||
|
||
struct 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 | ||
|
@@ -167,6 +172,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 Histogram Summary Statistics for the flush interval. | ||
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. super nit: don't capitalize "Histogram Summary Statistics" |
||
*/ | ||
virtual HistogramStatistics intervalStatistics() const 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. I'm not actually sure how this compiles. I think it's luck if |
||
|
||
/** | ||
* Returns the Cumulative Histogram Summary Statistics. | ||
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. super nit: don't capitalize "Cumulative Histogram Summary Statistics" |
||
*/ | ||
virtual HistogramStatistics cumulativeStatistics() const PURE; | ||
}; | ||
|
||
typedef std::shared_ptr<Histogram> HistogramSharedPtr; | ||
|
@@ -194,6 +214,11 @@ class Sink { | |
*/ | ||
virtual void flushGauge(const Gauge& gauge, uint64_t value) PURE; | ||
|
||
/** | ||
* Flush a histogram. | ||
*/ | ||
virtual void flushHistogram(const Histogram&) 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. nit: |
||
|
||
/** | ||
* 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,6 +288,11 @@ 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; | ||
|
@@ -294,6 +324,11 @@ class StoreRoot : public Store { | |
* down. | ||
*/ | ||
virtual void shutdownThreading() PURE; | ||
|
||
/** | ||
* Called during the flush process to merge all the thread local histograms. | ||
*/ | ||
virtual void mergeHistograms() PURE; | ||
}; | ||
|
||
typedef std::unique_ptr<StoreRoot> StoreRootPtr; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,13 @@ class Slot { | |
*/ | ||
virtual void runOnAllThreads(Event::PostCb cb) PURE; | ||
|
||
/** | ||
* Run a callback on all registered threads with a barrier. | ||
* @param cb supplies the callback to run on each thread. | ||
* @param main_callback supplies the callback to run on main thread after threads are done. | ||
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. Can you add a warning here that there is no guarantee that this callback will fire during shutdown. I think that's OK for now but I would like to clearly mark that in case we need that later. 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. s/main_callback/all_threads_complete_cb (or something). Also per above clarify this runs on the main thread. You should also ASSERT in the impl that this function is only called on the main thread. |
||
*/ | ||
virtual void runOnAllThreadsWithBarrier(Event::PostCb cb, Event::PostCb main_callback) 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 | ||
|
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.
Can we follow up on @jmillikin-stripe comment here? It does seem weird that the generated header is just being copied. Can you add a comment as to why this is OK if it is? cc @postwait
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.
The _config.h.in is an autoconf input template. It is not safe to copy it on all platforms. There is a
in the circllhist.c file that needs that that conditionally set depending on system headers.
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.
@postwait thanks. What is your suggestion here? Should I copy based on platform? or which platforms it is good to copy?
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.
libcircllhist (like many open source projects) is auto-tools based. When you build it, it will build-time detect which features are available on your platform and act on that. I don't know how to do the same thing in cmake. I am pretty sure that HAVE_ALLOCA_H is the only important thing in that header though -- so if you can get cmake to provide that, you're set.
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.
Looking at this, it is only alloca() that is a problem. I've opened a PR on libcircllhist to resolve this. It should be possible shortly to just ignore that header completely.
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.
@postwait thank you so much.
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.
@postwait Awesome. Thanks. I got the new version and removed the genRule and it works.