@mattklein123 capturing your question from the previous PR, so that we can continue the discussion.
Do we think cumulative has much benefit? I can't imagine it being very useful from a debugging perspective so I wonder if we should just drop it here? Thoughts?
Here is the current format
cluster.time.upstream_rq_time: P0(0,0) P25(0,0) P50(0,0) P75(0,1.015) P90(1.06387,1.07194) P95(1.08525,1.09093) P99(2.04331,2.07759) P99.9(4.08333,23) P100(23,140)

I think I can go either away here. It would be useful to compare against cumulative some times to see how much actually has it changed in the interval we are looking at.
Couple of options

1. print all interval histograms first like this
   cluster.time.upstream_rq_time: P0 0 P25 0 P50 0 P75 0 P90 1.06387 P9 1.08525 P99 2.04331 P99.9 4.08333 P100 23
   and after all are done, we add a line like "cumulative histograms" and print all cumulative histograms.
2. print interval and cumulative simultaneously as shown below

cluster.time.upstream_rq_time: P0 0 P25 0 P50 0 P75 0 P90 1.06387  P9 1.08525  P99 2.04331 P99.9 4.08333 P100 23 (Interval)
cluster.time.upstream_rq_time: P0 0 P25 0 P50 0 P75 0 P90 1.06387  P9 1.08525  P99 2.04331 P99.9 4.08333 P100 23 (Cumulative) 

I'm fine with whatever. Let's just keep what you have. We don't guarantee this format here so we can always change it later. It would be nice to document this though. Can you add some docs (and fixup the text about no histogram support) here? https://github.com/envoyproxy/envoy/blame/master/docs/root/operations/admin.rst#L184

Also, can you add a release note about this awesome feature?

fwiw the convention I've seen is use the name usedLockHeld() for this pattern. What you have is fine too. More important is for us to start adding thread annotation but AFAIK it doesn't do anything in Envoy builds yet.

I am fine with changed name - @mattklein123 what do you think?

Sure SGTM.

const Stats::ParentHistogramSharedPtr& histogram

I suspect this actually belongs in the class declaration so that its directives extend to the test-methods, rather than just SetUp().

underscore suffixes for these member vars.

Thanks for doing this! Make the tests much easier to read.

nit: typedefs at top of class

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

I admit I don't do this all of the time because sometimes it's got only one use in the declaration, right below it. But here your usage is broader, so for readability it makes sense to move to the top of the class.

I'm curious what happens here if the system shuts down and deletes the server InstanceImpl before the lambda runs here. I think the answer is that the shutdown will prevent this lambda from running, per a comment I saw elsewhere. I think it'd be nice to note that here as well, perhaps pointing to the doc for runOnAllThreads. And I think this guarantee is due to the semantics of main_thread_dispatcher_->post, which I haven't looked at yet :)

this repeated 4-line stanza could be factored out into a test helper method:
histogram_t* makeHistogram(values) { ... }

I don't think this needs to be, or should be, atomic. The done callback is going to be called exactly once.

Whenever I see atomics I go into hyper-paranoia mode because it's easy to create logical races with them, so I'd rather not have them declared as atomic if it's not (necessary and sufficient).

In this case, the race would be if mergeHistograms ran on another thread, then the EXPECT_TRUE would be a logical race independent of whether the data is atomic or not. In that case, the main remedy I'm aware of to allow this test to block until completion is using a condvar as you did elsewhere.

If this test always completes before mergeHistogram returns, then you don't need the atomic. If it doesn't, the atomic isn't sufficient.

@mattklein123 I'm wondering if the unit test infrastructure serializes threads and therefore I'm being overly paranoid about wanting to block test progress until they complete. FWIW In the past I've done unit testing with threads in full force, but then had common test infrastructure primitives to help quiesce before expecting results.

qq on this, now that I think of it...it's not clear how many threads there actually are when this is tested. Should we ensure there are 10 real threads running in this test?

currently it is 1. The thing that is being verified here is whether the main call back is called after the all post callbacks are executed and it is called as per the expected number of times asserted via thread_local_calls_. Both the things are happening.
The threads here are MockDispatcher instances so I am not really sure having 10 threads helps. Let @mattklein123 review this test and if he suggests it helps to add 10 threads, I will surely add tomorrow.

OK. If there is only one thread in the system I'm pretty convinced we need no condvars or atomics.

But since this is a test of threading infrastructure, I would hope we'd use real threads for this.

The test you have written only has a single thread. It's not useless, but it would be nice to have at least one other real thread. See the test immediately below this one ("Dispatcher"). You should be able to add another real thread and test the real behavior with 1 or more real threads. In that case, you will definitely need the lock/atomic/condvar to have the test correctly work.

The way I would structure the test is to startup 1 or more threaded dispatchers in blocking mode, then when you get the post event, just exit the dispatch loop. Then back on the main thread, you can wait for all worker threads to exit with join, etc.

