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.

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

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

Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
43 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
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
17 changes: 17 additions & 0 deletions bazel/external/libcircllhist.BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
cc_library(
name = "libcircllhist",
srcs = ["src/circllhist.c"],
hdrs = [
"src/circllhist.h",
"src/circllhist_config.h", # Generated.
],
includes = ["src"],
visibility = ["//visibility:public"],
)

genrule(
name = "circllhist_config",
srcs = ["src/circllhist_config.h.in"],
Copy link
Member

Choose a reason for hiding this comment

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

Can we follow up on @jmillikin-stripe comment here? It does seem weird that the generated header is just being copied. Can you add a comment as to why this is OK if it is? cc @postwait

Copy link

Choose a reason for hiding this comment

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

The _config.h.in is an autoconf input template. It is not safe to copy it on all platforms. There is a

#ifdef HAVE_ALLOCA_H

in the circllhist.c file that needs that that conditionally set depending on system headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@postwait thanks. What is your suggestion here? Should I copy based on platform? or which platforms it is good to copy?

Copy link

Choose a reason for hiding this comment

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

libcircllhist (like many open source projects) is auto-tools based. When you build it, it will build-time detect which features are available on your platform and act on that. I don't know how to do the same thing in cmake. I am pretty sure that HAVE_ALLOCA_H is the only important thing in that header though -- so if you can get cmake to provide that, you're set.

Copy link

Choose a reason for hiding this comment

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

Looking at this, it is only alloca() that is a problem. I've opened a PR on libcircllhist to resolve this. It should be possible shortly to just ignore that header completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@postwait thank you so much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@postwait Awesome. Thanks. I got the new version and removed the genRule and it works.

outs = ["src/circllhist_config.h"],
cmd = "cp $< $@",
)
11 changes: 11 additions & 0 deletions bazel/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
_boringssl()
_com_google_absl()
_com_github_bombela_backward()
_com_github_circonus_labs_libcircllhist()
_com_github_cyan4973_xxhash()
_com_github_eile_tclap()
_com_github_fmtlib_fmt()
Expand Down Expand Up @@ -215,6 +216,16 @@ def _com_github_bombela_backward():
actual = "@com_github_bombela_backward//:backward",
)

def _com_github_circonus_labs_libcircllhist():
_repository_impl(
name = "com_github_circonus_labs_libcircllhist",
build_file = "@envoy//bazel/external:libcircllhist.BUILD",
)
native.bind(
name = "libcircllhist",
actual = "@com_github_circonus_labs_libcircllhist//:libcircllhist",
)

