From bf363554934b2e6e3c0565f4bd2115f36c5b6503 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 3 Oct 2017 20:38:14 -0400 Subject: [PATCH 1/7] Replace ':' with '_' in stats cluster name. Signed-off-by: Henna Huang --- source/common/json/config_schemas.cc | 1 - source/common/upstream/upstream_impl.cc | 8 +++++++- source/common/upstream/upstream_impl.h | 1 + test/common/upstream/cluster_manager_impl_test.cc | 14 ++++++++------ 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index d97c587db3b5f..421c26edda600 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -1391,7 +1391,6 @@ const std::string Json::Schema::CLUSTER_SCHEMA(R"EOF( "properties" : { "name" : { "type" : "string", - "pattern" : "^[^:]+$", "minLength" : 1, "maxLength" : 60 }, diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index cd70bdc9d7716..f8df975f14c39 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -70,6 +70,12 @@ ClusterStats ClusterInfoImpl::generateStats(Stats::Scope& scope) { return {ALL_CLUSTER_STATS(POOL_COUNTER(scope), POOL_GAUGE(scope), POOL_TIMER(scope))}; } +std::string ClusterInfoImpl::clusterStatsName(const std::string& cluster_name) { + std::string stats_name = fmt::format("cluster.{}.", cluster_name); + std::replace(stats_name.begin(), stats_name.end(), ':', '_'); + return stats_name; +} + ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, const Network::Address::InstanceConstSharedPtr source_address, Runtime::Loader& runtime, Stats::Store& stats, @@ -81,7 +87,7 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, connect_timeout))), per_connection_buffer_limit_bytes_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)), - stats_scope_(stats.createScope(fmt::format("cluster.{}.", name_))), + stats_scope_(stats.createScope(clusterStatsName(name_))), stats_(generateStats(*stats_scope_)), features_(parseFeatures(config)), http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())), resource_managers_(config, runtime, name_), diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index a4563a02f118d..71482fb48d748 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -250,6 +250,7 @@ class ClusterInfoImpl : public ClusterInfo { }; static uint64_t parseFeatures(const envoy::api::v2::Cluster& config); + std::string clusterStatsName(const std::string& cluster_name); Runtime::Loader& runtime_; const std::string name_; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 3f54913d708e0..572269084af40 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -248,20 +248,22 @@ TEST_F(ClusterManagerImplTest, MaxClusterName) { "document key: #/name"); } -TEST_F(ClusterManagerImplTest, InvalidClusterNameChars) { +TEST_F(ClusterManagerImplTest, ValidClusterName) { const std::string json = R"EOF( { "clusters": [ { - "name": "cluster:" + "name": "cluster:name", + "connect_timeout_ms": 250, + "type": "static", + "lb_type": "round_robin", + "hosts": [{"url": "tcp://127.0.0.1:11001"}] }] } )EOF"; - EXPECT_THROW_WITH_MESSAGE(create(parseBootstrapFromJson(json)), Json::Exception, - "JSON at lines 4-6 does not conform to schema.\n Invalid schema: " - "#/properties/name\n Schema violation: pattern\n Offending document " - "key: #/name"); + create(parseBootstrapFromJson(json)); + EXPECT_EQ(0, factory_.stats_.counter("cluster.cluster_name.lb_local_cluster_not_ok").value()); } TEST_F(ClusterManagerImplTest, OriginalDstLbRestriction) { From ba3f83b1381f4d012f9a330d39718d71ae556680 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Tue, 3 Oct 2017 20:49:37 -0400 Subject: [PATCH 2/7] Fix docs Signed-off-by: Henna Huang --- docs/configuration/cluster_manager/cluster.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/configuration/cluster_manager/cluster.rst b/docs/configuration/cluster_manager/cluster.rst index 906af8aaaae4a..253b9a5d53db0 100644 --- a/docs/configuration/cluster_manager/cluster.rst +++ b/docs/configuration/cluster_manager/cluster.rst @@ -31,7 +31,7 @@ Cluster name *(required, string)* Supplies the name of the cluster which must be unique across all clusters. The cluster name is used when emitting :ref:`statistics `. - The cluster name can be at most 60 characters long, and must **not** contain ``:``. + The cluster name can be at most 60 characters long. .. _config_cluster_manager_type: From 4b853dd7fa4cca24162c33eab40d66e4675a4263 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 4 Oct 2017 14:27:29 -0400 Subject: [PATCH 3/7] Move character replacement to ScopeImpl Signed-off-by: Henna Huang --- docs/configuration/cluster_manager/cluster_manager.rst | 2 +- docs/configuration/listeners/listeners.rst | 2 +- source/common/stats/stats_impl.h | 10 +++++++++- source/common/stats/thread_local_store.h | 9 ++++++++- source/common/upstream/upstream_impl.cc | 8 +------- source/common/upstream/upstream_impl.h | 1 - source/server/listener_manager_impl.cc | 7 ++----- test/common/upstream/cluster_manager_impl_test.cc | 2 +- 8 files changed, 23 insertions(+), 18 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster_manager.rst b/docs/configuration/cluster_manager/cluster_manager.rst index 3e9870640b56d..dcd7075cc671e 100644 --- a/docs/configuration/cluster_manager/cluster_manager.rst +++ b/docs/configuration/cluster_manager/cluster_manager.rst @@ -55,7 +55,7 @@ Statistics ---------- The cluster manager has a statistics tree rooted at *cluster_manager.* with the following -statistics: +statistics. Any ``:`` character in the stats name is replaced with ``_``. .. csv-table:: :header: Name, Type, Description diff --git a/docs/configuration/listeners/listeners.rst b/docs/configuration/listeners/listeners.rst index 1673d9604ea68..9170a0ea00072 100644 --- a/docs/configuration/listeners/listeners.rst +++ b/docs/configuration/listeners/listeners.rst @@ -81,7 +81,7 @@ Statistics ---------- The listener manager has a statistics tree rooted at *listener_manager.* with the following -statistics: +statistics. Any ``:`` character in the stats name is replaced with ``_``. .. csv-table:: :header: Name, Type, Description diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index b1b1c6ab25637..f9d22e501c6f4 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -217,6 +217,7 @@ class IsolatedStoreImpl : public Store { // Stats::Scope Counter& counter(const std::string& name) override { return counters_.get(name); } + ScopePtr createScope(const std::string& name) override { return ScopePtr{new ScopeImpl(*this, name)}; } @@ -232,12 +233,19 @@ class IsolatedStoreImpl : public Store { private: struct ScopeImpl : public Scope { ScopeImpl(IsolatedStoreImpl& parent, const std::string& prefix) - : parent_(parent), prefix_(prefix) {} + : parent_(parent), prefix_(sanitizeStatsName(prefix)) {} // Stats::Scope ScopePtr createScope(const std::string& name) override { return ScopePtr{new ScopeImpl(parent_, prefix_ + name)}; } + // ':' is a reserved char in statsd. Do the translation here and in + // ThreadLocalStoreImpl::ScopeImpl. + std::string sanitizeStatsName(const std::string& name) { + std::string stats_name = name; + std::replace(stats_name.begin(), stats_name.end(), ':', '_'); + return stats_name; + } void deliverHistogramToSinks(const std::string&, uint64_t) override {} void deliverTimingToSinks(const std::string&, std::chrono::milliseconds) override {} Counter& counter(const std::string& name) override { return parent_.counter(prefix_ + name); } diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 22c9cdf27a03b..20ac5dfc4476b 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -81,8 +81,15 @@ class ThreadLocalStoreImpl : public StoreRoot { struct ScopeImpl : public Scope { ScopeImpl(ThreadLocalStoreImpl& parent, const std::string& prefix) - : parent_(parent), prefix_(prefix) {} + : parent_(parent), prefix_(sanitizeStatsName(prefix)) {} ~ScopeImpl(); + // ':' is a reserved char in statsd. Do the translation here and in + // IsolatedStoreImpl::ScopeImpl. + std::string sanitizeStatsName(const std::string& name) { + std::string stats_name = name; + std::replace(stats_name.begin(), stats_name.end(), ':', '_'); + return stats_name; + } // Stats::Scope Counter& counter(const std::string& name) override; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index f8df975f14c39..cd70bdc9d7716 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -70,12 +70,6 @@ ClusterStats ClusterInfoImpl::generateStats(Stats::Scope& scope) { return {ALL_CLUSTER_STATS(POOL_COUNTER(scope), POOL_GAUGE(scope), POOL_TIMER(scope))}; } -std::string ClusterInfoImpl::clusterStatsName(const std::string& cluster_name) { - std::string stats_name = fmt::format("cluster.{}.", cluster_name); - std::replace(stats_name.begin(), stats_name.end(), ':', '_'); - return stats_name; -} - ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, const Network::Address::InstanceConstSharedPtr source_address, Runtime::Loader& runtime, Stats::Store& stats, @@ -87,7 +81,7 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, connect_timeout))), per_connection_buffer_limit_bytes_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)), - stats_scope_(stats.createScope(clusterStatsName(name_))), + stats_scope_(stats.createScope(fmt::format("cluster.{}.", name_))), stats_(generateStats(*stats_scope_)), features_(parseFeatures(config)), http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())), resource_managers_(config, runtime, name_), diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 71482fb48d748..a4563a02f118d 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -250,7 +250,6 @@ class ClusterInfoImpl : public ClusterInfo { }; static uint64_t parseFeatures(const envoy::api::v2::Cluster& config); - std::string clusterStatsName(const std::string& cluster_name); Runtime::Loader& runtime_; const std::string name_; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index efcf86fdbab9f..49c5c3ecbcb3a 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -91,11 +91,8 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManag ASSERT(config.filter_chains().size() == 1); const auto& filter_chain = config.filter_chains()[0]; - // ':' is a reserved char in statsd. Do the translation here to avoid costly inline translations - // later. - std::string final_stat_name = fmt::format("listener.{}.", address_->asString()); - std::replace(final_stat_name.begin(), final_stat_name.end(), ':', '_'); - listener_scope_ = parent_.server_.stats().createScope(final_stat_name); + listener_scope_ = + parent_.server_.stats().createScope(fmt::format("listener.{}.", address_->asString())); if (filter_chain.has_tls_context()) { Ssl::ServerContextConfigImpl context_config(filter_chain.tls_context()); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 572269084af40..9fe4cce33e6f2 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -263,7 +263,7 @@ TEST_F(ClusterManagerImplTest, ValidClusterName) { )EOF"; create(parseBootstrapFromJson(json)); - EXPECT_EQ(0, factory_.stats_.counter("cluster.cluster_name.lb_local_cluster_not_ok").value()); + EXPECT_EQ(0, factory_.stats_.counter("cluster.cluster_name.upstream_rq_5xx").value()); } TEST_F(ClusterManagerImplTest, OriginalDstLbRestriction) { From 17760f678ff8db22e268d595ec2d7bf49e31d081 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 4 Oct 2017 14:30:38 -0400 Subject: [PATCH 4/7] Format fix Signed-off-by: Henna Huang --- source/common/stats/stats_impl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index f9d22e501c6f4..4c3bedeab0a4c 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -217,7 +217,6 @@ class IsolatedStoreImpl : public Store { // Stats::Scope Counter& counter(const std::string& name) override { return counters_.get(name); } - ScopePtr createScope(const std::string& name) override { return ScopePtr{new ScopeImpl(*this, name)}; } From 98e30fc77059793c1a3c8c0bc5ff9e13c98447d9 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 4 Oct 2017 16:15:05 -0400 Subject: [PATCH 5/7] Add common utility function and update tests. Signed-off-by: Henna Huang --- source/common/stats/stats_impl.cc | 6 ++++++ source/common/stats/stats_impl.h | 19 +++++++++++-------- source/common/stats/thread_local_store.h | 9 +-------- .../upstream/cluster_manager_impl_test.cc | 9 ++++++++- test/server/listener_manager_impl_test.cc | 15 +++++++++++++++ 5 files changed, 41 insertions(+), 17 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index aa07373366c73..731b99bce2bb5 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -10,6 +10,12 @@ namespace Envoy { namespace Stats { +std::string Utility::sanitizeStatsName(const std::string& name) { + std::string stats_name = name; + std::replace(stats_name.begin(), stats_name.end(), ':', '_'); + return stats_name; +} + void TimerImpl::TimespanImpl::complete(const std::string& dynamic_name) { std::chrono::milliseconds ms = std::chrono::duration_cast( std::chrono::steady_clock::now() - start_); diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 4c3bedeab0a4c..f0f31552390fb 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -17,6 +17,16 @@ namespace Envoy { namespace Stats { +/** + * Common stats utility routines. + */ +class Utility { +public: + // ':' is a reserved char in statsd. Do a character replacement to avoid costly inline + // translations later. + static std::string sanitizeStatsName(const std::string& name); +}; + /** * This structure is the backing memory for both CounterImpl and GaugeImpl. It is designed so that * it can be allocated from shared memory if needed. @@ -232,19 +242,12 @@ class IsolatedStoreImpl : public Store { private: struct ScopeImpl : public Scope { ScopeImpl(IsolatedStoreImpl& parent, const std::string& prefix) - : parent_(parent), prefix_(sanitizeStatsName(prefix)) {} + : parent_(parent), prefix_(Utility::sanitizeStatsName(prefix)) {} // Stats::Scope ScopePtr createScope(const std::string& name) override { return ScopePtr{new ScopeImpl(parent_, prefix_ + name)}; } - // ':' is a reserved char in statsd. Do the translation here and in - // ThreadLocalStoreImpl::ScopeImpl. - std::string sanitizeStatsName(const std::string& name) { - std::string stats_name = name; - std::replace(stats_name.begin(), stats_name.end(), ':', '_'); - return stats_name; - } void deliverHistogramToSinks(const std::string&, uint64_t) override {} void deliverTimingToSinks(const std::string&, std::chrono::milliseconds) override {} Counter& counter(const std::string& name) override { return parent_.counter(prefix_ + name); } diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 20ac5dfc4476b..90b9eddec01f0 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -81,15 +81,8 @@ class ThreadLocalStoreImpl : public StoreRoot { struct ScopeImpl : public Scope { ScopeImpl(ThreadLocalStoreImpl& parent, const std::string& prefix) - : parent_(parent), prefix_(sanitizeStatsName(prefix)) {} + : parent_(parent), prefix_(Utility::sanitizeStatsName(prefix)) {} ~ScopeImpl(); - // ':' is a reserved char in statsd. Do the translation here and in - // IsolatedStoreImpl::ScopeImpl. - std::string sanitizeStatsName(const std::string& name) { - std::string stats_name = name; - std::replace(stats_name.begin(), stats_name.end(), ':', '_'); - return stats_name; - } // Stats::Scope Counter& counter(const std::string& name) override; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 9fe4cce33e6f2..14b4c669b7d26 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -263,7 +263,14 @@ TEST_F(ClusterManagerImplTest, ValidClusterName) { )EOF"; create(parseBootstrapFromJson(json)); - EXPECT_EQ(0, factory_.stats_.counter("cluster.cluster_name.upstream_rq_5xx").value()); + cluster_manager_->clusters() + .find("cluster:name") + ->second.get() + .info() + ->statsScope() + .counter("foo") + .inc(); + EXPECT_EQ(1, factory_.stats_.counter("cluster.cluster_name.foo").value()); } TEST_F(ClusterManagerImplTest, OriginalDstLbRestriction) { diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index a1ce57aefce09..f1cea01a85daa 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -648,6 +648,21 @@ TEST_F(ListenerManagerImplTest, AddListenerFailure) { EXPECT_EQ(1UL, server_.stats_store_.counter("listener_manager.listener_create_failure").value()); } +TEST_F(ListenerManagerImplTest, StatsNameValidCharacterTest) { + const std::string json = R"EOF( + { + "address": "tcp://[::1]:10000", + "filters": [] + "bind_to_port": false, + } + )EOF"; + + manager_->addOrUpdateListener(parseListenerFromJson(json)); + manager_->listeners().front().get().listenerScope().counter("foo").inc(); + + EXPECT_EQ(1UL, server_.stats_store_.counter("listener.[__1]_10000.foo").value()); +} + TEST_F(ListenerManagerImplTest, DuplicateAddressDontBind) { InSequence s; From 3267af432c20fb2be52efdd3bc7058dba81a7057 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 4 Oct 2017 16:22:50 -0400 Subject: [PATCH 6/7] nit Signed-off-by: Henna Huang --- test/common/upstream/cluster_manager_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 14b4c669b7d26..d1f3173240757 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -270,7 +270,7 @@ TEST_F(ClusterManagerImplTest, ValidClusterName) { ->statsScope() .counter("foo") .inc(); - EXPECT_EQ(1, factory_.stats_.counter("cluster.cluster_name.foo").value()); + EXPECT_EQ(1UL, factory_.stats_.counter("cluster.cluster_name.foo").value()); } TEST_F(ClusterManagerImplTest, OriginalDstLbRestriction) { From 7cb6a76e62b94f02e5ddc3734b98824ec0a41f08 Mon Sep 17 00:00:00 2001 From: Henna Huang Date: Wed, 4 Oct 2017 16:26:59 -0400 Subject: [PATCH 7/7] fix Signed-off-by: Henna Huang --- test/server/listener_manager_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index f1cea01a85daa..a4663814d4232 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -652,8 +652,8 @@ TEST_F(ListenerManagerImplTest, StatsNameValidCharacterTest) { const std::string json = R"EOF( { "address": "tcp://[::1]:10000", - "filters": [] - "bind_to_port": false, + "filters": [], + "bind_to_port": false } )EOF";