feat(stats): split off transmission to `RawSender`, implement batching and queueing support, add streamlined prefix and suffix support #6267

kwvg · 2024-09-11T09:41:23Z

Motivation

This pull request achieves the goal originally set out in dash#5167, to migrate the base of our Statsd client implementation to one that is actively maintained. Statoshi (source) utilizes talebook/statsd-client-cpp, which in turn is inherited by Dash.

As Statsd is the only cross-platform reporting mechanism available (USDT requires a Linux host with superuser privileges and RPCs don't provide as much flexibility as desired), emphasis is placed on using Statsd as the primary way for the Dash daemon to report metrics related to node and network health.

As part of maintaining our Statsd client, this PR aims to migrate the base of our implementation to vthiery/cpp-statsd-client, a streamlined implementation that embraces C++11 with a thread-safe implementation of queueing and batching, which reduces the number of packets used to transmit stats.

These capabilities are optional and users can opt not to use them by setting -statsduration=0 to disable queueing (which will also disable batching) or -statsbatchsize=0, which will disable batching (but will not disable queueing unless requested explicitly).

Additional Information

Dependent on refactor(stats): modernize statsd::StatsdClient, make global unique_ptr #5167
RawSender (and by extension, RawMessage) strive to remain as unopinionated as possible, moving the responsibility to construct valid Statsd messages onto StatsdClient. This is to ensure that RawSender can be reused down the line or independently extended without impacting the Statsd client.
- RawMessage exists to provide extensions to std::vector<uint8_t> that make it easier to abstract away strings while also implementing some of its semantics like append() (and its alias, +=).
InitStatsClient() was introduced to keep StatsdClient indifferent to how arguments are obtained and sanitized before they're supplied to the constructor. This is to keep it indifferent to all the backwards-compatibility code for deprecated arguments still work.
When constructing the Statsd message, we can use %f without having to specify a precision as tinyformat automatically assumes a precision of 6 (source) and problems don't seem to be observed when using %f with integers (source).
- As a guardrail, there is a static_assert to ensure that a specialization of send() involving a non-arithmetic type will raise alarm when compiling (source).

Breaking changes

-statsenabled (replaced with specifying -statshost), -statshostname (replaced by -statssuffix) and -statsns (replaced by -statsprefix) have been deprecated and will be removed in a future release.

Checklist:

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

4faf35f chore: add `stats` as a pull request header scope (Kittywhiskers Van Gogh) Pull request description: ## Additional Information Would allow tagging #5167, #6267 and #6237 as `feat(stats)`. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code **(note: N/A)** - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ **(note: N/A)** ACKs for top commit: PastaPastaPasta: utACK 4faf35f UdjinM6: utACK 4faf35f thephez: utACK 4faf35f Tree-SHA512: 25cc28792351852b9e920d980b8814d93274bc53d22ce63fb1a5bf32821b10902d22b384a408e1c4a7b97239e53e235c45ac008d0f82e1afe5d6071b392acb47

github-actions · 2024-09-11T16:24:21Z

This pull request has conflicts, please rebase.

UdjinM6 · 2024-09-12T07:34:57Z

Should update test/util/data/non-backported.txt

diff --git a/test/util/data/non-backported.txt b/test/util/data/non-backported.txt
index 150bbb70c1..7684074874 100644
--- a/test/util/data/non-backported.txt
+++ b/test/util/data/non-backported.txt
@@ -29,7 +29,8 @@ src/rpc/quorums.cpp
 src/saltedhasher.*
 src/spork.*
 src/stacktraces.*
-src/statsd_client.*
+src/stats/*.cpp
+src/stats/*.h
 src/test/block_reward_reallocation_tests.cpp
 src/test/bls_tests.cpp
 src/test/dip0020opcodes_tests.cpp

UdjinM6 · 2024-09-12T07:54:24Z

not sure if can actually do d42c88f

develop:
network.terahashesPerSecond:0.000000|g
network.petahashesPerSecond:0.000000|g
network.exahashesPerSecond:0.000000|g

this PR:
network.terahashesPerSecond:1.03185e-07|g
network.petahashesPerSecond:1.03185e-10|g
network.exahashesPerSecond:1.03185e-13|g

UdjinM6 · 2024-09-12T08:25:09Z

old ideas

~~could probably make it work with smth like~~

diff --git a/src/stats/client.cpp b/src/stats/client.cpp
index cdcf36d281..b536a44db3 100644
--- a/src/stats/client.cpp
+++ b/src/stats/client.cpp
@@ -157,6 +157,8 @@ bool StatsdClient::send(const std::string& key, T1 value, const std::string& typ
         return true;
     }
 
+    if (value < EPSILON) value = 0;
+
     // Construct the message and if our message isn't always-send, report the sample rate
     std::string buf{strprintf("%s%s%s:%s|%s", m_prefix, key, m_suffix, ToString(value), type)};
     if (frequency < 1.f) {

which results in
network.terahashesPerSecond:0|g
network.petahashesPerSecond:0|g
network.exahashesPerSecond:0|g

~~a better option would probably be smth like~~

diff --git a/src/stats/client.cpp b/src/stats/client.cpp
index cdcf36d281..832085c8ef 100644
--- a/src/stats/client.cpp
+++ b/src/stats/client.cpp
@@ -6,7 +6,6 @@
 
 #include <stats/client.h>
 
-#include <util/string.h>
 #include <util/system.h>
 
 #include <cmath>
@@ -25,6 +24,7 @@ static constexpr char STATSD_NS_DELIMITER{'.'};
 static constexpr char STATSD_METRIC_COUNT[]{"c"};
 /** Character used to denote Statsd message type as gauge */
 static constexpr char STATSD_METRIC_GAUGE[]{"g"};
+static constexpr char STATSD_METRIC_GAUGE_DOUBLE[]{"g"};
 /** Characters used to denote Statsd message type as timing */
 static constexpr char STATSD_METRIC_TIMING[]{"ms"};
 } // anonymous namespace
