-
Notifications
You must be signed in to change notification settings - Fork 5.5k
stats: Add a new Counter Group metric type #16535
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 12 commits
142f5ff
a458c83
9ceac2f
53c26d6
2f3c7e9
8a1a56c
3699471
ae938ac
ab4bb65
e5f3799
96b7bb5
857a05f
5f66fc4
f07a458
6fb2b39
a1638b2
46a6771
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 |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ using CounterOptConstRef = absl::optional<std::reference_wrapper<const Counter>> | |
| using GaugeOptConstRef = absl::optional<std::reference_wrapper<const Gauge>>; | ||
| using HistogramOptConstRef = absl::optional<std::reference_wrapper<const Histogram>>; | ||
| using TextReadoutOptConstRef = absl::optional<std::reference_wrapper<const TextReadout>>; | ||
| using CounterGroupOptConstRef = absl::optional<std::reference_wrapper<const CounterGroup>>; | ||
| using ScopePtr = std::unique_ptr<Scope>; | ||
| using ScopeSharedPtr = std::shared_ptr<Scope>; | ||
|
|
||
|
|
@@ -178,6 +179,38 @@ class Scope { | |
| */ | ||
| virtual TextReadout& textReadoutFromString(const std::string& name) PURE; | ||
|
|
||
| /** | ||
| * Creates a CounterGroup from the stat name. Tag extraction will be performed on the name. | ||
| * @param name The name of the stat, obtained from the SymbolTable. | ||
| * @param descriptor The size of the group. | ||
| * @return a counter group within the scope's namespace with a particular value type. | ||
| */ | ||
| CounterGroup& counterGroupFromStatName(const StatName& name, | ||
| CounterGroupDescriptorSharedPtr descriptor) { | ||
| return counterGroupFromStatNameWithTags(name, absl::nullopt, descriptor); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a CounterGroup from the stat name and tags. If tags are not provided, tag extraction | ||
| * will be performed on the name. | ||
| * @param name The name of the stat, obtained from the SymbolTable. | ||
| * @param tags optionally specified tags. | ||
| * @param descriptor The size of the group. | ||
| * @return a counter group within the scope's namespace with a particular value type. | ||
| */ | ||
| virtual CounterGroup& | ||
| counterGroupFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags, | ||
| CounterGroupDescriptorSharedPtr descriptor) PURE; | ||
|
|
||
| /** | ||
| * TODO(#6667): this variant is deprecated: use counterGroupFromStatName. | ||
|
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. If it's deprecated can we avoid adding this for new things?
Contributor
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 wondered about this myself. I'm not quite sure how deprecated this variant is... it seems aspirational not actual. In particular, stats_macros.h uses it in the various POOL_*_PREFIX macros. Maybe someone has a plan to get rid of uses this method eventually but hasn't done so yet? I'm just cargo-culting from existing metric code so perhaps someone with more background in the metrics knows more? |
||
| * @param name The name, expressed as a string. | ||
| * @param descriptor The size of the group. | ||
| * @return a counter group within the scope's namespace with a particular value type. | ||
| */ | ||
| virtual CounterGroup& counterGroupFromString(const std::string& name, | ||
| CounterGroupDescriptorSharedPtr descriptor) PURE; | ||
|
|
||
| /** | ||
| * @param The name of the stat, obtained from the SymbolTable. | ||
| * @return a reference to a counter within the scope's namespace, if it exists. | ||
|
|
@@ -203,6 +236,12 @@ class Scope { | |
| */ | ||
| virtual TextReadoutOptConstRef findTextReadout(StatName name) const PURE; | ||
|
|
||
| /** | ||
| * @param The name of the stat, obtained from the SymbolTable. | ||
| * @return a reference to a text readout within the scope's namespace, if it exists. | ||
| */ | ||
| virtual CounterGroupOptConstRef findCounterGroup(StatName name) const PURE; | ||
|
|
||
| /** | ||
| * @return a reference to the symbol table. | ||
| */ | ||
|
|
@@ -249,6 +288,17 @@ class Scope { | |
| * was hit. | ||
| */ | ||
| virtual bool iterate(const IterateFn<TextReadout>& fn) const PURE; | ||
|
|
||
| /** | ||
| * Calls 'fn' for every counter group. Note that in the case of overlapping | ||
| * scopes, the implementation may call fn more than one time for each | ||
| * counter group. Iteration stops if `fn` returns false; | ||
| * | ||
| * @param fn Function to be run for every counter group, or until fn return false. | ||
| * @return false if fn(text_readout) return false during iteration, true if every counter group | ||
| * was hit. | ||
| */ | ||
| virtual bool iterate(const IterateFn<CounterGroup>& fn) const PURE; | ||
| }; | ||
|
|
||
| } // namespace Stats | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -503,7 +503,7 @@ class PrioritySet { | |
| /** | ||
| * All cluster stats. @see stats_macros.h | ||
| */ | ||
| #define ALL_CLUSTER_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, STATNAME) \ | ||
| #define ALL_CLUSTER_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, COUNTER_GROUPS, STATNAME) \ | ||
|
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: IIRC you don't need to add this if there are no COUNTER_GROUPS below, but perhaps this has changed.
Contributor
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. Hm, that doesn't seem to work for me when I removed "COUNTER_GROUP, " from this line: ./envoy/upstream/upstream.h:648:1: error: too many arguments provided to function-like macro invocation In particular, MAKE_STAT_NAMES_STRUCT does: which seems to have 1 argument for each of the different metric types. Is there something different I should be doing? |
||
| COUNTER(assignment_stale) \ | ||
| COUNTER(assignment_timeout_received) \ | ||
| COUNTER(bind_errors) \ | ||
|
|
@@ -595,7 +595,8 @@ class PrioritySet { | |
| * stats sink. See envoy.api.v2.endpoint.ClusterStats for the definition of upstream_rq_dropped. | ||
| * These are latched by LoadStatsReporter, independent of the normal stats sink flushing. | ||
| */ | ||
| #define ALL_CLUSTER_LOAD_REPORT_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, STATNAME) \ | ||
| #define ALL_CLUSTER_LOAD_REPORT_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, COUNTER_GROUP, \ | ||
| STATNAME) \ | ||
| COUNTER(upstream_rq_dropped) | ||
|
|
||
| /** | ||
|
|
@@ -607,7 +608,8 @@ class PrioritySet { | |
| * We also include stat-names in this structure that are used when composing | ||
| * the circuit breaker names, depending on priority settings. | ||
| */ | ||
| #define ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, STATNAME) \ | ||
| #define ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, COUNTER_GROUP, \ | ||
| STATNAME) \ | ||
| GAUGE(cx_open, Accumulate) \ | ||
| GAUGE(cx_pool_open, Accumulate) \ | ||
| GAUGE(rq_open, Accumulate) \ | ||
|
|
@@ -625,7 +627,8 @@ class PrioritySet { | |
| /** | ||
| * All stats tracking request/response headers and body sizes. Not used by default. | ||
| */ | ||
| #define ALL_CLUSTER_REQUEST_RESPONSE_SIZE_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, STATNAME) \ | ||
| #define ALL_CLUSTER_REQUEST_RESPONSE_SIZE_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, \ | ||
| COUNTER_GROUP, STATNAME) \ | ||
| HISTOGRAM(upstream_rq_headers_size, Bytes) \ | ||
| HISTOGRAM(upstream_rq_body_size, Bytes) \ | ||
| HISTOGRAM(upstream_rs_headers_size, Bytes) \ | ||
|
|
@@ -634,7 +637,8 @@ class PrioritySet { | |
| /** | ||
| * All stats around timeout budgets. Not used by default. | ||
| */ | ||
| #define ALL_CLUSTER_TIMEOUT_BUDGET_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, STATNAME) \ | ||
| #define ALL_CLUSTER_TIMEOUT_BUDGET_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, COUNTER_GROUP, \ | ||
| STATNAME) \ | ||
| HISTOGRAM(upstream_rq_timeout_budget_percent_used, Unspecified) \ | ||
| HISTOGRAM(upstream_rq_timeout_budget_per_try_percent_used, Unspecified) | ||
|
|
||
|
|
@@ -666,7 +670,8 @@ MAKE_STATS_STRUCT(ClusterTimeoutBudgetStats, ClusterTimeoutBudgetStatNames, | |
| * Struct definition for cluster circuit breakers stats. @see stats_macros.h | ||
| */ | ||
| struct ClusterCircuitBreakersStats { | ||
| ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(c, GENERATE_GAUGE_STRUCT, h, tr, GENERATE_STATNAME_STRUCT) | ||
| ALL_CLUSTER_CIRCUIT_BREAKERS_STATS(c, GENERATE_GAUGE_STRUCT, h, tr, GENERATE_COUNTER_GROUP_STRUCT, | ||
| GENERATE_STATNAME_STRUCT) | ||
| }; | ||
|
|
||
| using ClusterRequestResponseSizeStatsPtr = std::unique_ptr<ClusterRequestResponseSizeStats>; | ||
|
|
||
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.
Thinking about the usage model, I'm wondering where. the descriptors will come from.
I think it's probably OK to be unopinionated at this level. You could have the caller create a descriptor and then make a CounterGroup from that.
However what we want to avoid is having multiple copies of the same group, or you lose the benefit of memory savings.
If you leave this in the hands of the caller, I'm wondering whether (eg) Quic will have an appropriate context that is instantiated once per process, in which these descriptors can be explicitly instantiated and referenced. That would in some sense be ideal.
However if Quic (or other possible use-cases for this) doesn't have such a context, we might want to delegate the management of the descriptors to the Stats::Allocator. In that case, we could provide Stats::Allocator::makeCounterGroupDescriptor(...) which would take a string-list and return a canonical descriptor for that. That would need to take a lock on a the allocator's descriptor-cache, and hopefully that would not be overly contended.
Maybe this is worth a TODO here in allocator.h? WDYT?
Uh oh!
There was an error while loading. Please reload this page.
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.
My idea was that we'd have a global function like:
This way we allocate a single descriptor which is accessible to any place that want to create a counter group with the same set of suffixes. Does that sound plausible?
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.
My personal preference would be for that to come out of a context. I think also we might want to use StatName rather than string as the representation for the enum names within the descriptor, so that they could be sent to StatName::join() without taking a symbol table lock.
Doing that, though, means you need to pass a
SymbolTable&to the descriptor constructor.And then wouldn't be able to use a static because in tests, symbol-tables would be re-built with every test method, and you wouldn't be able to destruct the symbol-table for one test method while a static held ownership of live StatNames built it.
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.
Interesting. I don't see
StatName::join()in the code, but I did seeSymbolTable::join(). Is that the method you're referring to? Can you say more about where this method is called from? In particular, with this PR theCounterGroup::nameSuffix()currently returns anabsl::string_view. Would it need to return aStatName?