refactor(stats): modernize `statsd::StatsdClient`, make global `unique_ptr` #5167

kwvg · 2023-01-18T09:44:21Z

Additional Information

Support for transmitting stats to a Statsd server has been courtesy of Statoshi (repo), implemented Dec, 2020 by dash#2515 but since then, it hasn't gotten much attention aside from benefiting from codebase-wide changes and the occasional compiler appeasement. This pull request aims to give our statistics code some TLC.

Changes include:

Limiting initialization to solely during construction and moving the responsibility of fetching arguments outside of statsd::StatsdClient.
Using the RAII Socks wrapper as early as possible (we still need to construct a raw socket ourselves but this is done in the initializer and control is moved to the wrapper and everywhere else, the wrapper is used)
Utilizing existing networking code to generate the socket address
- This lets us trivially allow IPv6 connections as the responsibility to construct it safely is moved to CService.
Using std::string and our string manipulation capabilities (replacing snprintf with strprintf), replacing platform-specific types (replacing short with uint16_t).

Breaking Changes

None observed.

Checklist:

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

github-actions · 2023-01-19T18:38:08Z

This pull request has conflicts, please rebase.

github-actions · 2023-02-27T21:14:05Z

This pull request has conflicts, please rebase.

github-actions · 2023-04-04T17:45:01Z

This pull request has conflicts, please rebase.

PastaPastaPasta · 2024-09-10T02:51:33Z

src/init.cpp

    node.banman.reset();
    node.addrman.reset();
    node.netgroupman.reset();
+    ::statsClient.reset();


nit: stats: is not a valid scope

- Use `uint16_t` instead of `short`, `int64_t` instead of `size_t` - Get rid of the `errmsg` buffer and use `LogPrintf` to report errors - Use `strprintf` instead of `snprintf` - Rephrase networking error logs to allow inclusion of error strings

We can skip the computationally expensive dice-roll if our sample rate is zero as that means we should never send it. Also get rid of the `FastRandomContext::operator()` that isn't used anywhere else in the codebase.

Also, add log messages to inform us if we're skipping initialization or it has successfully been initialized.

As creating the socket is now the last step, we don't need `m_init` anymore. We can just see if a socket is successfully constructed and take that as our validity indicator. We'll also move it out of the inner `send` function so we can bail out before we bother with all the string processing and manipulation.

We cannot convert `DEFAULT_STATSD*` to `std::string_view`s as they're being used as default arguments and `GetArgs` expects `std::string`s

PastaPastaPasta

utACK cc998ab

UdjinM6

utACK cc998ab

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

, bitcoin#25619, bitcoin#27214, bitcoin#26261, bitcoin#27745, bitcoin#27529, bitcoin#28341, partial bitcoin#25331 (addrman backports: part 4) e544d3c fmt: apply `clang-format-diff.py` suggestions, satisfy linter (Kittywhiskers Van Gogh) 7da74ff merge bitcoin#28341: Use HashWriter over legacy CHashWriter (Kittywhiskers Van Gogh) c798b49 merge bitcoin#27529: fix `feature_addrman.py` on big-endian systems (Kittywhiskers Van Gogh) 7d149c9 merge bitcoin#27745: select addresses by network follow-up (Kittywhiskers Van Gogh) 1d82994 merge bitcoin#26261: cleanup `LookupIntern`, `Lookup` and `LookupHost` (Kittywhiskers Van Gogh) 231ff82 merge bitcoin#27214: Enable selecting addresses by network (Kittywhiskers Van Gogh) e825595 merge bitcoin#25619: avoid overriding non-virtual ToString() in CService and use better naming (Kittywhiskers Van Gogh) 2e9b48a merge bitcoin#26847: track AddrMan totals by network and table, improve precision of adding fixed seeds (Kittywhiskers Van Gogh) 79a550e merge bitcoin#26040: comment "add only reachable addresses to addrman" (Kittywhiskers Van Gogh) 1adb635 merge bitcoin#25678: skip querying dns seeds if -onlynet disables IPv4 and IPv6 (Kittywhiskers Van Gogh) 2d99be0 partial bitcoin#25331: Add HashWriter without ser-type and ser-version (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6243 * Dependent on #5167 ## Breaking Changes None observed. ## Checklist: - [x] I have performed a self-review of my own code - [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 - [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)_ ACKs for top commit: UdjinM6: utACK e544d3c Tree-SHA512: 63f014142c39c47bda3ac85dc6afeee8f2bfec71f033631bca16d41bb0785f4b090b3c860ddc3b3cf6c4a23558d3d102144fc83b065130c3f9ab91d0de8e4457

