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 4 commits intobitcoin:masterfrom
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_5_m |
|
🚧 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. |
vasild
force-pushed
the
getnetmsgstats
branch
from
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
|
|
|
|
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))
vasild
force-pushed
the
getnetmsgstats
branch
from
528b4d6 to
87d3d64
Compare
|
|
Introduce a new RPC `getnetmsgstats` that renders the global traffic stats from `CConnman`. Co-authored-by: Vasil Dimov <[email protected]>
Aggregation can be by either one of direction, network, connection type
or message type. For example if the following
```
{
"ipv4": { "ping": 3 },
"ipv6": { "ping": 4 }
}
```
is aggregated by network, then the result will be
```
{
"ping": 7
}
```
Co-authored-by: Vasil Dimov <[email protected]>
vasild
force-pushed
the
getnetmsgstats
branch
from
87d3d64 to
7d4e82c
Compare
|
|
|
Concept ACK Useful netstats for tracking. Normally, I'd sum and aggregate peer stats. This approach simplifies that. |
|
ACK 7d4e82c lgtm. Tested on master and with v30.1 in my node monitoring dashboard. |
danielabrozzoni
left a comment
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 :)
| if (node.m_transport->SetMessageToSend(*it)) { | ||
| // Update memory usage of send buffer (as *it will be deleted). | ||
| node.m_send_memusage -= memusage; | ||
| m_net_stats.Record(NetStats::SENT, |
There was a problem hiding this comment.
nit: I would add a short comment here and below to clarify why there are two separate Record calls. For example: "we record a message when it is accepted by the transport, and we record bytes later, incrementally, when they are actually written to the socket."
| s = dict(bytes=0, count=0) | ||
| #print(f'S json={json}') | ||
| for k, v in json.items(): | ||
| #print(f'S k={k}, v={v}') |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
sedited
requested review from
danielabrozzoni
and removed request for
danielabrozzoni
|
Ping @vasild for rebase |
11 participants
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!)