stats: Factor out creation of cluster-stats StatNames from creation of the stats, to save CPU during xDS updates #14028
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| #include "envoy/singleton/manager.h" | ||
| #include "envoy/ssl/context_manager.h" | ||
| #include "envoy/stats/store.h" | ||
| #include "envoy/stats/symbol_table.h" | ||
| #include "envoy/tcp/conn_pool.h" | ||
| #include "envoy/thread_local/thread_local.h" | ||
| #include "envoy/upstream/health_checker.h" | ||
|
|
@@ -35,6 +36,11 @@ | |
| #include "absl/container/node_hash_map.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| namespace Router { | ||
| struct RouterStatNames; | ||
| } | ||
|
|
||
| namespace Upstream { | ||
|
|
||
| /** | ||
|
|
@@ -300,6 +306,8 @@ class CdsApi { | |
|
|
||
| using CdsApiPtr = std::unique_ptr<CdsApi>; | ||
|
|
||
| struct ClusterManagerStatNames; | ||
|
|
||
| /** | ||
| * Factory for objects needed during cluster manager operation. | ||
| */ | ||
|
|
@@ -350,6 +358,26 @@ class ClusterManagerFactory { | |
| * Returns the secret manager. | ||
| */ | ||
| virtual Secret::SecretManager& secretManager() PURE; | ||
|
|
||
| /** | ||
| * Returns a struct with all the Stats::StatName objects needed by | ||
| * ClusterManager. This helps factor out some relatively heavy name | ||
| * construction which occur when there is a large CDS update during operation, | ||
| * relative to recreating all stats from strings on-the-fly. | ||
|
Member
There was a problem hiding this comment. Wouldn't these be Cluster/upstream stats and not cluster manager stats which I think would be global and created once?
Contributor
Author
There was a problem hiding this comment. I reverted the ClusterManager stats to their previous state. |
||
| * | ||
| * @return the stat names. | ||
| */ | ||
| virtual const ClusterManagerStatNames& clusterManagerStatNames() const PURE; | ||
|
|
||
| /** | ||
| * Returns a struct with all the Stats::StatName objects needed by the | ||
| * router. This helps factor out some relatively heavy name construction which | ||
| * occurs when there is a large CDS update during operation, relative to | ||
| * recreating all stats from strings on-the-fly. | ||
| * | ||
| * @return the stat names. | ||
| */ | ||
| virtual const Router::RouterStatNames& routerStatNames() const PURE; | ||
|
Contributor
Author
There was a problem hiding this comment. we hang the routerStatNames off the clusterManagerFactory because we happen to have that available in the place where we construct the router's FilterConfig. It's arguably not too clean. I could also just add a RouterContext and plumb that through the chain of constructors.
Member
There was a problem hiding this comment. From a cleanliness/tech debt perspective, we are trying to remove protocol specific stuff from cluster manager (@alyssawilk just started on some of this in terms of moving hard coded HTTP logic out to extensions). It seems to me the best way to do this would be create a singleton somewhere where the router config is created? Then it would just be created once and lookup?
Contributor
Author
There was a problem hiding this comment. I think I have a better place to put it: the Http::Context. I thought of that because of your comment about 'hard coded HTTP logic". Does that seem reasonable? And I have an Http::Context available to me where I needed the router stat names without excess plumbing.
Member
There was a problem hiding this comment. This is very router filter specific though. Can't it just be a singleton provided by the router filter factory?
Contributor
Author
There was a problem hiding this comment. Per slack, this is not the router filter, it was in source/common/router. I earlier had this PR in a state where I had put the names needed for Router::FilterStats into Http::Context, and that was pretty straightforward, but wasn't great for separation. Now I have a Router::Context. Plumbing that through was trivial but involved a lot of file edits due to parallel universes for mocks, validation servers, integration test servers, and whatnot. LMK what you think; I'm OK trying this out as a prod singleton too, but I think it would involve a few implicit dependencies that are now explicit (if verbose). For example in every test method we'd need to re-init the singleton with the test's symbol table.
Member
There was a problem hiding this comment. Yeah sorry I had forgotten the AsyncClient angle to this. Given that this is basically part of the core I'm fine putting it wherever you think is easiest.
Contributor
Author
There was a problem hiding this comment. I think it's a bit cleaner to have the router stats in their won context, though it expanded the number of files in the PR a bit. |
||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| #include "envoy/stats/stats_macros.h" | ||
|
|
||
| #pragma once | ||
|
|
||
| namespace Envoy { | ||
| namespace Router { | ||
|
|
||
| /** | ||
| * All router filter stats. @see stats_macros.h | ||
| */ | ||
| #define ALL_ROUTER_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT, STATNAME) \ | ||
| COUNTER(no_cluster) \ | ||
| COUNTER(no_route) \ | ||
| COUNTER(passthrough_internal_redirect_bad_location) \ | ||
| COUNTER(passthrough_internal_redirect_no_route) \ | ||
| COUNTER(passthrough_internal_redirect_predicate) \ | ||
| COUNTER(passthrough_internal_redirect_too_many_redirects) \ | ||
| COUNTER(passthrough_internal_redirect_unsafe_scheme) \ | ||
| COUNTER(rq_direct_response) \ | ||
| COUNTER(rq_redirect) \ | ||
| COUNTER(rq_reset_after_downstream_response_started) \ | ||
| COUNTER(rq_total) \ | ||
| STATNAME(retry) | ||
|
|
||
| MAKE_STAT_NAMES_STRUCT(RouterStatNames, ALL_ROUTER_STATS); | ||
|
|
||
| } // namespace Router | ||
| } // namespace Envoy |
Uh oh!
There was an error while loading. Please reload this page.