@mattklein123 I implemented similar to what you have described. In the unit testing setup, the post to main_dispatcher is not actually posting with DispatcherImpl as expected. I tried to post on main_dispatcher by just simply calling main_dispatcher.post() on Dispatcher test also but it is not posting correctly.. So I changed the existing test to test the sequence of callbacks and I think integration tests would any way test the posting part.

nit: "each of the histograms"

"after cb has been run on all registered threads."

summary.reserve(supported_quantiles_ref.size());

nit: // HistogramStatistics above this line.

nit: Move this below 376 (above interface overrides for clarity)

I'm fine with whatever. Let's just keep what you have. We don't guarantee this format here so we can always change it later. It would be nice to document this though. Can you add some docs (and fixup the text about no histogram support) here? https://github.com/envoyproxy/envoy/blame/master/docs/root/operations/admin.rst#L184

Also, can you add a release note about this awesome feature?

The test you have written only has a single thread. It's not useless, but it would be nice to have at least one other real thread. See the test immediately below this one ("Dispatcher"). You should be able to add another real thread and test the real behavior with 1 or more real threads. In that case, you will definitely need the lock/atomic/condvar to have the test correctly work.

The way I would structure the test is to startup 1 or more threaded dispatchers in blocking mode, then when you get the post event, just exit the dispatch loop. Then back on the main thread, you can wait for all worker threads to exit with join, etc.

Does this make all tests run as in sequence??? Wow, neat trick. I wish I had already known this. :)

Not exactly. https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md

By creating an object of type InSequence, all expectations in its scope are put into a sequence and have to occur sequentially. Since we are just relying on the constructor and destructor of this object to do the actual work, its name is really irrelevant.

Can you put a TODO in here somewhere to add summary histogram output for Prometheus? I think it's a trivial change and once that is done we basically have full Prometheus support which is awesome.

I've never heard of an issue like this before. Can you explain a bit more how this happens?

This is just copied from MockHistogram, I am not sure about these details. Sorry :-)

nit: Isn't the prefix assumed to have the trailing . included? So shouldn't this be duplicate histogram {}{}:?

nit: why not just get rid of the local variable and just return true here and false after the loop?

nit: I think this would be a bit more readable and less bug-prone if you used a normal for loop (for(size_t i = 0;...)) and used the index for both vectors rather than just one.

Disclaimer: I may be totally missing something here or this may have already been discussed.

High-level question: is there any guarantee that a histogram produced by the store/scope will only be used in the thread in which it is created? This implementation appears to depend on that guarantee. IIUC, this is not assumed for any of the existing stats or the old histograms. If this is assumed in this case, we need to be careful about how the histogram macros are currently used and be sure to clearly document this restriction.

For example:

auto my_hist = some_scope.histogram(some_name);
.
.
.
// Does this have to be in the same thread as the call above?
my_hist.recordValue(5);

for each histogram created we have ThreadLocal equivalent that is updated in that thread and and ParentHistogram that holds the summary across all the threads. So I think it is not an issue.

Sorry, I think that's exactly my concern. The thread from which you call Histogram& hist = some_scope.histogram(some_name); is assumed to be the same thread that will ultimately call hist.recordValue(). This assumption doesn't generally hold for stats. Many stats are created in the main thread and then passed around to other threads to modify their values since all the existing stats objects are completely thread safe. See this stats struct as an example: https://github.com/envoyproxy/envoy/blob/master/source/common/http/conn_manager_impl.h#L405. It is created in the HttpConnectionManagerConfig, which is initialized in the main thread. Then it is passed to all of the HttpConnectionManagers that are created for each request (can be on any worker thread). The worker threads are able to freely mutate the stats IIUC.

Hmm, yes. Great catch @mrice32. This is broken, at least for how we commonly use histograms. I think the best option to fix is probably to always return back a parent/main thread histogram, and then have recordValue() internally redirect to the TLS version (and create it if needed). I can have a look more later about other options.

Alright, I looked at the code again. Here is my suggestion on how to fix this (though I'm going to admit that this change is complicated enough I'm having trouble doing the design without also being able to try some stuff out via coding, so if it comes to that I can help out). It's going to require some code-rejiggering, but I don't think it's going to end up being too terribly difficult, and it will leave us in a cleaner/better place.

1. When you return a Histogram from the thread local store, what you will actually return effectively is the parent histogram, not the TLS histogram.
2. When recordValue() is called on the parent, at that point, you will do the TLS lookup/cache add, and then call recordValue() on the actual TLS version.

This should be pretty easily doable by just changing the flow of the histogram acquisition function that you have now.

@mattklein123 SGTM, I like that solution. I was trying to think of a way to do it without doing a cache lookup for every recordValue() call, but I don't think it would be possible without a TLS slot per histogram, and TLS isn't really built for that sort of scaling IIUC (and cache lookups are pretty cheap).

@jmarantz Not sure. I'm surprised that tsan didn't get upset about this in one of the existing integration tests.

I was trying to think of a way to do it without doing a cache lookup for every recordValue() call

