circuit breaker: track remaining resource count by spenceral · Pull Request #6269 · envoyproxy/envoy

spenceral · 2019-03-12T17:39:41Z

Description: Adds additional cluster stats to track the resources remaining until circuit breakers open.
Risk Level: Low
Testing: Added unit tests to verify gauges are set correctly.
Docs Changes: Added note about new stats in circuit breaking doc and documented them in cluster stats.
Release Notes: Added
Fixes #6163

Should these gauges be optional based on a config flag?

This commit adds cluster stats tracking the resources remaining until a circuit breaker opens. Signed-off-by: Spencer Lewis <[email protected]>

Signed-off-by: Spencer Lewis <[email protected]>

snowp · 2019-03-12T17:52:33Z

This adds 8 stats per cluster (4 for DEFAULT, 4 for HIGH priority) which will be used by everyone, which seems like it might warrant a config flag to avoid increasing memory usage per cluster. WDYT @htuch @jmarantz ?

Signed-off-by: Spencer Lewis <[email protected]>

venilnoronha · 2019-03-12T18:02:50Z

@snowp I think it's possible to configure stats via the StatsMatcher. So, we may not actually need a config flag.

Signed-off-by: Spencer Lewis <[email protected]>

snowp

Thanks, looks good for the most part

docs/root/configuration/cluster_manager/cluster_stats.rst

source/common/upstream/resource_manager_impl.h

test/common/http/http1/conn_pool_test.cc

snowp · 2019-03-12T20:17:28Z

test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc

