Add new NH statistic class CircllhistStatistic and SinkableCircllhist… #391
Conversation
|
/retest |
|
🙀 failed invoking rebuild of |
|
@oschaaf hi Otto, it seems all CI checks failed again due to same error as last time. I will try to trigger it to rerun, meanwhile please go ahead review the change. Thanks! |
|
/retest |
|
🙀 failed invoking rebuild of |
There was a problem hiding this comment.
This is awesome, and once we merge this it should be trivial to make all the histograms in NH use circllhist as a statistics backend.
I haven't been able to absorb the whole PR at high detail yet, so I'd love to have another pass at it a little bit later; but flushing out my first comments to get started.
| } | ||
| StatisticPtr combine(const Statistic& statistic) const override; | ||
| uint64_t significantDigits() const override { return 1; } |
There was a problem hiding this comment.
This seems a low guarantee; I wonder, can we improve? (HdrStatistic manages 4 significant digits in the way it we set it up).
There was a problem hiding this comment.
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.
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.
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.
Thank you mum4k@.
Added a comment here.
|
(I have asked for help with CI in the Slack nighthawk-maintainers channel; I'm still clueless as to what is causing the CI trouble here, and me creating mirror PRs and closing them once we merge isn't sustainable) |
oschaaf
added
waiting-for-changes
|
I did the magic trick again, creating a PR from my own repo with the same commits to trigger CI. Let's see what happens. |
|
Thanks Otto for the help! Sorry I'm on vacation till the end of this week,
I will try to fix the CI check (currently this is too much hassle :( ) and
update the PR next week.
…On Thu, Jul 2, 2020, 10:55 AM Otto van der Schaaf ***@***.***> wrote:
I did the magic trick again, creating a PR from my own repo with the same
commits to trigger CI. Let's see what happens.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#391 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEH6YKY5HDKWDYHFS43FODTRZSNXTANCNFSM4OM6IZXA>
.
|
…Statistic Signed-off-by: Qin Qin <[email protected]> Signed-off-by: qqustc <[email protected]> Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: qqustc <[email protected]> Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
qqustc
added
waiting-for-review
Signed-off-by: Qin Qin <[email protected]> Signed-off-by: qqustc <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
qqustc
added
waiting-for-review
| @@ -43,7 +43,7 @@ COVERAGE_VALUE=${COVERAGE_VALUE%?} | |||
|
|
|||
| if [ "$VALIDATE_COVERAGE" == "true" ] | |||
| then | |||
| COVERAGE_THRESHOLD=98.6 | |||
| COVERAGE_THRESHOLD=98.4 | |||
There was a problem hiding this comment.
Are we forced to lower the threshold? Which lines / behaviors we can't cover in this PR?
There was a problem hiding this comment.
https://14282-180498819-gh.circle-artifacts.com/0/coverage/source/common/statistic_impl.h.gcov.html
The lines not covered are std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }, where I added test EXPECT_DEATH(stat.tagExtractedName(), ".*"); but it does not count into coverage. So after talked to Otto, we decided to lower the coverage.
test/statistic_test.cc
Outdated
| util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( | ||
| TestEnvironment::runfilesPath("test/test_data/circllhist_proto_json.gold")), | ||
| parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); | ||
| // Instead of comparing proto's, we perform a string-based comparison, because that emits a |
There was a problem hiding this comment.
A more idiomatic approach would be to compare the proto and output a textual diff only if the comparison fails. That way we will be comparing via the binary representations that are the public API. The string are only for humans to read.
Note that EXPECT_EQ takes in an optional stream which can contain a custom error message - this is where we can inject the readable diff:
https://github.com/google/googletest/blob/master/googletest/docs/primer.md#assertions
There was a problem hiding this comment.
Changed to compare the proto. PTAL.
source/common/statistic_impl.h
Outdated
| class CircllhistStatistic : public StatisticImpl { | ||
| public: | ||
| CircllhistStatistic() { |
There was a problem hiding this comment.
Can we place the implementation of the constructor in the .cc file? It helps to keep the header files more readable and focused on the API.
Same comment applies to the destructor and the other methods and class.
There was a problem hiding this comment.
Done. Moved most of the implementation to .cc file and only leave some of the 1 line function in .h file. PTAL
| } | ||
| StatisticPtr combine(const Statistic& statistic) const override; | ||
| uint64_t significantDigits() const override { return 1; } |
There was a problem hiding this comment.
Once we move the implementation into the .cc file, we should add a comment explaining the single digit precision.
|
|
||
| private: | ||
| histogram_t* histogram_; |
There was a problem hiding this comment.
(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
There was a problem hiding this comment.
Thanks Jakub for the reference. It seems the hist_free(histogram_t *hist) is quite complicated, so I will not move it to a smart pointer for now
| class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistStatistic { | ||
| public: | ||
| SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, |
There was a problem hiding this comment.
Can we document the constructor?
source/common/statistic_impl.cc
Outdated
| StatisticPtr CircllhistStatistic::combine(const Statistic& statistic) const { | ||
| auto combined = std::make_unique<CircllhistStatistic>(); | ||
| const auto& b = dynamic_cast<const CircllhistStatistic&>(statistic); |
There was a problem hiding this comment.
Let's find a better (more expressive) variable name than just "b".
There was a problem hiding this comment.
changed it to "stat".
source/common/statistic_impl.cc
Outdated
| auto combined = std::make_unique<CircllhistStatistic>(); | ||
| const auto& b = dynamic_cast<const CircllhistStatistic&>(statistic); | ||
| hist_accumulate(combined->histogram_, &this->histogram_, 1); |
There was a problem hiding this comment.
Without seeing the API of the histogram, it is hard to figure out what does the literal 1 stand for. Is there an useful name we can give it by either making it a constant or placing an inline comment?
// Either:
const int good_name = 1;
hist_accumulate(combined->histogram_, &this->histogram_, good_name);
// Or:
hist_accumulate(combined->histogram_, &this->histogram_, /* good_name = */ 1);There was a problem hiding this comment.
Thanks Jakub for the advice. Added an inline comment.
source/common/statistic_impl.cc
Outdated
| } | ||
|
|
||
| std::vector<double> quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, |
There was a problem hiding this comment.
Can we add a comment indicating how did we select these quantile values?
source/common/statistic_impl.cc
Outdated
| } | ||
|
|
||
| std::vector<double> quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, |
There was a problem hiding this comment.
(optional) If this and the next vector can be const, marking them so helps the reader understand that they aren't going to be modified further down.
mum4k
added
waiting-for-changes
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
qqustc
added
waiting-for-review
| } | ||
| StatisticPtr combine(const Statistic& statistic) const override; | ||
| uint64_t significantDigits() const override { return 1; } |
There was a problem hiding this comment.
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.
| const absl::optional<int> worker_id() { return worker_id_; } | ||
|
|
||
| protected: |
There was a problem hiding this comment.
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?
mum4k
added
waiting-for-changes
source/common/statistic_impl.cc
Outdated
| } | ||
|
|
||
| // List of quantiles is based on hdr_proto_json.gold which lists the quantiles provided by |
There was a problem hiding this comment.
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.
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.
Signed-off-by: Qin Qin <[email protected]>
Thank you @mum4k for the review. |
qqustc
added
waiting-for-review
|
/retest |
|
🙀 failed invoking rebuild of |
Signed-off-by: Qin Qin <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
Add new NH statistic class CircllhistStatistic and SinkableCircllhistStatistic.
CircllhistStatistic uses Circllhist under the hood to compute statistics where Circllhist is used in the implementation of Envoy Histograms. Compared to HdrHistogram Circllhist trades precision for fast performance in merge and insertion according to #115.
SinkableCircllhistStatistic wraps the Envoy::Stats::Histogram interface and is used to flush histogram value to downstream Envoy stats Sinks.
Linked Issues: #344
Testing: unit tests