stats: refactor to enable non-hot-restart stats to have distinct representation from hot-restart stats. by jmarantz · Pull Request #3606 · envoyproxy/envoy

jmarantz · 2018-06-12T19:14:49Z

Description:

Currently the presence of hot_restart as an option requires that all stats to be allocated in block of memory pre-allocated at runtime. This may present scalability challenges in some environments for a couple of reasons:

Committing to the maximum number & size of stats on startup
Difficulty of employing complex structures such as reference-counted symbol tables to share common substrings
This PR does some refactoring to make it easier to create an alternate stats representation for the non-hot-restart case. Note that it does not actually do that: just makes it easier by virtualizing makeCounter and makeGauge.

See #3585 for more discussion.

Risk Level: Medium

Testing: //test/...

…network issues when running under docker. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…istinct representations ot be created for non-hot-restart case. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…-alloc case. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-06-13T00:11:20Z

Ready for a look I think.

jmarantz · 2018-06-13T12:39:28Z

@ambuc

…alStore::counter() and gauge(). Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-06-15T02:15:24Z

@ggreenway do you have time to take a look?

mrice32

Looks good on first pass! A few comments.

mrice32 · 2018-06-15T15:03:45Z

include/envoy/stats/stats.h

+   * @return Counter* a counter, or nullptr if allocation failed.
   */
-  virtual RawStatData* alloc(const std::string& name) PURE;
+  virtual Counter* makeCounter(const std::string& name, std::string& tag_extracted_name,


It seems that we removed the free(), but we're still returning raw pointers. Can you document the ownership model? How is the StatDataAllocator informed of when the stat is no longer needed by the caller? Is the user allowed to just delete the raw pointer?

Very good point here, and I think my design here needs a bit of rethinking in the context of how a caller would free stuff. The other comments I'll address later, and I'm flipping back to WIP-mode while I think about this :)

Actually this was easy to deal with by just returning a shared_ptr at this level.

mrice32 · 2018-06-15T15:19:21Z

include/envoy/stats/stats.h

+ * consumption.
 */