@@ -133,7 +133,7 @@ bool StatsdClient::gauge(const std::string& key, int64_t value, float frequency)
 
 bool StatsdClient::gaugeDouble(const std::string& key, double value, float frequency)
 {
-    return send(key, value, STATSD_METRIC_GAUGE, frequency);
+    return send(key, value, STATSD_METRIC_GAUGE_DOUBLE, frequency);
 }
 
 bool StatsdClient::timing(const std::string& key, uint64_t ms, float frequency)
@@ -158,7 +158,12 @@ bool StatsdClient::send(const std::string& key, T1 value, const std::string& typ
     }
 
     // Construct the message and if our message isn't always-send, report the sample rate
-    std::string buf{strprintf("%s%s%s:%s|%s", m_prefix, key, m_suffix, ToString(value), type)};
+    std::string buf;
+    if (type == STATSD_METRIC_GAUGE_DOUBLE) {
+        buf = strprintf("%s%s%s:%f|%s", m_prefix, key, m_suffix, value, type);
+    } else {
+        buf = strprintf("%s%s%s:%d|%s", m_prefix, key, m_suffix, value, type);
+    }
     if (frequency < 1.f) {
         buf += strprintf("|@%.2f", frequency);
     }

actually, strprintf is smart enough for this to work exactly as before

diff --git a/src/stats/client.cpp b/src/stats/client.cpp
index cdcf36d281..55f5f458c0 100644
--- a/src/stats/client.cpp
+++ b/src/stats/client.cpp
@@ -6,7 +6,6 @@
 
 #include <stats/client.h>
 
-#include <util/string.h>
 #include <util/system.h>
 
 #include <cmath>
@@ -158,7 +157,7 @@ bool StatsdClient::send(const std::string& key, T1 value, const std::string& typ
     }
 
     // Construct the message and if our message isn't always-send, report the sample rate
