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

ramaraochavali · 2018-04-12T02:05:54Z

Description:
Implements built-in log linear histogram. Fixes the issue #1965

Risk Level: High

Testing:
Unit, Integration and Manual testing.

Docs Changes:
NA

Release Notes:
Will create PR for version update once this is approved.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-04-15T14:40:30Z

include/envoy/stats/stats.h

+  virtual void beginMerge() PURE;
+};
+
+class ThreadLocalHistogramImpl;


I had to forward declare this because with out this, I have to explicitly add merge(histogram_t* target) to ThreadLocalHistogram interface which means we have to expose the libcircl to stats.h interface definition. Otherwise I have to use dynamic_cast in the ParentHistogramImpl merge method. LMK if there are any other better solutions.

I don't think we should be exposing libcircl to stats.h interface. Nor do I think we should be exposing ThreadLocalHistogramImpl. Can't you put the methods you need into the interface and use abstract classes except in the impl itself?

Scanning through stats.h, there are actually a couple of references to 'ThreadLocal' in StoreRoot, but otherwise there shouldn't really be references to a particular implementation from an interface. There's some kind of abstraction leak here, IMO.

ok. Thinking further, I think agree with you that ThreadLocalHistogram should not be in stats.h as it is not used by others in the system. I moved it to thread_local_store.h. Also I removed the addTlsHistogram method in ParentHistogram as that is an implementation detail. Now I think it looks clean. PTAL.

ramaraochavali · 2018-04-15T14:42:18Z

@jmarantz @mattklein123 thanks for your quick reviews. Today I have addressed the review feedback.
The major ones include

Interface Split
JSON format support
Documenting threading model
Also addressed all other review comments. PTAL and let me know any changes are required.

jmarantz

flushing comments...

jmarantz · 2018-04-15T19:57:27Z

include/envoy/stats/stats.h

+  virtual void beginMerge() PURE;
+};
+
+class ThreadLocalHistogramImpl;


I don't think we should be exposing libcircl to stats.h interface. Nor do I think we should be exposing ThreadLocalHistogramImpl. Can't you put the methods you need into the interface and use abstract classes except in the impl itself?

Scanning through stats.h, there are actually a couple of references to 'ThreadLocal' in StoreRoot, but otherwise there shouldn't really be references to a particular implementation from an interface. There's some kind of abstraction leak here, IMO.

jmarantz · 2018-04-15T20:00:02Z

include/envoy/stats/stats.h

+  /**
+   * This is used to keep track of all worker thread local histograms.
+   */
+  virtual void addTlsHistogram(TlsHistogramSharedPtr hist_ptr) PURE;


WDYT about making this a HistogramSharedPtr -- which references only the interface.

jmarantz · 2018-04-15T20:02:59Z

source/common/stats/stats_impl.cc

@@ -273,5 +273,34 @@ void RawStatData::initialize(absl::string_view key) {
  name_[xfer_size] = '\0';
 }

+const std::vector<double> HistogramStatisticsImpl::supported_quantiles_ = {
+    0, 0.25, 0.5, 0.75, 0.90, 0.95, 0.99, 0.999, 1};


See the comments in this PR about how to initialize std::vector, and https://github.com/envoyproxy/envoy/blob/master/STYLE.md

The Google C++ style guide points out that non-PoD static and global variables are forbidden. This includes types such as std::string. We encourage the use of the advice in the C++ FAQ on the static initialization fiasco for how to best handle this.

jmarantz · 2018-04-15T20:06:43Z

source/common/stats/stats_impl.cc

