Update stats interface in preparation for tags #1803

mrice32 · 2017-10-03T21:47:21Z

Tags will require all stats have non-trivial state associated with them. This enforces that all stats are represented as objects before being exported. As a part of this addition, there are a few structural changes to the way stats work in Envoy:

The Metric calss has been added as a base class for all stats objects. This will contain identifying information.
The Histogram class has replaced the Timer class. This object represents histograms of any data that can be represented by uint64_t. It also contains a method type() to identify the type of data represented (this includes Duration).
Timespan has been moved into an independent class, which will be created with a reference Histogram.
HISTOGRAM macros have been added to create Integer histogram objects.

This is the first stage of what was previously #1751 with some modifications based on the discussion in that PR.

Signed-off-by: Matt Rice <mattrice@google.com>

htuch

Generally looks great. Main question is why we need to have histogram aware of the underlying type, seems TimeSpan provides the abstraction needed already.

htuch · 2017-10-04T23:07:16Z

include/envoy/stats/stats.h

@@ -109,12 +108,12 @@ class Sink {
  /**
   * Flush a counter delta.
   */
-  virtual void flushCounter(const std::string& name, uint64_t delta) PURE;
+  virtual void flushCounter(const Metric& counter, uint64_t delta) PURE;


Shouldn't this be Counter (and Gauge) below?

htuch · 2017-10-04T23:08:59Z

include/envoy/stats/stats.h

+
+  enum ValueType {
+    Integer,
+    Duration,


What value does this subtyping provide? Would it be possible to treat everything as just integers at the interface level (and for time, it's just # of ms in the implementation already).

FWIW this would be fine for Lyft, my concern is for implementation where somehow a histogram is different from a timer. I'm not sure if that is a real issue or not... Let's discuss tomorrow.

htuch · 2017-10-04T23:09:40Z

include/envoy/stats/stats_macros.h

@@ -28,15 +28,18 @@ namespace Envoy {

 #define GENERATE_COUNTER_STRUCT(NAME) Stats::Counter& NAME##_;
 #define GENERATE_GAUGE_STRUCT(NAME) Stats::Gauge& NAME##_;
-#define GENERATE_TIMER_STRUCT(NAME) Stats::Timer& NAME##_;
+#define GENERATE_TIMER_STRUCT(NAME) Stats::Histogram& NAME##_;


Can we get rid of this distinction in this file and just switch to pure histogram?

I think this depends on if we decide to treat them differently from the user's perspective as discussed above.

htuch · 2017-10-04T23:10:27Z

include/envoy/stats/timespan.h

+   * Complete the timespan and send the time to the histogram.
+   */
+  virtual void complete() {
+    histogram_.recordValue(std::chrono::duration_cast<std::chrono::milliseconds>(


Should this stuff be in the source/common implementation rather than interface?

As was discussed in #1751, if we move this to source/common everything that wants to create a Timespan having to depend on some stats implementation lib rather than just the set of stats interfaces (everything else in stats currently works this way outside of the creation of the store). The implementation of this class is relatively small. I'm cool changing it if we're okay with those tradeoffs.

htuch · 2017-10-04T23:11:02Z

include/envoy/stats/timespan.h

+   */
+  virtual void complete() {
+    histogram_.recordValue(std::chrono::duration_cast<std::chrono::milliseconds>(
+                               std::chrono::steady_clock::now() - start_)


Use getRawDuration() here?

Good point. Updated.

Signed-off-by: Matt Rice <mattrice@google.com>

mrice32 · 2017-10-05T21:05:05Z

@htuch @mattklein123 Updated in line with our discussion earlier today. I added a small blurb to the docs and added a slightly longer comment above the histogram class. Let me know if you think more explanation's needed.

mattklein123

Thanks for doing this super tedious change. This is great. Few minor comments.

mattklein123 · 2017-10-05T22:06:27Z

include/envoy/stats/timespan.h

+namespace Stats {
+
+/**
+ * An individual timespan that flushes its measured value to a Duration histogram. The initial time


nit: I would probably remove "Duration"

mattklein123 · 2017-10-05T22:06:52Z

include/envoy/stats/timespan.h

+  Timespan(Histogram& histogram)
+      : histogram_(histogram), start_(std::chrono::steady_clock::now()) {}
+
+  virtual ~Timespan() {}


nit: not needed

Good point. Removed it and made all the methods non-virtual since nothing's inheriting from it.

mattklein123 · 2017-10-05T22:10:04Z

source/common/http/user_agent.cc

@@ -12,11 +12,11 @@ namespace Envoy {
 namespace Http {

 void UserAgent::completeConnectionLength(Stats::Timespan& span) {
-  if (!stats_) {
+  if (!stats_ || !scope_) {


nit: I think scope_ must be not nullptr if stats is not nullptr?

Yup, I was being a little conservative in case the implementation changes below, but I'm fine leaving it out of the if if that's not a concern.

Removed it and added a quick comment noting the expectation.

Signed-off-by: Matt Rice <mattrice@google.com>

mattklein123

LGTM, thanks. Will defer to @htuch for any further comments.

Before: $ bazel-bin/src/envoy/envoy --version bazel-bin/src/envoy/envoy version: 0/1.8.0-dev//DEBUG After: $ bazel-bin/src/envoy/envoy --version bazel-bin/src/envoy/envoy version: f315a32fc7c6f727fc9645cc1ca27d4160c1d0e0/1.8.0-dev/Clean/DEBUG Fixes envoyproxy#1803. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

…#1803) Description: Move main_ios_dist in artifacts workflow to Engflow remote execution and improve build times by ~2x Risk Level: Low Testing: See artifacts check Docs Changes: Release Notes: Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com> Signed-off-by: JP Simard <jp@jpsim.com>

mrice32 added 4 commits October 3, 2017 17:14

Stats updates in preparation for tags

74df4f9

Signed-off-by: Matt Rice <mattrice@google.com>

incorporate mattklein123 comments

574b6e7

Signed-off-by: Matt Rice <mattrice@google.com>

Fixed a comment

1325d7a

Signed-off-by: Matt Rice <mattrice@google.com>

slightly modified comment to force a CI retry

b9dee1b

Signed-off-by: Matt Rice <mattrice@google.com>

mrice32 force-pushed the stats_interface_update branch from ca45ce9 to b9dee1b Compare October 4, 2017 13:58

htuch reviewed Oct 4, 2017

View reviewed changes

mrice32 added 5 commits October 5, 2017 10:35

Sinks take gauges and counters rather than metric

01d69ff

Signed-off-by: Matt Rice <mattrice@google.com>

Reuse function in timespan

8254b5e

Signed-off-by: Matt Rice <mattrice@google.com>

Merge branch 'master' into stats_interface_update

bee9a07

Updated comment typo

437097e

Signed-off-by: Matt Rice <mattrice@google.com>

Removed typing of histograms and documented the decision

700297e

Signed-off-by: Matt Rice <mattrice@google.com>

mattklein123 reviewed Oct 5, 2017

View reviewed changes

mrice32 added 2 commits October 5, 2017 18:25

Corrected a few nits in Timespan and made methods non-virtual

a75d749

Signed-off-by: Matt Rice <mattrice@google.com>

Removed scope_ check and added comment

5bf6334

Signed-off-by: Matt Rice <mattrice@google.com>

mattklein123 approved these changes Oct 5, 2017

View reviewed changes

htuch approved these changes Oct 6, 2017

View reviewed changes

htuch merged commit d1431cf into envoyproxy:master Oct 6, 2017

mrice32 deleted the stats_interface_update branch October 6, 2017 14:14

ambuc mentioned this pull request Aug 17, 2018

stats: refactoring MetricImpl without strings #4190

Merged

Update stats interface in preparation for tags #1803

Update stats interface in preparation for tags #1803

Uh oh!

Conversation

mrice32 commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch 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!

mrice32 Oct 5, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mrice32 Oct 5, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrice32 commented Oct 3, 2017 •

edited

Loading

mrice32 Oct 5, 2017 •

edited

Loading

mrice32 commented Oct 5, 2017 •

edited

Loading

mrice32 Oct 5, 2017 •

edited

Loading