Skip to content
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.

Sign up for GitHub

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

Jump to bottom

stats: add built-in log linear histogram support #3059

Closed
wants to merge 66 commits into from
Closed

stats: add built-in log linear histogram support #3059

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
f0a7faa
per thread collection implementation
ramaraochavali Mar 28, 2018
1a35b0e
implemented merge logic
ramaraochavali Mar 29, 2018
5b66b74
fixed test cases
ramaraochavali Mar 30, 2018
8052ae7
formatted
ramaraochavali Mar 30, 2018
5d30b85
added support for flushHistograms and implemented for metrics sink
ramaraochavali Mar 30, 2018
a0bf03c
added runOnAllThreadsWithBarrier to TLS system
ramaraochavali Mar 31, 2018
dede1fd
addressed review comments
ramaraochavali Mar 31, 2018
e744253
handle hot restart case
ramaraochavali Mar 31, 2018
0aeac15
reacquire lock in mergeInternal
ramaraochavali Apr 1, 2018
c200974
new lock for merge process
ramaraochavali Apr 1, 2018
dd79e35
remove handle main thread
ramaraochavali Apr 1, 2018
9dcddf6
move worker_count to class level variable
ramaraochavali Apr 1, 2018
5e8169a
remove separate merge lock
ramaraochavali Apr 2, 2018
f35083c
use separate merge lock
ramaraochavali Apr 2, 2018
08cc211
renamed few methods, added used support for histograms
ramaraochavali Apr 2, 2018
a107b0f
removed exterc C usage and moved worker_count to shared ptr
ramaraochavali Apr 3, 2018
dadba03
added HistogramStatistics interface
ramaraochavali Apr 3, 2018
e41d7ef
moved histogram impl to threadlocal store along with merge_lock
ramaraochavali Apr 3, 2018
9da426e
fixed admin tsan violation
ramaraochavali Apr 3, 2018
92620e5
formatted
ramaraochavali Apr 3, 2018
6e6253a
compilation failure
ramaraochavali Apr 3, 2018
6ca87c4
calculate stats only once
ramaraochavali Apr 3, 2018
fa8f646
initialize stats
ramaraochavali Apr 3, 2018
2b5772f
corrected ref returns for other histograms
ramaraochavali Apr 3, 2018
1032ffc
merged master
ramaraochavali Apr 4, 2018
8266868
fixed compilation issues
ramaraochavali Apr 4, 2018
a041a3e
addressed review comments
ramaraochavali Apr 4, 2018
c9fef4b
added callback for mergeHistogram
ramaraochavali Apr 4, 2018
29646d0
addressed review comments and added basic test
ramaraochavali Apr 5, 2018
df348b2
added some tests
ramaraochavali Apr 5, 2018
68039f2
formatted
ramaraochavali Apr 5, 2018
1b75b93
formatted
ramaraochavali Apr 5, 2018
d5d4125
address review comments, added more tests
ramaraochavali Apr 5, 2018
16003e7
address review comments
ramaraochavali Apr 6, 2018
05d00a2
added more tests
ramaraochavali Apr 6, 2018
9189b9e
formatted
ramaraochavali Apr 6, 2018
1837956
updated the libcircl lib and added integration test
ramaraochavali Apr 7, 2018
8b1c7ab
added test case for runAllThreadsWithBarrier
ramaraochavali Apr 10, 2018
2894da8
removed unnecessary arg
ramaraochavali Apr 10, 2018
1e1f494
remove unnecessary code
ramaraochavali Apr 10, 2018
5ae99d3
addressed review comments, simiplified tests
ramaraochavali Apr 11, 2018
7d3844e
revert the time interval
ramaraochavali Apr 11, 2018
5ccad60
formatted
ramaraochavali Apr 11, 2018
339674c
addressed review comments
ramaraochavali Apr 12, 2018
33da1eb
formatted
ramaraochavali Apr 12, 2018
61eb8bb
corrected the test case wording
ramaraochavali Apr 12, 2018
60f79e6
remove ASSERT
ramaraochavali Apr 12, 2018
532a59b
added warn log for duplicate hist name
ramaraochavali Apr 13, 2018
aa5eda4
addressed some more review comments
ramaraochavali Apr 14, 2018
72d8850
fixed compilation issues
ramaraochavali Apr 14, 2018
cd0851f
make histogramptr const
ramaraochavali Apr 14, 2018
85f5967
addressed review comments
ramaraochavali Apr 15, 2018
c010914
revert unlock
ramaraochavali Apr 15, 2018
135d6f8
safe clear computed quantiles
ramaraochavali Apr 15, 2018
369877a
formatted
ramaraochavali Apr 15, 2018
97daa92
moved static initialization of supportedQuantiles to first access
ramaraochavali Apr 16, 2018
edf4a52
formatted
ramaraochavali Apr 16, 2018
b204bff
added setUp and tearDown methods
ramaraochavali Apr 16, 2018
e6669b2
moved ThreadLocalHistogram interface to thread_local_store
ramaraochavali Apr 16, 2018
64d8c2a
fixed test comments
ramaraochavali Apr 16, 2018
86f447d
fixed test cases
ramaraochavali Apr 17, 2018
af7aacd
address review comments
ramaraochavali Apr 17, 2018
e9547f6
remove unnecessary array during tls histogram merge
ramaraochavali Apr 17, 2018
60428d3
addressed some more test comments
ramaraochavali Apr 17, 2018
9ee5b0a
removed struct type def
ramaraochavali Apr 17, 2018
b46f01c
address review comments
ramaraochavali Apr 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions source/common/stats/stats_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,10 @@ const std::vector<double>& HistogramStatisticsImpl::supportedQuantiles() const {

std::string HistogramStatisticsImpl::summary() const {
std::vector<std::string> summary;
for (size_t i = 0; i < supportedQuantiles().size(); ++i) {
const std::vector<double>& supported_quantiles_ref = supportedQuantiles();
for (size_t i = 0; i < supported_quantiles_ref.size(); ++i) {
summary.push_back(
fmt::format("P{}: {}", 100 * supportedQuantiles()[i], computed_quantiles_[i]));
fmt::format("P{}: {}", 100 * supported_quantiles_ref[i], computed_quantiles_[i]));
}
return absl::StrJoin(summary, ", ");
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/stats/stats_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ class GaugeImpl : public Gauge, public MetricImpl {
*/
class HistogramStatisticsImpl : public HistogramStatistics, NonCopyable {
public:
HistogramStatisticsImpl() : computed_quantiles_(supported_quantiles_.size(), 0.0) {}
HistogramStatisticsImpl() : computed_quantiles_(supportedQuantiles().size(), 0.0) {}
/**
* HistogramStatisticsImpl object is constructed using the passed in histogram.
* @param histogram_ptr pointer to the histogram for which stats will be calculated. This pointer
Expand Down
76 changes: 36 additions & 40 deletions test/common/stats/thread_local_store_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,19 @@ class StatsThreadLocalStoreTest : public testing::Test, public RawStatDataAlloca
store_->addSink(sink_);
}

void setUp() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void SetUp() override { ....

then you can remove the explicit calls from the test methods. It has to be applicable to all the test-methods in the class, so you might want to make a subclass just for the histograms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a added a new sub class and made this override and removed explicit calls. similar change for TearDown.

InSequence s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit I've never been a huge gmock user. But I looked up InSequence and I suspect this usage model is not quite right. Check out https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md#ordered-vs-unordered-calls

I think this only affects what's in the scope of that declaration, which in this case is only one call. I would think it should either be declared at the top of SetUp or as a class member var.

store_->initializeThreading(main_thread_dispatcher_, tls_);
}

void tearDown() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void TearDown() override { ....

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_));
}

std::vector<uint64_t> h1_cumulative_values, h2_cumulative_values, h1_interval_values,
h2_interval_values;

Expand Down Expand Up @@ -265,8 +278,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) {
EXPECT_CALL(*this, free(_)).Times(5);
}
TEST_F(StatsThreadLocalStoreTest, BasicSingleHistogramMerge) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any test where we actually look at the merged results to see if the percentiles are as expected? I scanned through and didn't see one but I might have missed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added it in one of the tests now.

InSequence s;
store_->initializeThreading(main_thread_dispatcher_, tls_);
setUp();

Histogram& h1 = store_->histogram("h1");
EXPECT_EQ("h1", h1.name());
Expand All @@ -282,21 +294,11 @@ TEST_F(StatsThreadLocalStoreTest, BasicSingleHistogramMerge) {

EXPECT_EQ(1, validateMerge());

// Validate the summary format is as expected.
std::string expected_summary = "P0: 0, P25: 14, P50: 44, P75: 420, P90: 3220, P95: 3260, P99: "
"3292, P99.9: 3299.2, P100: 3300";
EXPECT_EQ(expected_summary, store_->histograms().front()->cumulativeStatistics().summary());

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_));
tearDown();
}

TEST_F(StatsThreadLocalStoreTest, BasicMultiHistogramMerge) {
InSequence s;
store_->initializeThreading(main_thread_dispatcher_, tls_);
setUp();

Histogram& h1 = store_->histogram("h1");
Histogram& h2 = store_->histogram("h2");
Expand All @@ -309,16 +311,11 @@ TEST_F(StatsThreadLocalStoreTest, BasicMultiHistogramMerge) {

EXPECT_EQ(2, validateMerge());

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_));
tearDown();
}

TEST_F(StatsThreadLocalStoreTest, MultiHistogramMultipleMerges) {
InSequence s;
store_->initializeThreading(main_thread_dispatcher_, tls_);
setUp();

Histogram& h1 = store_->histogram("h1");
Histogram& h2 = store_->histogram("h2");
Expand Down Expand Up @@ -347,16 +344,11 @@ TEST_F(StatsThreadLocalStoreTest, MultiHistogramMultipleMerges) {
// cumulativeSummary has right values.
EXPECT_EQ(2, validateMerge());

store_->shutdownThreading();
tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_));
tearDown();
}

TEST_F(StatsThreadLocalStoreTest, BasicScopeHistogramMerge) {
InSequence s;
store_->initializeThreading(main_thread_dispatcher_, tls_);
setUp();

ScopePtr scope1 = store_->createScope("scope1.");

Expand All @@ -369,17 +361,26 @@ TEST_F(StatsThreadLocalStoreTest, BasicScopeHistogramMerge) {
expectCallAndAccumulate(h2, 2);
EXPECT_EQ(2, validateMerge());

store_->shutdownThreading();
tearDown();
}

tls_.shutdownThread();
TEST_F(StatsThreadLocalStoreTest, BasicHistogramValidate) {
setUp();

// Includes overflow stat.
EXPECT_CALL(*this, free(_));
Histogram& h1 = store_->histogram("h1");
expectCallAndAccumulate(h1, 1);

EXPECT_EQ(1, validateMerge());

std::string expected_summary = "P0: 1, P25: 1.025, P50: 1.05, P75: 1.075, P90: 1.09, P95: 1.095, "
"P99: 1.099, P99.9: 1.0999, P100: 1.1";
EXPECT_EQ(expected_summary, store_->histograms().front()->cumulativeStatistics().summary());

tearDown();
}

TEST_F(StatsThreadLocalStoreTest, BasicHistogramUsed) {
InSequence s;
store_->initializeThreading(main_thread_dispatcher_, tls_);
setUp();

ScopePtr scope1 = store_->createScope("scope1.");

Expand Down Expand Up @@ -408,12 +409,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicHistogramUsed) {
EXPECT_TRUE(histogram->used());
}

store_->shutdownThreading();

tls_.shutdownThread();

// Includes overflow stat.
EXPECT_CALL(*this, free(_));
tearDown();
}

TEST_F(StatsThreadLocalStoreTest, ScopeDelete) {
Expand Down