Refactor Stats::RawStatData into a StatsOptions struct #3629

ambuc · 2018-06-14T16:30:06Z

WIP: Refactor Stats::RawStatData into a Stats::StatsOptions struct.

Description:
This change allows us to deprecate the statics inside Stats::RawStatData. Some side effects of this change are:

a) HeapRawStatDataAllocator no longer performs stat name truncation,
b) we now construct BlockMemoryHashSet, HotRestartImpl, C/L/RdsSubscription, and ThreadLocalStoreImpl as functions of this Stats::StatsOptions struct, and
c) Stats::RawStatData now looks more like a set of libraries for computing stat padding, as opposed to a source of truth for the maximum allowable name lengths.
Finally, a chain of functions starting under server.cc (translateBootstrap, translateClusterManagerBootstrap, translateListener, translateCluster, translateVirtualHost) have had Stats::StatsOptions& added to their interfaces, so that Utility::checkObjNameLength() can be called with the necessary context.

Risk Level: Medium

Fixes: #3508

jmarantz · 2018-06-14T17:24:07Z

source/common/stats/stats_impl.h

-  absl::string_view key() const {
-    return absl::string_view(name_, strnlen(name_, maxNameLength()));
+  absl::string_view keyGivenStatsOptions(const StatsOptions& stats_options) const {
+    return absl::string_view(name_, strnlen(name_, stats_options.maxNameLength()));


this really could be fixed in a separate PR, but the signature change makes it more obvious; IMO we should spend a few bytes and retain the actual length in the structure, rather than a nul byte, rather than having to call strlen on every access.

I think we need to support >256 characters so we should probably spend a few bytes and retain the length explicitly in the structure.

That will improve the speed (at a moderate cost of memory) and could be done in a very isolated way, reducing the # of changes in this PR.

I'm kinda on-board with this. Can we not just pre-truncate the name? As in, why can't we just pass an optional length to which we want name_ truncated? As @jmarantz mentioned, this will require us to make name_ null-terminated.

This gets truncated at write time in initialize() / safeInitialize(); i think we can remove the keyGivenStatsOptions entirely since name_ is already being null-terminated.

Yes, this works! I will replace all instances of keyGivenStatsOptions with key().

mrice32 · 2018-06-14T18:28:10Z

include/envoy/stats/stats.h

+class StatsOptions {
+public:
+  virtual ~StatsOptions() {}
+  virtual size_t maxNameLength() const PURE;


Please document these methods.

mrice32 · 2018-06-14T18:29:31Z

source/common/common/block_memory_hash_set.h


  /**
   * Returns the options structure that was used to construct the set.
   */
-  const BlockMemoryHashSetOptions& options() const { return control_->options; }
+  const BlockMemoryHashSetOptions& hash_set_options() const { return control_->hash_set_options; }


nit: hashSetOptions()

Actually I think i'm going to get rid of this function altogether, since it doesn't actually get called anywhere. If we need it later we can put it back.

mrice32 · 2018-06-14T21:27:45Z

source/common/stats/stats_impl.h

-  absl::string_view key() const {
-    return absl::string_view(name_, strnlen(name_, maxNameLength()));
+  absl::string_view keyGivenStatsOptions(const StatsOptions& stats_options) const {
+    return absl::string_view(name_, strnlen(name_, stats_options.maxNameLength()));


I'm kinda on-board with this. Can we not just pre-truncate the name? As in, why can't we just pass an optional length to which we want name_ truncated? As @jmarantz mentioned, this will require us to make name_ null-terminated.

mrice32 · 2018-06-14T21:28:52Z

source/exe/main_common.cc

@@ -62,7 +61,8 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) {
    auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion());
    Logger::Registry::initialize(options_.logLevel(), options_.logFormat(), log_lock);

-    stats_store_.reset(new Stats::ThreadLocalStoreImpl(restarter_->statsAllocator()));
+    stats_store_.reset(


nit:

stats_store_ = std::make_unique<Stats::ThreadLocalStoreImpl>(restarter_->statsAllocator());

mrice32 · 2018-06-14T21:54:38Z

test/common/stats/thread_local_store_test.cc

@@ -65,7 +67,7 @@ class HistogramTest : public testing::Test, public RawStatDataAllocator {
    }));

    EXPECT_CALL(*this, alloc("stats.overflow"));
-    store_.reset(new ThreadLocalStoreImpl(*this));
+    store_.reset(new ThreadLocalStoreImpl(options_, *this));


nit: std::make_unique

nit: can we use std::make_unique here?

mrice32 · 2018-06-14T21:55:12Z

test/common/stats/stats_impl_test.cc

@@ -477,15 +477,6 @@ TEST(TagProducerTest, CheckConstructor) {
      "No regex specified for tag specifier and no default regex for name: 'test_extractor'");
 }

-// Validate truncation behavior of RawStatData.
-TEST(RawStatDataTest, Truncate) {


Do we have an equivalent test that tests the truncation wherever it happens now?

Yes, HotRestartImplTest.truncateKey also tests this, but I'll put back in a native RawStatDataTest.Truncate test.

mrice32 · 2018-06-14T21:59:55Z

test/server/http/admin_test.cc

@@ -47,7 +47,7 @@ class AdminStatsTest : public testing::TestWithParam<Network::Address::IpVersion
    }));

    EXPECT_CALL(*this, alloc("stats.overflow"));
