Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Qt] Add interactive mempool graph #8550

Closed
wants to merge 2 commits into from

Conversation

@jonasschnelli
Copy link
Member

jonasschnelli commented Aug 19, 2016

Includes #8501

At the moment, the mempool graph is not very prominently placed (next to the debug window).
bildschirmfoto 2016-08-19 um 21 20 48

Features:

  • interactive graph with options for tx count, dynamic memory usage and minRelayFee/KB
  • collects stats in the background, changing the timespan will directly redraw (unlike our bandwidth graph)
  • Dynamic size drawing, window can be resized
  • The mempool graph is a QWidget which means, it could be placed together with other graphs in a combine multi-graph view (screensaver approach)

bildschirmfoto 2016-08-19 um 21 28 09

@jonasschnelli jonasschnelli added the GUI label Aug 19, 2016
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/08/stats_qt branch 2 times, most recently Aug 19, 2016
@MarcoFalke
Copy link
Member

MarcoFalke commented Aug 19, 2016

Concept ACK. Will test later.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/08/stats_qt branch Aug 19, 2016
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Aug 19, 2016

Added the missing button PNGs now to git. This should fix the compile issue.

@luke-jr
Copy link
Member

luke-jr commented Aug 19, 2016

Nice. Some people might want to see actual byte-size, weights, etc; probably will overflow the one-line-per-item paradigm quickly - maybe have a dropdown box to add stuff?

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Aug 19, 2016

Yes. Dropdown box could make sense. Adding a second graph below or on the right that just plots different data would probably also look good.

@MarcoFalke
Copy link
Member

MarcoFalke commented Aug 19, 2016

Nit: Fonts appear larger on my system. Otherwise this looks great!

screenshot from 2016-08-19 23-15-51

