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

Add node metrics screen #1375

Merged
merged 12 commits into from Oct 23, 2016

Conversation

Projects
None yet
5 participants
@str4d
Contributor

str4d commented Sep 10, 2016

Continuation of #1336
Closes #1331

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 5, 2016

Contributor

I have rebased this PR on beta 2.

Contributor

str4d commented Oct 5, 2016

I have rebased this PR on beta 2.

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 21, 2016

Contributor

☔️ The latest upstream changes (presumably #1578) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

zkbot commented Oct 21, 2016

☔️ The latest upstream changes (presumably #1578) made this pull request unmergeable. Please resolve the merge conflicts.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

I rebased this on current master (post-#1578).

Contributor

daira commented Oct 22, 2016

I rebased this on current master (post-#1578).

@@ -191,15 +192,20 @@ Value generate(const Array& params, bool fHelp)
std::function<bool(std::vector<unsigned char>)> validBlock =
[&pblock](std::vector<unsigned char> soln) {
pblock->nSolution = soln;
solutionTargetChecks.increment();

This comment has been minimized.

@daira

daira Oct 22, 2016

Contributor

This RPC method is only used for regtest, so it doesn't need any of the increment() calls. Does not block (these calls are harmless, just unnecessary).

@daira

daira Oct 22, 2016

Contributor

This RPC method is only used for regtest, so it doesn't need any of the increment() calls. Does not block (these calls are harmless, just unnecessary).

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

utACK.

Contributor

daira commented Oct 22, 2016

utACK.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

utACK, looks like @daira added the extra count calls for tromp's solver.

Note that the print statements in tromp's solver will interfere with the metrics screen output. I'll test it now and see if I'd need to fix it (which i can do now).

Contributor

str4d commented Oct 22, 2016

utACK, looks like @daira added the extra count calls for tromp's solver.

Note that the print statements in tromp's solver will interfere with the metrics screen output. I'll test it now and see if I'd need to fix it (which i can do now).

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

It's fine to just comment out those print statements, I think.

Contributor

daira commented Oct 22, 2016

It's fine to just comment out those print statements, I think.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Yeah, that's what I'll do for now.

Contributor

str4d commented Oct 22, 2016

Yeah, that's what I'll do for now.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Okay, it interferes with one of the auto-update lines, but doesn't obscure any information and doesn't break the UI. Still, I'll fix it now :)

Contributor

str4d commented Oct 22, 2016

Okay, it interferes with one of the auto-update lines, but doesn't obscure any information and doesn't break the UI. Still, I'll fix it now :)

str4d added some commits Sep 3, 2016

Add a persistent screen showing basic node metrics
The screen is implemented using ANSI Escape sequences.

Closes #1331
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Found a bug: the solver increment call for tromp's solver was added inside the solution for loop, so it is counting every found solution instead of every run. Rebasing on master to fix.

Contributor

str4d commented Oct 22, 2016

Found a bug: the solver increment call for tromp's solver was added inside the solution for loop, so it is counting every found solution instead of every run. Rebasing on master to fix.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

Oops, should have checked that more carefully.

Contributor

daira commented Oct 22, 2016

Oops, should have checked that more carefully.

Comment out print statements in tromp's solver
This prevents the solver interfering with the metrics screen.
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Rebased on master. Should be good to go :)

Contributor

str4d commented Oct 22, 2016

Rebased on master. Should be good to go :)

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

re-utACK. (I checked the fix for the bug @str4d mentioned, and the last commit commenting out printfs.)

Contributor

daira commented Oct 22, 2016

re-utACK. (I checked the fix for the bug @str4d mentioned, and the last commit commenting out printfs.)

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Assigning to @nathan-at-least for a decision, because @zookozcash really really really wants this merged, particularly as we also merged #1578 to include a faster miner (although that is not enabled by default, but everyone is going to use it anyway).

Contributor

str4d commented Oct 22, 2016

Assigning to @nathan-at-least for a decision, because @zookozcash really really really wants this merged, particularly as we also merged #1578 to include a faster miner (although that is not enabled by default, but everyone is going to use it anyway).

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

@nathan-at-least said:

I won't have time today, most likely. I'm comfortable with you deciding. If it still needs my input on Monday I'll look at it early.

My decision is yes, provided it merges and tests cleanly now. @zkbot r+

Contributor

daira commented Oct 22, 2016

@nathan-at-least said:

I won't have time today, most likely. I'm comfortable with you deciding. If it still needs my input on Monday I'll look at it early.

My decision is yes, provided it merges and tests cleanly now. @zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

📌 Commit dccc140 has been approved by daira

Contributor

zkbot commented Oct 22, 2016

📌 Commit dccc140 has been approved by daira

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

⌛️ Testing commit dccc140 with merge 5562f8d...

Contributor

zkbot commented Oct 22, 2016

⌛️ Testing commit dccc140 with merge 5562f8d...

zkbot pushed a commit that referenced this pull request Oct 22, 2016

zkbot
Auto merge of #1375 - str4d:1331-node-metrics, r=daira
Add node metrics screen

Continuation of #1336
Closes #1331

@daira daira modified the milestones: 1.0.0-rc2, 1.0.1 stabilization Oct 22, 2016

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

💔 Test failed - zcash

Contributor

zkbot commented Oct 22, 2016

💔 Test failed - zcash

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

@daira:

My decision is yes, provided it merges and tests cleanly now.

I just pushed the one-line fix to get the RPC tests running. Should I re-run?

Contributor

str4d commented Oct 22, 2016

@daira:

My decision is yes, provided it merges and tests cleanly now.

I just pushed the one-line fix to get the RPC tests running. Should I re-run?

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 22, 2016

Contributor

Gonna re-run

@zkbot r+

Contributor

str4d commented Oct 22, 2016

Gonna re-run

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

📌 Commit d6feeef has been approved by str4d

Contributor

zkbot commented Oct 22, 2016

📌 Commit d6feeef has been approved by str4d

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 22, 2016

Contributor

⌛️ Testing commit d6feeef with merge a884e5f...

Contributor

zkbot commented Oct 22, 2016

⌛️ Testing commit d6feeef with merge a884e5f...

zkbot pushed a commit that referenced this pull request Oct 22, 2016

zkbot
Auto merge of #1375 - str4d:1331-node-metrics, r=str4d
Add node metrics screen

Continuation of #1336
Closes #1331
@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 22, 2016

Contributor

@zkbot cancel

Contributor

daira commented Oct 22, 2016

@zkbot cancel

Disable metrics screen in RPC tests
Author: Jack Grigg <jack@z.cash>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>

@daira daira assigned daira and unassigned nathan-at-least Oct 23, 2016

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 23, 2016

Contributor

Please don't r+ until I've checked something locally. (I want to check how the metrics screen interacts with -printtoconsole.)

Contributor

daira commented Oct 23, 2016

Please don't r+ until I've checked something locally. (I want to check how the metrics screen interacts with -printtoconsole.)

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 23, 2016

Contributor

OK, showmetrics is disabled when printtoconsole is enabled. Just doing a smoke test...

Contributor

daira commented Oct 23, 2016

OK, showmetrics is disabled when printtoconsole is enabled. Just doing a smoke test...

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 23, 2016

Contributor

Smoke-tested ACK. @zkbot r+

Contributor

daira commented Oct 23, 2016

Smoke-tested ACK. @zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 23, 2016

Contributor

📌 Commit 02a4ace has been approved by daira

Contributor

zkbot commented Oct 23, 2016

📌 Commit 02a4ace has been approved by daira

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 23, 2016

Contributor

⌛️ Testing commit 02a4ace with merge a294b26...

Contributor

zkbot commented Oct 23, 2016

⌛️ Testing commit 02a4ace with merge a294b26...

zkbot pushed a commit that referenced this pull request Oct 23, 2016

zkbot
Auto merge of #1375 - str4d:1331-node-metrics, r=daira
Add node metrics screen

Continuation of #1336
Closes #1331
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 23, 2016

Contributor

☀️ Test successful - zcash

Contributor

zkbot commented Oct 23, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 02a4ace into zcash:master Oct 23, 2016

1 check passed

homu Test successful
Details

@str4d str4d deleted the str4d:1331-node-metrics branch Oct 23, 2016

@lfarkas

This comment has been minimized.

Show comment
Hide comment
@lfarkas

lfarkas Nov 4, 2016

this commit has a few bug since a few printf was the only one statement in the if condition above. anyway i'm in a process to update zcash's tromp to his current version.

lfarkas commented on dccc140 Nov 4, 2016

this commit has a few bug since a few printf was the only one statement in the if condition above. anyway i'm in a process to update zcash's tromp to his current version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment