Merge #6837: feat(stats): add support for IPv6, introduce functional tests, deprecate -statsport

e607836 docs: add release notes (Kittywhiskers Van Gogh)
b50c7f2 stats: add functional test for statsd reporting (Kittywhiskers Van Gogh)
592aa04 stats: add support for URLs in `statshost`, deprecate `statsport` (Kittywhiskers Van Gogh)
3640071 stats: extend connection support to IPv6 (Kittywhiskers Van Gogh)
131d536 stats: use `bilingual_str` for RawSender error messages (Kittywhiskers Van Gogh)
fced9f6 stats: add stricter validation for arguments (Kittywhiskers Van Gogh)
8d7e74f stats: bubble errors up, halt init if stats client reports errors (Kittywhiskers Van Gogh)
1508f4a stats: use string_view for fixed values (Kittywhiskers Van Gogh)
9e9ee35 stats: move implementation to source file, define interface (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Support for URLs have been introduced for `statshost` and can be formatted as `udp://127.0.0.1:8125`. As `statshost` can also include the port, `statsport` has been deprecated and will be removed in a future release of Dash Core.

  * Startup validation errors are no longer ignored and trigger an `InitError()`. Connection failure will **not** halt startup but lookup failure **will**, this is because `LookupHost()` is used to identify if the connection is IPv4/IPv6.

  ## Breaking Changes

  See release notes.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK e607836

Tree-SHA512: 69d0ed16bc0a23cc960dc9b550919677dd59dcff9fb5145ae1da9f99146b0e65d300044d9628f72ac058fab68b321b1dd04dfd3e80773bfcce72efd85a77d660

Author: PastaPastaPasta

doc/release-notes-6837.md

@@ -0,0 +1,17 @@
+Statistics
+----------
+
+- IPv6 hosts are now supported by the StatsD client.
+
+- `-statshost` now accepts URLs to allow specifying the protocol, host and port in one argument.
+
+- Specifying invalid values will no longer result in silent disablement of the StatsD client and will now cause errors
+  at startup.
+
+### Deprecations
+
+- `-statsport` has been deprecated and ports are now specified using the new URL syntax supported by `-statshost`.
+  `-statsport` will be removed in a future release.
+
+  - If both `-statsport` and `-statshost` with a URL specifying a port is supplied, the `-statsport` value will be
+    ignored.

src/bench/checkblock.cpp

@@ -11,6 +11,8 @@
 #include <streams.h>
 #include <validation.h>
 
+#include <memory>
+
 // These are the two major time-sinks which happen after we have fully received
 // a block off the wire, but before we can relay the block on to peers using
 // compact block relay.
@@ -38,8 +40,8 @@ static void DeserializeAndCheckBlockTest(benchmark::Bench& bench)
     ArgsManager bench_args;
     const auto chainParams = CreateChainParams(bench_args, CBaseChainParams::MAIN);
     // CheckBlock calls g_stats_client internally, we aren't using a testing setup
-    // so we need to do this manually.
-    ::g_stats_client = InitStatsClient(bench_args);
+    // so we need to do this manually. We can use the stub interface for this.
+    ::g_stats_client = std::make_unique<StatsdClient>();
 
     bench.unit("block").run([&] {
         CBlock block; // Note that CBlock caches its checked state, so we need to recreate it here

src/init.cpp

@@ -783,7 +783,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-statsbatchsize=<bytes>", strprintf("Specify the size of each batch of stats messages (default: %d)", DEFAULT_STATSD_BATCH_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD);
     argsman.AddArg("-statsduration=<ms>", strprintf("Specify the number of milliseconds between stats messages (default: %d)", DEFAULT_STATSD_DURATION), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD);
     argsman.AddArg("-statshost=<ip>", strprintf("Specify statsd host (default: %s)", DEFAULT_STATSD_HOST), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD);
-    argsman.AddArg("-statsport=<port>", strprintf("Specify statsd port (default: %u)", DEFAULT_STATSD_PORT), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD);
+    hidden_args.emplace_back("-statsport");
     argsman.AddArg("-statsperiod=<seconds>", strprintf("Specify the number of seconds between periodic measurements (default: %d)", DEFAULT_STATSD_PERIOD), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD);
     argsman.AddArg("-statsprefix=<string>", strprintf("Specify an optional string prepended to every stats key (default: %s)", DEFAULT_STATSD_PREFIX), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD);
     argsman.AddArg("-statssuffix=<string>", strprintf("Specify an optional string appended to every stats key (default: %s)", DEFAULT_STATSD_SUFFIX), ArgsManager::ALLOW_ANY, OptionsCategory::STATSD);
@@ -1584,11 +1584,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     fDiscover = args.GetBoolArg("-discover", true);
     const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)};
 
-    // 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 = InitStatsClient(args);
-
     {
 
         // Read asmap file if configured
@@ -1631,6 +1626,17 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         return InitError(strprintf(_("Invalid -socketevents ('%s') specified. Only these modes are supported: %s"), sem_str, GetSupportedSocketEventsStr()));
     }
 
+    // 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.
+    {
+        auto stats_client = StatsdClient::make(args);
+        if (!stats_client) {
+            return InitError(_("Cannot init Statsd client") + Untranslated(" (") + util::ErrorString(stats_client) + Untranslated(")"));
+        }
+        ::g_stats_client = std::move(*stats_client);
+    }
+
     assert(!node.banman);
     node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetIntArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
     assert(!node.connman);

src/stats/client.cpp

@@ -7,15 +7,27 @@
 #include <stats/client.h>
 
 #include <logging.h>
+#include <random.h>
 #include <stats/rawsender.h>
+#include <sync.h>
+#include <util/check.h>
+#include <util/strencodings.h>
 #include <util/system.h>
+#include <util/translation.h>
 
+#include <algorithm>
 #include <cmath>
+#include <limits>
 #include <random>
 
 namespace {
 /** Threshold below which a value is considered effectively zero */
 static constexpr float EPSILON{0.0001f};
+/** Delimiter segmenting scheme from the rest of the URL */
+static constexpr std::string_view URL_SCHEME_DELIMITER{"://"};
+
+/** Default port used to connect to a Statsd server */
+static constexpr uint16_t DEFAULT_STATSD_PORT{8125};
 
 /** Delimiter segmenting two fully formed Statsd messages */
 static constexpr char STATSD_MSG_DELIMITER{'\n'};
@@ -27,12 +39,120 @@ static constexpr char STATSD_METRIC_COUNT[]{"c"};
 static constexpr char STATSD_METRIC_GAUGE[]{"g"};
 /** Characters used to denote Statsd message type as timing */
 static constexpr char STATSD_METRIC_TIMING[]{"ms"};
+
+class StatsdClientImpl final : public StatsdClient
+{
+public:
+    explicit StatsdClientImpl(const std::string& host, uint16_t port, uint64_t batch_size, uint64_t interval_ms,
+                              const std::string& prefix, const std::string& suffix, std::optional<bilingual_str>& error);
+    ~StatsdClientImpl() = default;
+
+public:
+    bool dec(std::string_view key, float sample_rate) override { return count(key, -1, sample_rate); }
+    bool inc(std::string_view key, float sample_rate) override { return count(key, 1, sample_rate); }
+    bool count(std::string_view key, int64_t delta, float sample_rate) override { return _send(key, delta, STATSD_METRIC_COUNT, sample_rate); }
+    bool gauge(std::string_view key, int64_t value, float sample_rate) override { return _send(key, value, STATSD_METRIC_GAUGE, sample_rate); }
+    bool gaugeDouble(std::string_view key, double value, float sample_rate) override { return _send(key, value, STATSD_METRIC_GAUGE, sample_rate); }
+    bool timing(std::string_view key, uint64_t ms, float sample_rate) override { return _send(key, ms, STATSD_METRIC_TIMING, sample_rate); }
+
+    bool send(std::string_view key, double value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); }
+    bool send(std::string_view key, int32_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); }
+    bool send(std::string_view key, int64_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); }
+    bool send(std::string_view key, uint32_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); }
+    bool send(std::string_view key, uint64_t value, std::string_view type, float sample_rate) override { return _send(key, value, type, sample_rate); }
+
+    bool active() const override { return m_sender != nullptr; }
+
+private:
+    template <typename T1>
+    inline bool _send(std::string_view key, T1 value, std::string_view type, float sample_rate);
+
+private:
+    /* Mutex to protect PRNG */
+    mutable Mutex cs;
+    /* PRNG used to dice-roll messages that are 0 < f < 1 */
+    mutable FastRandomContext insecure_rand GUARDED_BY(cs);
+
+    /* Broadcasts messages crafted by StatsdClient */
+    std::unique_ptr<RawSender> m_sender{nullptr};
+
+    /* Phrase prepended to keys */
+    const std::string m_prefix{};
+    /* Phrase appended to keys */
+    const std::string m_suffix{};
+};
 } // anonymous namespace
 
 std::unique_ptr<StatsdClient> g_stats_client;
 