+void HistogramStatisticsImpl::refresh(const histogram_t* new_histogram_ptr) {
+  for (size_t i = 0; i < computed_quantiles_.size(); ++i) {
+    computed_quantiles_[i] = 0;
+  }


this is fine; you could also use std::fill(computed_quantiles_.begin(), computed_quantiles_.end(), 0.0); which is fewer lines. Up to you.

FYI in practice what you had earlier with clear() had two potential problems with it:

it would be legal for std::vector to release the backing store for .data() after the clear(), in which case you might possibly SEGV. But it's also possible std::vector would retain the storage until the next operation. This should also be caught by running CI with a debug STL. See envoy has 33 failing tests when run in an STL debug mode #2556 for why we are not doing this yet.

I don't think clear() would have actually had this behavior of resetting the values to zero. So it's surprising that it would work in a test. But maybe they happened to be zero anyway. Which test was catching this?

I have changed it to use the way you suggested. All the merge tests execute this logic. I think even though clear does not set it to 0, the hist_accumulate function might be updating it with the latest computed value. But I agree it is safe to initialize to zero and also added the ASSERT as you suggested in other comment.

jmarantz · 2018-04-15T20:11:25Z

source/common/stats/stats_impl.cc

+  for (size_t i = 0; i < computed_quantiles_.size(); ++i) {
+    computed_quantiles_[i] = 0;
+  }
+  hist_approx_quantile(new_histogram_ptr, supported_quantiles_.data(), supported_quantiles_.size(),


actually I'd put:
ASSERT(supported_quantiles_.size() == computed_quantiles_.size());
as that is what's being assumed in this line, without it being locally obvious that it must be the case.

jmarantz · 2018-04-15T20:12:27Z

source/common/stats/stats_impl.h

+  HistogramStatisticsImpl(const histogram_t* histogram_ptr);
+
+  std::string summary() const override;
+  const std::vector<double>& supportedQuantiles() const override { return supported_quantiles_; }


probably this method is where you'll want to lazy-initialize supported_quantiles_, and you should probably un-inline it.

jmarantz · 2018-04-15T20:19:57Z

source/server/http/admin.cc

  if (params.size() == 0) {
    // No Arguments so use the standard.
    for (auto stat : all_stats) {
      response.add(fmt::format("{}: {}\n", stat.first, stat.second));
    }
+    for (auto histogram : all_histograms) {


Sorry, I misunderstood your comment. What if interval_value is NaN and cumulative_value is not, or vice versa? Does the algorithm guarantee that can't happen?

jmarantz · 2018-04-16T03:49:38Z

test/common/stats/thread_local_store_test.cc

+
+  EXPECT_EQ(2, validateMerge());
+
+  store_->shutdownThreading();


I was just thinking about calling initializeThreading() in SetUp(), and putting InSequence in the class object (or maybe a histogram-related subclass. I wasn't thinking about initiating the histograms themselves as they are not all common.

But for TearDown, can't you do all these 3 lines? They seem pretty common. I'm not sure about the EXPECT_CALL below...could that be done in TearDown, or in the test body, which would re-order it?

jmarantz · 2018-04-16T03:53:40Z

test/common/stats/thread_local_store_test.cc

+
+  // 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";


I'm not sure I understand the math here. None of the inputs are over 3200; why is P100=3300?

Can you add a simpler example and/or explain in a code comment why the value is that way?

I have added a simple test case now and validated that the quantile outs are as per the test here .

@postwait can help with the maths aspect here?

Signed-off-by: Rama <rama.rao@salesforce.com>

jmarantz · 2018-04-16T13:23:06Z

test/common/stats/thread_local_store_test.cc

@@ -80,6 +80,19 @@ class StatsThreadLocalStoreTest : public testing::Test, public RawStatDataAlloca
    store_->addSink(sink_);
  }

+  void setUp() {


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.

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

jmarantz · 2018-04-16T13:24:47Z

test/common/stats/thread_local_store_test.cc

+    store_->initializeThreading(main_thread_dispatcher_, tls_);
+  }
+
+  void tearDown() {


void TearDown() override { ....

jmarantz · 2018-04-16T13:27:57Z

test/common/thread_local/thread_local_impl_test.cc

+                            *all_threads_complete = true;
+                          });
+
+  EXPECT_TRUE(*all_threads_complete);


this looks racy. runOnAllThreads returns instantly, calling the 'done' callback when it is done. The pattern I expect here is for you to establish a condition variable in your test, which you wait on here, and signal that condvar on the done callback.

You can also spin-loop on all_threads_complete since it is an atomic, but IMO it's better to use a condvar and make all_threads_complete a simple bool (not atomic, not shared).

I have changed the test to use condvar.

jmarantz · 2018-04-16T13:30:06Z

test/common/thread_local/thread_local_impl_test.cc

+  // Ensure that the thread local call back and all_thread_complete call back are called.
+  std::shared_ptr<std::atomic<bool>> all_threads_complete =
+      std::make_shared<std::atomic<bool>>(false);
+  std::shared_ptr<std::atomic<uint64_t>> thread_local_calls =


I don't think this needs to be shared; it's owned by the test method.

I think earlier we ran in to issue if it is not shared with TSAN build, because multiple thread update it may be. That is why we changed it to shared.

I think that's because before you put the condvar in, the test method could exit while threads were active. The condvar solves that properly, so you shouldn't need to share it anymore.

Signed-off-by: Rama <rama.rao@salesforce.com>

mattklein123

@ramaraochavali thank you for your patience here. I think we are very close. This is looking awesome. I dropped in a few more comments. I still haven't looked through tests. @jmarantz LMK when the tests pass muster for you and I will skim though them.

@mrice32 can you also look at this point?

mattklein123 · 2018-04-16T23:33:04Z

source/common/stats/stats_impl.h

+  void refresh(const histogram_t* new_histogram_ptr);
+
+private:
+  static const std::vector<double> supported_quantiles_;


Is this used anymore? I don't think so?

mattklein123 · 2018-04-16T23:34:06Z

source/common/stats/stats_impl.h

@@ -446,6 +482,7 @@ class IsolatedStoreImpl : public Store {
  // Stats::Store
  std::list<CounterSharedPtr> counters() const override { return counters_.toList(); }
  std::list<GaugeSharedPtr> gauges() const override { return gauges_.toList(); }
+  std::list<ParentHistogramSharedPtr> histograms() const override { return parent_histograms_; }


nit: You don't need the member variable, just returning {} should work.

mattklein123 · 2018-04-16T23:34:43Z

source/common/stats/thread_local_store.cc

+  std::unordered_set<std::string> names;
+  std::unique_lock<std::mutex> lock(lock_);
+  // TODO(ramaraochavali): As histograms don't share storage, there is a chance of duplicate names
+  // here. We need to process global storage for histograms similar to how we have a central storage


"We need process global storage..."

mattklein123 · 2018-04-16T23:36:43Z

source/common/stats/thread_local_store.cc

+      if (names.insert(hist_name).second) {
+        ret.push_back(parent_hist);
+      } else {
+        ENVOY_LOG(debug, "duplicate histogram {}.{}", scope->prefix_, hist_name);


Given that this will potentially have an effect on output, should this one actually be a warn? I could go either way on this. It's conceivable this would happen in the data path, though really it should only happen at config load time, so I'm not that worried about log spam.

You might also make the log message something like "duplicate histogram {}.{}: data loss will occur on output".

Can you also open a tracking issue for this? I have ideas on how to fix, but given that this won't effect too many people I don't think it's super urgent (ironically it will for sure effect us at Lyft since we share stats across listeners in certain cases).

Changed to warn and added this issue #3098

mattklein123 · 2018-04-16T23:40:23Z

source/common/stats/thread_local_store.cc

+    for (const TlsHistogramSharedPtr& tls_histogram : tls_histograms_) {
+      tls_histogram->merge(interval_histogram_);
+    }
+    histogram_t* hist_array[1];


I think my comment was lost, but I'm sure you can do lock.unlock() before this line. The lock only needs to protect tls_histograms_. I would add a comment along these lines.

mattklein123 · 2018-04-16T23:44:29Z

source/common/stats/thread_local_store.h

+ *    current_active index via which it writes to the correct histogram.
+ *  - When all workers have done, the main thread continues with the flush process where the
+ *    "actual" merging happens.
+ *  - As the active histograms are swapped in TLS histograms, On the main thread, we can be sure


nit: s/On/on

mattklein123 · 2018-04-16T23:44:48Z

source/common/stats/thread_local_store.h

@@ -62,8 +166,11 @@ class ThreadLocalStoreImpl : public StoreRoot {
  };

  // Stats::Store
+  // TODO(ramaraochavali): Consider changing the implementation of these methods to use vectors and
+  // use std::sort, rather than inserting into a map and pulling it out for better performance


nit: period end of sentence.

mattklein123 · 2018-04-16T23:46:12Z

source/server/http/admin.cc

  for (const Stats::CounterSharedPtr& counter : server_.stats().counters()) {
    all_stats.emplace(counter->name(), counter->value());
  }

  for (const Stats::GaugeSharedPtr& gauge : server_.stats().gauges()) {
    all_stats.emplace(gauge->name(), gauge->value());
  }
+  const std::list<Stats::ParentHistogramSharedPtr>& histograms = server_.stats().histograms();


technically you are storing a reference to a temporary here. Just copy. The compiler will use RVO.

mattklein123 · 2018-04-16T23:47:32Z

source/server/http/admin.cc

  if (params.size() == 0) {
    // No Arguments so use the standard.
    for (auto stat : all_stats) {
      response.add(fmt::format("{}: {}\n", stat.first, stat.second));
    }
+    for (auto histogram : all_histograms) {


@ramaraochavali my concern about the text output version is that it is not good for 1-line grep, which I think is a common use case (at least it is for me). It might be ugly, but can we figure out a format for the text version which has everything on a single line so we can grep for the stat?

mattklein123 · 2018-04-16T23:48:39Z

source/server/http/admin.cc

+
+    rapidjson::Value quantile_array(rapidjson::kArrayType);
+
+    for (size_t i = 0; i < histogram->intervalStatistics().supportedQuantiles().size(); ++i) {


Yeah +1 I think we can optimize. Right now since we don't have locked protos (or any protos) for the JSON output, I think we can change this stuff at any time. I would put in a TODO to think about optimizing when we switch over to protos for all admin JSON output.

jmarantz · 2018-04-17T03:00:04Z

test/common/thread_local/thread_local_impl_test.cc

                            EXPECT_EQ(*thread_local_calls, 1);
-                            *all_threads_complete = true;
+                            all_threads_complete_ = true;


you need to hold the mutex while modifying all_threads_complete_ and notifying the condvar. I'd do that using a std::unique-lock scoped around the those two lines.

done. please check once

jmarantz · 2018-04-17T03:01:30Z

test/common/thread_local/thread_local_impl_test.cc

                          });

-  EXPECT_TRUE(*all_threads_complete);
+  std::unique_lock<std::mutex> lock(cv_mutex_);


just hold this over the cv_.wait call and not during the shutdown calls.

done. Must be sleeping when i coded this

jmarantz · 2018-04-17T03:03:20Z

test/common/thread_local/thread_local_impl_test.cc

@@ -47,6 +47,9 @@ class ThreadLocalInstanceImplTest : public testing::Test {
  InstanceImpl tls_;
  Event::MockDispatcher main_dispatcher_;
  Event::MockDispatcher thread_dispatcher_;
+  std::condition_variable cv_;


Can you spell out "condvar_" in the variable rather this abbreviation?

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

More importantly, this is only used in one test, so can you move these variables into the test? You could put them in a local struct there if you like, so that it's easier to capture them all at once for the nested function.

moved them to a local struct and renamed the variables. Can you PTAL at this specific test case and see if that is what you are looking for?

jmarantz · 2018-04-17T03:07:15Z

test/common/thread_local/thread_local_impl_test.cc

+  // Ensure that the thread local call back and all_thread_complete call back are called.
+  std::shared_ptr<std::atomic<bool>> all_threads_complete =
+      std::make_shared<std::atomic<bool>>(false);
+  std::shared_ptr<std::atomic<uint64_t>> thread_local_calls =


I think that's because before you put the condvar in, the test method could exit while threads were active. The condvar solves that properly, so you shouldn't need to share it anymore.

jmarantz · 2018-04-17T03:11:31Z

test/common/stats/thread_local_store_test.cc

+
+  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, "


this is a weird artifact of the algorithm extrapolating off a degenerate case. Can I ask you to add another case where you add an even distribution of all integer values from 1 to 100 inclusive? I'm curious what you'll get. Maybe

PO: not sure....
P25: 25
P50: 50
P75: 75
P90: 90
P95: 95
P99: 99
P100: 100

or something close to it?

I have added that test case and this is what we get
P0: 0, P25: 25, P50: 50, P75: 75, P90: 90, P95: 95, P99: 99, P99.9: 99.9, P100: 100

jmarantz · 2018-04-17T03:13:54Z

test/common/stats/thread_local_store_test.cc

+    store_->addSink(sink_);
+  }
+
+  void SetUp() override {


I suspect you want the InSequence object to be declared as a the test class member var, rather than a local member, I confess I don't know what that does. I'm suggesting that because maybe it enforces some deterministic threading model, but I'm not sure.

I am also not sure on this. So just leaving it as is.

jmarantz · 2018-04-17T03:14:56Z

test/common/stats/thread_local_store_test.cc

+class HistogramTest : public testing::Test, public RawStatDataAllocator {
+public:
+  HistogramTest() {
+    ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](const std::string& name) -> RawStatData* {


Remove the ctor and move these lines to SetUp().

jmarantz · 2018-04-17T04:13:07Z

source/common/stats/stats_impl.cc

+}
+
+const std::vector<double>& HistogramStatisticsImpl::supportedQuantiles() const {
+  static const std::vector<double> supported_quantiles_ = {0,    0.25, 0.5,   0.75, 0.90,


local var should not have _ suffix.

jmarantz · 2018-04-17T04:25:30Z

source/common/stats/thread_local_store.cc

+    }
+    histogram_t* hist_array[1];
+    hist_array[0] = interval_histogram_;
+    hist_accumulate(cumulative_histogram_, hist_array, ARRAY_SIZE(hist_array));


You can just write

hist_accumulate(cumulative_histogram_, &interval_histogram_, 1);

and skip setting up the temp array

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-04-17T08:38:31Z

@mattklein123 addressed all the comments. PTAL.

Signed-off-by: Rama <rama.rao@salesforce.com>

jmarantz · 2018-04-17T14:09:35Z

test/common/thread_local/thread_local_impl_test.cc

+    bool all_threads_complete_;
+  };
+
+  thread_local_verification_info thread_local_data;


If you are going to define a type it should be called ThreadLocalVerificationInfo, but you don't need to define a type. Just do:

struct { std::atomic<uint64_t> thread_local_calls_{0}; std::condition_variable condvar_; std::mutex condvar_mutex_; bool all_threads_complete_; } thread_local_data;

jmarantz · 2018-04-17T14:11:18Z

test/common/thread_local/thread_local_impl_test.cc

+        thread_local_data.all_threads_complete_ = true;
+        thread_local_data.condvar_.notify_one();
+      });
+  std::unique_lock<std::mutex> lock(thread_local_data.condvar_mutex_);


put the mutex lock and the wait into a scope, so the lock is released before you call shutdownGlobalThreading. This sounds familiar, right? I am never sure if all my github comments are getting through.

In general hold locks for the minimum amount of time.

i did not get this specific comment. Will change now.

The comment was "just hold this over the cv_.wait call and not during the shutdown calls." and your response was "done. Must be sleeping when i coded this".

If you could iterate through the PR and look at all the comments exposed by "show outdated" links it would reduce the number of comments that wind up getting repeated, and speed up iteration.

jmarantz · 2018-04-17T14:13:14Z

test/common/thread_local/thread_local_impl_test.cc

+  std::unique_lock<std::mutex> lock(thread_local_data.condvar_mutex_);
+  thread_local_data.condvar_.wait(
+      lock, [&thread_local_data] { return thread_local_data.all_threads_complete_; });
+


You had EXPECT_TRUE(thread_local_data.all_threads_complete) here, right? Where did that go?

good catch. Some how got deleted looks like. Will add it back

jmarantz · 2018-04-17T14:16:32Z

test/common/stats/thread_local_store_test.cc

+  std::string h1_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";
+  std::string h2_expected_summary =


make these locals const. or just move the literals into their single use.

BTW I'm not really sure why you need the degenerate case; seems like you are testing behavior which is really at the boundary of sanity, and probably not that important. But it's nice to see that the even-distribution case is working.

i want to verify that case as well. I will leave it like that. I will make those variable local const.

jmarantz · 2018-04-17T14:21:34Z

test/common/stats/thread_local_store_test.cc

+  expectCallAndAccumulate(h2, 1);
+  expectCallAndAccumulate(h2, 2);
+
+  EXPECT_EQ(2, validateMerge());


You are not really validating that the merge did something sane because you are not showing the summary. WDYT about adding some data to both histograms so that, when merged, you get a result you expect?

Maybe have one histogram with [1, 1, 0, 0, 0, 0, 0, 0, 1, 1]; and see what the summary for that is. Make another histogram with [0, 0, 1, 1, 1, 1, 1, 1, 0, 0]. and see what the summary for that is. EXPECT_EQ on both of those. Then merge them, and EXPECT_EQ on the summary for that. Does that make sense?

I think the merge logic on our side is validated by the MultiMerge test. Are you trying to validate the libcircl hist_accumulate function? I tested that earlier IIRC and they were not same probably because of the insertion order. They will be close but little different. I will create a test case and send for @postwait's review.

There's no test called 'MultiMerge' but there are a number of tests that do merges, which check that the number of merged histograms is as expected. However, there's none I can see that looked at the values in the merged histogram. I scanned your message to @postwait and it might be reasonable for there to be some floating point sloppiness, in which case you could do inexact data matching in the test.

But it seems wrong to have no functional testing at all of the merged data.

I'm also fine with changing the data from what I suggested above to something that's easier to reason about in the context of the math behind circlehist.

@jmarantz Now I added the test BasicHistogramMergeSummary that validates two set of values 1-50 and 51-100 and validates the merged summary is equal to [1-100].

jmarantz · 2018-04-17T14:23:26Z

test/common/stats/thread_local_store_test.cc

+   * that can be asserted later.
+   */
+  uint64_t validateMerge() {
+


remove blank line here.

jmarantz · 2018-04-17T14:23:33Z

test/common/stats/thread_local_store_test.cc

+      h2_interval_values;
+
+  /**
+   * Validates taht Histogram merge happens as desired and returns the processed histogram count


s/taht/that/

jmarantz · 2018-04-17T14:24:32Z

source/server/http/admin.cc

+
+    rapidjson::Value quantile_array(rapidjson::kArrayType);
+
+    for (size_t i = 0; i < histogram->intervalStatistics().supportedQuantiles().size(); ++i) {


right a TODO should be here -- actually a warning not to use the data until another round of optimization.

jmarantz · 2018-04-17T14:26:13Z

source/server/http/admin.cc

  if (params.size() == 0) {
    // No Arguments so use the standard.
    for (auto stat : all_stats) {
      response.add(fmt::format("{}: {}\n", stat.first, stat.second));
    }
+    for (auto histogram : all_histograms) {


this format LGTM; easy to parse with a shell script if needed.

jmarantz · 2018-04-17T14:30:30Z

test/common/stats/thread_local_store_test.cc

+    EXPECT_CALL(*this, alloc("stats.overflow"));
+    store_.reset(new ThreadLocalStoreImpl(*this));
+    store_->addSink(sink_);
+    InSequence s;


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.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-04-17T15:53:30Z

@jmarantz reg the json compact issue, I moved the TODO to the right place and created a envoy issue. Not sure if we want to warn not to use that till next optimization. @mattklein123 WDYT?

ramaraochavali · 2018-04-17T15:57:49Z

I did not realize that you wanted me to "scope" it.

…

On Tue, Apr 17, 2018 at 9:05 PM, Joshua Marantz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/common/thread_local/thread_local_impl_test.cc <#3059 (comment)>: > + std::condition_variable condvar_; + std::mutex condvar_mutex_; + bool all_threads_complete_; + }; + + thread_local_verification_info thread_local_data; + + tlsptr->runOnAllThreads( + [&thread_local_data]() -> void { ++thread_local_data.thread_local_calls_; }, + [&thread_local_data]() -> void { + EXPECT_EQ(thread_local_data.thread_local_calls_, 1); + std::unique_lock<std::mutex> lock(thread_local_data.condvar_mutex_); + thread_local_data.all_threads_complete_ = true; + thread_local_data.condvar_.notify_one(); + }); + std::unique_lock<std::mutex> lock(thread_local_data.condvar_mutex_); The comment was "just hold this over the cv_.wait call and not during the shutdown calls." and your response was "done. Must be sleeping when i coded this". If you could iterate through the PR and look at all the comments exposed by "show outdated" links it would reduce the number of comments that wind up getting repeated, ands speed up iteration. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3059 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQaGqHOljcLO_1-I4hsihFmAFC8kZf7Uks5tpgu_gaJpZM4TRBTs> .

jmarantz

Thanks for sticking through this. Mostly nits on my part now, except for the lack of a functional test for the merged histogram data.

I still want to look at the threading model -- I haven't dived into that yet, and I want to mainly to learn myself more about Envoy threading. I expect @mattklein123 will scrutinize that.

I raised an issue with github on why this PR unicorns...it's not that big. But I would suggest, if we need to clone it again, to squash the commits into one. We'd be losing the comments anyway, I think. But maybe the high commit-count is what blows up GH.

jmarantz · 2018-04-17T16:33:29Z

test/common/stats/thread_local_store_test.cc

+    HistogramStatisticsImpl h2_interval_statistics(hist2_interval);
+
+    for (const Stats::ParentHistogramSharedPtr& histogram : histogram_list) {
+      if (histogram->name() == "h1") {


OK this pattern of looping over the histograms is a bit awkward and I think it might be worth having a test-class helper-method:

typedef std::map<std::string, Stats::ParentHistogramSharedPtr> NameHistogramMap NameHistogramMap makeHistogramMap(list) { ... };

with usage in tests

const Stats::ParentHistogramSharedPtr& h1 = histogram_map["h1"]; EXPECT_EQ(h1->cumulativeStatistics().summary(), h1_cumulative_statistics.summary()); ...

rather than looping and if/elseing the cases. If it were just one of these then it might not be worth it, but there are several, here & below.

jmarantz · 2018-04-17T16:33:41Z

test/common/stats/thread_local_store_test.cc

+    EXPECT_CALL(sink_, onHistogramComplete(Ref(histogram), record_value));
+    histogram.recordValue(record_value);
+
+    if (histogram.name().find("h1") != std::string::npos) {


jmarantz · 2018-04-17T16:45:02Z

test/common/thread_local/thread_local_impl_test.cc

@@ -82,6 +82,43 @@ TEST_F(ThreadLocalInstanceImplTest, All) {
  tls_.shutdownThread();
 }

+// Validate ThreadLocal::runOnAllThreads behavior with all_thread_complete call back.
+TEST_F(ThreadLocalInstanceImplTest, RunOnAllThreads) {
+


rm blank line

jmarantz · 2018-04-17T16:45:30Z

test/extensions/stats_sinks/metrics_service/metrics_service_integration_test.cc

 TEST_P(MetricsServiceIntegrationTest, BasicFlow) {
  initialize();
+  // Send an empty request so that histogram values merged for cluster_0.
+  codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));


extra parens around lookupPort

jmarantz · 2018-04-17T16:53:46Z

test/common/stats/thread_local_store_test.cc

+  expectCallAndAccumulate(h2, 1);
+  expectCallAndAccumulate(h2, 2);
+
+  EXPECT_EQ(2, validateMerge());


There's no test called 'MultiMerge' but there are a number of tests that do merges, which check that the number of merged histograms is as expected. However, there's none I can see that looked at the values in the merged histogram. I scanned your message to @postwait and it might be reasonable for there to be some floating point sloppiness, in which case you could do inexact data matching in the test.

But it seems wrong to have no functional testing at all of the merged data.

I'm also fine with changing the data from what I suggested above to something that's easier to reason about in the context of the math behind circlehist.

mrice32

Sorry about the delay. Just starting to look through this - here are a few surface-level comments. Really cool stuff! Awesome work!

mrice32 · 2018-04-17T01:26:34Z

source/common/stats/stats_impl.cc

+}
+
+const std::vector<double>& HistogramStatisticsImpl::supportedQuantiles() const {
+  static const std::vector<double> supported_quantiles_ = {0,    0.25, 0.5,   0.75, 0.90,


nit: supported_quantiles_ -> supported_quantiles

mrice32 · 2018-04-17T15:11:25Z

source/common/stats/thread_local_store.cc

+  // here. We need to process global storage for histograms similar to how we have a central storage
+  // in shared memory for counters/gauges.
+  for (ScopeImpl* scope : scopes_) {
+    for (auto name_histogram_pair : scope->central_cache_.histograms_) {


nit: why not auto &?

mrice32 · 2018-04-17T15:24:20Z

source/common/stats/thread_local_store.cc

+    tls_->runOnAllThreads(
+        [this]() -> void {
+          for (ScopeImpl* scope : scopes_) {
+            for (auto name_histogram_pair :


nit: why not auto &?

mrice32 · 2018-04-17T15:25:26Z

source/common/stats/thread_local_store.h

+  void recordValue(uint64_t value) override;
+
+  bool used() const override { return flags_ & Flags::Used; }
+  void beginMerge() override { current_active_ = 1 - current_active_; }


jmarantz · 2018-04-17T18:32:38Z

source/common/stats/thread_local_store.cc

+}
+
+void ThreadLocalHistogramImpl::merge(histogram_t* target) {
+  hist_accumulate(target, &histograms_[1 - current_active_], 1);


make an inline class method "otherHistogramIndex()" for 1 - currentActive_, and also save it in a temp and use it in both places in this function.

jmarantz · 2018-04-17T18:33:22Z

source/common/stats/thread_local_store.h

+  bool used() const override { return flags_ & Flags::Used; }
+  void beginMerge() override {
+    // this switches the current_active_ between 1 and 0.
+    current_active_ = 1 - current_active_;


factor out '1 - current_active_' here too.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-04-18T07:20:33Z

Closing this and will create another PR as this is taking time to load getting unicorn for most of the folks.

ramaraochavali added 30 commits March 28, 2018 20:41

per thread collection implementation

f0a7faa

Signed-off-by: Rama <rama.rao@salesforce.com>

implemented merge logic

1a35b0e

Signed-off-by: Rama <rama.rao@salesforce.com>

fixed test cases

5b66b74

Signed-off-by: Rama <rama.rao@salesforce.com>

formatted

8052ae7

Signed-off-by: Rama <rama.rao@salesforce.com>

added support for flushHistograms and implemented for metrics sink

5d30b85

Signed-off-by: Rama <rama.rao@salesforce.com>

added runOnAllThreadsWithBarrier to TLS system

a0bf03c

Signed-off-by: Rama <rama.rao@salesforce.com>

addressed review comments

dede1fd

Signed-off-by: Rama <rama.rao@salesforce.com>

handle hot restart case

e744253

Signed-off-by: Rama <rama.rao@salesforce.com>

reacquire lock in mergeInternal

0aeac15

Signed-off-by: Rama <rama.rao@salesforce.com>

new lock for merge process

c200974

Signed-off-by: Rama <rama.rao@salesforce.com>

remove handle main thread

dd79e35

Signed-off-by: Rama <rama.rao@salesforce.com>

move worker_count to class level variable

9dcddf6

Signed-off-by: Rama <rama.rao@salesforce.com>

remove separate merge lock

5e8169a

Signed-off-by: Rama <rama.rao@salesforce.com>

use separate merge lock

f35083c

Signed-off-by: Rama <rama.rao@salesforce.com>

renamed few methods, added used support for histograms

08cc211

Signed-off-by: Rama <rama.rao@salesforce.com>

removed exterc C usage and moved worker_count to shared ptr

a107b0f

Signed-off-by: Rama <rama.rao@salesforce.com>

added HistogramStatistics interface

dadba03

Signed-off-by: Rama <rama.rao@salesforce.com>

moved histogram impl to threadlocal store along with merge_lock

e41d7ef

Signed-off-by: Rama <rama.rao@salesforce.com>

fixed admin tsan violation

9da426e

Signed-off-by: Rama <rama.rao@salesforce.com>

formatted

92620e5

Signed-off-by: Rama <rama.rao@salesforce.com>

compilation failure

6e6253a

Signed-off-by: Rama <rama.rao@salesforce.com>

calculate stats only once

6ca87c4

Signed-off-by: Rama <rama.rao@salesforce.com>

initialize stats

fa8f646

Signed-off-by: Rama <rama.rao@salesforce.com>

corrected ref returns for other histograms

2b5772f

Signed-off-by: Rama <rama.rao@salesforce.com>

merged master

1032ffc

Signed-off-by: Rama <rama.rao@salesforce.com>

fixed compilation issues

8266868

Signed-off-by: Rama <rama.rao@salesforce.com>

addressed review comments

a041a3e

Signed-off-by: Rama <rama.rao@salesforce.com>

added callback for mergeHistogram

c9fef4b

Signed-off-by: Rama <rama.rao@salesforce.com>

addressed review comments and added basic test

29646d0

Signed-off-by: Rama <rama.rao@salesforce.com>

added some tests

df348b2

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali commented Apr 15, 2018

View reviewed changes

jmarantz reviewed Apr 15, 2018

View reviewed changes

jmarantz reviewed Apr 16, 2018

View reviewed changes

ramaraochavali added 4 commits April 16, 2018 09:38

moved static initialization of supportedQuantiles to first access

97daa92

Signed-off-by: Rama <rama.rao@salesforce.com>

formatted

edf4a52

Signed-off-by: Rama <rama.rao@salesforce.com>

added setUp and tearDown methods

b204bff

Signed-off-by: Rama <rama.rao@salesforce.com>

moved ThreadLocalHistogram interface to thread_local_store

e6669b2

Signed-off-by: Rama <rama.rao@salesforce.com>

jmarantz reviewed Apr 16, 2018

View reviewed changes

fixed test comments

64d8c2a

Signed-off-by: Rama <rama.rao@salesforce.com>

mattklein123 reviewed Apr 16, 2018

View reviewed changes

jmarantz reviewed Apr 17, 2018

View reviewed changes

ramaraochavali added 2 commits April 17, 2018 10:26

fixed test cases

86f447d

Signed-off-by: Rama <rama.rao@salesforce.com>

address review comments

af7aacd

Signed-off-by: Rama <rama.rao@salesforce.com>

remove unnecessary array during tls histogram merge

e9547f6

Signed-off-by: Rama <rama.rao@salesforce.com>

jmarantz reviewed Apr 17, 2018

View reviewed changes

ramaraochavali added 2 commits April 17, 2018 21:14

addressed some more test comments

60428d3

Signed-off-by: Rama <rama.rao@salesforce.com>

removed struct type def

9ee5b0a

Signed-off-by: Rama <rama.rao@salesforce.com>

jmarantz reviewed Apr 17, 2018

View reviewed changes

mrice32 reviewed Apr 17, 2018

View reviewed changes

jmarantz reviewed Apr 17, 2018

View reviewed changes

address review comments

b46f01c

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali closed this Apr 18, 2018

ramaraochavali mentioned this pull request Apr 18, 2018

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

Closed

jmarantz mentioned this pull request Apr 18, 2018

PRs with lots of commits will not load in github, showing unicorn instead. #3125

Closed


		rapidjson::Value quantile_array(rapidjson::kArrayType);

		for (size_t i = 0; i < histogram->intervalStatistics().supportedQuantiles().size(); ++i) {


		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, "

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

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

Conversation

ramaraochavali commented Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

ramaraochavali commented Apr 15, 2018

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Apr 12, 2018 •

edited

Loading

ramaraochavali Apr 16, 2018 •

edited

Loading

ramaraochavali Apr 16, 2018 •

edited

Loading

ramaraochavali Apr 17, 2018 •

edited

Loading

jmarantz Apr 17, 2018 •

edited

Loading

ramaraochavali Apr 17, 2018 •

edited

Loading

jmarantz Apr 17, 2018 •

edited

Loading