-    std::string buf{strprintf("%s%s%s:%s|%s", m_prefix, key, m_suffix, ToString(value), type)};
+    std::string buf{strprintf("%s%s%s:%.6f|%s", m_prefix, key, m_suffix, value, type)};
     if (frequency < 1.f) {
         buf += strprintf("|@%.2f", frequency);
     }

%f works too... for all int-s and float.. thanks to tinyformat internal magic :D

src/stats/rawsender.cpp

src/stats/rawsender.h

github-actions · 2024-09-12T14:44:00Z

This pull request has conflicts, please rebase.

In upcoming commits, message sending will be split off into a separate file and stats capabilities will be fleshed out. Prepare for that by giving it its own directory. Also get rid of `statsd` namespace, it is entirely unnecessary. Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

test/lint/lint-cppcheck-dash.sh

knst

UB due to noexcept should be fixed before merging it

src/statsd_client.h

src/stats/rawsender.cpp

knst · 2024-10-03T11:44:37Z

src/stats/rawsender.cpp

if you can't open socker or create m_sock, the RawSender is useless object.

Instead, just throw exception from constructor and after that you don't need to check anything like if (m_sock) because object is always valid.

see relevant comment for StatsdClient

Would like to avoid exceptions where possible, StatsdClient already prints errors generated and reaps the inactive object immediately (source), think it's fine as-is.

knst · 2024-10-03T11:46:17Z

src/stats/client.cpp

stats: move message sending logic to RawSender

instead having here std::optional just throw exception from RawSender constructor;
If m_sender is created - it's always valid. If m_sender is not created - it's nullptr and code is simpler

knst · 2024-10-03T11:47:26Z

src/stats/rawsender.cpp

if socket is not created, no need to count failures of sending data: all of the will be failed;

The success and failure counters also serve as a way to count the number of requests sent for processing, even if they all eventually fail. Think it's fine, as-is.

src/stats/client.h

knst · 2024-10-03T11:54:48Z

src/stats/rawsender.h

Consider using tl:unexpected instead from util/expected.h

using RawSenderError = tl::expected<void, std::error>; ... return {}; // success return std::string{"ERROR"}; // failure

knst · 2024-10-03T11:56:17Z

src/stats/rawsender.h

here's only one possible m_server can be, use instead:

-std::pair<struct sockaddr_storage, socklen_t> m_server{{}, sizeof(struct sockaddr_storage)}; +struct sockaddr_storagem_server{}; +socklen_t server_len{sizeof(struct sockaddr_storage)};

I would rather they be one variable.

I would rather they be one variable.

why? The only purpose for all class Sock is to be a wrapper around this m_server

m_server has nothing to do with the creation of the SOCKET or the subsequent m_socks wrapper, considering that IPv4 UDP params are hardcoded (source). It is populated when resolving the address (source) and then used by ::sendto (source). The former requires taking size information as a pointer, requiring us to store it as a variable.

It's fine as-is.

src/stats/rawsender.h

knst · 2024-10-03T12:27:34Z

src/init.cpp

add TODO to remove it in v23

Already have a PR that gets rid of legacy code, can open it and mark as draft until we're in the merging window for that version, that okay?

src/test/util/setup_common.cpp

knst · 2024-10-04T07:09:16Z

src/stats/rawsender.h

RawSender is not a good name for this class, because it's just std::vector and has nothing to do with 'RawSender'.

Move this one to client.h and maybe rename it to 'RawStat' or StatRawMessage or something similar.

RawMessage was made explicitly for RawSender (source, source), please refer to discussion on RawMessage (source).

knst · 2024-10-04T07:10:26Z

src/stats/rawsender.h

RawSender is a global namespace is not a good name, when it has nothing to do without stats, it's not in utils.

let's re-introduce back namespace stats maybe? also you can move RawSender completely inside client.cpp, it's no need to be in it's own module.

