-
Notifications
You must be signed in to change notification settings - Fork 89
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 all 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,94 @@ 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; | ||
| // circllhist has low significant digit precision as a result of base 10 | ||
| // algorithm. | ||
| uint64_t significantDigits() const override { return 1; } | ||
| 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 |
||
|
|
||
| protected: | ||
|
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 you help me understand why this is protected rather than public?
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. Ah, there is no special reason other than to make its access more restrictive. So changed it to public.
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. I apologize, I made a typo in the comment. I meant to say "rather than private". We should always prefer private methods and fields as they minimize the public API and allow us to execute faster (internal changes). Can this be private instead? |
||
| // This is used in child class 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
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| // 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.
This seems a low guarantee; I wonder, can we improve? (HdrStatistic manages 4 significant digits in the way it we set it up).
Uh oh!
There was an error while loading. Please reload this page.
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.
Discussed offline. Confirmed that circllhist only has 2 digit decimal precision as a result of base 10 algorithm.
#115 (comment)
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
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.
Once we move the implementation into the .cc file, we should add a comment explaining the single digit precision.
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.
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.
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.
Thank you mum4k@.
Added a comment here.