+    // to the resource max
    for (const Stats::GaugeSharedPtr& gauge : host_->cluster_.stats_store_.gauges()) {
-      EXPECT_EQ(0U, gauge->value());
+      if (!absl::StrContains(gauge->name(), "circuit_breakers")) {


I think this whitelisting is fine, thought it might make sense at one point to add a initialValue() function to Gauge so that these checks can just be EXPECT_EQ(gauge->initialValue(), gauge->value()), but that would arguably fall out of the scope of this PR

I will defer to your (or anyone else's) judgment on whether or not to make that change here. It makes sense to me.

Signed-off-by: Spencer Lewis <[email protected]>

snowp

One comment but otherwise LGTM

source/common/upstream/resource_manager_impl.h

jmarantz · 2019-03-13T14:29:13Z

@snowp Thanks for the heads up. Yeah, adding 8 additional stats per cluster will, by default, increase the per-cluster cost by probably over 1k bytes, which is significant It might not be reasonable to expect high-cluster-cardinality sites that care about memory to explicitly construct efficient stats-matcher expressions to exclude the ones they don't want.

Also note that stats-matcher isn't free. Currently it adds CPU overhead to every stat lookup, and after #6207 is merged (if its merged :), it will not add CPU overhead, but it will add some smaller memory overhead for excluded stats that we hold in a name-cache.

If we can register the stats only if actually useful or explicitly enabled, that might be better.

snowp · 2019-03-13T15:22:55Z

@jmarantz Thanks for chiming in, sounds like this should be guarded by a config flag (since circuit breakers cant be turned off themselves).

From what I can tell to make registration optional we'd avoid calling gauge(std::string) on the StatsStore and add a nullGauge() function to it so we can point to a null gauge instead?

Signed-off-by: Spencer Lewis <[email protected]>

* master: (59 commits) http fault: add response rate limit injection (envoyproxy#6267) xds: introduce initial_fetch_timeout option to limit initialization time (envoyproxy#6048) test: fix cpuset-threads tests (envoyproxy#6278) server: add an API for registering for notifications for server instance life… (envoyproxy#6254) remove remains of TestBase (envoyproxy#6286) dubbo_proxy: Implement the routing of Dubbo requests (envoyproxy#5973) Revert "stats: add new BoolIndicator stat type (envoyproxy#5813)" (envoyproxy#6280) runtime: codifying runtime guarded features (envoyproxy#6134) mysql_filter: fix integration test flakes (envoyproxy#6272) tls: update BoringSSL to debed9a4 (3683). (envoyproxy#6273) rewrite buffer implementation to eliminate evbuffer dependency (envoyproxy#5441) Remove the dependency from TimeSystem to libevent by using the Event::Scheduler abstraction as a delegate. (envoyproxy#6240) fuzz: fix use of literal in default initialization. (envoyproxy#6268) http: add HCM functionality required for rate limiting (envoyproxy#6242) Disable mysql_integration_test until it is deflaked. (envoyproxy#6250) test: use ipv6_only IPv6 addresses in custom cluster integration tests. (envoyproxy#6260) tracing: If parent span is propagated with empty string, it causes th… (envoyproxy#6263) upstream: fix oss-fuzz issue envoyproxy#11095. (envoyproxy#6220) Wire up panic mode subset to receive updates (envoyproxy#6221) docs: clarify xds docs with warming information (envoyproxy#6236) ...

Signed-off-by: Spencer Lewis <[email protected]>

snowp

Great, I think this looks good. @jmarantz Mind looking over the stats related changes?

jmarantz

Thanks for setting up this limitation, and for being explicit in the doc, etc.

mattklein123

Thanks looks awesome. 1 small test comment.

/wait

mattklein123 · 2019-03-20T18:55:16Z

test/common/http/http1/conn_pool_test.cc

+    // gauges which default to the resource max.
    for (const Stats::GaugeSharedPtr& gauge : cluster_->stats_store_.gauges()) {
-      EXPECT_EQ(0U, gauge->value());
+      if (!absl::StrContains(gauge->name(), "remaining")) {


This seems a little fragile to me. Is it possible to do a regex match here which matches on "circuit_breaker.*.remaining"? Same below? Also given this pattern has been copy/pasted into a bunch of different tests, maybe move into a utility function in case we need to do anything like this in the future?

Sure thing, I gave this a shot. Thanks for taking a look.

Signed-off-by: Spencer Lewis <[email protected]>

mattklein123

Thank you!

spenceral added 2 commits March 12, 2019 10:30

circuit breaker: track remaining resource count

681b177

This commit adds cluster stats tracking the resources remaining until a circuit breaker opens. Signed-off-by: Spencer Lewis <[email protected]>

fix code format

af44636

Signed-off-by: Spencer Lewis <[email protected]>

type out decrementing to pass spellcheck

708e6e4

Signed-off-by: Spencer Lewis <[email protected]>

fix tests

b7592b6

Signed-off-by: Spencer Lewis <[email protected]>

dnoe assigned snowp Mar 12, 2019

snowp suggested changes Mar 12, 2019

View reviewed changes

address snowp's pr comments

b29f4a0

Signed-off-by: Spencer Lewis <[email protected]>

snowp suggested changes Mar 13, 2019

View reviewed changes

source/common/upstream/resource_manager_impl.h Outdated Show resolved Hide resolved

copy current resource value

337f335

Signed-off-by: Spencer Lewis <[email protected]>

klarose mentioned this pull request Mar 15, 2019

upstream: allow configuration of connection pool limits #6298

Merged

spenceral added 4 commits March 15, 2019 14:34

toggle new gauges based on circuit breaker proto

28f47d5

Signed-off-by: Spencer Lewis <[email protected]>

implement nullGauge in tls

e9c8b5b

Signed-off-by: Spencer Lewis <[email protected]>

fix test

741295d

Signed-off-by: Spencer Lewis <[email protected]>

snowp previously approved these changes Mar 20, 2019

View reviewed changes

jmarantz previously approved these changes Mar 20, 2019

View reviewed changes

snowp assigned mattklein123 Mar 20, 2019

mattklein123 requested changes Mar 20, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 20, 2019

add utility func to check gauges zeroed

40fa909

Signed-off-by: Spencer Lewis <[email protected]>

spenceral dismissed stale reviews from jmarantz and snowp via 40fa909 March 21, 2019 02:14

repokitteh-read-only bot removed the waiting label Mar 21, 2019

mattklein123 approved these changes Mar 21, 2019

View reviewed changes

snowp merged commit 0cd4054 into envoyproxy:master Mar 21, 2019

spenceral deleted the addl-cb-stats branch March 21, 2019 16:53

ramaraochavali mentioned this pull request Oct 23, 2025

disable track remaining circuit breaker metrics by default istio/istio#58070

Merged

16 tasks

Conversation

spenceral commented Mar 12, 2019

Uh oh!

snowp commented Mar 12, 2019

Uh oh!

venilnoronha commented Mar 12, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowp Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

spenceral Mar 12, 2019 • 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!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snowp commented Mar 13, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

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

mattklein123 Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

spenceral Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spenceral Mar 12, 2019 •

edited

Loading

jmarantz commented Mar 13, 2019 •

edited

Loading