Skip to content
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

rds: add config reload time stat for rds #17033

Merged
merged 12 commits into from
Jun 22, 2021
1 change: 1 addition & 0 deletions docs/root/configuration/overview/mgmt_server.rst
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ The following statistics are generated for all subscriptions.
:widths: 1, 1, 2

config_reload, Counter, Total API fetches that resulted in a config reload due to a different config
config_reload_time, Gauge, Timestamp of the last config reload as milliseconds since the epoch
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config_reload is only applicable/implemented for RDS/SRDS and VHDS - but added here generically. I followed the same pattern and added this new stat here even though it is implemented only for RDS and SRDS. If it is better to move to rds stats page - I can move.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you name this stat config_reload_time_ms?

init_fetch_timeout, Counter, Total :ref:`initial fetch timeouts <envoy_v3_api_field_config.core.v3.ConfigSource.initial_fetch_timeout>`
update_attempt, Counter, Total API fetches attempted
update_success, Counter, Total API fetches completed successfully
Expand Down
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ New Features
* bandwidth_limit: added new :ref:`HTTP bandwidth limit filter <config_http_filters_bandwidth_limit>`.
* bootstrap: added :ref:`dns_resolution_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.dns_resolution_config>` to aggregate all of the DNS resolver configuration in a single message. By setting one such configuration option *no_default_search_domain* as true the DNS resolver will not use the default search domains. And by setting the configuration *resolvers* we can specify the external DNS servers to be used for external DNS query.
* cluster: added :ref:`dns_resolution_config <envoy_v3_api_field_config.cluster.v3.Cluster.dns_resolution_config>` to aggregate all of the DNS resolver configuration in a single message. By setting one such configuration option *no_default_search_domain* as true the DNS resolver will not use the default search domains.
* config: added stat :ref:`config_reload_time <subscription_statistics>`.
* crash support: restore crash context when continuing to processing requests or responses as a result of an asynchronous callback that invokes a filter directly. This is unlike the call stacks that go through the various network layers, to eventually reach the filter. For a concrete example see: ``Envoy::Extensions::HttpFilters::Cache::CacheFilter::getHeaders`` which posts a callback on the dispatcher that will invoke the filter directly.
* dns resolver: added *DnsResolverOptions* protobuf message to reconcile all of the DNS lookup option flags. By setting the configuration option :ref:`use_tcp_for_dns_lookups <envoy_v3_api_field_config.core.v3.DnsResolverOptions.use_tcp_for_dns_lookups>` as true we can make the underlying dns resolver library to make only TCP queries to the DNS servers and by setting the configuration option :ref:`no_default_search_domain <envoy_v3_api_field_config.core.v3.DnsResolverOptions.no_default_search_domain>` as true the DNS resolver library will not use the default search domains.
* dns resolver: added *DnsResolutionConfig* to combine :ref:`dns_resolver_options <envoy_v3_api_field_config.core.v3.DnsResolutionConfig.dns_resolver_options>` and :ref:`resolvers <envoy_v3_api_field_config.core.v3.DnsResolutionConfig.resolvers>` in a single protobuf message. The field *resolvers* can be specified with a list of DNS resolver addresses. If specified, DNS client library will perform resolution via the underlying DNS resolvers. Otherwise, the default system resolvers (e.g., /etc/resolv.conf) will be used.
Expand Down
4 changes: 3 additions & 1 deletion source/common/router/rds_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription(
fmt::format("RdsRouteConfigSubscription local-init-target {}", route_config_name_),
[this]() { subscription_->start({route_config_name_}); }),
local_init_manager_(fmt::format("RDS local-init-manager {}", route_config_name_)),
stat_prefix_(stat_prefix), stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}),
stat_prefix_(stat_prefix),
stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_), POOL_GAUGE(*scope_))}),
route_config_provider_manager_(route_config_provider_manager),
manager_identifier_(manager_identifier), optional_http_filters_(optional_http_filters) {
const auto resource_name = getResourceName();
Expand Down Expand Up @@ -131,6 +132,7 @@ void RdsRouteConfigSubscription::onConfigUpdate(
std::unique_ptr<Cleanup> resume_rds;
if (config_update_info_->onRdsUpdate(route_config, version_info)) {
stats_.config_reload_.inc();
stats_.config_reload_time_.set(DateUtil::nowToMilliseconds(factory_context_.timeSource()));
if (config_update_info_->protobufConfiguration().has_vhds() &&
config_update_info_->vhdsConfigurationChanged()) {
ENVOY_LOG(
Expand Down
5 changes: 3 additions & 2 deletions source/common/router/rds_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,16 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider {
/**
* All RDS stats. @see stats_macros.h
*/
#define ALL_RDS_STATS(COUNTER) \
#define ALL_RDS_STATS(COUNTER, GAUGE) \
COUNTER(config_reload) \
GAUGE(config_reload_time, NeverImport) \
COUNTER(update_empty)

/**
* Struct definition for all RDS stats. @see stats_macros.h
*/
struct RdsStats {
ALL_RDS_STATS(GENERATE_COUNTER_STRUCT)
ALL_RDS_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT)
};

class RdsRouteConfigProviderImpl;
Expand Down
1 change: 1 addition & 0 deletions source/common/router/scoped_rds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
}
stats_.all_scopes_.set(scoped_route_map_.size());
stats_.config_reload_.inc();
stats_.config_reload_time_.set(DateUtil::nowToMilliseconds(factory_context_.timeSource()));
}

void ScopedRdsConfigSubscription::onRdsConfigUpdate(const std::string& scope_name,
Expand Down
1 change: 1 addition & 0 deletions source/common/router/scoped_rds.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class InlineScopedRoutesConfigProvider : public Envoy::Config::ImmutableConfigPr
// clang-format off
#define ALL_SCOPED_RDS_STATS(COUNTER, GAUGE) \
COUNTER(config_reload) \
GAUGE(config_reload_time, NeverImport) \
COUNTER(update_empty) \
GAUGE(all_scopes, Accumulate) \
GAUGE(on_demand_scopes, Accumulate) \
Expand Down
4 changes: 4 additions & 0 deletions test/common/router/rds_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,10 @@ TEST_F(RdsImplTest, Basic) {
// Old config use count should be 1 now.
EXPECT_EQ(1, config.use_count());
EXPECT_EQ(2UL, scope_.counter("foo.rds.foo_route_config.config_reload").value());
EXPECT_NE(0, scope_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this flake if the test runs in <1ms?

If you are just trying to ensure that the gauge exists, you can use findGaugeByString, which is on class TestStore, which is the base class of MockIsolatedStatsStore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.. Will change to ensure gauge exists.

.gauge("foo.rds.foo_route_config.config_reload_time",
Stats::Gauge::ImportMode::NeverImport)
.value());
}

// validate there will be exception throw when unknown factory found for per virtualhost typed
Expand Down
4 changes: 4 additions & 0 deletions test/common/router/scoped_rds_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,10 @@ route_configuration_name: foo_routes
EXPECT_EQ(2UL,
server_factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload")
.value());
EXPECT_NE(0, server_factory_context_.scope_
.gauge("foo.scoped_rds.foo_scoped_routes.config_reload_time",
Stats::Gauge::ImportMode::NeverImport)
.value());
// now scope key "x-bar-key" points to nowhere.
EXPECT_THAT(getScopedRdsProvider()->config<ScopedConfigImpl>()->getRouteConfig(
TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}),
Expand Down