From f3a4844b0ae143988b2732d6841d21a4bb2eca22 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 12 Sep 2024 16:25:13 +0000 Subject: [PATCH] stats: implicitly treat stats as enabled if `statshost` is specified This is to avoid the odd circumstance where stats don't work because you specified everything but didn't explicitly enable it. Using `-statsenabled` has been deprecated and will be removed in the next major release. --- src/init.cpp | 14 ++++---------- src/net.cpp | 2 +- src/stats/client.cpp | 28 ++++++++++++++++++++++++++++ src/stats/client.h | 9 +++++++-- src/test/util/setup_common.cpp | 10 +--------- 5 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 639a6f2395..4ee582dde6 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -771,7 +771,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-rpcworkqueue=", strprintf("Set the depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC); argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC); - argsman.AddArg("-statsenabled", strprintf("Publish internal stats to statsd (default: %u)", DEFAULT_STATSD_ENABLE), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD); + hidden_args.emplace_back("-statsenabled"); argsman.AddArg("-statsbatchsize=", strprintf("Specify the size of each batch of stats messages (default: %d)", DEFAULT_STATSD_BATCH_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD); argsman.AddArg("-statsduration=", strprintf("Specify the number of milliseconds between stats messages (default: %d)", DEFAULT_STATSD_DURATION), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD); argsman.AddArg("-statshost=", strprintf("Specify statsd host (default: %s)", DEFAULT_STATSD_HOST), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD); @@ -838,7 +838,7 @@ static void StartupNotify(const ArgsManager& args) static void PeriodicStats(ArgsManager& args, ChainstateManager& chainman, const CTxMemPool& mempool) { - assert(args.GetBoolArg("-statsenabled", DEFAULT_STATSD_ENABLE)); + assert(::g_stats_client->active()); CCoinsStats stats{CoinStatsHashType::NONE}; chainman.ActiveChainstate().ForceFlushStateToDisk(); if (WITH_LOCK(cs_main, return GetUTXOStats(&chainman.ActiveChainstate().CoinsDB(), std::ref(chainman.m_blockman), stats, RpcInterruptionPoint, chainman.ActiveChain().Tip()))) { @@ -1541,13 +1541,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // We need to initialize g_stats_client early as currently, g_stats_client is called // regardless of whether transmitting stats are desirable or not and if // g_stats_client isn't present when that attempt is made, the client will crash. - ::g_stats_client = std::make_unique(args.GetArg("-statshost", DEFAULT_STATSD_HOST), - args.GetArg("-statsport", DEFAULT_STATSD_PORT), - args.GetArg("-statsbatchsize", DEFAULT_STATSD_BATCH_SIZE), - args.GetArg("-statsduration", DEFAULT_STATSD_DURATION), - args.GetArg("-statshostname", DEFAULT_STATSD_HOSTNAME), - args.GetArg("-statsns", DEFAULT_STATSD_NAMESPACE), - args.GetBoolArg("-statsenabled", DEFAULT_STATSD_ENABLE)); + ::g_stats_client = InitStatsClient(args); { @@ -2278,7 +2272,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) #endif // ENABLE_WALLET } - if (args.GetBoolArg("-statsenabled", DEFAULT_STATSD_ENABLE)) { + if (::g_stats_client->active()) { int nStatsPeriod = std::min(std::max((int)args.GetArg("-statsperiod", DEFAULT_STATSD_PERIOD), MIN_STATSD_PERIOD), MAX_STATSD_PERIOD); node.scheduler->scheduleEvery(std::bind(&PeriodicStats, std::ref(*node.args), std::ref(chainman), std::cref(*node.mempool)), std::chrono::seconds{nStatsPeriod}); } diff --git a/src/net.cpp b/src/net.cpp index 98daa17117..8561b69828 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1688,7 +1688,7 @@ void CConnman::NotifyNumConnectionsChanged(CMasternodeSync& mn_sync) void CConnman::CalculateNumConnectionsChangedStats() { - if (!gArgs.GetBoolArg("-statsenabled", DEFAULT_STATSD_ENABLE)) { + if (!::g_stats_client->active()) { return; } diff --git a/src/stats/client.cpp b/src/stats/client.cpp index df5c3ea027..f9c1e66355 100644 --- a/src/stats/client.cpp +++ b/src/stats/client.cpp @@ -59,6 +59,34 @@ static constexpr char STATSD_METRIC_TIMING[]{"ms"}; std::unique_ptr g_stats_client; +std::unique_ptr InitStatsClient(const ArgsManager& args) +{ + auto is_enabled = args.GetBoolArg("-statsenabled", /*fDefault=*/false); + auto host = args.GetArg("-statshost", /*fDefault=*/""); + + if (is_enabled && host.empty()) { + // Stats are enabled but host has not been specified, then use + // default host. This is to preserve old behavior. + host = DEFAULT_STATSD_HOST; + } else if (!host.empty()) { + // Host is specified but stats are not explcitly enabled. Assume + // that if a host has been specified, we want stats enabled. This + // is new behaviour and will substitute old behaviour in a future + // release. + is_enabled = true; + } + + return std::make_unique( + host, + args.GetArg("-statsport", DEFAULT_STATSD_PORT), + args.GetArg("-statsbatchsize", DEFAULT_STATSD_BATCH_SIZE), + args.GetArg("-statsduration", DEFAULT_STATSD_DURATION), + args.GetArg("-statshostname", DEFAULT_STATSD_HOSTNAME), + args.GetArg("-statsns", DEFAULT_STATSD_NAMESPACE), + is_enabled + ); +} + StatsdClient::StatsdClient(const std::string& host, uint16_t port, uint64_t batch_size, uint64_t interval_ms, const std::string& nodename, const std::string& ns, bool enabled) : m_nodename{nodename}, diff --git a/src/stats/client.h b/src/stats/client.h index fb152daec8..c06c8dda05 100644 --- a/src/stats/client.h +++ b/src/stats/client.h @@ -13,10 +13,9 @@ #include #include +class ArgsManager; class RawSender; -/** Default state of Statsd enablement */ -static constexpr bool DEFAULT_STATSD_ENABLE{false}; /** Default port used to connect to a Statsd server */ static constexpr uint16_t DEFAULT_STATSD_PORT{8125}; /** Default host assumed to be running a Statsd server */ @@ -57,6 +56,9 @@ public: template bool send(const std::string& key, T1 value, const std::string& type, float sample_rate = 1.f); + /* Check if a StatsdClient instance is ready to send messages */ + bool active() const { return m_sender != nullptr; } + private: /* Mutex to protect PRNG */ mutable Mutex cs; @@ -72,6 +74,9 @@ private: const std::string m_ns; }; +/** Parses arguments and constructs a StatsdClient instance */ +std::unique_ptr InitStatsClient(const ArgsManager& args); + /** Global smart pointer containing StatsdClient instance */ extern std::unique_ptr g_stats_client; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 7d7c898d58..c9265b85e0 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -183,15 +183,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve SetupNetworking(); InitSignatureCache(); InitScriptExecutionCache(); - ::g_stats_client = std::make_unique( - m_node.args->GetArg("-statshost", DEFAULT_STATSD_HOST), - m_node.args->GetArg("-statsport", DEFAULT_STATSD_PORT), - m_node.args->GetArg("-statsbatchsize", DEFAULT_STATSD_BATCH_SIZE), - m_node.args->GetArg("-statsduration", DEFAULT_STATSD_DURATION), - m_node.args->GetArg("-statshostname", DEFAULT_STATSD_HOSTNAME), - m_node.args->GetArg("-statsns", DEFAULT_STATSD_NAMESPACE), - m_node.args->GetBoolArg("-statsenabled", DEFAULT_STATSD_ENABLE) - ); + ::g_stats_client = InitStatsClient(*m_node.args); m_node.chain = interfaces::MakeChain(m_node); m_node.netgroupman = std::make_unique(/*asmap=*/std::vector());