def _com_github_cyan4973_xxhash():
_repository_impl(
name = "com_github_cyan4973_xxhash",
Expand Down
4 changes: 4 additions & 0 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ REPOSITORY_LOCATIONS = dict(
commit = "44ae9609e860e3428cd057f7052e505b4819eb84", # 2018-02-06
remote = "https://github.com/bombela/backward-cpp",
),
com_github_circonus_labs_libcircllhist = dict(
commit = "bb73e93ba23b5746ea15d95c21ee8536168c486c", # 2018-03-18
remote = "https://github.com/circonus-labs/libcircllhist",
),
com_github_cyan4973_xxhash = dict(
commit = "7caf8bd76440c75dfe1070d3acfbd7891aea8fca", # v0.6.4
remote = "https://github.com/Cyan4973/xxHash",
Expand Down
7 changes: 7 additions & 0 deletions include/envoy/stats/stats.h
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ class Store : public Scope {
* @return a list of all known gauges.
*/
virtual std::list<GaugeSharedPtr> gauges() const PURE;

virtual std::list<HistogramSharedPtr> histograms() const {
Copy link
Member

Choose a reason for hiding this comment

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

same, PURE, every store should implement.

std::list<HistogramSharedPtr> empty_list;
return empty_list;
}
};

typedef std::unique_ptr<Store> StorePtr;
Expand Down Expand Up @@ -294,6 +299,8 @@ class StoreRoot : public Store {
* down.
*/
virtual void shutdownThreading() PURE;

virtual void mergeHistograms() {}
Copy link
Member

Choose a reason for hiding this comment

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

same: please also add doc comments for all interface functions (above also).

};

typedef std::unique_ptr<StoreRoot> StoreRootPtr;
Expand Down
3 changes: 3 additions & 0 deletions source/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ envoy_cc_library(
name = "stats_lib",
srcs = ["stats_impl.cc"],
hdrs = ["stats_impl.h"],
external_deps = [
"libcircllhist",
],
deps = [
"//include/envoy/common:time_interface",
"//include/envoy/server:options_interface",
Expand Down
126 changes: 126 additions & 0 deletions source/common/stats/stats_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@

#include "absl/strings/string_view.h"

extern "C" {
#include <circllhist.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

#include "circlehist.h"

<> should be used for system includes (e.g. included with the OS or compiler). Also I would've thought this should be in a subdir.

}

namespace Envoy {
namespace Stats {

Expand Down Expand Up @@ -369,6 +373,128 @@ class HistogramImpl : public Histogram, public MetricImpl {
Store& parent_;
};

/**
* Structure to hold statistical data of histogram.
*/
struct HistogramStatistics {
public:
HistogramStatistics(histogram_t* histogram_ptr) {
double quantile_in[] = {0, 0.25, 0.5, 0.75, 0.90, 0.95, 0.99, 0.999, 1};
Copy link
Contributor

Choose a reason for hiding this comment

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

factor out this constant so it only appears once. I see it also in grpc_metrics_service_impl.h:149

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 am not able to make it const - because of my c++ knowledge levels :-). But made it class variable and used it every where.

hist_approx_quantile(histogram_ptr, quantile_in, ARRAY_SIZE(quantile_in), quantile_out_);
}

std::string summary() const {
return fmt::format("P0: {} , P25: {}, P50: {}, P75: {}, P90: {}, P95: {}, P99: {}, P100: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT of constructing this by looping over the quantiles_in_ and quantiles_out_ vectors, so they'll never get out of sink. In fact it looks like they are out of sink now as you have a .999 in the quantiles_in_ initializer above; only 8 values in your format string and 9 args below.

Easy to miss. Let's avoid that by constructing this from the source of truth.

Copy link
Member

Choose a reason for hiding this comment

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

+1

quantile_out_[0], quantile_out_[1], quantile_out_[2], quantile_out_[3],
quantile_out_[4], quantile_out_[5], quantile_out_[6], quantile_out_[7],
quantile_out_[8]);
}

private:
double quantile_out_[9];
Copy link
Contributor

Choose a reason for hiding this comment

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

double quantile_out_[ARRAY_SIZE(quantile_in)];

};

/**
* Log Linear Histogram implementation per thread.
Copy link
Member

Choose a reason for hiding this comment

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

Can we move ThreadLocalHistogramImpl and HistogramParentImpl directly into thread_local_store. We are going to have to deal with some locking issues and I would like things to be more explicitly coupled. We can break it apart later if necessary.

*/
class ThreadLocalHistogramImpl : public Histogram, public MetricImpl {
public:
ThreadLocalHistogramImpl(const std::string& name, Store& parent, std::string&& tag_extracted_name,
std::vector<Tag>&& tags)
: MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), parent_(parent) {
histograms_[0] = hist_alloc();
histograms_[1] = hist_alloc();
current_active_ = 0;
Copy link
Member

Choose a reason for hiding this comment

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

initialize this and next line in initializer list

}

~ThreadLocalHistogramImpl() {
hist_free(histograms_[0]);
hist_free(histograms_[1]);
}
// Stats::Histogram
void recordValue(uint64_t value) override {
hist_insert_intscale(histograms_[current_active_], value, 0, 1);
parent_.deliverHistogramToSinks(*this, value);
}

void beginMerge() {
if (current_active_ == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: current_active_ = 1 - current_active_;

current_active_ = 1;
} else {
current_active_ = 0;
}
}

void merge(histogram_t* target) {
histogram_t* hist_array[1];
if (current_active_ == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: hist_array[0] = histograms_[1 - current_active_];

hist_array[0] = histograms_[1];
} else {
hist_array[0] = histograms_[0];
}
hist_accumulate(target, hist_array, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

s/1/ARRAY_SIZE(hist_array)/

hist_clear(hist_array[0]);
}

Store& parent_;
int current_active_;
histogram_t* histograms_[2];
};

typedef std::shared_ptr<ThreadLocalHistogramImpl> TlsHistogramSharedPtr;

/**
* Log Linear Histogram implementation that is stored in the main thread.
*/
class HistogramParentImpl : public Histogram, public MetricImpl {
public:
HistogramParentImpl(const std::string& name, Store& parent, std::string&& tag_extracted_name,
std::vector<Tag>&& tags)
: MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), parent_(parent) {
interval_histogram_ = hist_alloc();
cumulative_histogram_ = hist_alloc();
}

~HistogramParentImpl() {
hist_free(interval_histogram_);
hist_free(cumulative_histogram_);
}

// Stats::Histogram
void recordValue(uint64_t) override {}

void addTlsHistogram(HistogramSharedPtr hist_ptr) { tls_histograms_.emplace_back(hist_ptr); }

void merge() {
hist_clear(interval_histogram_);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you comment about the threading model here? or maybe just point to Matt's design in github?

for (auto tls_histogram : tls_histograms_) {
TlsHistogramSharedPtr tls_histogram_ptr =
std::dynamic_pointer_cast<ThreadLocalHistogramImpl>(tls_histogram);
tls_histogram_ptr->merge(interval_histogram_);
}
histogram_t* hist_array[1];
hist_array[0] = interval_histogram_;
hist_accumulate(cumulative_histogram_, hist_array, 1);
}

HistogramStatistics getIntervalHistogramStatistics() {
HistogramStatistics interval_histogram(interval_histogram_);
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest: return HistogramStatistics(interval_histogram_);
...just for brevity

return interval_histogram;
}

HistogramStatistics getCumulativieHistogramStatistics() {
HistogramStatistics cumulative_histogram(cumulative_histogram_);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

return cumulative_histogram_;
}

Store& parent_;
std::list<HistogramSharedPtr> tls_histograms_;
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use the more specific type here to avoid dynamic_cast?

histogram_t* interval_histogram_;
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think in looking at this now I would make HistogramStatistics an interface, then where you need it, have an impl that wraps the circl implementation, which you can then return via const reference.

histogram_t* cumulative_histogram_;
};

typedef std::shared_ptr<HistogramParentImpl> ParentHistogramSharedPtr;

/**
* Implementation of RawStatDataAllocator that just allocates a new structure in memory and returns
* it.
Expand Down
64 changes: 57 additions & 7 deletions source/common/stats/thread_local_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,22 @@ std::list<GaugeSharedPtr> ThreadLocalStoreImpl::gauges() const {
return ret;
}

std::list<HistogramSharedPtr> ThreadLocalStoreImpl::histograms() const {
// Handle de-dup due to overlapping scopes.
std::list<HistogramSharedPtr> ret;
std::unordered_set<std::string> names;
std::unique_lock<std::mutex> lock(lock_);
for (ScopeImpl* scope : scopes_) {
for (auto histogram : scope->central_cache_.histograms_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The style-guide (https://google.github.io/styleguide/cppguide.html#auto) leaves some room for interpretation, but my reading is that in this case and a few others in this PR, you should use explicit types.

If you were using iterator objects, those are good candidates for 'auto' because their types are fairly obvious and not necessarily important to spell out.

e..g.

for (auto it = collection.begin(); it != collection.end(); ++it) {
  explicit_type& val = *it;
  ...
};

or

for (explicit_type& val : collection) {
  ...
};

But it's possible there's an established style here I'm going against the grain in suggesting.

Copy link
Contributor Author

@ramaraochavali ramaraochavali Apr 6, 2018

Choose a reason for hiding this comment

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

I am changing it like this to be type explicit. Is that fine?

  for (ScopeImpl* scope : scopes_) {
    for (auto histogram : scope->central_cache_.histograms_) {
      **const std::string& hist_name = histogram.first;
      const ParentHistogramSharedPtr& parent_hist = histogram.second;**
      if (names.insert(hist_name).second) {
        ret.push_back(parent_hist);
      }
    }
  }

if (names.insert(histogram.first).second) {
ret.push_back(histogram.second);
Copy link
Contributor

Choose a reason for hiding this comment

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

probably worth a note here that the histograms will come back in arbitrary order.

suggestion: return a vector rather than a list, and build that vector after accumulating the set, so that:
a) you can let go of the lock before converting to a vector
b) you can do vector.reserve(names_.size()) before building 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.

@jmarantz have not addressed this yet. will address in next round. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize on re-read that there's an established pattern of using lists rather than for returning collections of stats. @mattklein123 -- I'm curious why that choice?

But for this PR I retract this suggestion -- it probably doesn't save that much to release the lock earlier. I would like to see some comments as to why this de-duping is necessary though; that seems like something must have gone wrong upstream for it to be needed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember why this was a list. I think this was historical. Free free to return a vector if you want to change the various usages.

re: The de-dup, that is required due to the threaded store uses scopes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we incorporate the scopes into the histogram names somehow, so we have distinct ones for each scope? It seems like just dropping them would lose data.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yes, this is a problem for histograms which don't use shared mem backing store. It's not an issue for counters/gauges. Good catch. This will need to be fixed somehow.

Copy link
Contributor

@jmarantz jmarantz Apr 5, 2018

Choose a reason for hiding this comment

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

Cool... @ramaraochavali you can maybe leave a TODO here, and maybe even a warning log that we are dropping content?

@ambuc See also #2453

Copy link
Contributor Author

@ramaraochavali ramaraochavali Apr 6, 2018

Choose a reason for hiding this comment

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

@jmarantz I have added a todo and not changing the return type now based on your comment above about "I retract this suggestion". Is that fine?

}
}
}

return ret;
}

void ThreadLocalStoreImpl::initializeThreading(Event::Dispatcher& main_thread_dispatcher,
ThreadLocal::Instance& tls) {
main_thread_dispatcher_ = &main_thread_dispatcher;
Expand All @@ -75,6 +91,36 @@ void ThreadLocalStoreImpl::shutdownThreading() {
shutting_down_ = true;
}

void ThreadLocalStoreImpl::mergeHistograms() {
if (!shutting_down_) {
std::unique_lock<std::mutex> lock(lock_);
// TODO: Wait till all threads have been checked-in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattklein123 I need your help here. How do I wait till all worker threads have finished? Is there a standard pattern in c++/envoy code here that I can use? Tried to look at latch/barrier support but could not find such thing. This is the only thing pending from basic implementation.

Copy link
Member

Choose a reason for hiding this comment

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

In a perfect world you could add barrier/notify functionality into the TLS itself, e.g., runOnAllThreadsBarrier(...). If you don't want to do that, I would just add a function to TLS to get the number of workers, then in the run on all threads lambda, decrement an atomic counter. Whichever wins the race to 0 can post back to the main thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the method in TLS system.

tls_->runOnAllThreads([this]() -> void {
for (ScopeImpl* scope : scopes_) {
for (auto histogram : tls_->getTyped<TlsCache>().scope_cache_[scope].histograms_) {
TlsHistogramSharedPtr tls_histogram_ptr =
std::dynamic_pointer_cast<ThreadLocalHistogramImpl>(histogram.second);
Copy link
Member

Choose a reason for hiding this comment

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

remove dynamic_cast (by storing impls if needed). I will stop commenting but basically the change should have no dynamic_casts.

tls_histogram_ptr->beginMerge();
}
}
});
if (main_thread_dispatcher_) {
Copy link
Member

Choose a reason for hiding this comment

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

I think main_thread_dispatcher_ must be non-nullptr here. I would skip this check? Obviously this also needs to get moved inside the run on all threads lambda above, see my comment above.

main_thread_dispatcher_->post([this]() -> void { mergeInternal(); });
}
}
}

void ThreadLocalStoreImpl::mergeInternal() {
std::unique_lock<std::mutex> lock(lock_);
for (ScopeImpl* scope : scopes_) {
Copy link
Member

Choose a reason for hiding this comment

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

I would use histograms() here, which will acquire the lock and quickly sort through histograms that need merging. Then, you can operate on each individual histogram safely without any lock since the merging process will take a larger amount of time.

for (auto histogram : scope->central_cache_.histograms_) {
ParentHistogramSharedPtr parent_hist_ptr =
std::dynamic_pointer_cast<HistogramParentImpl>(histogram.second);
Copy link
Member

Choose a reason for hiding this comment

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

to remove all the dynamic casts here, I would change the map to store impls so you can use them locally.

parent_hist_ptr->merge();
}
}
}

void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) {
std::unique_lock<std::mutex> lock(lock_);
ASSERT(scopes_.count(scope) == 1);
Expand Down Expand Up @@ -215,18 +261,22 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {

std::unique_lock<std::mutex> lock(parent_.lock_);
HistogramSharedPtr& central_ref = central_cache_.histograms_[final_name];

std::vector<Tag> tags;
std::string tag_extracted_name = parent_.getTagsForName(final_name, tags);
if (!central_ref) {
std::vector<Tag> tags;
std::string tag_extracted_name = parent_.getTagsForName(final_name, tags);
central_ref.reset(
new HistogramImpl(final_name, parent_, std::move(tag_extracted_name), std::move(tags)));
central_ref.reset(new HistogramParentImpl(final_name, parent_, std::move(tag_extracted_name),
std::move(tags)));
}
HistogramSharedPtr hist_tls_ptr;
hist_tls_ptr.reset(new ThreadLocalHistogramImpl(final_name, parent_,
Copy link
Member

Choose a reason for hiding this comment

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

nit: merge into previous line, or do directly in the context of addTlsHistogram()

std::move(tag_extracted_name), std::move(tags)));
dynamic_cast<HistogramParentImpl&>(*central_ref).addTlsHistogram(hist_tls_ptr);
Copy link
Member

Choose a reason for hiding this comment

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

no dynamic cast. You can store a local ref/pointer if needed to have access to the impl, or just the local map to storing impls.


if (tls_ref) {
*tls_ref = central_ref;
*tls_ref = hist_tls_ptr;
}

return *central_ref;
return *hist_tls_ptr;
}

} // namespace Stats
Expand Down
5 changes: 5 additions & 0 deletions source/common/stats/thread_local_store.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class ThreadLocalStoreImpl : public StoreRoot {
// Stats::Store
std::list<CounterSharedPtr> counters() const override;
std::list<GaugeSharedPtr> gauges() const override;
std::list<HistogramSharedPtr> histograms() const override;

// Stats::StoreRoot
void addSink(Sink& sink) override { timer_sinks_.push_back(sink); }
Expand All @@ -74,6 +75,10 @@ class ThreadLocalStoreImpl : public StoreRoot {
ThreadLocal::Instance& tls) override;
void shutdownThreading() override;

void mergeHistograms() override;

void mergeInternal();
Copy link
Member

Choose a reason for hiding this comment

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

private


private:
struct TlsCacheEntry {
std::unordered_map<std::string, CounterSharedPtr> counters_;
Expand Down
15 changes: 15 additions & 0 deletions source/server/http/admin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "common/network/listen_socket_impl.h"
#include "common/profiler/profiler.h"
#include "common/router/config_impl.h"
#include "common/stats/stats_impl.h"
#include "common/upstream/host_utility.h"

#include "absl/strings/str_replace.h"
Expand Down Expand Up @@ -392,6 +393,7 @@ Http::Code AdminImpl::handlerStats(absl::string_view url, Http::HeaderMap& respo
Http::Code rc = Http::Code::OK;
const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url);
std::map<std::string, uint64_t> all_stats;
std::map<std::string, std::string> all_histograms;
for (const Stats::CounterSharedPtr& counter : server_.stats().counters()) {
all_stats.emplace(counter->name(), counter->value());
}
Expand All @@ -400,11 +402,24 @@ Http::Code AdminImpl::handlerStats(absl::string_view url, Http::HeaderMap& respo
all_stats.emplace(gauge->name(), gauge->value());
}

for (const Stats::HistogramSharedPtr& histogram : server_.stats().histograms()) {
Stats::ParentHistogramSharedPtr parent_hist_ptr =
std::dynamic_pointer_cast<Stats::HistogramParentImpl>(histogram);
Copy link
Member

Choose a reason for hiding this comment

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

no dynamic cast

all_histograms.emplace(
parent_hist_ptr->name(),
fmt::format("\n \t Interval: {} \n \t Cumulative: {}",
parent_hist_ptr->getIntervalHistogramStatistics().summary(),
parent_hist_ptr->getCumulativieHistogramStatistics().summary()));
}

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) {
response.add(fmt::format("{}: {}\n", histogram.first, histogram.second));
}
} else {
const std::string format_key = params.begin()->first;
const std::string format_value = params.begin()->second;
Expand Down
1 change: 1 addition & 0 deletions source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ void InstanceImpl::flushStats() {
server_stats_->days_until_first_cert_expiring_.set(
sslContextManager().daysUntilFirstCertExpires());

stats_store_.mergeHistograms();
Copy link
Member

Choose a reason for hiding this comment

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

If we do this, the merge will essentially always be one interval behind the flush interval. Is this what we want? Do we want to wait into the merge is complete? If so, my thinking was to have mergeHistograms() take a lambda which would fire when the merge is complete, and then the flush would complete.

Also, on this topic, for thought: I do wonder if the histogram merge interval should be separately configurable from the main stat flush interval. (E.g., let's say you wanted to flush stats/gauges every 500ms, you might want to wait longer to get more histogram data per interval). I would put in a TODO to consider this in the future.

Copy link
Contributor Author

@ramaraochavali ramaraochavali Mar 31, 2018

Choose a reason for hiding this comment

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

I did not get the first part. Since we block till all tls histograms are merged then main merge goes in sequentially - is the lambda still required? I may be missing some thing

InstanceUtil::flushCountersAndGaugesToSinks(config_->statsSinks(), stats_store_);
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename this function to something more appropriate? flushMetricsToSinks() ?

stat_flush_timer_->enableTimer(config_->statsFlushInterval());
}
Expand Down
Loading