-
Notifications
You must be signed in to change notification settings - Fork 94
Add new NH statistic class CircllhistStatistic and SinkableCircllhist… #391
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
Changes from 17 commits
af1ea54
43d7ee2
77249b9
bdecd69
2c14561
97ccc8a
6876450
8fd0a05
cd6b29e
684fe0b
dc655a4
fc298c5
e1356ec
a9d4d5d
e4d6672
db78892
8903b60
2f0e580
f01e18f
93bf7e8
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 |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
|
|
||
| #include "external/dep_hdrhistogram_c/src/hdr_histogram.h" | ||
| #include "external/envoy/source/common/common/logger.h" | ||
| #include "external/envoy/source/common/stats/histogram_impl.h" | ||
|
|
||
| #include "common/frequency.h" | ||
|
|
||
|
|
@@ -150,4 +151,91 @@ class HdrStatistic : public StatisticImpl { | |
| struct hdr_histogram* histogram_; | ||
| }; | ||
|
|
||
| } // namespace Nighthawk | ||
| /** | ||
| * CircllhistStatistic uses Circllhist under the hood to compute statistics. | ||
| * Circllhist is used in the implementation of Envoy Histograms, compared to HdrHistogram it trades | ||
| * precision for fast performance in merge and insertion. For more info, please see | ||
| * https://github.com/circonus-labs/libcircllhist | ||
| */ | ||
| class CircllhistStatistic : public StatisticImpl { | ||
| public: | ||
| CircllhistStatistic(); | ||
| ~CircllhistStatistic() override; | ||
|
|
||
| void addValue(uint64_t value) override; | ||
| double mean() const override; | ||
| double pvariance() const override; | ||
| double pstdev() const override; | ||
| StatisticPtr combine(const Statistic& statistic) const override; | ||
| uint64_t significantDigits() const override { return 1; } | ||
|
Member
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 seems a low guarantee; I wonder, can we improve? (HdrStatistic manages 4 significant digits in the way it we set it up).
Contributor
Author
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. Discussed offline. Confirmed that circllhist only has 2 digit decimal precision as a result of base 10 algorithm. Since this is out of current work scope, decided to leave the discussion of potential impacts for future work when we try to switch to circllhist
Collaborator
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. Once we move the implementation into the .cc file, we should add a comment explaining the single digit precision.
Collaborator
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 comment may have been missed. Even if we end up not moving this to the .cc file, we should add a comment explaining the choice of "1" as the precision.
Contributor
Author
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. Thank you mum4k@. |
||
| StatisticPtr createNewInstanceOfSameType() const override; | ||
| nighthawk::client::Statistic toProto(SerializationDomain domain) const override; | ||
|
|
||
| private: | ||
| histogram_t* histogram_; | ||
|
Collaborator
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. (optional) Using a raw pointer here opens us to memory leaks and all the usual problems of raw pointers. Would it be possible to move this to a smart pointer (std::unique_ptr) with a custom deleter? https://www.bfilipek.com/2016/04/custom-deleters-for-c-smart-pointers.html
Contributor
Author
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. Thanks Jakub for the reference. It seems the |
||
| }; | ||
|
|
||
| /** | ||
| * In order to be able to flush a histogram value to downstream Envoy stats Sinks, abstract class | ||
| * SinkableStatistic takes the Scope reference in the constructor and wraps the | ||
| * Envoy::Stats::HistogramHelper interface. Implementation of sinkable Nighthawk Statistic class | ||
| * will inherit from this class. | ||
| */ | ||
| class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { | ||
| public: | ||
| // Calling HistogramImplHelper(SymbolTable& symbol_table) constructor to construct an empty | ||
| // MetricImpl. This is to bypass the complicated logic of setting up SymbolTable/StatName in | ||
| // Envoy. | ||
| SinkableStatistic(Envoy::Stats::Scope& scope, absl::optional<int> worker_id); | ||
| ~SinkableStatistic() override; | ||
|
|
||
| // Currently Envoy Histogram Unit supports {Unspecified, Bytes, Microseconds, Milliseconds}. By | ||
| // default, Nighthawk::Statistic uses nanosecond as the unit of latency histograms, so Unspecified | ||
| // is returned here to isolate Nighthawk Statistic from Envoy Histogram Unit. | ||
| Envoy::Stats::Histogram::Unit unit() const override; | ||
| Envoy::Stats::SymbolTable& symbolTable() override; | ||
| // Return the id of the worker where this statistic is defined. Per worker | ||
| // statistic should always set worker_id. Return absl::nullopt when the | ||
| // statistic is not defined per worker. | ||
| const absl::optional<int> worker_id() { return worker_id_; } | ||
|
Collaborator
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. Since this one isn't an override and is public, we should document it. We should also explicitly say why does it return absl::optional and what does it mean when the value isn't present.
Contributor
Author
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. Done. PTAL |
||
|
|
||
| // This is used for delivering the histogram data to sinks. | ||
| Envoy::Stats::Scope& scope_; | ||
|
|
||
| private: | ||
| // worker_id can be used in downstream stats Sinks as the stats tag. | ||
| absl::optional<int> worker_id_; | ||
|
oschaaf marked this conversation as resolved.
Outdated
|
||
| }; | ||
|
|
||
| // Implementation of sinkable Nighthawk Statistic with HdrHistogram. | ||
| class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { | ||
| public: | ||
| // The constructor takes the Scope reference which is used to flush a histogram value to | ||
| // downstream stats Sinks through deliverHistogramToSinks(). | ||
| SinkableHdrStatistic(Envoy::Stats::Scope& scope, absl::optional<int> worker_id = absl::nullopt); | ||
|
|
||
| // Envoy::Stats::Histogram | ||
| void recordValue(uint64_t value) override; | ||
| bool used() const override { return count() > 0; } | ||
| // Overriding name() to return Nighthawk::Statistic::id(). | ||
| std::string name() const override { return id(); } | ||
| std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } | ||
| }; | ||
|
|
||
| // Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. | ||
| class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistStatistic { | ||
| public: | ||
| // The constructor takes the Scope reference which is used to flush a histogram value to | ||
| // downstream stats Sinks through deliverHistogramToSinks(). | ||
| SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, | ||
|
Collaborator
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 we document the constructor?
Contributor
Author
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. Done |
||
| absl::optional<int> worker_id = absl::nullopt); | ||
|
|
||
| // Envoy::Stats::Histogram | ||
| void recordValue(uint64_t value) override; | ||
| bool used() const override { return count() > 0; } | ||
| // Overriding name() to return Nighthawk::Statistic::id(). | ||
| std::string name() const override { return id(); } | ||
| std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } | ||
| }; | ||
|
|
||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,7 @@ COVERAGE_VALUE=${COVERAGE_VALUE%?} | |
|
|
||
| if [ "$VALIDATE_COVERAGE" == "true" ] | ||
| then | ||
| COVERAGE_THRESHOLD=98.6 | ||
| COVERAGE_THRESHOLD=98.4 | ||
|
Collaborator
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. Are we forced to lower the threshold? Which lines / behaviors we can't cover in this PR?
Contributor
Author
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. https://14282-180498819-gh.circle-artifacts.com/0/coverage/source/common/statistic_impl.h.gcov.html The lines not covered are |
||
| COVERAGE_FAILED=$(echo "${COVERAGE_VALUE}<${COVERAGE_THRESHOLD}" | bc) | ||
| if test ${COVERAGE_FAILED} -eq 1; then | ||
| echo Code coverage ${COVERAGE_VALUE} is lower than limit of ${COVERAGE_THRESHOLD} | ||
|
|
@@ -52,4 +52,4 @@ then | |
| echo Code coverage ${COVERAGE_VALUE} is good and higher than limit of ${COVERAGE_THRESHOLD} | ||
| fi | ||
| fi | ||
| echo "HTML coverage report is in ${COVERAGE_DIR}/index.html" | ||
| echo "HTML coverage report is in ${COVERAGE_DIR}/index.html" | ||
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.
Note: I think HdrHistogram will emit more percentiles as the number of (differently-valued) data points added grows. So it's not a fixed set with HdrHistogram.
I am not sure if there is an equivalent way to iterate the available buckets with libcircllhist to get more detail out, and I am also not sure this is really a problem, but I thought it would be good to call out the difference here.
(The only direct implication I figure it would have today is that the Fortio plots would be drawn with less data points)
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.
Thanks Otto for the information! Updated the comment to just
List of quantiles is based on hdr_proto_json.gold.From the APIs in circllhist.h, there is no easy way to iterate the quantiles. If the listed data points turn out to be not enough in the future, we can always add more here.