-std::unique_ptr<StatsdClient> InitStatsClient(const ArgsManager& args)
+util::Result<std::unique_ptr<StatsdClient>> StatsdClient::make(const ArgsManager& args)
 {
+    auto host = args.GetArg("-statshost", DEFAULT_STATSD_HOST);
+    if (host.empty()) {
+        LogPrintf("Transmitting stats are disabled, will not init Statsd client\n");
+        return std::make_unique<StatsdClient>();
+    }
+
+    const int64_t batch_size = args.GetIntArg("-statsbatchsize", DEFAULT_STATSD_BATCH_SIZE);
+    if (batch_size < 0) {
+        return util::Error{_("-statsbatchsize cannot be configured with a negative value.")};
+    }
+
+    const int64_t interval_ms = args.GetIntArg("-statsduration", DEFAULT_STATSD_DURATION);
+    if (interval_ms < 0) {
+        return util::Error{_("-statsduration cannot be configured with a negative value.")};
+    }
+
+    auto port_arg = args.GetIntArg("-statsport", DEFAULT_STATSD_PORT);
+    if (args.IsArgSet("-statsport")) {
+        // Port range validation if -statsport is specified.
+        if (port_arg < 1 || port_arg > std::numeric_limits<uint16_t>::max()) {
+            return util::Error{strprintf(_("Port must be between %d and %d, supplied %d"), 1,
+                                         std::numeric_limits<uint16_t>::max(), port_arg)};
+        }
+    }
+    uint16_t port = static_cast<uint16_t>(port_arg);
+
+    // Could be a URL, try to parse it.
+    const size_t scheme_idx{host.find(URL_SCHEME_DELIMITER)};
+    if (scheme_idx != std::string::npos) {
+        // Parse the scheme and trim it out of the URL if we succeed
+        if (scheme_idx == 0) {
+            return util::Error{_("No text before the scheme delimiter, malformed URL")};
+        }
+        std::string scheme{ToLower(host.substr(/*pos=*/0, scheme_idx))};
+        if (scheme != "udp") {
+            return util::Error{_("Unsupported URL scheme, must begin with udp://")};
+        }
+        host = host.substr(scheme_idx + URL_SCHEME_DELIMITER.length());
+
+        // Strip trailing slashes and parse the port
+        const size_t colon_idx{host.rfind(':')};
+        if (colon_idx != std::string::npos) {
+            // Remove all forward slashes found after the port delimiter (colon)
+            host = std::string(
+                host.begin(), host.end() - [&colon_idx, &host]() {
+                    const size_t slash_idx{host.find('/', /*pos=*/colon_idx + 1)};
+                    return slash_idx != std::string::npos ? host.length() - slash_idx : 0;
+                }());
+            uint16_t port_url{0};
+            SplitHostPort(host, port_url, host);
+            if (port_url != 0) {
+                if (args.IsArgSet("-statsport")) {
+                    LogPrintf("%s: Supplied URL with port, ignoring -statsport\n", __func__);
+                }
+                port = port_url;
+            }
+        } else {
+            // There was no port specified, remove everything after the first forward slash
+            host = host.substr(/*pos=*/0, host.find("/"));
+        }
+
+        if (host.empty()) {
+            return util::Error{_("No host specified, malformed URL")};
+        }
+    }
+
     auto sanitize_string = [](std::string string) {
         // Remove key delimiters from the front and back as they're added back in
         // the constructor
@@ -43,72 +163,40 @@ std::unique_ptr<StatsdClient> InitStatsClient(const ArgsManager& args)
         return string;
     };
 
-    return std::make_unique<StatsdClient>(args.GetArg("-statshost", DEFAULT_STATSD_HOST),
-                                          args.GetIntArg("-statsport", DEFAULT_STATSD_PORT),
-                                          args.GetIntArg("-statsbatchsize", DEFAULT_STATSD_BATCH_SIZE),
-                                          args.GetIntArg("-statsduration", DEFAULT_STATSD_DURATION),
-                                          sanitize_string(args.GetArg("-statsprefix", DEFAULT_STATSD_PREFIX)),
-                                          sanitize_string(args.GetArg("-statssuffix", DEFAULT_STATSD_SUFFIX)));
+    std::optional<bilingual_str> error_opt;
+    auto statsd_ptr = std::make_unique<StatsdClientImpl>(
+        host, port, batch_size, interval_ms,
+        sanitize_string(args.GetArg("-statsprefix", DEFAULT_STATSD_PREFIX)),
+        sanitize_string(args.GetArg("-statssuffix", DEFAULT_STATSD_SUFFIX)), error_opt);
+    if (error_opt.has_value()) {
+        statsd_ptr.reset();
+        return util::Error{error_opt.value()};
+    }
+    return {std::move(statsd_ptr)};
 }
 
