From 55fd426c10082c4b4c1ad1babe22cc3c7254e9e0 Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Sun, 8 Apr 2018 21:34:14 -0500 Subject: [PATCH 01/10] stats: Update createStatsSink to pass in prefix to sink constructor Signed-off-by: Akhil Thampy --- source/extensions/stat_sinks/statsd/config.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/stat_sinks/statsd/config.cc b/source/extensions/stat_sinks/statsd/config.cc index 5fa1ebbd51fe9..eb7fe0c3f496c 100644 --- a/source/extensions/stat_sinks/statsd/config.cc +++ b/source/extensions/stat_sinks/statsd/config.cc @@ -25,13 +25,13 @@ Stats::SinkPtr StatsdSinkFactory::createStatsSink(const Protobuf::Message& confi Network::Address::resolveProtoAddress(statsd_sink.address()); ENVOY_LOG(debug, "statsd UDP ip address: {}", address->asString()); return std::make_unique(server.threadLocal(), std::move(address), - false); + false, statsd_sink.prefix()); } case envoy::config::metrics::v2::StatsdSink::kTcpClusterName: ENVOY_LOG(debug, "statsd TCP cluster: {}", statsd_sink.tcp_cluster_name()); return std::make_unique( server.localInfo(), statsd_sink.tcp_cluster_name(), server.threadLocal(), - server.clusterManager(), server.stats()); + server.clusterManager(), server.stats(), statsd_sink.prefix()); default: // Verified by schema. NOT_REACHED; From 7c51e0c14df85d30791680be66b0134ba24ca7a2 Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Sun, 8 Apr 2018 21:40:02 -0500 Subject: [PATCH 02/10] stats: add support for custom prefixes in statsd sinks Signed-off-by: Akhil Thampy --- .../stat_sinks/common/statsd/statsd.cc | 35 +++++++++++++------ .../stat_sinks/common/statsd/statsd.h | 17 ++++++--- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 4f30a049443df..786571b3aa78f 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -38,28 +38,34 @@ void Writer::write(const std::string& message) { } UdpStatsdSink::UdpStatsdSink(ThreadLocal::SlotAllocator& tls, - Network::Address::InstanceConstSharedPtr address, const bool use_tag) + Network::Address::InstanceConstSharedPtr address, const bool use_tag, + const std::string _prefix) : tls_(tls.allocateSlot()), server_address_(std::move(address)), use_tag_(use_tag) { tls_->set([this](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(this->server_address_); }); + if (_prefix.size() == 0) { + prefix = Statsd::DEFAULT_PREFIX; + } else { + prefix = _prefix; + } } void UdpStatsdSink::flushCounter(const Stats::Counter& counter, uint64_t delta) { const std::string message( - fmt::format("envoy.{}:{}|c{}", getName(counter), delta, buildTagStr(counter.tags()))); + fmt::format("{}.{}:{}|c{}", prefix, getName(counter), delta, buildTagStr(counter.tags()))); tls_->getTyped().write(message); } void UdpStatsdSink::flushGauge(const Stats::Gauge& gauge, uint64_t value) { - const std::string message( - fmt::format("envoy.{}:{}|g{}", getName(gauge), value, buildTagStr(gauge.tags()))); + const std::string message(fmt::format("{}.{}:{}|g{}", prefix, getName(gauge), value, + buildTagStr(gauge.tags()))); tls_->getTyped().write(message); } void UdpStatsdSink::onHistogramComplete(const Stats::Histogram& histogram, uint64_t value) { // For statsd histograms are all timers. - const std::string message(fmt::format("envoy.{}:{}|ms{}", getName(histogram), + const std::string message(fmt::format("{}.{}:{}|ms{}", prefix, getName(histogram), std::chrono::milliseconds(value).count(), buildTagStr(histogram.tags()))); tls_->getTyped().write(message); @@ -86,11 +92,10 @@ const std::string UdpStatsdSink::buildTagStr(const std::vector& tags return "|#" + StringUtil::join(tag_strings, ","); } -char TcpStatsdSink::STAT_PREFIX[] = "envoy."; - TcpStatsdSink::TcpStatsdSink(const LocalInfo::LocalInfo& local_info, const std::string& cluster_name, ThreadLocal::SlotAllocator& tls, - Upstream::ClusterManager& cluster_manager, Stats::Scope& scope) + Upstream::ClusterManager& cluster_manager, Stats::Scope& scope, + const std::string _prefix) : tls_(tls.allocateSlot()), cluster_manager_(cluster_manager), cx_overflow_stat_(scope.counter("statsd.cx_overflow")) { @@ -100,6 +105,11 @@ TcpStatsdSink::TcpStatsdSink(const LocalInfo::LocalInfo& local_info, tls_->set([this](Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(*this, dispatcher); }); + if (_prefix.size() == 0) { + prefix = Statsd::DEFAULT_PREFIX; + } else { + prefix = _prefix; + } } TcpStatsdSink::TlsSink::TlsSink(TcpStatsdSink& parent, Event::Dispatcher& dispatcher) @@ -135,8 +145,10 @@ void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value // This written this way for maximum perf since with a large number of stats and at a high flush // rate this can become expensive. const char* snapped_current = current_slice_mem_; - memcpy(current_slice_mem_, STAT_PREFIX, sizeof(STAT_PREFIX) - 1); - current_slice_mem_ += sizeof(STAT_PREFIX) - 1; + memcpy(current_slice_mem_, parent_.getPrefix().c_str(), parent_.getPrefix().size()); + current_slice_mem_ += parent_.getPrefix().size(); + *current_slice_mem_ = '.'; + current_slice_mem_ += 1; memcpy(current_slice_mem_, name.c_str(), name.size()); current_slice_mem_ += name.size(); *current_slice_mem_++ = ':'; @@ -178,7 +190,8 @@ void TcpStatsdSink::TlsSink::onTimespanComplete(const std::string& name, // Ultimately it would be nice to perf optimize this path also, but it's not very frequent. It's // also currently not possible that this interleaves with any counter/gauge flushing. ASSERT(current_slice_mem_ == nullptr); - Buffer::OwnedImpl buffer(fmt::format("envoy.{}:{}|ms\n", name, ms.count())); + Buffer::OwnedImpl buffer( + fmt::format("{}.{}:{}|ms\n", parent_.getPrefix().c_str(), name, ms.count())); write(buffer); } diff --git a/source/extensions/stat_sinks/common/statsd/statsd.h b/source/extensions/stat_sinks/common/statsd/statsd.h index 78f0a76fc9a7d..a350c2b3d3622 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.h +++ b/source/extensions/stat_sinks/common/statsd/statsd.h @@ -14,6 +14,7 @@ namespace StatSinks { namespace Common { namespace Statsd { +static const std::string DEFAULT_PREFIX = "envoy"; /** * This is a simple UDP localhost writer for statsd messages. */ @@ -38,13 +39,16 @@ class Writer : public ThreadLocal::ThreadLocalObject { class UdpStatsdSink : public Stats::Sink { public: UdpStatsdSink(ThreadLocal::SlotAllocator& tls, Network::Address::InstanceConstSharedPtr address, - const bool use_tag); + const bool use_tag, const std::string _prefix = DEFAULT_PREFIX); // For testing. UdpStatsdSink(ThreadLocal::SlotAllocator& tls, const std::shared_ptr& writer, - const bool use_tag) + const bool use_tag, const std::string _prefix = DEFAULT_PREFIX) : tls_(tls.allocateSlot()), use_tag_(use_tag) { tls_->set( [writer](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return writer; }); + if (_prefix.size() != 0) { + prefix = _prefix; + } } // Stats::Sink @@ -57,6 +61,7 @@ class UdpStatsdSink : public Stats::Sink { // Called in unit test to validate writer construction and address. int getFdForTests() { return tls_->getTyped().getFdForTests(); } bool getUseTagForTest() { return use_tag_; } + std::string getPrefix() { return prefix; } private: const std::string getName(const Stats::Metric& metric); @@ -65,6 +70,8 @@ class UdpStatsdSink : public Stats::Sink { ThreadLocal::SlotPtr tls_; Network::Address::InstanceConstSharedPtr server_address_; const bool use_tag_; + // Prefix for all flushed stats. + std::string prefix; }; /** @@ -74,7 +81,7 @@ class TcpStatsdSink : public Stats::Sink { public: TcpStatsdSink(const LocalInfo::LocalInfo& local_info, const std::string& cluster_name, ThreadLocal::SlotAllocator& tls, Upstream::ClusterManager& cluster_manager, - Stats::Scope& scope); + Stats::Scope& scope, std::string _prefix = DEFAULT_PREFIX); // Stats::Sink void beginFlush() override { tls_->getTyped().beginFlush(true); } @@ -95,6 +102,8 @@ class TcpStatsdSink : public Stats::Sink { std::chrono::milliseconds(value)); } + std::string getPrefix() { return prefix; } + private: struct TlsSink : public ThreadLocal::ThreadLocalObject, public Network::ConnectionCallbacks { TlsSink(TcpStatsdSink& parent, Event::Dispatcher& dispatcher); @@ -130,7 +139,7 @@ class TcpStatsdSink : public Stats::Sink { static constexpr uint32_t FLUSH_SLICE_SIZE_BYTES = (1024 * 16); // Prefix for all flushed stats. - static char STAT_PREFIX[]; + std::string prefix; Upstream::ClusterInfoConstSharedPtr cluster_info_; ThreadLocal::SlotPtr tls_; From a75b05db7c655c21428263e71668c85b826c9028 Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Sun, 8 Apr 2018 22:04:06 -0500 Subject: [PATCH 03/10] stats: add tests for custom prefix Signed-off-by: Akhil Thampy --- .../stats_sinks/statsd/config_test.cc | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/test/extensions/stats_sinks/statsd/config_test.cc b/test/extensions/stats_sinks/statsd/config_test.cc index e01101d261006..99a14fe7ae37c 100644 --- a/test/extensions/stats_sinks/statsd/config_test.cc +++ b/test/extensions/stats_sinks/statsd/config_test.cc @@ -45,6 +45,101 @@ TEST(StatsConfigTest, ValidTcpStatsd) { EXPECT_NE(dynamic_cast(sink.get()), nullptr); } +TEST(StatsConfigTest, UdpSinkDefaultPrefix) { + const std::string name = Config::StatsSinkNames::get().STATSD; + auto defaultPrefix = Common::Statsd::DEFAULT_PREFIX; + + envoy::config::metrics::v2::StatsdSink sink_config; + envoy::api::v2::core::Address& address = *sink_config.mutable_address(); + envoy::api::v2::core::SocketAddress& socket_address = *address.mutable_socket_address(); + socket_address.set_protocol(envoy::api::v2::core::SocketAddress::UDP); + socket_address.set_address("127.0.0.1"); + socket_address.set_port_value(8125); + EXPECT_EQ(sink_config.prefix(), ""); + + Server::Configuration::StatsSinkFactory* factory = + Registry::FactoryRegistry::getFactory(name); + ASSERT_NE(factory, nullptr); + ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto(); + MessageUtil::jsonConvert(sink_config, *message); + + NiceMock server; + Stats::SinkPtr sink = factory->createStatsSink(*message, server); + EXPECT_NE(sink, nullptr); + EXPECT_NE(dynamic_cast(sink.get()), nullptr); + EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), defaultPrefix); +} + +TEST(StatsConfigTest, UdpSinkCustomPrefix) { + const std::string name = Config::StatsSinkNames::get().STATSD; + const std::string customPrefix = "prefix.test"; + + envoy::config::metrics::v2::StatsdSink sink_config; + envoy::api::v2::core::Address& address = *sink_config.mutable_address(); + envoy::api::v2::core::SocketAddress& socket_address = *address.mutable_socket_address(); + socket_address.set_protocol(envoy::api::v2::core::SocketAddress::UDP); + socket_address.set_address("127.0.0.1"); + socket_address.set_port_value(8125); + sink_config.set_prefix(customPrefix); + EXPECT_NE(sink_config.prefix(), ""); + + Server::Configuration::StatsSinkFactory* factory = + Registry::FactoryRegistry::getFactory(name); + ASSERT_NE(factory, nullptr); + ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto(); + MessageUtil::jsonConvert(sink_config, *message); + + NiceMock server; + Stats::SinkPtr sink = factory->createStatsSink(*message, server); + EXPECT_NE(sink, nullptr); + EXPECT_NE(dynamic_cast(sink.get()), nullptr); + EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), customPrefix); +} + +TEST(StatsConfigTest, TcpSinkDefaultPrefix) { + const std::string name = Config::StatsSinkNames::get().STATSD; + + envoy::config::metrics::v2::StatsdSink sink_config; + auto defaultPrefix = Common::Statsd::DEFAULT_PREFIX; + sink_config.set_tcp_cluster_name("fake_cluster"); + + Server::Configuration::StatsSinkFactory* factory = + Registry::FactoryRegistry::getFactory(name); + ASSERT_NE(factory, nullptr); + EXPECT_EQ(sink_config.prefix(), ""); + ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto(); + MessageUtil::jsonConvert(sink_config, *message); + + NiceMock server; + Stats::SinkPtr sink = factory->createStatsSink(*message, server); + EXPECT_NE(sink, nullptr); + EXPECT_NE(dynamic_cast(sink.get()), nullptr); + EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), defaultPrefix); +} + +TEST(StatsConfigTest, TcpSinkCustomPrefix) { + const std::string name = Config::StatsSinkNames::get().STATSD; + + envoy::config::metrics::v2::StatsdSink sink_config; + std::string prefix = "prefixTest"; + sink_config.set_tcp_cluster_name("fake_cluster"); + ASSERT_NE(sink_config.prefix(), prefix); + sink_config.set_prefix(prefix); + EXPECT_EQ(sink_config.prefix(), prefix); + Server::Configuration::StatsSinkFactory* factory = + Registry::FactoryRegistry::getFactory(name); + ASSERT_NE(factory, nullptr); + + ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto(); + MessageUtil::jsonConvert(sink_config, *message); + + NiceMock server; + Stats::SinkPtr sink = factory->createStatsSink(*message, server); + EXPECT_NE(sink, nullptr); + EXPECT_NE(dynamic_cast(sink.get()), nullptr); + EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), prefix); +} + class StatsConfigLoopbackTest : public testing::TestWithParam {}; INSTANTIATE_TEST_CASE_P(IpVersions, StatsConfigLoopbackTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), From 2e0cd2c6f20dcbefd8ad89b36ef2ebb21304943f Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Mon, 9 Apr 2018 11:16:30 -0500 Subject: [PATCH 04/10] stats: fix format Signed-off-by: Akhil Thampy --- source/extensions/stat_sinks/common/statsd/statsd.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 786571b3aa78f..692241aee1874 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -58,8 +58,8 @@ void UdpStatsdSink::flushCounter(const Stats::Counter& counter, uint64_t delta) } void UdpStatsdSink::flushGauge(const Stats::Gauge& gauge, uint64_t value) { - const std::string message(fmt::format("{}.{}:{}|g{}", prefix, getName(gauge), value, - buildTagStr(gauge.tags()))); + const std::string message( + fmt::format("{}.{}:{}|g{}", prefix, getName(gauge), value, buildTagStr(gauge.tags()))); tls_->getTyped().write(message); } From 2a021a82f086ae5433197ccad538fc98ee0f427d Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Mon, 9 Apr 2018 16:04:58 -0500 Subject: [PATCH 05/10] stats: fix tests Signed-off-by: Akhil Thampy --- test/extensions/stats_sinks/statsd/config_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/stats_sinks/statsd/config_test.cc b/test/extensions/stats_sinks/statsd/config_test.cc index 99a14fe7ae37c..dab6605bf1377 100644 --- a/test/extensions/stats_sinks/statsd/config_test.cc +++ b/test/extensions/stats_sinks/statsd/config_test.cc @@ -67,7 +67,7 @@ TEST(StatsConfigTest, UdpSinkDefaultPrefix) { Stats::SinkPtr sink = factory->createStatsSink(*message, server); EXPECT_NE(sink, nullptr); EXPECT_NE(dynamic_cast(sink.get()), nullptr); - EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), defaultPrefix); + EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), defaultPrefix); } TEST(StatsConfigTest, UdpSinkCustomPrefix) { @@ -93,7 +93,7 @@ TEST(StatsConfigTest, UdpSinkCustomPrefix) { Stats::SinkPtr sink = factory->createStatsSink(*message, server); EXPECT_NE(sink, nullptr); EXPECT_NE(dynamic_cast(sink.get()), nullptr); - EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), customPrefix); + EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), customPrefix); } TEST(StatsConfigTest, TcpSinkDefaultPrefix) { From f472f78895b47c8c419dbc43eb1f248bf68ee575 Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Mon, 16 Apr 2018 11:02:16 -0500 Subject: [PATCH 06/10] stats: use CONSTRUCT_ON_FIRST_USE, clean up prefix initialization, and bump data-plane-api SHA Signed-off-by: Akhil Thampy --- .../stat_sinks/common/statsd/statsd.cc | 29 +++++++------------ .../stat_sinks/common/statsd/statsd.h | 24 +++++++-------- .../stats_sinks/statsd/config_test.cc | 4 +-- 3 files changed, 24 insertions(+), 33 deletions(-) diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 692241aee1874..a863cb99c4705 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -39,33 +39,29 @@ void Writer::write(const std::string& message) { UdpStatsdSink::UdpStatsdSink(ThreadLocal::SlotAllocator& tls, Network::Address::InstanceConstSharedPtr address, const bool use_tag, - const std::string _prefix) - : tls_(tls.allocateSlot()), server_address_(std::move(address)), use_tag_(use_tag) { + const std::string& prefix) + : tls_(tls.allocateSlot()), server_address_(std::move(address)), use_tag_(use_tag), + prefix_(prefix.empty() ? Statsd::getDefaultPrefix() : prefix) { tls_->set([this](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(this->server_address_); }); - if (_prefix.size() == 0) { - prefix = Statsd::DEFAULT_PREFIX; - } else { - prefix = _prefix; - } } void UdpStatsdSink::flushCounter(const Stats::Counter& counter, uint64_t delta) { const std::string message( - fmt::format("{}.{}:{}|c{}", prefix, getName(counter), delta, buildTagStr(counter.tags()))); + fmt::format("{}.{}:{}|c{}", prefix_, getName(counter), delta, buildTagStr(counter.tags()))); tls_->getTyped().write(message); } void UdpStatsdSink::flushGauge(const Stats::Gauge& gauge, uint64_t value) { const std::string message( - fmt::format("{}.{}:{}|g{}", prefix, getName(gauge), value, buildTagStr(gauge.tags()))); + fmt::format("{}.{}:{}|g{}", prefix_, getName(gauge), value, buildTagStr(gauge.tags()))); tls_->getTyped().write(message); } void UdpStatsdSink::onHistogramComplete(const Stats::Histogram& histogram, uint64_t value) { // For statsd histograms are all timers. - const std::string message(fmt::format("{}.{}:{}|ms{}", prefix, getName(histogram), + const std::string message(fmt::format("{}.{}:{}|ms{}", prefix_, getName(histogram), std::chrono::milliseconds(value).count(), buildTagStr(histogram.tags()))); tls_->getTyped().write(message); @@ -95,9 +91,9 @@ const std::string UdpStatsdSink::buildTagStr(const std::vector& tags TcpStatsdSink::TcpStatsdSink(const LocalInfo::LocalInfo& local_info, const std::string& cluster_name, ThreadLocal::SlotAllocator& tls, Upstream::ClusterManager& cluster_manager, Stats::Scope& scope, - const std::string _prefix) - : tls_(tls.allocateSlot()), cluster_manager_(cluster_manager), - cx_overflow_stat_(scope.counter("statsd.cx_overflow")) { + const std::string& prefix) + : prefix_(prefix.empty() ? Statsd::getDefaultPrefix() : prefix), tls_(tls.allocateSlot()), + cluster_manager_(cluster_manager), cx_overflow_stat_(scope.counter("statsd.cx_overflow")) { Config::Utility::checkClusterAndLocalInfo("tcp statsd", cluster_name, cluster_manager, local_info); @@ -105,11 +101,6 @@ TcpStatsdSink::TcpStatsdSink(const LocalInfo::LocalInfo& local_info, tls_->set([this](Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(*this, dispatcher); }); - if (_prefix.size() == 0) { - prefix = Statsd::DEFAULT_PREFIX; - } else { - prefix = _prefix; - } } TcpStatsdSink::TlsSink::TlsSink(TcpStatsdSink& parent, Event::Dispatcher& dispatcher) @@ -134,7 +125,7 @@ void TcpStatsdSink::TlsSink::beginFlush(bool expect_empty_buffer) { void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value, char stat_type) { ASSERT(current_slice_mem_ != nullptr); - // 40 > 6 (prefix) + 4 (random chars) + 30 for number (bigger than it will ever be) + // 40 > 6 (prefix_) + 4 (random chars) + 30 for number (bigger than it will ever be) const uint32_t max_size = name.size() + 40; if (current_buffer_slice_.len_ - usedBuffer() < max_size) { endFlush(false); diff --git a/source/extensions/stat_sinks/common/statsd/statsd.h b/source/extensions/stat_sinks/common/statsd/statsd.h index a350c2b3d3622..e796f4be9ff79 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.h +++ b/source/extensions/stat_sinks/common/statsd/statsd.h @@ -7,6 +7,7 @@ #include "envoy/upstream/cluster_manager.h" #include "common/buffer/buffer_impl.h" +#include "common/common/macros.h" namespace Envoy { namespace Extensions { @@ -14,7 +15,8 @@ namespace StatSinks { namespace Common { namespace Statsd { -static const std::string DEFAULT_PREFIX = "envoy"; +static const std::string& getDefaultPrefix() { CONSTRUCT_ON_FIRST_USE(std::string, "envoy"); } + /** * This is a simple UDP localhost writer for statsd messages. */ @@ -39,16 +41,14 @@ class Writer : public ThreadLocal::ThreadLocalObject { class UdpStatsdSink : public Stats::Sink { public: UdpStatsdSink(ThreadLocal::SlotAllocator& tls, Network::Address::InstanceConstSharedPtr address, - const bool use_tag, const std::string _prefix = DEFAULT_PREFIX); + const bool use_tag, const std::string& prefix = getDefaultPrefix()); // For testing. UdpStatsdSink(ThreadLocal::SlotAllocator& tls, const std::shared_ptr& writer, - const bool use_tag, const std::string _prefix = DEFAULT_PREFIX) - : tls_(tls.allocateSlot()), use_tag_(use_tag) { + const bool use_tag, const std::string& prefix = getDefaultPrefix()) + : tls_(tls.allocateSlot()), use_tag_(use_tag), + prefix_(prefix.empty() ? getDefaultPrefix() : prefix) { tls_->set( [writer](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return writer; }); - if (_prefix.size() != 0) { - prefix = _prefix; - } } // Stats::Sink @@ -61,7 +61,7 @@ class UdpStatsdSink : public Stats::Sink { // Called in unit test to validate writer construction and address. int getFdForTests() { return tls_->getTyped().getFdForTests(); } bool getUseTagForTest() { return use_tag_; } - std::string getPrefix() { return prefix; } + const std::string& getPrefix() { return prefix_; } private: const std::string getName(const Stats::Metric& metric); @@ -71,7 +71,7 @@ class UdpStatsdSink : public Stats::Sink { Network::Address::InstanceConstSharedPtr server_address_; const bool use_tag_; // Prefix for all flushed stats. - std::string prefix; + const std::string& prefix_; }; /** @@ -81,7 +81,7 @@ class TcpStatsdSink : public Stats::Sink { public: TcpStatsdSink(const LocalInfo::LocalInfo& local_info, const std::string& cluster_name, ThreadLocal::SlotAllocator& tls, Upstream::ClusterManager& cluster_manager, - Stats::Scope& scope, std::string _prefix = DEFAULT_PREFIX); + Stats::Scope& scope, const std::string& prefix = getDefaultPrefix()); // Stats::Sink void beginFlush() override { tls_->getTyped().beginFlush(true); } @@ -102,7 +102,7 @@ class TcpStatsdSink : public Stats::Sink { std::chrono::milliseconds(value)); } - std::string getPrefix() { return prefix; } + std::string getPrefix() { return prefix_; } private: struct TlsSink : public ThreadLocal::ThreadLocalObject, public Network::ConnectionCallbacks { @@ -139,7 +139,7 @@ class TcpStatsdSink : public Stats::Sink { static constexpr uint32_t FLUSH_SLICE_SIZE_BYTES = (1024 * 16); // Prefix for all flushed stats. - std::string prefix; + const std::string& prefix_; Upstream::ClusterInfoConstSharedPtr cluster_info_; ThreadLocal::SlotPtr tls_; diff --git a/test/extensions/stats_sinks/statsd/config_test.cc b/test/extensions/stats_sinks/statsd/config_test.cc index dab6605bf1377..872826bbb10d7 100644 --- a/test/extensions/stats_sinks/statsd/config_test.cc +++ b/test/extensions/stats_sinks/statsd/config_test.cc @@ -47,7 +47,7 @@ TEST(StatsConfigTest, ValidTcpStatsd) { TEST(StatsConfigTest, UdpSinkDefaultPrefix) { const std::string name = Config::StatsSinkNames::get().STATSD; - auto defaultPrefix = Common::Statsd::DEFAULT_PREFIX; + auto defaultPrefix = Common::Statsd::getDefaultPrefix(); envoy::config::metrics::v2::StatsdSink sink_config; envoy::api::v2::core::Address& address = *sink_config.mutable_address(); @@ -100,7 +100,7 @@ TEST(StatsConfigTest, TcpSinkDefaultPrefix) { const std::string name = Config::StatsSinkNames::get().STATSD; envoy::config::metrics::v2::StatsdSink sink_config; - auto defaultPrefix = Common::Statsd::DEFAULT_PREFIX; + auto defaultPrefix = Common::Statsd::getDefaultPrefix(); sink_config.set_tcp_cluster_name("fake_cluster"); Server::Configuration::StatsSinkFactory* factory = From 40d503bed4d786519706f6470bbb4c00283898d8 Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Mon, 16 Apr 2018 16:12:24 -0500 Subject: [PATCH 07/10] stats: fix namespaces in config_test after repo reorg Signed-off-by: Akhil Thampy --- test/extensions/stats_sinks/statsd/config_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/extensions/stats_sinks/statsd/config_test.cc b/test/extensions/stats_sinks/statsd/config_test.cc index 872826bbb10d7..9a707b4e7cfe2 100644 --- a/test/extensions/stats_sinks/statsd/config_test.cc +++ b/test/extensions/stats_sinks/statsd/config_test.cc @@ -46,7 +46,7 @@ TEST(StatsConfigTest, ValidTcpStatsd) { } TEST(StatsConfigTest, UdpSinkDefaultPrefix) { - const std::string name = Config::StatsSinkNames::get().STATSD; + const std::string name = StatsSinkNames::get().STATSD; auto defaultPrefix = Common::Statsd::getDefaultPrefix(); envoy::config::metrics::v2::StatsdSink sink_config; @@ -71,7 +71,7 @@ TEST(StatsConfigTest, UdpSinkDefaultPrefix) { } TEST(StatsConfigTest, UdpSinkCustomPrefix) { - const std::string name = Config::StatsSinkNames::get().STATSD; + const std::string name = StatsSinkNames::get().STATSD; const std::string customPrefix = "prefix.test"; envoy::config::metrics::v2::StatsdSink sink_config; @@ -97,7 +97,7 @@ TEST(StatsConfigTest, UdpSinkCustomPrefix) { } TEST(StatsConfigTest, TcpSinkDefaultPrefix) { - const std::string name = Config::StatsSinkNames::get().STATSD; + const std::string name = StatsSinkNames::get().STATSD; envoy::config::metrics::v2::StatsdSink sink_config; auto defaultPrefix = Common::Statsd::getDefaultPrefix(); @@ -118,7 +118,7 @@ TEST(StatsConfigTest, TcpSinkDefaultPrefix) { } TEST(StatsConfigTest, TcpSinkCustomPrefix) { - const std::string name = Config::StatsSinkNames::get().STATSD; + const std::string name = StatsSinkNames::get().STATSD; envoy::config::metrics::v2::StatsdSink sink_config; std::string prefix = "prefixTest"; From 807f7bf931a095d4db26c9a74e5d711d0bf2a51a Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Fri, 20 Apr 2018 16:09:38 -0500 Subject: [PATCH 08/10] stats: address review comments Signed-off-by: Akhil Thampy --- .../stat_sinks/common/statsd/statsd.cc | 4 +-- .../stat_sinks/common/statsd/statsd.h | 8 ++--- .../stats_sinks/statsd/config_test.cc | 32 ++++++++++++------- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index a863cb99c4705..1310c6b8bea8d 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -125,8 +125,8 @@ void TcpStatsdSink::TlsSink::beginFlush(bool expect_empty_buffer) { void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value, char stat_type) { ASSERT(current_slice_mem_ != nullptr); - // 40 > 6 (prefix_) + 4 (random chars) + 30 for number (bigger than it will ever be) - const uint32_t max_size = name.size() + 40; + // 34 > 4 (random chars) + 30 for number (bigger than it will ever be) + const uint32_t max_size = name.size() + parent_.getPrefix().size() + 40; if (current_buffer_slice_.len_ - usedBuffer() < max_size) { endFlush(false); beginFlush(false); diff --git a/source/extensions/stat_sinks/common/statsd/statsd.h b/source/extensions/stat_sinks/common/statsd/statsd.h index e796f4be9ff79..a15c1cc58767b 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.h +++ b/source/extensions/stat_sinks/common/statsd/statsd.h @@ -61,7 +61,7 @@ class UdpStatsdSink : public Stats::Sink { // Called in unit test to validate writer construction and address. int getFdForTests() { return tls_->getTyped().getFdForTests(); } bool getUseTagForTest() { return use_tag_; } - const std::string& getPrefix() { return prefix_; } + const std::string getPrefix() { return prefix_; } private: const std::string getName(const Stats::Metric& metric); @@ -71,7 +71,7 @@ class UdpStatsdSink : public Stats::Sink { Network::Address::InstanceConstSharedPtr server_address_; const bool use_tag_; // Prefix for all flushed stats. - const std::string& prefix_; + const std::string prefix_; }; /** @@ -102,7 +102,7 @@ class TcpStatsdSink : public Stats::Sink { std::chrono::milliseconds(value)); } - std::string getPrefix() { return prefix_; } + const std::string getPrefix() { return prefix_; } private: struct TlsSink : public ThreadLocal::ThreadLocalObject, public Network::ConnectionCallbacks { @@ -139,7 +139,7 @@ class TcpStatsdSink : public Stats::Sink { static constexpr uint32_t FLUSH_SLICE_SIZE_BYTES = (1024 * 16); // Prefix for all flushed stats. - const std::string& prefix_; + const std::string prefix_; Upstream::ClusterInfoConstSharedPtr cluster_info_; ThreadLocal::SlotPtr tls_; diff --git a/test/extensions/stats_sinks/statsd/config_test.cc b/test/extensions/stats_sinks/statsd/config_test.cc index 9a707b4e7cfe2..40ed862e2461f 100644 --- a/test/extensions/stats_sinks/statsd/config_test.cc +++ b/test/extensions/stats_sinks/statsd/config_test.cc @@ -65,9 +65,11 @@ TEST(StatsConfigTest, UdpSinkDefaultPrefix) { NiceMock server; Stats::SinkPtr sink = factory->createStatsSink(*message, server); - EXPECT_NE(sink, nullptr); - EXPECT_NE(dynamic_cast(sink.get()), nullptr); - EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), defaultPrefix); + ASSERT_NE(sink, nullptr); + + auto udp_sink = dynamic_cast(sink.get()); + ASSERT_NE(udp_sink, nullptr); + EXPECT_EQ(udp_sink->getPrefix(), defaultPrefix); } TEST(StatsConfigTest, UdpSinkCustomPrefix) { @@ -91,9 +93,11 @@ TEST(StatsConfigTest, UdpSinkCustomPrefix) { NiceMock server; Stats::SinkPtr sink = factory->createStatsSink(*message, server); - EXPECT_NE(sink, nullptr); - EXPECT_NE(dynamic_cast(sink.get()), nullptr); - EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), customPrefix); + ASSERT_NE(sink, nullptr); + + auto udp_sink = dynamic_cast(sink.get()); + ASSERT_NE(udp_sink, nullptr); + EXPECT_EQ(udp_sink->getPrefix(), customPrefix); } TEST(StatsConfigTest, TcpSinkDefaultPrefix) { @@ -112,9 +116,11 @@ TEST(StatsConfigTest, TcpSinkDefaultPrefix) { NiceMock server; Stats::SinkPtr sink = factory->createStatsSink(*message, server); - EXPECT_NE(sink, nullptr); - EXPECT_NE(dynamic_cast(sink.get()), nullptr); - EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), defaultPrefix); + ASSERT_NE(sink, nullptr); + + auto tcp_sink = dynamic_cast(sink.get()); + ASSERT_NE(tcp_sink, nullptr); + EXPECT_EQ(tcp_sink->getPrefix(), defaultPrefix); } TEST(StatsConfigTest, TcpSinkCustomPrefix) { @@ -135,9 +141,11 @@ TEST(StatsConfigTest, TcpSinkCustomPrefix) { NiceMock server; Stats::SinkPtr sink = factory->createStatsSink(*message, server); - EXPECT_NE(sink, nullptr); - EXPECT_NE(dynamic_cast(sink.get()), nullptr); - EXPECT_EQ(dynamic_cast(sink.get())->getPrefix(), prefix); + ASSERT_NE(sink, nullptr); + + auto tcp_sink = dynamic_cast(sink.get()); + ASSERT_NE(tcp_sink, nullptr); + EXPECT_EQ(tcp_sink->getPrefix(), prefix); } class StatsConfigLoopbackTest : public testing::TestWithParam {}; From 34bc6461c7a2fb84ba0e077242004a987610d931 Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Mon, 23 Apr 2018 12:10:06 -0500 Subject: [PATCH 09/10] Address review comments Signed-off-by: Akhil Thampy --- source/extensions/stat_sinks/common/statsd/statsd.cc | 4 ++-- source/extensions/stat_sinks/common/statsd/statsd.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 1310c6b8bea8d..5bb7098dc4f05 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -125,8 +125,8 @@ void TcpStatsdSink::TlsSink::beginFlush(bool expect_empty_buffer) { void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value, char stat_type) { ASSERT(current_slice_mem_ != nullptr); - // 34 > 4 (random chars) + 30 for number (bigger than it will ever be) - const uint32_t max_size = name.size() + parent_.getPrefix().size() + 40; + // 34 > 4 (postfix chars, e.g., "|ms\n") + 30 for number (bigger than it will ever be) + const uint32_t max_size = name.size() + parent_.getPrefix().size() + 34; if (current_buffer_slice_.len_ - usedBuffer() < max_size) { endFlush(false); beginFlush(false); diff --git a/source/extensions/stat_sinks/common/statsd/statsd.h b/source/extensions/stat_sinks/common/statsd/statsd.h index a15c1cc58767b..f891cec85bedb 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.h +++ b/source/extensions/stat_sinks/common/statsd/statsd.h @@ -61,7 +61,7 @@ class UdpStatsdSink : public Stats::Sink { // Called in unit test to validate writer construction and address. int getFdForTests() { return tls_->getTyped().getFdForTests(); } bool getUseTagForTest() { return use_tag_; } - const std::string getPrefix() { return prefix_; } + const std::string& getPrefix() { return prefix_; } private: const std::string getName(const Stats::Metric& metric); @@ -102,7 +102,7 @@ class TcpStatsdSink : public Stats::Sink { std::chrono::milliseconds(value)); } - const std::string getPrefix() { return prefix_; } + const std::string& getPrefix() { return prefix_; } private: struct TlsSink : public ThreadLocal::ThreadLocalObject, public Network::ConnectionCallbacks { From 1399b2be47e9f8a0e0353632fd7e542d22d88df2 Mon Sep 17 00:00:00 2001 From: Akhil Thampy Date: Mon, 23 Apr 2018 18:19:36 -0500 Subject: [PATCH 10/10] address nits Signed-off-by: Akhil Thampy --- source/extensions/stat_sinks/common/statsd/statsd.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 5bb7098dc4f05..edba7ce99af04 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -125,8 +125,9 @@ void TcpStatsdSink::TlsSink::beginFlush(bool expect_empty_buffer) { void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value, char stat_type) { ASSERT(current_slice_mem_ != nullptr); - // 34 > 4 (postfix chars, e.g., "|ms\n") + 30 for number (bigger than it will ever be) - const uint32_t max_size = name.size() + parent_.getPrefix().size() + 34; + // 36 > 1 ("." after prefix) + 1 (":" after name) + 4 (postfix chars, e.g., "|ms\n") + 30 for + // number (bigger than it will ever be) + const uint32_t max_size = name.size() + parent_.getPrefix().size() + 36; if (current_buffer_slice_.len_ - usedBuffer() < max_size) { endFlush(false); beginFlush(false); @@ -138,8 +139,7 @@ void TcpStatsdSink::TlsSink::commonFlush(const std::string& name, uint64_t value const char* snapped_current = current_slice_mem_; memcpy(current_slice_mem_, parent_.getPrefix().c_str(), parent_.getPrefix().size()); current_slice_mem_ += parent_.getPrefix().size(); - *current_slice_mem_ = '.'; - current_slice_mem_ += 1; + *current_slice_mem_++ = '.'; memcpy(current_slice_mem_, name.c_str(), name.size()); current_slice_mem_ += name.size(); *current_slice_mem_++ = ':';