stats: unrevert and fix histograms #3262
Conversation
|
@ramaraochavali @jmarantz @mrice32 PTAL. Unfortunately, I have some kind of HW issue with my computer and it's randomly crashing under load. I won't be able to get a replacement until Monday morning when I am back from Europe so if there are major changes required to this they are going to have to wait. |
| @@ -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 = "0c44450723e34c9d8768e69b11bf919be83fd2ed", # 2018-04-30 | |||
There was a problem hiding this comment.
There are couple of additional commits that @postwait has added after this. But we can stick with this for now.
|
@mattklein123 Thanks for putting together this PR. I looked at the second commit and LGTM. |
| for (ScopeImpl* scope : scopes_) { | ||
| for (const auto& name_histogram_pair : | ||
| tls_->getTyped<TlsCache>().scope_cache_[scope].histograms_) { | ||
| for (const auto& scopes : tls_->getTyped<TlsCache>().scope_cache_) { |
There was a problem hiding this comment.
scopes or scope? Previously it was 'scope' with a concrete type. WDYT of using a concrete type for this one as it's not super-obvious locally what type this resolves to, and I suspect it's not particularly verbose.
There was a problem hiding this comment.
Sure will do. Caveat being that my computer is Fd and I'm having trouble doing anything real so probably will need to wait till Monday.
There was a problem hiding this comment.
OK go ahead and merge then...I can do these myself easily after you merge. I know we're trying to try these out so no need to delay a week for a style nit.
| return **tls_ref; | ||
| } | ||
|
|
||
| std::unique_lock<std::mutex> lock(parent_.lock_); |
There was a problem hiding this comment.
I don't think we need to take this lock here. Kinda unrelated side note: I think we can make the getTagsForName method on ThreadLocalStore const.
There was a problem hiding this comment.
Yup that's right. I think I also noticed that at some point and then forgot about it amidst all the issues. Thanks for mentioning it. I will see if I can nurse my computer into making some of these small changes.
There was a problem hiding this comment.
@mattklein123 if you have trouble with your computer, merge this PR and I can do the follow-up PR addressing these minor comments if that’s ok.
There was a problem hiding this comment.
Sure, that's fine. @htuch can you approve/merge if this looks good?
This change unreverts: Also fixes envoyproxy#3223 Please see the 2nd commit for the actual changes. The changes are: Bring in a new histogram library version with the fix (and more debug checking). Fix a small issue with scope iteration during merging. Remove de-dup for histograms until we iterate to shared storage for overlapping scope histograms. Personally, I found this very confusing when debugging and I think the way I changed it is better for now given the code we have. Signed-off-by: Matt Klein <mklein@lyft.com>
This change unreverts:
#3130
#3183
#3219
Also fixes #3223
Please see the 2nd commit for the actual changes. The changes are:
checking).
storage for overlapping scope histograms. Personally, I found
this very confusing when debugging and I think the way I changed
it is better for now given the code we have.