-class RawStatDataAllocator {
+class StatDataAllocator {


It seems like this is a general stat factory class that determines the impl for each type of stat (and how it is allocated). If it is able to choose the representation for gauges and counters, why not also allow it to make histograms?

added TODO -- making a parallel path for histograms would require moving the histogram impl out of thread_local_store and into stats_impl, which seems OK to me, but not in this PR. @ramaraochavali @mattklein123 WDYT?

@jmarantz originally I have added it to Stats_impl only. But later moved to thread_local_store based on @mattklein123 suggestion. I think it is ok to bring it back to stats impl.

mrice32 · 2018-06-15T15:24:59Z

source/common/stats/thread_local_store.h

+    // result, creating it with the heap allocator.
+    template <class StatType>
+    StatType&
+    safeMakeStat(const std::string& name,


Can we add more documentation here about each argument. I'm only asking because the signature is reasonably complicated.

OK I left some. I'm not sure it's super-illuminating as the semantics around tls_are rather complex :)

mrice32 · 2018-06-15T15:26:43Z

source/common/stats/thread_local_store.cc

    return **tls_ref;
  }
-
-  // We must now look in the central store so we must be locked. We grab a reference to the


Why did we remove this comment?

good catch; replaced.

mrice32 · 2018-06-15T15:29:04Z

include/envoy/stats/stats.h


 /**
- * Abstract interface for allocating a RawStatData.
+ * Abstract interface for allocating statistics. Alternate


nit: remove Alternate since the interface doesn't impose a default.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

ggreenway · 2018-06-15T23:22:12Z

I am swamped right now; not sure that I'll have time to review.

mrice32

LGTM other than one minor comment about the interface.

mrice32 · 2018-06-18T18:05:28Z

include/envoy/stats/stats.h

+   * @return Counter* a counter, or nullptr if allocation failed.
   */
-  virtual RawStatData* alloc(const std::string& name) PURE;
+  virtual CounterSharedPtr makeCounter(const std::string& name, std::string& tag_extracted_name,


tag_extracted_name and tags are passed as references not so they can be modified and read by the caller, but so that they can be moved. I think a better way to communicate this in the interface is by making them rvalue references (for example: std::string&& tag_extracted_name), which will force all callers to wrap them in std::move() or pass an actual rvalue.

Also, the interface should document that if the returned CounterSharedPtr is nullptr, then these objects will not be moved/modified (we assume this at the callsite).

Similar comments for makeGauge().

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mrice32 · 2018-06-18T18:30:40Z

include/envoy/stats/stats.h

+   * @return Counter* a gauge, or nullptr if allocation failed.
   */
-  virtual void free(RawStatData& data) PURE;
+  virtual GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name,


Please also document when these these rvalue references will be moved and when they will not (if allocation fails, they will not be moved, so that they may be used again by the caller).

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

One tiny nit. Sorry I thought I could power through this but this is a morning review. Will do first thing.

mattklein123 · 2018-06-19T00:03:14Z

include/envoy/stats/stats.h

+   * @param name the full name of the stat.
+   * @param tag_extracted_name the name of the stat with tag-values stripped out.
+   * @param tags the extracted tag values.
+   * @return Counter* a counter, or nullptr if allocation failed, in which case tag_extracted_name


nit: doc comment out of date, same below

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mrice32

Sweet! LGTM

mattklein123

Looks great. Nice cleanup. One question on readability.

mattklein123 · 2018-06-19T22:28:46Z

source/common/stats/thread_local_store.h

    Histogram& tlsHistogram(const std::string& name, ParentHistogramImpl& parent) override;

+    template <class StatType>
+    using MakeStatFn = std::shared_ptr<StatType> (StatDataAllocator::*)(


nit: This type of function pointer syntax took me by surprise and I suspect most people are not familiar with it. Is there any more readable way of doing this? Potentially a normal lambda that is passed an instance of StatDataAllocator? This is not a big deal, just throwing it out there.

I can use std::function but I would have to make static method intermediates which I think is less readable.

In the past I have added macros for defining and calling pointer to member functions, which helps a bit I think. That's my preferred option. WDYT?

There is also std::invoke, which I had never used before. But I wasn't about to get that to with and I didn't think it was all that readable either.

If you're going to use pointer to member function, I think macros are the preferred method for readability.

Why would you need static intermediates to use std::function? Can't you capture 'this' in the closure?

You're right; I don't need a static intermediary, but I think the way the code is structured I do need an intermediate closure, because I'm not starting with 'this', I'm starting with two alternate allocators. I do have that working -- it's a little messy IMO -- and I'll push that for reference, and then I'll push the version where I use pointer to member functions, but via macros, and I'm fine with either of those.

…ction. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ros to aid readability. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-06-20T02:08:43Z

OK ptal at the two options:

Use std::function (requires an inline intermediary): 3c75f26

Use pointer-to-member function, with macros to aid readability (current state of PR).

I'm happy to go with either.

mattklein123 · 2018-06-20T04:13:01Z

I have a slight preference for the std::function version, but I think either is a big improvement. @ggreenway any preference?

ggreenway · 2018-06-20T16:23:30Z

I also have a slight preference for the std::function version. I think fewer people will be confused by it; pointer to member function is an infrequently used language feature.

…with macros to aid readability." This reverts commit b9eee85. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-06-20T16:32:55Z

std::function version is fine with me, that is now the state of this PR.

jmarantz added 7 commits June 12, 2018 14:43

Don't checkout code when just formatting. For some users this causes …

8a7c8c8

…network issues when running under docker. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Adds new virtual interfaces for makeCounter and makeGauge, to allow d…

a024c86

…istinct representations ot be created for non-hot-restart case. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add some comments.

a1172d2

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Revert some of the refactorings which were not correct for the failed…

0553dbe

…-alloc case. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove an unused atomic counter I had added earlier.

a6c5aef

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into virtualize-make-stats

84b7ced

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Fill out some comments.

7bb9aea

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~WIP: stats: refactor to enable non-hot-restart stats to have distinct representation from hot-restart stats.~~ stats: refactor to enable non-hot-restart stats to have distinct representation from hot-restart stats. Jun 13, 2018

Use a template member function to factor out common bits of ThreadLoc…

95ee08d

…alStore::counter() and gauge(). Signed-off-by: Joshua Marantz <jmarantz@google.com>

mrice32 reviewed Jun 15, 2018

View reviewed changes

jmarantz changed the title ~~stats: refactor to enable non-hot-restart stats to have distinct representation from hot-restart stats.~~ WIP stats: refactor to enable non-hot-restart stats to have distinct representation from hot-restart stats. Jun 15, 2018

Addressed Matt Rice's review comments.

45231c9

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~WIP stats: refactor to enable non-hot-restart stats to have distinct representation from hot-restart stats.~~ stats: refactor to enable non-hot-restart stats to have distinct representation from hot-restart stats. Jun 15, 2018

ambuc mentioned this pull request Jun 18, 2018

Refactor Stats::RawStatData into a StatsOptions struct #3629

Merged

mrice32 previously approved these changes Jun 18, 2018

View reviewed changes

Use lvalue refs for makeCounter and makeGuard.

a2ffc3b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed mrice32’s stale review via a2ffc3b June 18, 2018 18:26

mrice32 reviewed Jun 18, 2018

View reviewed changes

document that the moving doesn't actually occur if alloc fails.

219deee

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 reviewed Jun 19, 2018

View reviewed changes

mattklein123 self-assigned this Jun 19, 2018

Correct doxygen return-types.

ef3190d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mrice32 previously approved these changes Jun 19, 2018

View reviewed changes

mattklein123 reviewed Jun 19, 2018

View reviewed changes

Trial balloon of using std::function in lieu of pointer to member fun…

3c75f26

…ction. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed mrice32’s stale review via 3c75f26 June 20, 2018 01:42

Revert makeStats helper to a pointer-to-member function, but with mac…

b9eee85

…ros to aid readability. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Revert "Revert makeStats helper to a pointer-to-member function, but …

d5100c4

…with macros to aid readability." This reverts commit b9eee85. Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 approved these changes Jun 20, 2018

View reviewed changes

mattklein123 merged commit b34adb5 into envoyproxy:master Jun 20, 2018

jmarantz deleted the virtualize-make-stats branch June 20, 2018 18:09

Conversation

jmarantz commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Jun 13, 2018

Uh oh!

jmarantz commented Jun 13, 2018

Uh oh!

jmarantz commented Jun 15, 2018

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Jun 15, 2018

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jun 20, 2018

Uh oh!

mattklein123 commented Jun 20, 2018

Uh oh!

ggreenway commented Jun 20, 2018

Uh oh!

jmarantz commented Jun 20, 2018

Uh oh!

Reviewers

jmarantz commented Jun 12, 2018 •

edited

Loading

mrice32 Jun 18, 2018 •

edited

Loading