rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'#29418
rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'#29418vasild wants to merge 3 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29418. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
bc156d8 to
91c90d7
Compare
|
Concept ACK 91c90d759503aa3fa15b330bba2148698cdc271d. |
|
Needs rebase if still relevant |
|
just fyi as this came up: You could count up message stats with the inbound and outbound message tracepoints. However, this does only gives you stats from the points on where you start hooking into the tracepoints, might not be as user friendly (RPC is easier to use than the tracepoints), and the tracepoints currently only work on Linux. While I personally don't need it, I see why some node operators need traffic statistics. E.g. How much of my (limited monthly) traffic is used up by inbound connections? |
|
Converted to draft due to failing CI and inactivity |
|
|
…e constant b3efb48 protocol: make message types constexpr (Vasil Dimov) 2fa9de0 net: make the list of known message types a compile time constant (Vasil Dimov) Pull request description: Turn the `std::vector` to `std::array` because it is cheaper and allows us to have the number of the messages as a compile time constant: `ALL_NET_MESSAGE_TYPES.size()` which can be used in future code to build other `std::array`s with that size. --- This change is part of #29418 but it makes sense on its own and would be good to have it, regardless of the fate of #29418. Also, if this is merged, that would reduce the size of #29418, thus the current standalone PR. ACKs for top commit: achow101: ACK b3efb48 jonatack: ACK b3efb48 maflcko: utACK b3efb48 🎊 willcl-ark: ACK b3efb48 Tree-SHA512: 6d3860c138c64514ebab13d97ea67893e2d346dfac30a48c3d9bc769a1970407375ea4170afdb522411ced306a14a9af4eede99e964d1fb1ea3efff5d5eb57af
|
|
danielabrozzoni
left a comment
There was a problem hiding this comment.
Concept ACK, reviewed up to 5ad1f5d, I still haven't finished the review of the last commit
I wonder if in the functional test, instead of hard-coding the message stats values, we could check how many messages/bytes are sent using P2PInterface... I'm not sure how this would look like or if it's possible at all, I'll try it out and report back :)
|
Ping @vasild for rebase |
|
@vasild are you still working on this? |
|
Yes. This and a few others got pushed way back on my TODO list, slowly processing it, thanks for the patience. |
There was a problem hiding this comment.
Concept ACK. Seems like useful info to expose.
Approach-wise, I like that it adds very little logic in the RPC layer. I quite strongly opppose the aggregation option, just a simple bitcoin-cli getnetmsgstats provides a user all the info they need. Visualization/aggregation can easily be done by the user, e.g. using jq or any other tool / aggregation they like, so adding that to our interface unnecessarily locks us into backwards compatibility promises and code/interface complexity (even if it's not a crazy amount).
I'm not sure I'll review this in-depth anytime soon, but will try to keep engaging on high-level.
Before this change only per-peer stats were gathered. They are lost when the peer disconnects. So, collect the traffic stats globally in `CConnman`, broken down by * direction (sent or received, (2)) * network of the peer (IPv4, IPv6, Tor, I2P, CJDNS (5)) * connection type (inbound, full outbound, feeler, etc, (6)) * message type (verack, ping, etc, (36))
Introduce a new RPC `getnetmsgstats` that renders the global traffic stats from `CConnman`. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
|
Alright, that's it. I was thinking the same for some time now - "Should we do aggregation on the server side, given that it is more like a visualization/presentation thing?". Thanks for pushing here. I have now dropped the aggregation, which means removing commit d5a1ad9 Thanks! |
…s a compile time constant 68c9212 protocol: make message types constexpr (Vasil Dimov) 2eec318 net: make the list of known message types a compile time constant (Vasil Dimov) Pull request description: Turn the `std::vector` to `std::array` because it is cheaper and allows us to have the number of the messages as a compile time constant: `ALL_NET_MESSAGE_TYPES.size()` which can be used in future code to build other `std::array`s with that size. --- This change is part of bitcoin/bitcoin#29418 but it makes sense on its own and would be good to have it, regardless of the fate of bitcoin/bitcoin#29418. Also, if this is merged, that would reduce the size of bitcoin/bitcoin#29418, thus the current standalone PR. ACKs for top commit: achow101: ACK 68c9212 jonatack: ACK 68c9212 maflcko: utACK 68c9212 🎊 willcl-ark: ACK 68c9212 Tree-SHA512: 6d3860c138c64514ebab13d97ea67893e2d346dfac30a48c3d9bc769a1970407375ea4170afdb522411ced306a14a9af4eede99e964d1fb1ea3efff5d5eb57af
…s a compile time constant 2dc6997 protocol: make message types constexpr (Vasil Dimov) df89ced net: make the list of known message types a compile time constant (Vasil Dimov) Pull request description: Turn the `std::vector` to `std::array` because it is cheaper and allows us to have the number of the messages as a compile time constant: `ALL_NET_MESSAGE_TYPES.size()` which can be used in future code to build other `std::array`s with that size. --- This change is part of bitcoin/bitcoin#29418 but it makes sense on its own and would be good to have it, regardless of the fate of bitcoin/bitcoin#29418. Also, if this is merged, that would reduce the size of bitcoin/bitcoin#29418, thus the current standalone PR. ACKs for top commit: achow101: ACK 2dc6997 jonatack: ACK 2dc6997 maflcko: utACK 2dc6997 🎊 willcl-ark: ACK 2dc6997 Tree-SHA512: 6d3860c138c64514ebab13d97ea67893e2d346dfac30a48c3d9bc769a1970407375ea4170afdb522411ced306a14a9af4eede99e964d1fb1ea3efff5d5eb57af
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Introduce a new RPC,
getnetmsgstatsto retrieve traffic bytes and count of messages possibly broken down by:Implements: #26337 Add per message stats to getnettotals rpc
Examples:
bitcoin-cli getnetmsgstats '["direction", "network", "connection_type", "message_type"]'
bitcoin-cli getnetmsgstats '["direction", "network", "connection_type"]'
bitcoin-cli getnetmsgstats '["direction", "connection_type", "message_type"]'
bitcoin-cli getnetmsgstats
Previous incarnations of this:
#27534 rpc: add 'getnetmsgstats', new rpc to view network message statistics (Thank you, @satsie!)
#28926 rpc: add 'getnetmsgstats' RPC (Thank you, @willcl-ark!)