Why is this all commented?

The tests were using the old sink interface. I need to update.

nit: can't you just move this ++it up into the for loop statement and get rid of the else?

nit: since you're breaking, can't you just ignore the return value?

Why are these commented?

old code

I'm concerned that a per-cluster cache where elements are only added but never deleted is going to effectively be a memory leak. Also, can you describe what you want to happen if a cluster is deleted and then another cluster is, many flushes later, added under the same name? IIUC, with the current implementation, this will result in that cluster using the old, unrelated cluster's cache. I have a few ideas of how to flush the cache, but before proposing anything, I'd like to understand what the desired behavior is for naming collisions.

Good catch, I did not consider those cases. I think the best would be to detect when a cluster no longer exists and remove from the cache map. What are your ideas?

I haven't thought it through completely, but I think one solution would be to basically check the differences in sizes in the two maps (the cluster map in the server and the sink's cache map) after you finish updating the caches. Since you've already added any missing clusters to the sink's map, it's only possible that the sink map is larger. If it is larger, you can iterate through the sink's map and look up each name in the server's cluster map and delete any that aren't found in the server's map. It's not an incredibly elegant solution, but I'm not sure if there are any better options for trying to sync two unordered maps.

Almost done handling the clusters removal + updating the test. How can I simulate removing and adding back clusters during operation? (not in a test)

We should be using each cluster's stats scope to get cluster-specific stats. Otherwise, they will not be deallocated when the cluster is removed.

Not sure I understand the comment about not being deallocated when the cluster is removed. Are you referring to using Upstream::ClusterStats? or the Shared pointer Upstream::ClusterInfoConstSharedPtr?

We should be using Upstream::ClusterInfo::statsScope() and calling counter(stat_name) on that (the name will be different since the scope already assumes the cluster prefix) - see here.

The stat scope system is a little confusing with respect to how it manages memory. When you request a stat from a scope using scope.counters(stat_name), the scope will then ensure the underlying stat stays alive as long as that scope is alive. If we pull the cluster stats from the server scope, that means the stats that we're requesting are required to live as long as the server's stats scope, which is the life of the Envoy instance. If we grab them from the cluster scope, they will be deallocated when the cluster is removed as one would expect for cluster-specific stats.

Is it new interface? It is very good for us, since using this, we won't need to save the concatenated string of the statistic's name per cluster. What about the stats we are currently getting from Upstream::ClusterStats::stats()? Take them also from the Upstream::ClusterInfo::statsScope() instead?

Yup, it should make things much easier. Technically, you could pull any stats in Upstream::ClusterStats::stats() from Upstream::ClusterInfo::statsScope(), but it's better the way you have it. Upstream::ClusterStats::stats() is essentially a struct holding stats from Upstream::ClusterInfo::statsScope() to access them without doing a map lookup.

nit: method name is misleading. Can we change to something like writeClusterStats(), appendClusterStats(), or addClusterStatsToStream() (as the methods are named below)?

nit: since rolling_window represents a time, but it's represented as an integer, can you change the name to indicate that it's a time - like rolling_window_ms or window_period_ms? As a reader, this was a bit confusing to me.

I think this series of operations could be simplified a bit. Could we combine this loop with the updateRollingWindowMap loop above? We could also move the cache cleanup to the very end (since this is probably the least time sensitive operation). Then we can do a single lookup of the cache for a particular cluster and pass it through to updateRollingWindowMap and getClusterStats.

Are these names just here for the purpose of testing and logging? I don' think the full stat names are still needed since we're looking the stats up within the cluster scope. It appears that they're only used in printRollingWindows(), which seems to only be used in tests and log messages? Not sure how others feel, but I think we should probably avoid storing this state if it's just for logging and testing. If we want to keep that method for logging and testing, we could just let it construct whatever preferred stat name is needed on the fly.

Pushed new version. Please see if it seems more reasonable now. Another option is to totally remove it. It is there just for logging. Not needed for testing.

Why do we need to const cast this? If this interface returns a const StreamDecoderFilterCallbacks&, I think there's something wrong with it. StreamDecoderFilterCallbacks has no const methods, which means a const one would essentially be useless, right? I think this should be updated to return a non-const StreamDecoderFilterCallbacks&.

We wanted to return only const refs when we worked on the AdminStream. It forced me to use the const_cast for the hystrix implementation. We overlooked the fact there are no const methods on it anyway.

nit: I think we generally prefer:

if (callbacks_list_.empty()) {
  return;
}

nit: you could avoid the redundant lookup by doing something like:

std::unique_ptr<ClusterStatsCache>& cluster_stats_cache_ptr = cluster_stats_cache_map_[cluster_name];
if (cluster_stats_cache_ptr == nullptr) {
  cluster_stats_cache_ptr = std::make_unique<ClusterStatsCache>(cluster_name);
}
// Use cluster_stats_cache_ptr here

nit: why not just do the cache lookup based on cluster name out here and pass a reference to the cache into both methods?

IIUC - you mean to move the std::unique_ptr<ClusterStatsCache>& cluster_stats_cache_ptr = cluster_stats_cache_map_[cluster_name]; up to flush()? Please see the new version , is this what you had in mind?