-StatsdClient::StatsdClient(const std::string& host, uint16_t port, uint64_t batch_size, uint64_t interval_ms,
-                           const std::string& prefix, const std::string& suffix) :
+StatsdClientImpl::StatsdClientImpl(const std::string& host, uint16_t port, uint64_t batch_size, uint64_t interval_ms,
+                                   const std::string& prefix, const std::string& suffix,
+                                   std::optional<bilingual_str>& error) :
     m_prefix{[prefix]() { return !prefix.empty() ? prefix + STATSD_NS_DELIMITER : prefix; }()},
     m_suffix{[suffix]() { return !suffix.empty() ? STATSD_NS_DELIMITER + suffix : suffix; }()}
 {
-    if (host.empty()) {
-        LogPrintf("Transmitting stats are disabled, will not init StatsdClient\n");
-        return;
-    }
-
-    std::optional<std::string> error_opt;
     m_sender = std::make_unique<RawSender>(host, port,
                                            std::make_pair(batch_size, static_cast<uint8_t>(STATSD_MSG_DELIMITER)),
-                                           interval_ms, error_opt);
-    if (error_opt.has_value()) {
-        LogPrintf("ERROR: %s, cannot initialize StatsdClient.\n", error_opt.value());
+                                           interval_ms, error);
+    if (error.has_value()) {
         m_sender.reset();
         return;
     }
 
     LogPrintf("StatsdClient initialized to transmit stats to %s:%d\n", host, port);
 }
 
