peers-tab: add connection duration column to tableview #543

RandyMcMillan · 2022-02-03T02:14:21Z

This change adds an "Age" column to the peers table view,
which displays the duration of each peer's connection.

RandyMcMillan · 2022-02-03T02:15:37Z

RandyMcMillan · 2022-02-03T11:57:22Z

commit 363f181 - the column is resizing to accommodate 1 h 1 m 20 s time format.

RandyMcMillan · 2022-02-03T13:35:41Z

commit 281b65b

we make adjustments so that the peers tab collapses down to a useful "desktop widget".

RandyMcMillan · 2022-02-03T13:41:55Z

This PR and #540 will work well together. Both the network graph and peers tab will be useful in a minimized window size and present information in a well formatted way.

jarolrod

What is the reason to add this column to the peers table? Why is it a useful metric for a user of the GUI?

jarolrod · 2022-02-04T06:14:04Z

src/qt/peertablemodel.h

        /*: Title of Peers Table column which describes the type of
            peer connection. The "type" describes why the connection exists. */
+        tr("Duration"),
+        /*: Title of Peers Table column which describes the duration of


You added this translator comment in reverse, the comment on lines 95 and 96 should be above line 94 in this diff.

Thanks for your attention on this.

RandyMcMillan · 2022-02-04T11:27:52Z

What is the reason to add this column to the peers table? Why is it a useful metric for a user of the GUI?

I guess one could ask - why peer id address direction type network ping sent received or user agent are useful.

And the fact that there is so much visual real estate NOT BEING USED FOR ANYTHING AT ALL - I am not even sure what the concern or hesitation to adding more columns to the table view would be. On wider screens more than half of the available real-estate is LITERALLY EMPTY SPACE.

RandyMcMillan · 2022-02-04T11:42:46Z

If - by your assertion - the connection duration is NOT USEFUL - why track it? Why not rip it out of the Client Model - a system could use that memory and resources for something else.

RandyMcMillan · 2022-02-04T12:33:55Z

The command line equivalent bitcoin-cli -netinfo 5 we report age to the user. So adding "Duration" to the gui tableview is a parallel to the command line report. Changing the column name to age seems more appropriate than debating whether or not it should be added. I follow the naming convention in the code using Duration GUIUtil::formatDurationStr().

NOTE: I agree with using a 3 char column title in the command line equivalent - but in the gui - Duration is more appropriate IMO.

RandyMcMillan · 2022-02-04T12:58:47Z

Note: I like the idea of verbosity in the command line version.
bitcoin-cli -netinfo
bitcoin-cli -netinfo 1
bitcoin-cli -netinfo 2
etc...

If defending the nuance and minutia of gui design changes wasn't so ridiculous (in this repo) - we could consider adding similar functionality to the gui (levels of verbosity to the table) - but I literally don't get paid enough (approaching nothing) to deal with that type of design process unless every one else was already on board.

ghost · 2022-02-04T18:49:18Z

Concept ACK

The connection duration is a useful metric to see when viewing the peer table - sorting the tableview with the connection duration is made possible.

Duration is also useful combined with other things because users can see how much data was sent and received in that duration.

jonatack · 2022-02-04T18:51:37Z

Concept ACK. I do look frequently at age in -netinfo. The direction and type columns should remain together, as combined they form the connection type, like <-> and type in -netinfo, see enum class ConnectionType in src/net.h. I see age/duration as being next to the id column, like in -netinfo, or the data sent/recv ones as @prayank23 mentioned. It may be good to make the output less verbose somehow (seconds seems unneeded). If you use signet, you won't need to black out sensitive parts of your screenshots.

Edit: the commits seem overly fine-grained, maybe combine the first three or four into one.

jonatack · 2022-02-04T18:55:11Z

src/qt/peertablemodel.h

        tr("Direction"),
+        /*: Title of Peers Table column which indicates the duration (length of time)
+            since the peer connection started. */
+        tr("Duration"),


I like Age, but if you don't, maybe consider Uptime.

used Age as column name

jonatack · 2022-02-04T18:55:42Z

src/qt/rpcconsole.cpp

            ui->banlistWidget->setColumnWidth(BanTableModel::Address, BANSUBNET_COLUMN_WIDTH);
            ui->banlistWidget->setColumnWidth(BanTableModel::Bantime, BANTIME_COLUMN_WIDTH);
        }