…ement batching and queueing support, add streamlined prefix and suffix support cc1a75a docs: add release notes (Kittywhiskers Van Gogh) 39625f1 stats: drop copyright notice from `stats/client.cpp` (Kittywhiskers Van Gogh) 18a2e48 stats: rename `statsns` to clearer `statsprefix` (Kittywhiskers Van Gogh) 42918c2 stats: rename `statshostname` to more appropriate `statssuffix` (Kittywhiskers Van Gogh) f3a4844 stats: implicitly treat stats as enabled if `statshost` is specified (Kittywhiskers Van Gogh) 69603a8 stats: miscellaneous changes and housekeeping (Kittywhiskers Van Gogh) 3e12ac0 stats: deduplicate `send` and `sendDouble` logic (Kittywhiskers Van Gogh) bf44fc3 feat(stats): introduce support for batching messages (Kittywhiskers Van Gogh) 38b1643 feat(stats): introduce support for queuing messages (Kittywhiskers Van Gogh) fc4a736 stats: move message sending logic to `RawSender` (Kittywhiskers Van Gogh) 9269068 stats: move `statsd_client` to `stats` directory (Kittywhiskers Van Gogh) f782dfd stats: remove double indentation in header file (Kittywhiskers Van Gogh) Pull request description: ## Motivation This pull request achieves the goal originally set out in [dash#5167](#5167), to migrate the base of our Statsd client implementation to one that is actively maintained. Statoshi ([source](https://github.com/jlopp/statoshi/blob/54c3ffdcf0f85af1795d5398ca362744464a7b06/src/statsd_client.cpp)) utilizes [talebook/statsd-client-cpp](https://github.com/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](https://github.com/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 #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](https://github.com/dashpay/dash/blob/17110f50b3adb1b8a51348259e752a760ab3ee0a/src/tinyformat.h#L673)) and problems don't seem to be observed when using `%f` with integers ([source](#6267 (comment))). * 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](https://github.com/dashpay/dash/blob/a0ce720207cf010cf8af8be7acd30561ed970b30/src/stats/client.cpp#L145)). ## 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: - [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 **(note: N/A)** - [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: knst: utACK cc1a75a UdjinM6: utACK cc1a75a Tree-SHA512: b038419f2b6d807dac40a04d23c5046fbaa95beedb88f5a9e4c06a7042c2f5da7e01c72c4a2744bce10878cafc747136d6599dcd86ae1be0782ad4194d5b7bec

kwvg force-pushed the statsd branch 9 times, most recently from 5fd7e65 to 6fa6ac3 Compare January 18, 2023 13:27

github-actions bot added the needs rebase label Jan 19, 2023

kwvg force-pushed the statsd branch from 6fa6ac3 to e64ac7a Compare February 20, 2023 19:01

github-actions bot removed the needs rebase label Feb 20, 2023

kwvg force-pushed the statsd branch 6 times, most recently from 39e5bcf to 8dc657b Compare February 24, 2023 08:19

github-actions bot added the needs rebase label Feb 27, 2023

kwvg force-pushed the statsd branch from 8dc657b to 5527932 Compare March 1, 2023 07:22

github-actions bot removed the needs rebase label Mar 1, 2023

github-actions bot added the needs rebase label Apr 4, 2023

kwvg force-pushed the statsd branch from 5527932 to 50f27bd Compare April 5, 2023 09:14

github-actions bot removed the needs rebase label Apr 5, 2023

kwvg force-pushed the statsd branch from 50f27bd to cbfd6cd Compare April 5, 2023 21:30

PastaPastaPasta mentioned this pull request Apr 17, 2023

backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 #5309

Merged

3 tasks

github-actions bot added the needs rebase label Apr 17, 2023

docs: add copyright notice to source file, update notice in header

85890dd

kwvg mentioned this pull request Sep 6, 2024

backport: merge bitcoin#25678, #26040, #26847, #25619, #27214, #26261, #27745, #27529, #28341, partial bitcoin#25331 (addrman backports: part 4) #6254

Merged

5 tasks

kwvg force-pushed the statsd branch from c12c19d to cb32769 Compare September 6, 2024 21:22

kwvg requested a review from PastaPastaPasta September 6, 2024 22:27

PastaPastaPasta reviewed Sep 10, 2024

View reviewed changes

kwvg mentioned this pull request Sep 10, 2024

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

Merged

5 tasks

kwvg force-pushed the statsd branch 2 times, most recently from 7c153c2 to e2ca375 Compare September 10, 2024 19:08

kwvg mentioned this pull request Sep 11, 2024

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

Merged

5 tasks

kwvg changed the title ~~refactor: modernize statsd::StatsdClient, make global unique_ptr, allow connections to IPv6 hosts~~ feat(stats): modernize statsd::StatsdClient, make global unique_ptr, allow connections to IPv6 hosts Sep 11, 2024

kwvg added 7 commits September 11, 2024 14:33

stats: clean up randomization code, move FastRandomContext inward

4bc727c

We can skip the computationally expensive dice-roll if our sample rate is zero as that means we should never send it. Also get rid of the `FastRandomContext::operator()` that isn't used anywhere else in the codebase.

stats: move init logic into constructor

2def905

Also, add log messages to inform us if we're skipping initialization or it has successfully been initialized.

stats: use Socks wrapper, use CService to generate our sockaddr

dbbfc8d

stats: stop using error codes, switch over to bool

9f96723

stats: const-ify variables and arguments

0401c58

We cannot convert `DEFAULT_STATSD*` to `std::string_view`s as they're being used as default arguments and `GetArgs` expects `std::string`s

kwvg force-pushed the statsd branch from e2ca375 to 0401c58 Compare September 11, 2024 14:47

kwvg requested a review from PastaPastaPasta September 11, 2024 14:47

kwvg changed the title ~~feat(stats): modernize statsd::StatsdClient, make global unique_ptr, allow connections to IPv6 hosts~~ refactor(stats): modernize statsd::StatsdClient, make global unique_ptr, allow connections to IPv6 hosts Sep 11, 2024

kwvg changed the title ~~refactor(stats): modernize statsd::StatsdClient, make global unique_ptr, allow connections to IPv6 hosts~~ refactor(stats): modernize statsd::StatsdClient, make global unique_ptr Sep 11, 2024

fmt: apply clang-format-diff.py suggestions

cc998ab

PastaPastaPasta approved these changes Sep 11, 2024

View reviewed changes

UdjinM6 approved these changes Sep 11, 2024

View reviewed changes

PastaPastaPasta merged commit 96685be into dashpay:develop Sep 11, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(stats): modernize `statsd::StatsdClient`, make global `unique_ptr` #5167

refactor(stats): modernize `statsd::StatsdClient`, make global `unique_ptr` #5167

Uh oh!

kwvg commented Jan 18, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Jan 19, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

github-actions bot commented Apr 4, 2023

Uh oh!

PastaPastaPasta Sep 10, 2024

Uh oh!

PastaPastaPasta left a comment

Uh oh!

UdjinM6 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor(stats): modernize statsd::StatsdClient, make global unique_ptr #5167

refactor(stats): modernize statsd::StatsdClient, make global unique_ptr #5167

Uh oh!

Conversation

kwvg commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Information

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Jan 19, 2023

Uh oh!

github-actions bot commented Feb 27, 2023

Uh oh!

github-actions bot commented Apr 4, 2023

Uh oh!

PastaPastaPasta Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor(stats): modernize `statsd::StatsdClient`, make global `unique_ptr` #5167

refactor(stats): modernize `statsd::StatsdClient`, make global `unique_ptr` #5167

kwvg commented Jan 18, 2023 •

edited

Loading