RawSender (and by extension, RawMessage) strive to remain as unopinionated as possible, moving the responsibility to construct valid Statsd messages onto StatsdClient. This is to ensure that RawSender can be reused down the line or independently extended without impacting the Statsd client.

Please refer to PR description for reasoning behind it being split out.

knst · 2024-10-04T07:25:23Z

src/stats/rawsender.cpp

try to use a helper CreateSockTCP from netbase.h instead re-writing low-level code

CreateSockTCP, as the name suggests, is used for TCP connections. Currently Statsd messages are transmitted over UDP. One of the earlier drafts of #5167 did create a CreateSockUDP but it would not only make future backports affecting netbase.{cpp,h} painful, it was clunky from the get-go.

Furthermore, Sock only envisions the usage of TCP, providing only a wrapper for ::send() (and not ::sendto(), which is generally an applicable way to send messages when using UDP) (source)

`RawSender` is inspired by `UDPSender` from `vthiery/cpp-statsd-client` and separating it out of `StatsdClient` is needed to implement queueing and batching support in upcoming commits. This is the start of migrating our Statsd codebase to `cpp-statsd-client`.

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

Additionally, let's remove the key sanitation logic since keys are hardcoded. Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

- `timing` cannot contain a negative value, change to `uint64_t` - remove `sendDouble`, use template specialization of `Send` instead, that can be reused as `send` - rename `value` in `count()` to `delta` - add more comments `gaugeDouble` has been kept around because utilizing templates for it would require us to either write template specializations for every kind of `gauge` value (implicit conversions will not save us from this) or move all logic to the header, neither option seems desirable.

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.

`statshostname` implies that alongside specifying the host, the user needs to specify a separate "hostname" field in order to connect when in fact, it is an entirely optional suffix field applied to stats keys. Rename it to `statssuffix` and deprecate `statshostname`, the latter will be removed in a subsequent major release.

`statsns`, aside from being an unclear name, in its default behaviour, doesn't use the namespace delimiter contaminates the root namespace of the key. Let's take care of that by deprecating `statsns` with its replacement, `statsprefix`, that behaves properly.

This marks the completion of our transition from code based on `talebook/statsd-client-cpp` to code based on `vthiery/cpp-statsd-client`. Also, we long stopped using `snprintf`, remove it from exclusions list.

knst

utACK cc1a75a

but I'd probably provide later extra PR to address my comments. I don't want to block this PR by nits and refactorings

UdjinM6

utACK cc1a75a

PastaPastaPasta · 2024-10-04T18:00:30Z

does NOT LGTM yet cc1a75a

, but I have no objections to get it merged now

what does this mean? Do you approve or not?

knst · 2024-10-04T18:09:07Z

what does this mean? Do you approve or not?

edited message to make it more clear.

kwvg added this to the 21.2 milestone Sep 11, 2024

kwvg mentioned this pull request Sep 11, 2024

chore: add stats as a pull request header type and scope #6262

Merged

5 tasks

kwvg force-pushed the statsd2 branch 2 times, most recently from 1b1c768 to 1b87c07 Compare September 11, 2024 12:18

github-actions bot added the needs rebase label Sep 11, 2024

kwvg force-pushed the statsd2 branch from 1b87c07 to 9ecf1fe Compare September 11, 2024 18:19

github-actions bot removed the needs rebase label Sep 11, 2024

kwvg force-pushed the statsd2 branch from 9ecf1fe to 84ed624 Compare September 11, 2024 18:27

kwvg requested review from PastaPastaPasta and UdjinM6 September 11, 2024 18:48

kwvg force-pushed the statsd2 branch from 84ed624 to 4d90597 Compare September 12, 2024 06:42

UdjinM6 reviewed Sep 12, 2024

View reviewed changes

src/stats/rawsender.cpp Outdated Show resolved Hide resolved

UdjinM6 reviewed Sep 12, 2024