+        ui->banlistWidget->horizontalHeader()->setSectionResizeMode( 0, QHeaderView::ResizeToContents);


Here and line 694 as well

Suggested change

ui->banlistWidget->horizontalHeader()->setSectionResizeMode( 0, QHeaderView::ResizeToContents);

ui->banlistWidget->horizontalHeader()->setSectionResizeMode(0, QHeaderView::ResizeToContents);

removed white space

RandyMcMillan · 2022-02-04T23:24:13Z

@jonatack - thanks for the constructive feedback - I appreciate your reasoning. I will re-implement accordingly.

RandyMcMillan · 2022-02-05T02:57:11Z

commit 22b1a40 implements suggested changes.
Squashed commits per feedback

RandyMcMillan · 2022-02-05T03:14:40Z

Screenshots of 22b1a40

If anybody wants to send a patch truncating the time format - I will gladly add it with attribution.
Note: We could also wait - There are a couple things we could still add to the table view.
For example we could add a version field - that lends itself to sorting...and we aren't really short on column space...

ghost

utACK 22b1a40

luke-jr · 2022-02-05T22:37:28Z

Concept ACK

I do note the sort will be the same as sorting by ID...

RandyMcMillan · 2022-02-08T19:37:38Z

@luke-jr - good observation - thanks again for your great feed back - (my apologize for the other thing - I will make the mempool-tab repo more "normal" 😄 )

jonatack

Debug build is clean and seems to be working. Can you update, squash and fix the commit names, descriptions, and code organization? i.e. make this ready for more final review.

It would be nice to remove the seconds display--seems noisy, distracting and unneeded. Nodes can be up for weeks, months or longer.

sorting the tableview with the connection duration is made possible.

Suggest removing the above from the pull description, as it is the same as the existing sort by peer id. Albeit with the current version, the sort orders of Peer and Age are inversed.

RandyMcMillan · 2022-02-11T04:38:15Z

Commit: d01c0b3

Cleaned up commit structure and messages.
The Age column now displays the connection duration in seconds for the first minute - then switches to hour/minute format when connection age is greater than 60 seconds.

jonatack

Tested approach ACK, this seems much improved.

src/qt/peertablemodel.cpp

jonatack · 2022-02-11T12:44:05Z

src/qt/peertablemodel.cpp

+                return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::seconds>(time_now-rec->nodeStats.m_connected));
+            }else{
+                return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::minutes>(time_now-rec->nodeStats.m_connected));
+            }


d01c0b3 suggestions

clang-format

use chrono literals, e.g. 60s instead of std::chrono::seconds{60}

