Moved from https://github.com/bitcoin/bitcoin/pull/21806
I find this far more useful for than the totals. For example, I can easily see which nodes are not really talking with this metric, and not so much with the totals (as is current functionality).
Example:-
217 | @@ -218,6 +218,7 @@ namespace GUIUtil 218 | QString formatNiceTimeOffset(qint64 secs); 219 | 220 | QString formatBytes(uint64_t bytes); 221 | + QString formatBps(uint64_t bytes);
Should add doxygen documentation
hmmm. well, the units indicate that it is a different metric, so I'm not sure if the addition of the word "total" is needed. What do other people think?
@jarolrod given that formatBps is so similar to formatBytes, I'm averse to doing the Doxygen for formatBytes as part of this pull, and I'm also averse to doing Doxygen for just formatBps without formatBytes in this pull, but I'm fine with doing a later pull request to add the Doxygen for both formatBytes and formatBps. I think the Doxygen therefore belongs in a separate pull request tackling a number of missing functions.
773 | @@ -774,6 +774,18 @@ QString formatBytes(uint64_t bytes) 774 | return QObject::tr("%1 GB").arg(bytes / 1'000'000'000); 775 | } 776 | 777 | +QString formatBps(uint64_t bytes) 778 | +{ 779 | + if (bytes < 1'000) 780 | + return QObject::tr("%1 B/s").arg(bytes);
should add Translator comments
see: https://doc.qt.io/qt-5/i18n-source-translation.html#translator-comments
Concept ACK
I agree that this is more useful than showing the totals. One can still see the totals by clicking on the peer.
One issue is that Sent and Received are now used in two different contexts. In the peer table, it's denoting how many bytes are currently being sent/received. In the sidebar it's denoting the total sent/received.
My suggestion is to change the Sent/Received text in the sidebar to Total Sent/Total Received.
121 | @@ -122,9 +122,9 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const 122 | case Ping: 123 | return GUIUtil::formatPingTime(rec->nodeStats.m_min_ping_time); 124 | case Sent: 125 | - return GUIUtil::formatBytes(rec->nodeStats.nSendBytes); 126 | + return GUIUtil::formatBps(rec->nodeStats.nSendBytes / (GetSystemTimeInSeconds() + 1 - rec->nodeStats.nTimeConnected)); 127 | case Received: 128 | - return GUIUtil::formatBytes(rec->nodeStats.nRecvBytes); 129 | + return GUIUtil::formatBps(rec->nodeStats.nRecvBytes / (GetSystemTimeInSeconds() + 1 - rec->nodeStats.nTimeConnected));
ISTM this calculation will be useful shortly after the connection time but for long-lived peers would provide little to no real-time feedback. Maybe use the time in seconds since last message sent/received like in -netinfo?
done
Worth exploring. The sidebar probably should be updated to display the same data, possibly in addition to the totals.
Changed to WIP as I just noticed the column sorting is still using the old values.
You also could convert it to a draft:
ah, thank you - I was wondering if that was possible... the terminology confuses me though as I'd say it's ready for review, but not perhaps ready to be merged.
Worth exploring. The sidebar probably should be updated to display the same data, possibly in addition to the totals.
I considered doing this, but I think it's an overall advantage to not have too much data there. It's more useful to have bandwidth in the columns, and totals in the sidebar, IMHO.
What is the base period to calculate data-transfer rate?
779 | + if (bytes < 10'000) 780 | + return QObject::tr("%1 bps").arg(bytes); 781 | + if (bytes < 10'000'000) 782 | + return QObject::tr("%1 kbps").arg(bytes / 1'000); 783 | + if (bytes < 10'000'000'000) 784 | + return QObject::tr("%1 Mbps").arg(bytes / 1'000'000);
if (bytes < 10'000)
//: "Bit per second."
return QObject::tr("%1 bps").arg(bytes);
if (bytes < 10'000'000)
//: "Kilobit per second."
return QObject::tr("%1 kbps").arg(bytes / 1'000);
if (bytes < 10'000'000'000)
//: "Megabit per second."
return QObject::tr("%1 Mbps").arg(bytes / 1'000'000);
Oh, just noted that here are bytes not bits. In that case the correct contraction is upper case "B", not "b": Bps, kBps, MBps.
Oh, just noted that here are bytes not bits. In that case the correct contraction is upper case "B", not "b": Bps, kBps, MBps.
why is the k small but the M is big?
ah, ok. will change.
What is the base period to calculate data-transfer rate?
In my first pull request it was from connection time to current time. Now it's from connection time to time of last send/recv.
Maybe have both and let the user decide which to hide/show?
Maybe have both and let the user decide which to hide/show?
where would you place the option?
Just have 4 columns, total in, total out, rate in, rate out.
Other applications let you rightclick the header area to hide/show them, but even if we can't have that, users can resize them to ~0 width.
Just have 4 columns, total in, total out, rate in, rate out.
Other applications let you rightclick the header area to hide/show them, but even if we can't have that, users can resize them to ~0 width.
It's a pain having to resize columns, and also they cannot be resized to zero. I'd rather wait until there's the ability to disable columns before creating a pull to add rate in addition to total bytes. There are already too many columns IMHO.
Tested ACK cf39e7d7b17e0affc1214cb2bb64de3f89093679
Compiled and Tested on Ubuntu 20.04
It makes more sense to add the rate of sending and receiving of data than the total as suggested by op. But I have just one doubt
return GUIUtil::formatBps(rec->nodeStats.nSendBytes * 8 / (rec->nodeStats.nLastSend+1 - rec->nodeStats.nTimeConnected));
By the logic used by op, the displayed rate is the average rate of data transferred by the peer. But I personally would prefer that if the displayed rate was instantaneous and not average because it might happen that a node is inactive for quite some time, but the data they transferred earlier was so high, that the rate column is showing a significantly large rate of data transfer.
And also:
For values larger than 1000 bps the data should be shown in kbps, but it is not happening as in the highlighted example.
For values larger than 1000 bps the data should be shown in kbps, but it is not happening as in the highlighted example.
I agree. I'll upload an update shortly.
By the logic used by op, the displayed rate is the average rate of data transferred by the peer. But I personally would prefer that if the displayed rate was instantaneous and not average because it might happen that a node is inactive for quite some time, but the data they transferred earlier was so high, that the rate column is showing a significantly large rate of data transfer.
Yes, this does happen, for example in the case where there's IBD (initial block download) which has much higher transfer rates and then skews the numbers once that completes. I have another commit, which deals with this, and allows the calculation to be reset at the end of IBD. Shall I include that with this pull, or perhaps raise another pull in the future to include that?
Changes since my last review:
bits variable has been changed from uint64_t to float.peertablemodel.cpp to avoid division by zero.Changing bits variable from int to float has to lead to displaying sent and receive value with decimals. This makes the displayed data look a lot better compared to the previous version. Great Work, @rebroad!
I am adding some screenshots to emphasize the difference between the old commit and the latest commit.
| Old Commit | Latest Commit |
|---|---|
|
 |
Shall I include that with this pull, or perhaps raise another pull in the future to include that?
IMO, I think it would be better to add this as an additional commit to this PR. This is a significant change. Let’s not wait for the opening of another and then waiting for it to get merged.
Changes since my last review:
10'000 -> 100'000I have another commit, which deals with this, and allows the calculation to be reset at the end of IBD. Shall I include that with this pull, or perhaps raise another pull in the future to include that?
Any updates on the above. Have you planned on adding it as another commit, or have you decided to do so in a future PR?
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?
Closing due to inactivity. Feel free to reopen.