-StatsdClient::~StatsdClient() {}
-
-bool StatsdClient::dec(const std::string& key, float sample_rate) { return count(key, -1, sample_rate); }
-
-bool StatsdClient::inc(const std::string& key, float sample_rate) { return count(key, 1, sample_rate); }
-
-bool StatsdClient::count(const std::string& key, int64_t delta, float sample_rate)
-{
-    return send(key, delta, STATSD_METRIC_COUNT, sample_rate);
-}
-
-bool StatsdClient::gauge(const std::string& key, int64_t value, float sample_rate)
-{
-    return send(key, value, STATSD_METRIC_GAUGE, sample_rate);
-}
-
-bool StatsdClient::gaugeDouble(const std::string& key, double value, float sample_rate)
-{
-    return send(key, value, STATSD_METRIC_GAUGE, sample_rate);
-}
-
-bool StatsdClient::timing(const std::string& key, uint64_t ms, float sample_rate)
-{
-    return send(key, ms, STATSD_METRIC_TIMING, sample_rate);
-}
-
 template <typename T1>
-bool StatsdClient::send(const std::string& key, T1 value, const std::string& type, float sample_rate)
+inline bool StatsdClientImpl::_send(std::string_view key, T1 value, std::string_view type, float sample_rate)
 {
     static_assert(std::is_arithmetic<T1>::value, "Must specialize to an arithmetic type");
 
-    if (!m_sender) {
-        return false;
-    }
-
     // Determine if we should send the message at all but claim that we did even if we don't
     sample_rate = std::clamp(sample_rate, 0.f, 1.f);
     bool always_send = std::fabs(sample_rate - 1.f) < EPSILON;
@@ -125,16 +213,10 @@ bool StatsdClient::send(const std::string& key, T1 value, const std::string& typ
     }
 
     // Send it and report an error if we encounter one
-    if (auto error_opt = m_sender->Send(msg); error_opt.has_value()) {
-        LogPrintf("ERROR: %s.\n", error_opt.value());
+    if (auto error_opt = Assert(m_sender)->Send(msg); error_opt.has_value()) {
+        LogPrintf("ERROR: %s.\n", error_opt->original);
         return false;
     }
 
     return true;
 }
-
-template bool StatsdClient::send(const std::string& key, double value, const std::string& type, float sample_rate);
-template bool StatsdClient::send(const std::string& key, int32_t value, const std::string& type, float sample_rate);
-template bool StatsdClient::send(const std::string& key, int64_t value, const std::string& type, float sample_rate);
-template bool StatsdClient::send(const std::string& key, uint32_t value, const std::string& type, float sample_rate);
-template bool StatsdClient::send(const std::string& key, uint64_t value, const std::string& type, float sample_rate);