drop unneeded casts and comment that echoes the code (and shouldn't be in doxygen format here)

suggested diff

+using namespace std::chrono_literals; + PeerTableModel::PeerTableModel(interfaces::Node& node, QObject* parent) : QAbstractTableModel(parent), m_node(node), @@ -72,11 +75,11 @@ QVariant PeerTableModel::data(const QModelIndex& index, int role) const case NetNodeId: return (qint64)rec->nodeStats.nodeid; case Age: - if(std::chrono::duration_cast<std::chrono::minutes>(time_now-rec->nodeStats.m_connected) < std::chrono::seconds{60}){ - /* display connection "Age" in seconds for first minute */ - return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::seconds>(time_now-rec->nodeStats.m_connected)); - }else{ - return GUIUtil::formatDurationStr(std::chrono::duration_cast<std::chrono::minutes>(time_now-rec->nodeStats.m_connected)); + if (time_now - rec->nodeStats.m_connected < 60s) { + return GUIUtil::formatDurationStr(time_now - rec->nodeStats.m_connected); + } else { + return GUIUtil::formatDurationStr( + std::chrono::duration_cast<std::chrono::minutes>(time_now - rec->nodeStats.m_connected)); }

If this becomes larger, say, to display in DD:HH format after 24 hours it may make sense to extract it to a helper function.

The ResizeToContents settings should accommodate the duration string as the connection persists longer than 24 hours.

NOTE: The fact that <chrono> is formatting the string for # s, # m then # h # m suggests that it will
format for # d # h # m. I guess we could spoof the time to prove/test this.

I applied your suggested diff with attribution.

The fact that <chrono> is formatting the string for # s, # m then # h # m suggests that it will format for # d # h # m. I guess we could spoof the time to prove/test this.

Tested with a few different values and you're right: 23 h 59 s, 1 d, 1 d 1 h 59 m, 1 d 2 h, 1 d 2 h 1 m, etc. The spacing makes the column wider and a bit noisier than necessary. Could be improved here or as a follow-up.

- const auto time_now{GetTime<std::chrono::seconds>()}; + const auto time_now{GetTime<std::chrono::seconds>() + 86380s};

src/qt/peertablesortproxy.cpp

src/qt/rpcconsole.cpp

ghost

tACK a2e44c1

jonatack · 2022-02-14T16:08:13Z

src/qt/guiutil.cpp

+    if (hours >= 24) return QObject::tr("%1 d").arg(hours / 24);
+    if (hours) return QObject::tr("%1 h").arg(hours);
+    const auto minutes{std::chrono::duration_cast<std::chrono::minutes>(age).count()};
+    return QObject::tr("%1 m").arg(minutes);


@RandyMcMillan, looking at this further, I think the function in 349c597 can be simplified and improved as follows:

@@ -736,12 +736,10 @@ QString FormatPeerAge(std::chrono::seconds time_connected) { const auto time_now{GetTime<std::chrono::seconds>()}; const auto age{time_now - time_connected}; - if (age < 60s) return QObject::tr("%1 s").arg(age.count()); - const auto hours{std::chrono::duration_cast<std::chrono::hours>(age).count()}; - if (hours >= 24) return QObject::tr("%1 d").arg(hours / 24); - if (hours) return QObject::tr("%1 h").arg(hours); - const auto minutes{std::chrono::duration_cast<std::chrono::minutes>(age).count()}; - return QObject::tr("%1 m").arg(minutes); + if (age >= 24h) return QObject::tr("%1 d").arg(age / 24h); + if (age >= 1h) return QObject::tr("%1 h").arg(age / 1h); + if (age >= 1min) return QObject::tr("%1 m").arg(age / 1min); + return QObject::tr("%1 s").arg(age / 1s); }

I repushed an update to https://github.com/jonatack/gui/commits/gui-peers-tab-age-column with the changes in #543 (comment) and #543 (comment) if you want to cherry-pick them.

Should probably add translator notes too

Should probably add translator notes too

Agree, am doing this for several of these (FormatPeerAge, formatDurationStr, TimeDurationField, etc.) as a follow-up.

RandyMcMillan · 2022-02-14T18:48:18Z

Rebased to 25a91a5
commit abf93dd includes a more compact implementation of the
QString FormatPeerAge(std::chrono::seconds time_connected)
helper function.

jonatack · 2022-02-14T20:21:37Z

ACK 35d685a

I like that the Peer and Age columns have the opposite default sort direction, displaying both the newest and oldest peers by default.

Note that there is a Connection Time in the peer details that displays the Age in a more verbose format. I think it's ok as-is, as the formats are different, or maybe might propose renaming the former to Age (Connection Time).

ghost

reACK 35d685a

RandyMcMillan · 2022-02-14T22:28:50Z

This has been a fun collaborative effort. 😄

I enjoyed watching a rough idea get polished into an "ACK-able" state.

shaavan

ACK 35d685a

I agree with the concept of adding an Age column in the Node window table because I agree with all the reasons for its approval discussed above. To briefly reiterate them:

We have unused screen real estate which could be used more productively.
The Age column is displayed in the -netinfo command for bitcoin-cli. It makes logical sense to keep our GUI consistent with the cli.
Age column is frequently used by users through -netinfo command. So it would be a good idea to add the GUI.

Regarding the code:

I like the structuring of the FormatPeerPage function. The code is clean and easy to understand.
I like the idea of displaying only the highest degree of time, instead of precise time. This gives users a general idea of how a node has been contacted to them, while also not taking too much screen real estate, and user’s attention.
I think naming the column “Age” seems appropriate to me. This keeps it consistent with bitcoin-cli's naming.

Here are some screenshots I have added showing the correct working of this PR, on Ubuntu 20.04

Master	PR

w0xlt

tACK 35d685a on Ubuntu 21.10 Qt 5.15.2

promag

Concept ACK.

promag · 2022-02-22T23:18:16Z

src/qt/guiutil.cpp

    return strList.join(" ");
 }

+QString FormatPeerAge(std::chrono::seconds time_connected)


abf93dd

I think this could be idempotent QString FormatAge(std::chrono::seconds age), to avoid GetTime.

Note that PeerTableModel has a timer to refresh the data, just call GetTime once there.

@promag I agree this would be an improvement and am looking into it, but am not sure how to integrate GetTime into the peertablemodel.cpp QTimer code. Pointers welcome.

promag · 2022-02-22T23:20:53Z

src/qt/peertablemodel.cpp

        case NetNodeId:
            return (qint64)rec->nodeStats.nodeid;
+        case Age:
+            return GUIUtil::FormatPeerAge(rec->nodeStats.m_connected);


852b036

Following my previous comment, this could be

return GUIUtil::FormatAge(m_now - rec->nodeStats.m_connected);

jonatack · 2022-03-11T11:59:32Z

This pull has 4 ACKs and could perhaps be merged now that v23 has been branched off, which would enable moving forward on planned follow-ups like #543 (comment), while still allowing a possible improvement per #543 (comment).

src/qt/rpcconsole.cpp

RandyMcMillan · 2022-03-12T19:29:29Z

7ddff98 merged requested changes.
Will squash after re-review.

jonatack · 2022-03-13T13:44:16Z

ACK. Squashing the changes into the relevant commit (in this case, the last commit 35d685a) when re-pushing an update can save time for reviewers and speed up the merge. See the "Squashing Commits" section of CONTRIBUTING.md.

Co-authored-by: randymcmillan <randy.lee.mcmillan@gmail.com>

Co-authored-by: Jon Atack <jon@atack.com>

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

RandyMcMillan · 2022-03-16T09:02:48Z

51708c4 squashed merged requested changes.
rebased on top of 310ba92

jonatack · 2022-03-16T20:04:44Z

re-ACK 51708c4

Jamewood · 2022-03-17T04:10:24Z