nit: Sorry for being indecisive on this, but could we instead just pass the rolling_window_ms around as type std::chrono::milliseconds rather than a uint64_t? This means that if the flush interval that you're accessing starts being represented as std::chrono::microseconds or std::chrono::seconds, you'll see a compile-time error. I know this is unlikely, but we've had problems with outdated .count() interpretations in the past.

nit: this could be made a bit simpler/faster by doing the following:

for (auto it = cluster_stats_cache_map_.begin(); it != cluster_stats_cache_map_.end();) {
  if (clusters.find(it.first) == clusters.end()) {
    it = cluster_stats_cache_map_.erase(it);
  } else {
    ++it;
  }
}

Doesn't seem like you need to prefix this variable-name with 'AccessControlAllow', since that's in the struct name.

why not just put this in the initializer on line 47?

const bool no_chunks = headers.NoChunks();

actually there's just a single-use, maybe you can just write

if (saw_content_length || headers.NoChunks()) {

below, and skip the temp?

looking at the call-sites you could kill this version, and have the 2-arg ctor init like this:

  : server_(server),
    current_index_(num_of_buckets ? num_of_buckets : DEFAULT_NUM_OF_BUCKETS_),
    window_size_(current_index_ + 1) {

That would simplify call-sites.

nit: s/register/Registers/ , and specify doxygen for args per convention, here & below.

and below
a/remove/Removes/, s/Add/Adds/, etc.

not necessary to initialize an empty vector; just leave out the {}

s/{}//

move member vars below the methods.

I think this should be a class not a struct.

https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

But if this is a clear pattern in Envoy it's OK to leave it as a struct, in which case you don't need to suffix the member vars with _

Actually by that definition, it seems to me that it should be a struct and not a class. It passively holds the rolling windows, which are manipulated by the sink's methods. What do you think?

OK, that's fine; I guess the methods are just printers. And actually the _ suffix is called out by STYLE.md as a local rule. Leave as is then :)

trim_data_before_key, trim_data_after_value.

I'd put the comment above the call to buffer.add(data); -- I think it'll format more nicely that way.

Set cluster. (sentences start with a capital letter, end with a period). Here & below.

nit: you could do this in one line:

EXPECT_NE(std::string::npos, rolling_map.find("test_cluster1.total"));

the advantage is that if it fails the test error message would be a little easier to debug because it would include the source of the find call.

probably want to make these ASSERT_NE because you don't want to use relational operators below on pos1 or pos2 if they are npos.

My general comment here is that I can only peephole review the tests because they are so long, so it's hard for me to tell what's going on. Can you factor out some common blocks into helpers? E.g. this pattern of checking that a specific bunch of values in a message are 0 is repeated 3x in this file. Factor that out into a common method. This is also the case likely with a bunch of other stanzas, such as as setting up a bunch of mocks.

Thanks for pointing it out. I am factoring out a bunch of stuff. Will push once ready.
Problem with helper method for the checkers is, as you pointed out in the past, it would be hard to debug error messages from lines inside the function. But the test code would be more readable. I can group most of the checkers into readable methods. Any suggestion here?

ASSERT_EQ

I wonder if this pattern of using find on the cluster-names and this dance with the substr etc could be done easier with absl::StrSplit. That would be the case if there were a known delimiter between the clusters.

There are known delimiters, but the order of the clusters is not guaranteed, so we'll need to add more actions to find which is which. (or otherwise, add sorting in the code that produces the stream)

A helper method to populate & return a map would be be useful to remove this kind of complicated pattern of finding substring delimiters in this way. In addition to require a handful of lines that require some scrutiny, the current pattern could fail if either cluster-name turned out to be a substring of some other syntax in the data.

you don't need this, as on the next line you kill the previous value completely.

is 12 arbitrary? I feel like some of the golden values you are comparing to below are somehow related to the constants here, but it's not super-obvious what the relationship is. E.g. I see a few cases throughout this file where rollingCountSemaphoreRejected is tested against some expected constant, but I can't tell why that constant is expected. E.g. why is errorCount 160?

Can we at least have comments explaining the relationship?

Some of the values are arbitrary. We set some values to simulate the statistics and then we calculate the expected values in the stream. I was thinking we could do that in a function that will set the counter values and then calculate expected results and check them, this will make things more obvious. This function can be used in many places throughout the test. But again, I have the same concern that when the function fails, it will be hard to debug.

I'm not following you why failures in a function are hard to debug.

Do you mean that if there's an EXPECT failure in a helper function, you might not see the whole call-stack in a test log? I agree it's awesome when you can diagnose and fix failures purely based on a test log. However, I don't think it justifies flattening and inlining what logically should be functions.

You can also make test logs directly actionable by passing enough context to the function to print, but usually the clutter is not worth it.

And you can actually debug tests with gdb, in which case a function does not present any barrier to debuggability. Note that debugging envoy unit tests with gdb is not super-obvious even if you are otherwise familiar with gdb, but there is a helper script:

tools/bazel-test-gdb //test/common/common:utility_test -c dbg

And the high order bit here, is: this test is filled with magic constants and it's not clear why they are the right ones.

ok, I'll put them in functions and then we'll look and see if it can be improved for debugging.