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: 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/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/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 b1b1c6ab25637..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,7 +242,7 @@ class IsolatedStoreImpl : public Store { private: struct ScopeImpl : public Scope { ScopeImpl(IsolatedStoreImpl& parent, const std::string& prefix) - : parent_(parent), prefix_(prefix) {} + : parent_(parent), prefix_(Utility::sanitizeStatsName(prefix)) {} // Stats::Scope ScopePtr createScope(const std::string& name) override { diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 22c9cdf27a03b..90b9eddec01f0 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -81,7 +81,7 @@ class ThreadLocalStoreImpl : public StoreRoot { struct ScopeImpl : public Scope { ScopeImpl(ThreadLocalStoreImpl& parent, const std::string& prefix) - : parent_(parent), prefix_(prefix) {} + : parent_(parent), prefix_(Utility::sanitizeStatsName(prefix)) {} ~ScopeImpl(); // Stats::Scope 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 3f54913d708e0..d1f3173240757 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -248,20 +248,29 @@ 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)); + cluster_manager_->clusters() + .find("cluster:name") + ->second.get() + .info() + ->statsScope() + .counter("foo") + .inc(); + EXPECT_EQ(1UL, 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..a4663814d4232 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;