-    store_.reset(new Stats::ThreadLocalStoreImpl(*this));
+    store_.reset(new Stats::ThreadLocalStoreImpl(options_, *this));


nit: std::make_unique

mrice32 · 2018-06-14T22:04:40Z

source/common/stats/stats_impl.cc

@@ -153,14 +136,9 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector<Tag>
 }

 RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) {
-  RawStatData* data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));
+  RawStatData* data = static_cast<RawStatData*>(::calloc(RawStatData::sizeGivenName(name), 1));


If this no longer truncates, the names of the stats that overflow into the heap allocator in the hot restart case will no longer be truncated. This means that the overflowed names will be inconsistent with those that were allocated in the block allocator. Can we make sure that we truncate in that case to avoid the inconsistency?

This sounds good. Rather than propagate a truncation throughout ThreadLocalStore, I've put the .substr() fn inline where make_stat calls the heap_allocator in `thread_local_store.cc.

ambuc · 2018-06-18T17:33:44Z

I'm going to hold off on this until #3606, and maybe even close-and-reopen depending on how big the differential is.

ambuc · 2018-06-25T14:07:16Z

@mattklein123 This PR removes truncation from HeapRawStatDataAllocator, since it doesn't have context from the top-level options struct, and we want to get rid of deeply nested statics. Am I correct in thinking that this stat name truncation isn't necessary here? @jmarantz @mrice32

mattklein123 · 2018-06-25T14:21:45Z

Am I correct in thinking that this stat name truncation isn't necessary here?

I haven't looked at this PR yet. Do you mean is it not necessary because there is no limit inside the heap allocator? I don't think there is any particular requirement to do truncation.

jmarantz

I think the key question here: does this get a whole lot simpler given that @mattklein123 has indicated that no truncation is required except in the hot-restart allocator.

And in that case, it has the options already, so I think this PR can be a lot simpler.

jmarantz · 2018-06-25T19:34:13Z

source/common/stats/stats_impl.h

   */
-  static size_t maxStatSuffixLength() { return MAX_STAT_SUFFIX_LENGTH; }
+  static uint64_t sizeGivenName(const std::string& name);


name could be specified as absl::string_view with no perf or compatibility downside here, I think.

jmarantz · 2018-06-25T19:35:59Z

source/common/stats/stats_impl.cc