(PR #543)

Jamewood · 2022-03-17T04:50:46Z

51708c4

…

On Wed, Mar 16, 2022, 9:05 PM Jon Atack ***@***.***> wrote: re-ACK 51708c4 <51708c4> — Reply to this email directly, view it on GitHub <#543 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWXCCBYJ3XSYP7MP53GSSL3VAI5G3ANCNFSM5NNV222A> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

RandyMcMillan · 2022-04-01T21:54:29Z

@Jamewood - thank you for the ACK if this was your intention - please ACK in the appropriate format - and remove the other comments. 😀

shaavan

reACK 51708c4

Changes since my last review:

Dropped “Magic numbers”. i.e.,
1 → PeerTableModel::Age
0 → BanTableModel::Address
Rebased over master.

I agree with the idea of dropping “magic” numbers because:

It makes code easier to understand and reason with.
It makes code robust to change in enum ordering under the class’s definition.

hebasto

ACK 51708c4, I have reviewed the code and it looks OK, I agree it can be merged.

… to tableview 51708c4 gui: peersWidget - ResizeToContents Age and IP/Netmask columns (randymcmillan) 209301a gui: add Age column to peers tab (randymcmillan) 127de22 gui: add FormatPeerAge() utility helper (Jon Atack) Pull request description: This change adds an "Age" column to the peers table view, which displays the duration of each peer's connection. ACKs for top commit: jonatack: re-ACK 51708c4 Jamewood: > re-ACK 51708c4 shaavan: reACK 51708c4 hebasto: ACK 51708c4, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 27323f7080ec0d3fcdbf1b190fba1cd2d7406840ab6607c221cf8af950db9134e22721cc5a88f4fc4f390d8b05e98bc4b7521661a31fadad9e2c6c6390e71788

jarolrod reviewed Feb 4, 2022

View reviewed changes

jonatack reviewed Feb 4, 2022

View reviewed changes

RandyMcMillan force-pushed the 1643853831-peers-tab-add-duration-column branch from 16c9ebf to 22b1a40 Compare February 5, 2022 02:54

ghost approved these changes Feb 5, 2022

View reviewed changes

jonatack reviewed Feb 8, 2022

View reviewed changes

hebasto changed the title ~~gui: peers-tab: add connection duration column to tableview~~ peers-tab: add connection duration column to tableview Feb 9, 2022

hebasto added the Feature label Feb 9, 2022

RandyMcMillan force-pushed the 1643853831-peers-tab-add-duration-column branch 3 times, most recently from e4d1d6c to d01c0b3 Compare February 11, 2022 04:21

jonatack reviewed Feb 11, 2022

View reviewed changes

RandyMcMillan force-pushed the 1643853831-peers-tab-add-duration-column branch 2 times, most recently from 139f2ed to 795a03d Compare February 11, 2022 19:29

ghost approved these changes Feb 14, 2022

View reviewed changes

jonatack reviewed Feb 14, 2022

View reviewed changes

RandyMcMillan force-pushed the 1643853831-peers-tab-add-duration-column branch from a2e44c1 to 35d685a Compare February 14, 2022 18:39

ghost approved these changes Feb 14, 2022

View reviewed changes

shaavan approved these changes Feb 15, 2022

View reviewed changes

w0xlt approved these changes Feb 18, 2022

View reviewed changes

jonatack mentioned this pull request Feb 20, 2022

refactor: use std::chrono for formatDurationStr() helper #549

Merged

promag reviewed Feb 22, 2022

View reviewed changes

hebasto requested changes Mar 12, 2022

View reviewed changes

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

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

RandyMcMillan requested a review from hebasto March 12, 2022 19:28

jonatack and others added 3 commits March 16, 2022 04:54

gui: add FormatPeerAge() utility helper

127de22

Co-authored-by: randymcmillan <randy.lee.mcmillan@gmail.com>

gui: add Age column to peers tab

209301a

Co-authored-by: Jon Atack <jon@atack.com>

gui: peersWidget - ResizeToContents Age and IP/Netmask columns

51708c4

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

RandyMcMillan force-pushed the 1643853831-peers-tab-add-duration-column branch from 7ddff98 to 51708c4 Compare March 16, 2022 08:56

shaavan approved these changes Apr 9, 2022

View reviewed changes

hebasto approved these changes Apr 12, 2022

View reviewed changes

hebasto merged commit 0f46e73 into bitcoin-core:master Apr 12, 2022

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

	ui->banlistWidget->horizontalHeader()->setSectionResizeMode( 0, QHeaderView::ResizeToContents);
	ui->banlistWidget->horizontalHeader()->setSectionResizeMode(0, QHeaderView::ResizeToContents);

peers-tab: add connection duration column to tableview #543

peers-tab: add connection duration column to tableview #543

Uh oh!

Conversation

RandyMcMillan commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Feb 3, 2022

Uh oh!

RandyMcMillan commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Feb 3, 2022

Uh oh!

RandyMcMillan commented Feb 3, 2022

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!

RandyMcMillan commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Feb 4, 2022

Uh oh!

ghost commented Feb 4, 2022

Uh oh!

jonatack commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

RandyMcMillan commented Feb 4, 2022

Uh oh!

RandyMcMillan commented Feb 5, 2022

Uh oh!

RandyMcMillan commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Feb 5, 2022

Uh oh!

RandyMcMillan commented Feb 8, 2022

Uh oh!

jonatack left a comment • 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!

RandyMcMillan commented Feb 11, 2022

Uh oh!

jonatack left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan Feb 11, 2022 • 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!

RandyMcMillan commented Feb 3, 2022 •

edited

Loading

RandyMcMillan commented Feb 3, 2022 •

edited

Loading

RandyMcMillan commented Feb 4, 2022 •

edited

Loading

RandyMcMillan commented Feb 4, 2022 •

edited

Loading

RandyMcMillan commented Feb 4, 2022 •

edited

Loading

jonatack commented Feb 4, 2022 •

edited

Loading

RandyMcMillan commented Feb 5, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack left a comment •

edited

Loading

RandyMcMillan Feb 11, 2022 •

edited

Loading

jonatack commented Feb 14, 2022 •

edited

Loading

jonatack Mar 6, 2022 •

edited

Loading

jonatack commented Mar 13, 2022 •

edited

Loading