Display send/recv in Bps instead of totals in the debug window #305

rebroad · 2021-04-29T17:52:57Z

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:-

jarolrod

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.

jarolrod · 2021-04-30T06:25:33Z

src/qt/guiutil.h

@@ -218,6 +218,7 @@ namespace GUIUtil
    QString formatNiceTimeOffset(qint64 secs);

    QString formatBytes(uint64_t bytes);
+    QString formatBps(uint64_t bytes);


Should add doxygen documentation

see: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-doxygen-compatible-comments

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 I don't think doxygen docs exist for formatBytes, so perhaps that can be done in another pull request to do it for a number of functions.

@rebroad
Since you're adding this function (formatBps) you should add the Doxygen comment with it. formatBytes not including a Doxygen when it was introduced is another issue that can be addressed in another PR.

@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.

jarolrod · 2021-04-30T06:30:44Z

src/qt/guiutil.cpp

+QString formatBps(uint64_t bytes)
+{
+    if (bytes < 1'000)
+        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

@jarolrod does your suggestion still apply now that I've changed it to bps (bits per second)? Is bps different in other languages?

A translator comment is the easiest way to let translators know what "bps" is.

src/qt/peertablemodel.cpp

jonatack · 2021-04-30T07:51:54Z

Worth exploring. The sidebar probably should be updated to display the same data, possibly in addition to the totals.

hebasto · 2021-04-30T10:28:25Z

@rebroad

Changed to WIP as I just noticed the column sorting is still using the old values.

You also could convert it to a draft:

rebroad · 2021-04-30T21:30:13Z

@rebroad

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.

rebroad · 2021-04-30T21:42:32Z

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.

hebasto · 2021-05-02T09:44:26Z

What is the base period to calculate data-transfer rate?

hebasto · 2021-05-02T09:48:27Z

src/qt/guiutil.cpp

+    if (bytes < 10'000)
+        return QObject::tr("%1 bps").arg(bytes);
+    if (bytes < 10'000'000)
+        return QObject::tr("%1 kbps").arg(bytes / 1'000);
+    if (bytes < 10'000'000'000)
+        return QObject::tr("%1 Mbps").arg(bytes / 1'000'000);


Suggested change

if (bytes < 10'000)

return QObject::tr("%1 bps").arg(bytes);

if (bytes < 10'000'000)

return QObject::tr("%1 kbps").arg(bytes / 1'000);

if (bytes < 10'000'000'000)

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.

@hebasto actually, they are bits, but I ought to change the variable name to reflect that. Will do so, and thanks for the guidance on the commentation.

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?

https://en.wikipedia.org/wiki/Kilo-
https://en.wikipedia.org/wiki/Mega-

ah, ok. will change.

rebroad · 2021-05-07T10:15:15Z

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.

luke-jr · 2021-06-12T20:25:53Z

Maybe have both and let the user decide which to hide/show?

rebroad · 2021-06-26T14:32:29Z

Maybe have both and let the user decide which to hide/show?

where would you place the option?

luke-jr · 2021-06-26T14:57:24Z

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.

rebroad · 2021-06-26T15:45:52Z

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.

shaavan

Tested ACK cf39e7d

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.

rebroad · 2021-09-14T19:04:14Z

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.

rebroad · 2021-09-16T17:17:02Z

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?

shaavan

Changes since my last review:

The sent data is displayed in kbps when it is greater than 1000bps.
bits variable has been changed from uint64_t to float.
Modifies Sent and Recieve case in 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.

shaavan

Changes since my last review:

Correction has been made at line 809: 10'000 -> 100'000

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?

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?

rebroad · 2021-10-18T21:01:15Z

@shaavan I am still testing the functionality for resetting at the end of IBD - have you already created a method for detecting this? I'd be interested in having a look at your code if so.

shaavan · 2021-10-19T12:44:16Z

@rebroad

have you already created a method for detecting this?

Unfortunately, not yet. However, I have some ideas on how to do so. Let me try to implement this as soon as possible.

DrahtBot · 2021-12-10T09:47:27Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2022-03-21T12:11:57Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

hebasto · 2022-04-13T15:15:51Z

Closing due to inactivity. Feel free to reopen.

rebroad marked this pull request as ready for review April 29, 2021 18:00

hebasto added Design Feature labels Apr 29, 2021

jarolrod reviewed Apr 30, 2021

View reviewed changes

jonatack reviewed Apr 30, 2021

View reviewed changes

src/qt/peertablemodel.cpp Outdated Show resolved Hide resolved

rebroad changed the title ~~Display send/recv in Bps instead of totals in the debug window~~ WIP - Display send/recv in Bps instead of totals in the debug window Apr 30, 2021

rebroad force-pushed the SendRecvSpeed-gui branch from 6cfd19b to 620e138 Compare April 30, 2021 21:25

rebroad marked this pull request as draft April 30, 2021 21:28

rebroad force-pushed the SendRecvSpeed-gui branch from 620e138 to 817f6be Compare April 30, 2021 21:34

rebroad changed the title ~~WIP - Display send/recv in Bps instead of totals in the debug window~~ Display send/recv in Bps instead of totals in the debug window Apr 30, 2021

rebroad marked this pull request as ready for review April 30, 2021 21:43

hebasto added UX All about "how to get things done" and removed Design labels May 2, 2021

hebasto reviewed May 2, 2021

View reviewed changes

rebroad force-pushed the SendRecvSpeed-gui branch 5 times, most recently from 7ba85be to 22ecf65 Compare May 7, 2021 14:30

rebroad force-pushed the SendRecvSpeed-gui branch 2 times, most recently from 3a32569 to 9260220 Compare June 26, 2021 14:56

rebroad force-pushed the SendRecvSpeed-gui branch from 9260220 to cf39e7d Compare June 26, 2021 14:58

shaavan reviewed Jul 30, 2021

View reviewed changes

rebroad force-pushed the SendRecvSpeed-gui branch from cf39e7d to da35fa3 Compare September 16, 2021 17:13

shaavan reviewed Sep 20, 2021

View reviewed changes

Display send/recv in bps instead of totals in the debug window

c1a692b

rebroad force-pushed the SendRecvSpeed-gui branch from da35fa3 to c1a692b Compare October 12, 2021 12:49

shaavan reviewed Oct 14, 2021

View reviewed changes

rebroad mentioned this pull request Dec 7, 2021

Show ToolTip on Network Traffic graph #492

Closed

3 tasks

DrahtBot added the Needs rebase label Dec 10, 2021

hebasto closed this Apr 13, 2022

bitcoin-core locked and limited conversation to collaborators Apr 13, 2023

Display send/recv in Bps instead of totals in the debug window #305

Display send/recv in Bps instead of totals in the debug window #305

Uh oh!

Conversation

rebroad commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebroad May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Apr 30, 2021

Uh oh!

hebasto commented Apr 30, 2021

Uh oh!

rebroad commented Apr 30, 2021

Uh oh!

rebroad commented Apr 30, 2021

Uh oh!

hebasto commented May 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebroad commented May 7, 2021

Uh oh!

luke-jr commented Jun 12, 2021

Uh oh!

rebroad commented Jun 26, 2021

Uh oh!

luke-jr commented Jun 26, 2021

Uh oh!

rebroad commented Jun 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

rebroad commented Sep 14, 2021

Uh oh!

rebroad commented Sep 16, 2021

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

rebroad commented Oct 18, 2021

Uh oh!

shaavan commented Oct 19, 2021

Uh oh!

DrahtBot commented Dec 10, 2021

rebroad commented Apr 29, 2021 •

edited

Loading

rebroad May 1, 2021 •

edited

Loading

rebroad commented Jun 26, 2021 •

edited

Loading