-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(stats): modernize statsd::StatsdClient
, make global unique_ptr
#5167
Conversation
5fd7e65
to
6fa6ac3
Compare
This pull request has conflicts, please rebase. |
39e5bcf
to
8dc657b
Compare
This pull request has conflicts, please rebase. |
This pull request has conflicts, please rebase. |
There's little sense in passing a ref to `ArgsManager` just to set a few values because we'll be `const`-ing them in an upcoming commit. Arguments supplied are expected to last the lifetime of the program's instance and there's little reason to keep re-fetching those values.
There doesn't seem to be a benefit to keeping these values in a separate struct.
src/init.cpp
Outdated
@@ -289,6 +289,7 @@ void PrepareShutdown(NodeContext& node) | |||
node.banman.reset(); | |||
node.addrman.reset(); | |||
node.netgroupman.reset(); | |||
::statsClient.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: stats:
is not a valid scope
7c153c2
to
e2ca375
Compare
statsd::StatsdClient
, make global unique_ptr
, allow connections to IPv6 hostsstatsd::StatsdClient
, make global unique_ptr
, allow connections to IPv6 hosts
- 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
statsd::StatsdClient
, make global unique_ptr
, allow connections to IPv6 hostsstatsd::StatsdClient
, make global unique_ptr
, allow connections to IPv6 hosts
statsd::StatsdClient
, make global unique_ptr
, allow connections to IPv6 hostsstatsd::StatsdClient
, make global unique_ptr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK cc998ab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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:
statsd::StatsdClient
.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)CService
.std::string
and our string manipulation capabilities (replacingsnprintf
withstrprintf
), replacing platform-specific types (replacingshort
withuint16_t
).Breaking Changes
None observed.
Checklist: