-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Update stats interface in preparation for tags #1803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
74df4f9
574b6e7
1325d7a
b9dee1b
01d69ff
8254b5e
bee9a07
437097e
700297e
a75d749
5bf6334
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,79 +19,78 @@ class Instance; | |
|
|
||
| namespace Stats { | ||
|
|
||
| /** | ||
| * General interface for all stats objects. | ||
| */ | ||
| class Metric { | ||
| public: | ||
| virtual ~Metric() {} | ||
| /** | ||
| * Returns the full name of the Metric. | ||
| */ | ||
| virtual const std::string& name() const PURE; | ||
| }; | ||
|
|
||
| /** | ||
| * An always incrementing counter with latching capability. Each increment is added both to a | ||
| * global counter as well as periodic counter. Calling latch() returns the periodic counter and | ||
| * clears it. | ||
| */ | ||
| class Counter { | ||
| class Counter : public virtual Metric { | ||
| public: | ||
| virtual ~Counter() {} | ||
| virtual void add(uint64_t amount) PURE; | ||
| virtual void inc() PURE; | ||
| virtual uint64_t latch() PURE; | ||
| virtual std::string name() PURE; | ||
| virtual void reset() PURE; | ||
| virtual bool used() PURE; | ||
| virtual uint64_t value() PURE; | ||
| virtual bool used() const PURE; | ||
| virtual uint64_t value() const PURE; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<Counter> CounterSharedPtr; | ||
|
|
||
| /** | ||
| * A gauge that can both increment and decrement. | ||
| */ | ||
| class Gauge { | ||
| class Gauge : public virtual Metric { | ||
| public: | ||
| virtual ~Gauge() {} | ||
|
|
||
| virtual void add(uint64_t amount) PURE; | ||
| virtual void dec() PURE; | ||
| virtual void inc() PURE; | ||
| virtual std::string name() PURE; | ||
| virtual void set(uint64_t value) PURE; | ||
| virtual void sub(uint64_t amount) PURE; | ||
| virtual bool used() PURE; | ||
| virtual uint64_t value() PURE; | ||
| virtual bool used() const PURE; | ||
| virtual uint64_t value() const PURE; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<Gauge> GaugeSharedPtr; | ||
|
|
||
| /** | ||
| * An individual timespan that is owned by a timer. The initial time is captured on construction. | ||
| * A timespan must be completed via complete() for it to be stored. If the timespan is deleted | ||
| * this will be treated as a cancellation. | ||
| * A histogram that records values one at a time. | ||
| */ | ||
| class Timespan { | ||
| class Histogram : public virtual Metric { | ||
| public: | ||
| virtual ~Timespan() {} | ||
| virtual ~Histogram() {} | ||
|
|
||
| enum ValueType { | ||
| Integer, | ||
| Duration, | ||
| }; | ||
|
|
||
| /** | ||
| * Complete the span using the default name of the timer that the span was allocated from. | ||
| * Informs the user how the values should be interpreted. | ||
| */ | ||
| virtual void complete() PURE; | ||
| virtual ValueType type() const PURE; | ||
|
|
||
| /** | ||
| * Complete the span using a dynamic name. This is useful if a span needs to get counted | ||
| * against a timer with a dynamic name. | ||
| * Records an unsigned value. If a timer, values are in units of milliseconds. | ||
| */ | ||
| virtual void complete(const std::string& dynamic_name) PURE; | ||
| virtual void recordValue(uint64_t value) PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<Timespan> TimespanPtr; | ||
|
|
||
| /** | ||
| * A timer that can capture timespans. | ||
| */ | ||
| class Timer { | ||
| public: | ||
| virtual ~Timer() {} | ||
|
|
||
| virtual TimespanPtr allocateSpan() PURE; | ||
| virtual std::string name() PURE; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<Timer> TimerSharedPtr; | ||
| typedef std::shared_ptr<Histogram> HistogramSharedPtr; | ||
|
|
||
| /** | ||
| * A sink for stats. Each sink is responsible for writing stats to a backing store. | ||
|
|
@@ -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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be |
||
|
|
||
| /** | ||
| * Flush a gauge value. | ||
| */ | ||
| virtual void flushGauge(const std::string& name, uint64_t value) PURE; | ||
| virtual void flushGauge(const Metric& gauge, uint64_t value) PURE; | ||
|
|
||
| /** | ||
| * This will be called after beginFlush(), some number of flushCounter(), and some number of | ||
|
|
@@ -125,12 +124,7 @@ class Sink { | |
| /** | ||
| * Flush a histogram value. | ||
| */ | ||
| virtual void onHistogramComplete(const std::string& name, uint64_t value) PURE; | ||
|
|
||
| /** | ||
| * Flush a timespan value. | ||
| */ | ||
| virtual void onTimespanComplete(const std::string& name, std::chrono::milliseconds ms) PURE; | ||
| virtual void onHistogramComplete(const Histogram& histogram, uint64_t value) PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<Sink> SinkPtr; | ||
|
|
@@ -157,12 +151,7 @@ class Scope { | |
| /** | ||
| * Deliver an individual histogram value to all registered sinks. | ||
| */ | ||
| virtual void deliverHistogramToSinks(const std::string& name, uint64_t value) PURE; | ||
|
|
||
| /** | ||
| * Deliver an individual timespan completion to all registered sinks. | ||
| */ | ||
| virtual void deliverTimingToSinks(const std::string& name, std::chrono::milliseconds ms) PURE; | ||
| virtual void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) PURE; | ||
|
|
||
| /** | ||
| * @return a counter within the scope's namespace. | ||
|
|
@@ -175,9 +164,9 @@ class Scope { | |
| virtual Gauge& gauge(const std::string& name) PURE; | ||
|
|
||
| /** | ||
| * @return a timer within the scope's namespace. | ||
| * @return a histogram within the scope's namespace with a particular value type. | ||
| */ | ||
| virtual Timer& timer(const std::string& name) PURE; | ||
| virtual Histogram& histogram(Histogram::ValueType type, const std::string& name) PURE; | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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##_; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get rid of this distinction in this file and just switch to pure histogram?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this depends on if we decide to treat them differently from the user's perspective as discussed above. |
||
| #define GENERATE_HISTOGRAM_STRUCT(NAME) Stats::Histogram& NAME##_; | ||
|
|
||
| #define FINISH_STAT_DECL_(X) + std::string(#X)), | ||
|
|
||
| #define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(PREFIX FINISH_STAT_DECL_ | ||
| #define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(PREFIX FINISH_STAT_DECL_ | ||
| #define POOL_TIMER_PREFIX(POOL, PREFIX) (POOL).timer(PREFIX FINISH_STAT_DECL_ | ||
| #define POOL_TIMER_PREFIX(POOL, PREFIX) (POOL).histogram(Stats::Histogram::ValueType::Duration, PREFIX FINISH_STAT_DECL_ | ||
| #define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(Stats::Histogram::ValueType::Integer, PREFIX FINISH_STAT_DECL_ | ||
|
|
||
| #define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "") | ||
| #define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "") | ||
| #define POOL_TIMER(POOL) POOL_TIMER_PREFIX(POOL, "") | ||
| #define POOL_HISTOGRAM(POOL) POOL_HISTOGRAM_PREFIX(POOL, "") | ||
| } // Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| #pragma once | ||
|
|
||
| #include <chrono> | ||
|
|
||
| #include "envoy/common/exception.h" | ||
| #include "envoy/common/time.h" | ||
| #include "envoy/stats/stats.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Stats { | ||
|
|
||
| /** | ||
| * An individual timespan that flushes its measured value to a Duration histogram. The initial time | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would probably remove "Duration" |
||
| * is captured on construction. A timespan must be completed via complete() for it to be stored. If | ||
| * the timespan is deleted this will be treated as a cancellation. | ||
| */ | ||
| class Timespan { | ||
| public: | ||
| Timespan(Histogram& histogram) : histogram_(histogram), start_(std::chrono::steady_clock::now()) { | ||
| if (histogram.type() != Histogram::ValueType::Duration) { | ||
| throw EnvoyException("Cannot intialize a timespan with a non-time valued histogram"); | ||
| } | ||
| } | ||
|
|
||
| virtual ~Timespan() {} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: not needed
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Removed it and made all the methods non-virtual since nothing's inheriting from it. |
||
|
|
||
| /** | ||
| * Complete the timespan and send the time to the histogram. | ||
| */ | ||
| virtual void complete() { | ||
| histogram_.recordValue(std::chrono::duration_cast<std::chrono::milliseconds>( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this stuff be in the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As was discussed in #1751, if we move this to |
||
| std::chrono::steady_clock::now() - start_) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Updated. |
||
| .count()); | ||
| } | ||
|
|
||
| /** | ||
| * Get duration since the creation of the span. | ||
| */ | ||
| virtual std::chrono::milliseconds getRawDuration() { | ||
| return std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - | ||
| start_); | ||
| } | ||
|
|
||
| private: | ||
| Histogram& histogram_; | ||
| const MonotonicTime start_; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<Timespan> TimespanPtr; | ||
|
|
||
| } // namespace Stats | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.