View reviewed changes

src/stats/rawsender.h Outdated Show resolved Hide resolved

src/stats/rawsender.h Outdated Show resolved Hide resolved

github-actions bot added the needs rebase label Sep 12, 2024

kwvg and others added 2 commits September 12, 2024 14:53

stats: remove double indentation in header file

f782dfd

kwvg force-pushed the statsd2 branch from 4d90597 to a0ce720 Compare September 12, 2024 16:50

github-actions bot removed the needs rebase label Sep 12, 2024

kwvg force-pushed the statsd2 branch from a0ce720 to 9c79081 Compare September 12, 2024 17:04

kwvg force-pushed the statsd2 branch from 9c79081 to 8a20f46 Compare September 12, 2024 17:14

kwvg requested a review from UdjinM6 September 12, 2024 17:40

UdjinM6 reviewed Sep 12, 2024

View reviewed changes

test/lint/lint-cppcheck-dash.sh Outdated Show resolved Hide resolved

kwvg force-pushed the statsd2 branch from 8a20f46 to fb06d0d Compare September 13, 2024 06:46

kwvg force-pushed the statsd2 branch from d9974fa to aad67b0 Compare September 15, 2024 08:35

UdjinM6 requested a review from knst September 27, 2024 14:59

knst requested changes Oct 3, 2024

View reviewed changes

kwvg dismissed UdjinM6’s stale review via ad63686 October 4, 2024 06:51

kwvg force-pushed the statsd2 branch from aad67b0 to ad63686 Compare October 4, 2024 06:51

knst reviewed Oct 4, 2024

View reviewed changes

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

knst reviewed Oct 4, 2024

View reviewed changes

kwvg requested review from UdjinM6 and knst October 4, 2024 08:09

kwvg and others added 10 commits October 4, 2024 08:47

feat(stats): introduce support for queuing messages

38b1643

feat(stats): introduce support for batching messages

bf44fc3

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

stats: deduplicate send and sendDouble logic

3e12ac0

Additionally, let's remove the key sanitation logic since keys are hardcoded. Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

stats: implicitly treat stats as enabled if statshost is specified

f3a4844

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.

stats: drop copyright notice from stats/client.cpp

39625f1

This marks the completion of our transition from code based on `talebook/statsd-client-cpp` to code based on `vthiery/cpp-statsd-client`. Also, we long stopped using `snprintf`, remove it from exclusions list.

docs: add release notes

cc1a75a

kwvg force-pushed the statsd2 branch from ad63686 to cc1a75a Compare October 4, 2024 08:49

knst approved these changes Oct 4, 2024

View reviewed changes

UdjinM6 approved these changes Oct 4, 2024

View reviewed changes

PastaPastaPasta merged commit 251f16b into dashpay:develop Oct 4, 2024

UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024

kwvg mentioned this pull request Sep 9, 2025

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

Merged

5 tasks

feat(stats): split off transmission to RawSender, implement batching and queueing support, add streamlined prefix and suffix support #6267

feat(stats): split off transmission to RawSender, implement batching and queueing support, add streamlined prefix and suffix support #6267

Uh oh!

Conversation

kwvg commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Additional Information

Breaking changes

Checklist:

Uh oh!

github-actions bot commented Sep 11, 2024

Uh oh!

UdjinM6 commented Sep 12, 2024

Uh oh!

UdjinM6 commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 12, 2024

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Oct 4, 2024

Uh oh!

feat(stats): split off transmission to `RawSender`, implement batching and queueing support, add streamlined prefix and suffix support #6267

feat(stats): split off transmission to `RawSender`, implement batching and queueing support, add streamlined prefix and suffix support #6267

kwvg commented Sep 11, 2024 •

edited

Loading

UdjinM6 commented Sep 12, 2024 •

edited

Loading

UdjinM6 commented Sep 12, 2024 •

edited

Loading

knst left a comment •

edited

Loading