+
+void RawStatData::safeInitialize(absl::string_view key, const StatsOptions& stats_options) {
+  ASSERT(!initialized());
+  if (key.size() > stats_options.maxNameLength()) {


maybe be more explicit here naming this function:

truncateAndInit()

ambuc · 2018-06-26T14:10:57Z

I think this PR doesn't get that much simpler given that only hot restart needs truncation. A lot of the complexity of this PR comes from Utility::checkObjNameLength() needing access to stats_options, which means CdsJson::translateCluster/LdsJson::translateListener/RdsJson::translateVirtualHost need to be passed stats_options, which means CdsSubscription needs a Stats::Scope, and that BootstrapJson::translateBootstrap needs stats_options... but the buck stops there, since translateBootstrap gets called in the top-level server.cc. I don't think this is too bad, since it allows us to preserve the same level of verbosity to users accidentally name clusters (or routes, or listeners) something too long.

ambuc · 2018-06-26T19:33:06Z

@mattklein123 This is ready for review.

jmarantz · 2018-06-26T19:44:19Z

source/common/config/bootstrap_json.cc

@@ -24,7 +25,7 @@ void BootstrapJson::translateClusterManagerBootstrap(
    auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters()->Add();
    Config::CdsJson::translateCluster(*json_sds->getObject("cluster"),
                                      absl::optional<envoy::api::v2::core::ConfigSource>(),
-                                      *cluster);
+                                      *cluster, stats_options);


This is kind of an interesting maze to follow for those of us that don't know this part of the code deeply. But with a bit of grepping I think you can or should be able to pull the stats options from Cluster, which knows about ClusterManager, which knows about a bunch of things. I'm not sure if you can exactly get to the OptionsImpl from any of those, but it given all the things passed into ClusterManagerImpl's ctor, I think it would not be unreasonable to also have it see the options.

source/common/upstream/cluster_manager_impl.h

WDYT?

I think it would be reasonable for ClusterManager and ListenerManager and HttpConnectionManager (for routes) to know about some higher-level thing with access to a StatsOptions (such as ServerImpl). Right now I've embedded StatsOptions inside Scope, and Scope as a member of a Server::Configuration::FactoryContext, which is already present in a lot of these locations. I'm just not sure it would really save us that much complexity.

mattklein123 · 2018-07-03T18:25:11Z

@ambuc @mrice32 @jmarantz what's the status of this PR? Does it need a final review? Looks like it needs a master merge?

ambuc · 2018-07-06T14:15:56Z

@mattklein123 Sorry for dropping the ball on this. I've done a merge, so this PR is now ready for a final review.

As noted above, the bulk of this PR isn't actually due to the addition of a statsOptions struct. Most of the delta comes from the eradication of a static call inside Utility::checkObjNameLength(). That utility now needs access to a top-level statsOptions struct for consistency, and I've had to modify a number of interfaces to accommodate this. If this makes the PR harder to read, I could tease it out into a separate one, but let me know what you think.

mattklein123

I went through at a high level and have some small comments. This is tricky code so I'm mostly assuming that @jmarantz and @mrice32 already went through in detail. Generally LGTM, nice work!

mattklein123 · 2018-07-09T21:08:49Z

include/envoy/stats/stats.h

+  virtual size_t maxNameLength() const PURE;
+
+  /**
+   * The max allowed length of the object part of a stat name.


What is the object part and the suffix part of a stat? Can you potentially provide an example in the comments?

There's more color in /source/common/stats/stats_impl.h, which I'm not sure how much of to duplicate in the include file. So the object part is something like cluster.<cluster_name>.outlier_detection, and the suffix part is one of the statistics listed in the docs here like ejections_enforced_consecutive_gateway_failure. We just want to limit the lengths of these halves separately. Do you think it's worth adding that to the include too?

Maybe just comment about where the other comments are? In general I would expect the public includes to have the detail so I can understand what things mean, but fine to link to other places. Whatever works. It's mostly that this nomenclature caught me off guard so I think it might confuse others also.

That sounds good to me. I did some work in 732af4f.

mattklein123 · 2018-07-09T21:13:55Z

source/common/stats/stats_impl.cc

+  ref_count_ = 1;
+
+  uint64_t xfer_size = key.size();
+  memcpy(name_, key.data(), xfer_size);


so where is initialize called now? Does this need some safety guard for cope size? If not, can you add a comment?

So there are two initialize fns now -- one, RawStatData::initialize(key), which is used in cases where no truncation is performed (like in HeapRawStatDataAllocator::alloc(name)). The other, RawStatData::truncateAndInit(key, stats_options), gets called in BlockMemoryHashSet::insert(key), which has access to a local stats_options_ struct. So the former case doesn't guard for some max size.

Can we somehow assert that we aren't going to overwrite memory here? It's not super clear to me how we know that this is a safe copy (or if heap allocated pass in the size of the heap allocation and assert that)?

mattklein123 · 2018-07-09T21:16:04Z

source/common/upstream/cds_subscription.h

@@ -55,6 +56,7 @@ class CdsSubscription : public Http::RestApiFetcher,
  Config::SubscriptionCallbacks<envoy::api::v2::Cluster>* callbacks_ = nullptr;
  Config::SubscriptionStats stats_;
  const absl::optional<envoy::api::v2::core::ConfigSource>& eds_config_;
+  const Stats::Scope& scope_;


Instead of storing a reference to the scope here, would it better to store reference to the stat options? We are slightly splitting hairs but it potentially seems a bit clearer/safer to me.

Makes sense -- I put statsOptions into the scope struct, and found it made more contextual sense to add scope as an argument to a lot of higher-level constructors or factories. But as those factories call lower-level utilities (parseHttpConnectionManagerFromJson, it makes more sense to give them the specific arguments they need, like the statsOptions struct. I've changed the interfaces for CdsJson::translateCluster, LdsJson::translateListener, and some others.

mattklein123 · 2018-07-09T21:16:54Z

source/server/hot_restart_impl.cc

  return fmt::format("{}.{}.{}.{}", VERSION, sizeof(SharedMemory), max_num_stats,
-                     max_stat_name_len);
+                     stats_options.maxNameLength());


This change doesn't change the shared memory layout at all, right? Just being extra paranoid I would double check to make sure nothing changes w/ hot restart and with the output versions, as we have had issues in this area in the past. I'm not sure what our test status is on hot restart versions changing.

It doesn't change that last argument to fmt:;format(), since that value max_stat_name_len used to be the sum of max_obj_name_len and maxStatSuffixLength(), and it still is. But I'll rerun the hot_restart_impl_test on this and upstream master and just make sure.

I reran this test for master and this branch and the hot restart versionString is the same. 😄 In master right now, the server/mocks.cc sets maxObjNameLength() to return 150 by default. This new test does not, so the versionStrings look different -- but if we set

stats_options_.max_obj_name_length_ = 150

somewhere in TEST_F(HotRestartImplTest, versionString), then the versionStrings are identical.

This is somewhat unrelated to your change, but WDYT about actually committing the hot restart version string into a test somewhere and verifying that it is what we expect it to be? That would warn us about a change in the hot restart version that we didn't intend? Thoughts?

(Perhaps this goes in the hot restart test itself)?

I like this -- I could hardcode that string into hotrestart_test.sh for comparison, merge in that PR first, and then verify that this PR doesn't need any changes to pass that test.

+1, thank you.

@mattklein123 see #3832.

mattklein123 · 2018-07-09T21:19:13Z

source/server/lds_subscription.h

@@ -51,6 +52,7 @@ class LdsSubscription : public Http::RestApiFetcher,
  const LocalInfo::LocalInfo& local_info_;
  Config::SubscriptionCallbacks<envoy::api::v2::Listener>* callbacks_ = nullptr;
  Config::SubscriptionStats stats_;
+  const Stats::Scope& scope_;


similar comment here about scope (and options), maybe applies elsewhere also.

Agreed -- see #3629 (comment).

mattklein123 · 2018-07-09T21:19:56Z

test/common/common/block_memory_hash_set_test.cc

      memcpy(name, key.data(), xfer);
      name[xfer] = '\0';
    }
-    static uint64_t size() { return sizeof(TestValue); }
+    static uint64_t sizeGivenStatsOptions(const Stats::StatsOptions& stats_options) {
+      (void)stats_options;


nit: UNREFERENCED_PARAMATER or just delete arg variable name. Same elsewhere if I missed it.

This change allows us to deprecate the statics inside Stats::RawStatData. Some side effects of this change are: a) HeapRawStatDataAllocator no longer performs stat name truncation, b) we now construct BlockMemoryHashSet, HotRestartImpl, C/L/RdsSubscription, and ThreadLocalStoreImpl as functions of this Stats::StatsOptions struct, and c) Stats::RawStatData now looks more like a set of libraries for computing stat padding, as opposed to a source of truth for the maximum allowable name lengths. Finally, a chain of functions starting under server.cc (translateBootstrap, translateClusterManagerBootstrap, translateListener, translateCluster, translateVirtualHost) have had Stats::StatsOptions& added to their interfaces, so that Utility::checkObjNameLength() can be called with the necessary context. Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123

LGTM, some small nits. Given the risk of this type of change would definitely like @mrice32 to take another pass though. Thank you!

mattklein123 · 2018-07-10T22:15:40Z

include/envoy/stats/stats.h

+ * object name length can be overridden. The default initialization is used in IsolatedStatImpl, and
+ * the user-overridden struct is stored in Options.
+ *
+ *  As noted in the comment above StatsOptionsImpl in source/common/stats/stats_impl.h, a stat name


nit: errant space, start of sentence

mattklein123 · 2018-07-10T22:18:01Z

include/envoy/stats/stats.h

+ *  As noted in the comment above StatsOptionsImpl in source/common/stats/stats_impl.h, a stat name
+ * often contains both a string whose length is user-defined (cluster_name in the below example),
+ * and a specific statistic name generated by Envoy, which are often as long as
+ * `upstream_cx_connect_attempts_exceeded`. To make room for growth on both fronts, we limit the max


nit: not sure the example here is really helpful, I would just delete it.

mattklein123 · 2018-07-10T22:20:43Z

source/common/stats/stats_impl.cc

+  ref_count_ = 1;
+
+  uint64_t xfer_size = key.size();
+  memcpy(name_, key.data(), xfer_size);


Can we somehow assert that we aren't going to overwrite memory here? It's not super clear to me how we know that this is a safe copy (or if heap allocated pass in the size of the heap allocation and assert that)?

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc · 2018-07-11T14:16:29Z

@mattklein123 I added an ASSERT in 2aa9d8c. HeapRawStatData::Allocator calls RawStatData::initialize() only after figuring out how many bytes to allocate, for which it uses RawStatData::sizeGivenName(name). sizeGivenName(name) does the round-up arithmetic and always returns a number larger than name.size(). This plus the ASSERT() makes me feel better about it.

mattklein123

LGTM, but would still like a final approval from @mrice32. Thank you!

ambuc · 2018-07-11T17:10:51Z

@mattklein123 @mrice32 I want to write a golden file test for the internal representation (as hex maybe? not sure) of a stat just to be extremely sure that this didn't change. Thoughts on the maintainability of such a test?

mattklein123 · 2018-07-11T17:16:58Z

@mattklein123 @mrice32 I want to write a golden file test for the internal representation (as hex maybe? not sure) of a stat just to be extremely sure that this didn't change. Thoughts on the maintainability of such a test?

As long as it's well commented and someone other than you can understand the test and how to modify it, no objections from me.

mrice32

This is really cool - I think it's simpler and more cohesive now! A few minor comments. I'm still looking through how the various options are set to make sure the behavior doesn't change, but it looks good so far.

mrice32 · 2018-07-11T17:43:07Z

source/common/stats/stats_impl.cc

-  // expensive to performing a map lookup, since both require copying a truncated version of the
-  // string before doing the hash lookup.
+  uint64_t num_bytes_to_allocate = RawStatData::sizeGivenName(name);
+  RawStatData* data = static_cast<RawStatData*>(::calloc(num_bytes_to_allocate, 1));


I know this wasn't being done before, but shouldn't we return early if calloc returns a nullptr so we don't segfault by trying to write to it? If we're not equipped to handle this failure at the callsite, then maybe we should just throw instead of returning nullptr?

mrice32 · 2018-07-11T17:51:49Z

source/common/stats/stats_impl.cc

+  name_[xfer_size] = '\0';
+}
+
+void RawStatData::truncateAndInit(absl::string_view key, const StatsOptions& stats_options) {


There may be a subtlety here that I'm missing here, but can we just truncate the string_view, and pass it to initialize()? It seems like the majority of initialize() and truncateAndInit() are the same.

I actually really like this. I will refactor truncateAndInit() and checkAndInit() to use the same (private) initialize() method under the hood -- this makes the difference in logic very explicit.

mrice32 · 2018-07-11T17:55:06Z

source/common/stats/stats_impl.cc

+  ref_count_ = 1;
+
+  uint64_t xfer_size = key.size();
+  ASSERT(xfer_size <= num_bytes_allocated);


This assert seems strange - isn't num_bytes_allocated the number of bytes allocated to store the entire stat object rather than just the portion reserved for name_?

Oops, you are correct. This should be
ASSERT(sizeof(RawStatData) + xfer_size + 1 <= num_bytes_allocated). I'll change it.

mrice32 · 2018-07-11T19:07:50Z

source/extensions/filters/http/dynamo/dynamo_utility.cc


  std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name);
  // Truncate the table prefix if the current string is too large.
  if (stats_table_prefix.size() > remaining_size) {
    stats_table_prefix = stats_table_prefix.substr(0, remaining_size);
  }
+


Extra line?

mrice32

I've convinced myself that the hot restart layout hasn't changed. Outside of the comments above, LGTM.

mrice32 · 2018-07-11T20:15:06Z

test/common/stats/thread_local_store_test.cc

@@ -37,7 +38,7 @@ class StatsThreadLocalStoreTest : public testing::Test, public RawStatDataAlloca
    }));

    EXPECT_CALL(*this, alloc("stats.overflow"));
-    store_.reset(new ThreadLocalStoreImpl(*this));
+    store_.reset(new ThreadLocalStoreImpl(options_, *this));


Sorry, tossing in a new comment because replies don't seem to show up at the bottom. nit: can we use std::make_unique here?

ambuc · 2018-07-11T21:02:56Z

@mattklein123 See #3843 for the internal memory representation test. I checked locally and the hash doesn't change, but I still want to merge that PR in first as proof.

…ion logic Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

…ntation (#3843) This PR adds a test to ensure that the internal memory representation of a RawStatData hasn't unintentionally changed. We take a hash of a struct by getting the Hex::encode() hex string representation of it, and then taking the HashUtil::xxHash64() hash of it. This will help ensure that #3629 and other PRs which deal with stats don't accidentally change RawStatData. Risk Level: Low Testing: N/A Docs Changes: None Release Notes: None Signed-off-by: James Buckland <jbuckland@google.com>

…ions Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 · 2018-07-12T14:28:49Z

source/common/stats/stats_impl.cc

+
+void RawStatData::checkAndInit(absl::string_view key, uint64_t num_bytes_allocated) {
+  uint64_t xfer_size = key.size();
+  ASSERT(sizeof(RawStatData) + xfer_size + 1 <= num_bytes_allocated);


It seems like we compute the in-memory size of a particular RawStatData often. Do you think it's worth making some utility function that takes the length of the string (without the null terminator) and computes the size so we make sure we do it the same way every time?

I like this note. I did some refactoring work in aeb9251.

…structSizeWithOptions() Signed-off-by: James Buckland <jbuckland@google.com>

mrice32

LGTM other than a little weirdness in ThreadLocalStoreImpl. Thanks for working on this - this is awesome!

mrice32 · 2018-07-12T17:14:14Z

source/common/stats/thread_local_store.cc

@@ -182,7 +183,8 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
    if (stat == nullptr) {
      parent_.num_last_resort_stats_.inc();
      stat =
-          make_stat(parent_.heap_allocator_, name, std::move(tag_extracted_name), std::move(tags));
+          make_stat(parent_.heap_allocator_, name.substr(0, parent_.statsOptions().maxNameLength()),


This seems fishy. Since we now throw if the heap allocator fails to allocate, this statement will never be called in the non-hot restart case. However, it's still an embedded assumption about the specific behavior of all allocators - fixed length if they can fail and non-fixed length if they cannot fail. Is there any problem with changing the hot restart allocator to perform the fallback rather than relying on the caller? Since this is technically correct, I think it's fine to leave it this way for now, but please toss in a TODO to get rid of this assumption.

Added this todo in cb0fd8a.

Signed-off-by: James Buckland <jbuckland@google.com>

…ions

mattklein123 · 2018-07-12T23:42:27Z

source/common/stats/stats_impl.cc

  RawStatData* data = static_cast<RawStatData*>(::calloc(num_bytes_to_allocate, 1));
-  data->initialize(name, num_bytes_to_allocate);
+  if (data == nullptr) {
+    throw EnvoyException("HeapRawStatDataAllocator: unable to allocate a new stat");


This might be caught. Can you throw some other type of exception? (Like whatever type new() throws).

I changed it to std::bad_alloc() in 4cad816.

Signed-off-by: James Buckland <jbuckland@google.com>

…ions Signed-off-by: James Buckland <jbuckland@google.com>

ambuc force-pushed the refactor-StatsOptions branch from e8bf899 to d7ff9ba Compare June 14, 2018 16:32

jmarantz reviewed Jun 14, 2018

View reviewed changes

mrice32 reviewed Jun 14, 2018

View reviewed changes

mattklein123 self-assigned this Jun 19, 2018

ambuc force-pushed the refactor-StatsOptions branch 2 times, most recently from 9daa293 to 1c2620a Compare June 25, 2018 13:36

ambuc changed the title ~~WIP: Refactor Stats::RawStatData into a StatsOptions struct.~~ Refactor Stats::RawStatData into a StatsOptions struct Jun 25, 2018

jmarantz reviewed Jun 25, 2018

View reviewed changes

jmarantz mentioned this pull request Jun 26, 2018

HeapStatData with a distinct allocation mechanism for RawStatData #3710

Merged

jmarantz reviewed Jun 26, 2018

View reviewed changes

This was referenced Jun 26, 2018

test-utillity: Remove test-utility wrapper for Buffer::toString(). #3736

Merged

Remove test-utility wrapper for Buffer::toString(). envoyproxy/envoy-filter-example#51

Merged

ambuc force-pushed the refactor-StatsOptions branch 2 times, most recently from d3e0880 to 2de48fe Compare July 6, 2018 14:10

mattklein123 reviewed Jul 9, 2018

View reviewed changes

ambuc added 8 commits July 10, 2018 09:54

Store StatsOptions values as size_t

7bfa762

Signed-off-by: James Buckland <jbuckland@google.com>

Add heapAllocator stat name truncation in ThreadLocalStore, plus nits

27990bf

Signed-off-by: James Buckland <jbuckland@google.com>

Remove unused BlockMemoryHashSet accessor function

b92ab75

Signed-off-by: James Buckland <jbuckland@google.com>

Rename safeInitialize() to truncateAndInit()

3a7aff1

Signed-off-by: James Buckland <jbuckland@google.com>

Change interface of sizeGivenName()

3f38407

Signed-off-by: James Buckland <jbuckland@google.com>

Use new translateBootstrap interface in v1_to_bootstrap

19f9f3c

Signed-off-by: James Buckland <jbuckland@google.com>

Fix formatting

06bd4c2

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123 reviewed Jul 10, 2018

View reviewed changes

ambuc added 2 commits July 11, 2018 10:12

Edit stats inline documentation

71f6b24

Signed-off-by: James Buckland <jbuckland@google.com>

Provide safe copy assertion in RawStatData::initialize()

2aa9d8c

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123 previously approved these changes Jul 11, 2018

View reviewed changes

mrice32 reviewed Jul 11, 2018

View reviewed changes

ambuc mentioned this pull request Jul 11, 2018

test: add test for consistency of RawStatData internal memory representation #3843

Merged

ambuc added 2 commits July 11, 2018 17:39

Abstract checkAndInit() and truncateAndInit() with shared initializat…

911fc5d

…ion logic Signed-off-by: James Buckland <jbuckland@google.com>

Formatting fixes and nits

dbcdb72

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc dismissed mattklein123’s stale review via dbcdb72 July 11, 2018 21:39

Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…

9a30244

…ions Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 reviewed Jul 12, 2018

View reviewed changes

Refactor sizeGivenName(), sizeGivenStatsOptions() into structSize(), …

aeb9251

…structSizeWithOptions() Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 previously approved these changes Jul 12, 2018

View reviewed changes

Add todo addressing embedded assumption about allocator fallback

cb0fd8a

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc dismissed mrice32’s stale review via cb0fd8a July 12, 2018 17:46

Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…

19e0311

…ions

mattklein123 reviewed Jul 12, 2018

View reviewed changes

ambuc added 3 commits July 13, 2018 10:15

Throw std::bad_alloc() in HeapRawStatDataAllocator::alloc()

4cad816

Signed-off-by: James Buckland <jbuckland@google.com>

Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…

9c6b880

…ions Signed-off-by: James Buckland <jbuckland@google.com>

Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…

c1fa6fb

…ions Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 approved these changes Jul 13, 2018

View reviewed changes

mattklein123 approved these changes Jul 13, 2018

View reviewed changes

mattklein123 merged commit c92a301 into envoyproxy:master Jul 13, 2018

ambuc deleted the refactor-StatsOptions branch July 13, 2018 19:00

Refactor Stats::RawStatData into a StatsOptions struct #3629

Refactor Stats::RawStatData into a StatsOptions struct #3629

Conversation

ambuc commented Jun 14, 2018 • edited Loading

jmarantz Jun 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambuc commented Jun 18, 2018 • edited Loading

ambuc commented Jun 25, 2018

mattklein123 commented Jun 25, 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

ambuc commented Jun 26, 2018

ambuc commented Jun 26, 2018

Choose a reason for hiding this comment

ambuc Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

mattklein123 commented Jul 3, 2018

ambuc commented Jul 6, 2018

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

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

ambuc commented Jul 11, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

ambuc commented Jul 11, 2018

mattklein123 commented Jul 11, 2018

mrice32 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

mrice32 left a comment

Choose a reason for hiding this comment

ambuc commented Jun 14, 2018 •

edited

Loading

jmarantz Jun 14, 2018 •

edited

Loading

ambuc commented Jun 18, 2018 •

edited

Loading

ambuc Jun 26, 2018 •

edited

Loading