I think in the future we could add some type of "direct TLS access" histogram for use cases in which the user knows they are safe, but IMO I wouldn't worry about it for now and we can track as a future perf improvement...

@ramaraochavali one other thing we should do here is keep track of the thread ID that the TLS histograms are created on and make sure that recordValue() is only called on that same thread. You could add ASSERTs along those lines pretty easily, and it would also help document the threading model.

@mattklein123 @mrice32 I have added a tlsHistogram method in the ScopeImpl that creates ThreadLocalHistogram since the ScopeImpl has all the things needed for putting in scope cache etc. It is working fine. Can you PTAL? I think we should be good with this.

@mattklein123 I have added the ASSERT that you mentioned.

@mrice32 are you fine with this change?

It seems strange that this interface is under source/ rather than include/, especially since ParentHistogram is under include/ and its docs implicitly reference the thread local histograms.

It was discussed that we do not want to expose the ThreadLocalHistogram in the stats interface as it is implementation detail. I would adjust the docs of ParentHistogram not to refer this in stats.h definition.

That makes sense. But in that same vein, why do we need to expose ParentHistogram in the interface?

It is because the rest of the code like admin, server and stats sinks especially Metrics Service sink use it to push data to sinks.

Ahhh, I see. Thanks for the explanation. Sorry to keep pulling at this, as it's really just a nit, but I do have one last question - why do we need the abstract base class? It seems that all uses (including TlsHistogramSharedPtr) explicitly reference the ThreadLocalHistogramImpl. Do you think that we'll ultimately have other TLS histogram implementations? If so, maybe we should use the base class in those places?

I think you can just remove this interface entirely. Is it even needed? I would just fold it all into the implementation below which is specific to this thread_local_store implementation?

@mattklein123 +1

Yes. We can do that. I have changed. PTAL.

At a minimum, can you leave a TODO to get the multi-thread version test running, along with more detailed information about why your previous attempt didn't work?

I don't think this data is thread-local. Maybe just rename the struct to be 'thread_status'

suggest early-exit here if parent is shutting down or doesn't have TLS, simplifying conditionals below.

It's actually a little strange to me that you allocate a histogram object even if you are shutting down. WDYT of returning a Histogram* that's allowed to be nullptr and handling that case at the call-site?

Alternative what you have is OK but deserves some comments for the call-during-shutdown case.

I'm also not sure when parent.tls_ might be null.

It follows the same logic as counter(), histogram() etc are followed. I added the comment here as well.

mergeHistograms is async. So I wonder if you should keep track of whether there's one in progress, and skip merging in that case. I think the behavior if you induce two or three merges in a row before merge_complete_cb, the behavior may be hard to reason about.

In that case it may be sufficient to simply drop the callback; that would seem to work well for the call-site in InstanceImpl::flushStats()

I do not think there would be two merges happening in parallel unless the merge takes abnormally long. It is called from flushStats which is invoked for every stats flush interval default value of which is 5 sec. This might happen if the flush interval is configured very low, may be less than sec or few milliseconds. So I think the tracking complication is not required here.

All it takes is for a thread to not get scheduled for a few seconds because the machine is busy, and this will be an issue. Don't you think it's worth checking?

I am not sure if it is really required or not but I see no harm in adding that check. Added. PTAL.

I don't think this can happen with how we're using this method since the timer is only rescheduled in the last line of the merge_complete_cb, which is called only after all the merging is done. (Feel free to correct me if I'm misunderstanding the flow.)

If we imagine that this method may be used in other circumstances, I think a check like this could make sense, but this method has a pretty specific use case. Maybe instead we could leave out the check, but document how this method should be used (it can only be called again once the passed in callback is called)? We could still keep track of whether a merge is in progress and just ASSERT this check rather than branch on it. Not super opinionated, but it just sometimes makes code (especially complicated code :) ) less readable when inactive constraints are used in branches.

@mrice32 I think you are probably right, but there are two call-sites to flushStats() from server.cc. It's not immediately obvious to me that there may never be any other initiators. That is a private method now.

I think this is a very cheap check to add, to avoid having to think too hard about what very confusing (and possibly crashy) behavior might arise from having two or three concurrent mergeHistogram calls running. WDYT?

@mrice32 After pushed this I also realized that the timer is rescheduled only at the last line of the merge_complete_cb. I left it because it does not harm having this as it is safe and cheap trick as @jmarantz mentioned.

That's fair - I think the check makes sense. But I'm somewhat opposed to the idea of having concurrent merges appear as if they are part of the normal program flow and no-op when they happen. I would personally prefer to document how the method should be used and ASSERT(!merge_in_progress_). Anyway, either way is fine, that's just my personal preference. In either case, I think the behavior should be clearly documented in the declaration.

ASSERT would be OK with me. Is there an Envoy equivalent to

if (cond) {
  LOG(DFATAL) << "error message";
}

Where in release-builds this will log an error, and in debug builds it will also abort?

And in any case documenting how the methods should be used would be great.

ok. I have added the ASSERT and updated the documentation of this method. PTAL.

runOnAllThreads

changed