@MarcoFalke
MarcoFalke reviewed Aug 19, 2016
View changes
src/qt/mempoolstats.cpp Outdated
{
dynMemUsageValueItem->setPlainText(GUIUtil::formatBytes(vSamples.back().dynMemUsage));
txCountValueItem->setPlainText(QString::number(vSamples.back().txCount));
minFeeValueItem->setPlainText(QString::number(vSamples.back().minFeePerK));

This comment has been minimized.

@MarcoFalke

MarcoFalke Aug 19, 2016 Member

Nit: Missing value. I think this is in satoshis. What about using BTC?

@MarcoFalke
MarcoFalke reviewed Aug 20, 2016
View changes
src/qt/bitcoingui.cpp Outdated
@@ -351,6 +354,11 @@ void BitcoinGUI::createActions()
// initially disable the debug window menu item
openRPCConsoleAction->setEnabled(false);

showMempoolStatsAction = new QAction(platformStyle->TextColorIcon(":/icons/mempoolstats"), tr("&Mempool Statistics"), this);

This comment has been minimized.

@MarcoFalke

MarcoFalke Aug 20, 2016 Member

I can't find this icon

This comment has been minimized.

@jonasschnelli

jonasschnelli Aug 20, 2016 Author Member

Indeed,... this does not exists yet. Will create one.

This comment has been minimized.

@MarcoFalke

MarcoFalke Aug 20, 2016 Member

http://typicons.com/ has some chart icons ;)

This comment has been minimized.

@luke-jr

luke-jr Aug 20, 2016 Member

Please remember to keep in mind licensing. typicons are CC-BY-3.0, which is a non-free license.

This comment has been minimized.

@MarcoFalke

MarcoFalke Aug 20, 2016 Member

Huh, I recall he gave you permission to dual-license as MIT?

dae0a89

This comment has been minimized.

@jonasschnelli

jonasschnelli Aug 20, 2016 Author Member

@luke-jr: keep in mind that we have permission from the author to distribute them under MIT: #6367 (comment)

This comment has been minimized.

@luke-jr

luke-jr Aug 20, 2016 Member

Ah, forgot about that. Thanks for the reminder.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/08/stats_qt branch Aug 20, 2016
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Aug 20, 2016

Switched to "Arial" as font and added a auto-size-adjustment.
Also added the "charts" icon for the menu.

@MarcoFalke
Copy link
Member

MarcoFalke commented Aug 20, 2016

Nit: run optimize-pngs and mention the icons in assets_attribution?

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/08/stats_qt branch 3 times, most recently Aug 20, 2016
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Aug 20, 2016

Optimized the pngs and added them to the assets attribution file.

@ghost
Copy link

ghost commented Aug 20, 2016

Concept ACK.

@isle2983
Copy link
Contributor

isle2983 commented Aug 21, 2016

Nit1: The graph window is always drawn on top of the main window, even when the main window is active. It makes it annoying to use the rest of the GUI while this is open.

Nit2: The quantity labels for the Dynamic Memory Usage values on the left hand side of the graph are cramped and cut off some of the characters. See "500.00 KB" in the screenshot. It is like that for larger values once it gets into the 100MBs too.

mempoolstats

@rebroad
Copy link
Contributor

rebroad commented Aug 24, 2016

I'm not sure what I'm doing wrong but I've merged this and the graph is empty, and has been for an hour now... :-s i.e. Dynamic Memory Usage is stuck on 0 bytes, Amount of Txs also 0, MinRelayFee also zero.

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Aug 24, 2016

@rebroad: what does getmempoolinfo tells you? Are you in sync on mainnet (or testnet)?

@rebroad
Copy link
Contributor

rebroad commented Aug 25, 2016

@jonasschnelli problem solved - I had blocksonly=1 in the config file!! It's working really nicely now - great job!! I love how it can be resized and the period of time changed without the graph resetting (unlike the network traffic graph!).

Also, ACK

@fanquake
Copy link
Member

fanquake commented Aug 26, 2016

OS X screenshots:
init
more-than-10

@luke-jr
Copy link
Member

luke-jr commented Aug 27, 2016

Using images for basically-a-checkbox gives a non-native look on every system. It looks like we can just colour a QCheckBox?

@paveljanik
Copy link
Contributor

paveljanik commented Sep 7, 2016

Rebase needed.

Concept ACK

Agree about checkboxes...

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/08/stats_qt branch Sep 9, 2016
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Sep 9, 2016

Rebased on top of #8501

@MarcoFalke
MarcoFalke reviewed Sep 9, 2016
View changes
src/Makefile.qt.include Outdated
@@ -234,6 +237,11 @@ RES_ICONS = \
qt/res/icons/bitcoin.ico \
qt/res/icons/bitcoin_testnet.ico \
qt/res/icons/bitcoin.png \
qt/res/icons/button_off.png \
qt/res/icons/button_on_blue.png \

This comment has been minimized.

@MarcoFalke

MarcoFalke Sep 9, 2016 Member

@jonasschnelli What do you think about using QCheckBox with setPallette() as suggested by @luke-jr ?

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 15, 2016 Author Member

I think Checkboxes are appropriate for now.

But we should consider using non-standard UI elements to improve the look and feel. I know, developers hate this. :-)

But have a look at #5896. I think people liked it because the look and feel was different then just "Arial 12 and standard UI elements".

This comment has been minimized.

@luke-jr

luke-jr Nov 24, 2016 Member

Non-standard UI elements ruin the look and feel...

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 24, 2016 Author Member

Not sure. That's probably a matter of taste.
The thing why I'd like to keep the non-standard UI elements is, that I'm using a QGraphicScene at this point. Adding a QWidget (checkbox) at this point breaks the flexibility of reusing the QGraphicScene and draw it in any scale.

I think the checkbox could be different then the one thats currently used. And it probably should be draw internally and not use a PNG.

This comment has been minimized.

@luke-jr

luke-jr Nov 24, 2016 Member

Because it's a matter of taste is precisely why it should use standard UI elements. The standard element should do all the drawing. If the user doesn't like their standard widgets, they can change them.

src/main.cpp Outdated
@@ -2837,6 +2844,9 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
for(unsigned int i=0; i < pblock->vtx.size(); i++)
txChanged.push_back(std::make_tuple(pblock->vtx[i], pindexNew, i));

// update mempool stats
CStats::DefaultStats()->addMempoolSample(mempool.size(), mempool.DynamicMemoryUsage(), mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK());

This comment has been minimized.

@paveljanik

paveljanik Sep 15, 2016 Contributor

Can you please do something with this long line? It is there ~3 times and does a simple thing - worth a better method to add a sample?

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 15, 2016 Author Member

Hmm.. I think this line is reasonable? How would you improve it?

This comment has been minimized.

@paveljanik

paveljanik Sep 15, 2016 Contributor

Pass const ref. mempool only and do the rest in the method?

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 15, 2016 Author Member

I like the current layering concept. IMO its preferable that the stats.cpp module has no knowledge over the txmempool.h's classes, etc., I prefer to keep it as "dump" as possible.

@luke-jr
Copy link
Member

luke-jr commented Feb 23, 2017

FWIW, rebased (at least part of the way) in my stats_rpc-0.14 and stats_qt-0.14 branches.

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Feb 23, 2017

This is based on #8501 (core change with additional RPC command) which has to get right first.
Another solution would be to not depend on a core statistics "module" and therefore collect the stats in a GUI container only. Maybe this would be the better first step.

@jtimon
Copy link
Member

jtimon commented Feb 23, 2017

Concept ACK needs rebase

@ghost
Copy link

ghost commented Mar 8, 2017

The graphics are good, the design does not forget
Here you can see the different designs of graphs
http://www.bittbox.com/showcase/30-flat-analytics-ui-dashboards

I like this one


https://dribbble.com/shots/1719845-Statistics-General-trends

@laanwj
Copy link
Member

laanwj commented Jun 6, 2017

Seems the discussion here bled out.

I'd say please review the code, and then get it merged. Visual details etc can be changed later. Real-time statistics is one reason why people like to run their own nodes.

@laanwj
Copy link
Member

laanwj commented Jun 28, 2017

Needs rebase.

@Sjors
Copy link
Member

Sjors commented Nov 9, 2017

Concept ACK.

I can't compile this (log gist). I'll try again after the rebase.

A stacked chart of mempool bytes at different feerates would be much more interesting and useful

Agreed.

It would be nice to have a little chart icon in the bottom right corner of the wallet (in a future PR).

That said, I also agree with:

Visual details etc can be changed later.

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Nov 9, 2017

This is based on #8501 and the question is, should it be Qt only (no Qt free stats collector) or should we try to get #8501 done?
(will rebase both PRs soon)

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/08/stats_qt branch 2 times, most recently Nov 22, 2017
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/08/stats_qt branch to 63fb116 Nov 22, 2017
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Nov 22, 2017

Rebased.
It's currently not Qt4 compatible due to a lambda signal connect (travis reported / working on it).

@laanwj
Copy link
Member

laanwj commented Nov 23, 2017

It's currently not Qt4 compatible due to a lambda signal connect (travis reported / working on it).

If it's a lot of work, feel free to leave "nice but not critical" features such as this out when compiling with Qt4.

@PierreRochard
Copy link
Contributor

PierreRochard commented Dec 15, 2017

ACK with Qt5

Copy link
Contributor

jb55 left a comment

Hmm for some reason I'm getting this error on my system:

qt/mempoolstats.cpp: In member function ‘void MempoolStats::drawChart()’:
qt/mempoolstats.cpp:183:42: error: ‘ceil’ was not declared in this scope
     maxValueSize = ceil(maxValueSize*0.11)*10; //use size steps of 10dip
                                          ^
qt/mempoolstats.cpp:256:108: error: ‘log10’ was not declared in this scope
     int64_t dynMemUsagelog10Val = pow(10.0f, floor(log10(maxDynMemUsage*paddingTopSizeFactor-minDynMemUsage)));
                                                                                                            ^
qt/mempoolstats.cpp:256:109: error: ‘floor’ was not declared in this scope
     int64_t dynMemUsagelog10Val = pow(10.0f, floor(log10(maxDynMemUsage*paddingTopSizeFactor-minDynMemUsage)));
                                                                                                             ^
qt/mempoolstats.cpp:256:110: error: ‘pow’ was not declared in this scope
     int64_t dynMemUsagelog10Val = pow(10.0f, floor(log10(maxDynMemUsage*paddingTopSizeFactor-minDynMemUsage)));
                                                                                                              ^

I fixed it with this patch:

diff --git a/src/qt/mempoolstats.cpp b/src/qt/mempoolstats.cpp
index 139efa1a9..af33a655b 100644
--- a/src/qt/mempoolstats.cpp
+++ b/src/qt/mempoolstats.cpp
@@ -8,6 +8,7 @@
 #include <qt/clientmodel.h>
 #include <qt/guiutil.h>
 #include <stats/stats.h>
+#include <math.h>
 
 static const char *LABEL_FONT = "Arial";
 static int LABEL_TITLE_SIZE = 22;
Copy link
Member

Sjors left a comment

I did not need @jb55's patch on MacOS 10.13.2.

Although it can wait for a followup commit, I would prefer if more of the UI was built using interface builder (see #11950). That would it easier for other to improve the looks.

This should be easy for the header section above the chart and the duration toggles below. It's probably not realistic for the grid and X, Y axis and colored lines, although it would be nice if they can be styled using visual tools.

I left some inline comments, but didn't see any must-fix stuff.

The drawChart() function is rather large. Some stuff needs to be drawn only once, other stuff maybe only when there's a resize. Maybe that can be split? It might be safer to move some of the calculations into their own functions and test those against boundary values.

@@ -0,0 +1,423 @@
// Copyright (c) 2016 The Bitcoin Core developers

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

Nit: bump to 2018?

}

// draw vertical grid
int amountOfLinesV = 4;

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

Nit: numberOfLinesV


QPainterPath dynMemUsageGridPath(QPointF(currentX, bottom));

// draw horizontal grid

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

Move grid drawing to a function?


if (!titleItem)
{
// create labels (only once)

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

Move the once-only stuff to another function?

QCheckBox *cb0 = new QCheckBox("Dynamic Memory Usage");
cb0->setStyleSheet("background-color: rgb(255,255,255);");
dynMemUsageSwitch = scene->addWidget(cb0);
connect(cb0, &QCheckBox::stateChanged, [cb0, this](){ drawDynMemUsage = cb0->isChecked(); drawChart(); });

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

Move the user interacting bits to another function?

Also, do these really need to get redrawn every time? I'm not worried about performance, just code complexity. Adding and removing UI elements where it's not needed might also make using the interface builder more difficult.

connect(model, SIGNAL(mempoolStatsDidUpdate()), this, SLOT(drawChart()));
}

void MempoolStats::drawChart()

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

Rename to drawChartFrame()? It draws a lot more than the chart.

allDataLabel->setPos((width()-totalWidth)/2.0+last10MinLabel->boundingRect().width()+lastHourLabel->boundingRect().width()+lastDayLabel->boundingRect().width()+30,height()-filterBottomPadding);

// don't paint the grind/graph if there are no or only a signle sample
if (vSamples.size() < 2)

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

drawEmptyChart()

{
noDataItem->setVisible(true);
return;
}

This comment has been minimized.

@Sjors

Sjors Jan 8, 2018 Member

else {
  drawChart(vSamples, ...)
}
@laanwj
Copy link
Member

laanwj commented May 14, 2018

This has been lingering for a long time. I would prefer if it makes it in, of course, it has tons of concept ACKs (the most of any open PR, according to bitcoinacks). But otherwise should we close and put this up for grabs?

@laanwj
Copy link
Member

laanwj commented Aug 31, 2018

Although it would be really nice to have, this has been inactive for very long.
Closing and adding "up for grabs".

@rebroad
Copy link
Contributor

rebroad commented Aug 20, 2020

@laanwj A rebased version is this seems to be in Bitcoin Knots. Should I open a new pull request using this rebased version?

@jonatack
Copy link
Member

jonatack commented Aug 20, 2020

This looks very nice. @rebroad I would review/test it if you pick it up.

@luke-jr
Copy link
Member

luke-jr commented Aug 20, 2020

Note that #8501 was updated after this, and in a conflicting way. In Knots, I'm using an older version of it.

So there is some work to be done, but it sounds like a good idea since @jonasschnelli seems to have abandoned it.

(If you're not up to merging #8501 with this, maybe it'd be fine to use the older version in Knots and the code can be updated later when the RPC interface is added)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.