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

[RPC] Add an uptime command that displays the amount of time (in seconds) bitcoind has been running #10400

Merged
merged 1 commit into from Jun 27, 2017

Conversation

Projects
None yet
7 participants
@rvelhote
Contributor

rvelhote commented May 14, 2017

Hello everyone,

I am working on a tool that displays information about a bitcoin node using RPC. One of the parameters I would like to include is the uptime of the bitcoin daemon. Currently, as far as I can tell, there is no way of obtaining such information.

After thinking a bit on how to perform this task, I concluded that the easiest way to do it is to check the modification time of the bitcoind.pid file. This has the disadvantage that it does not work in Windows because bitcoind.pid is not created.

I propose the following solution in this PR and submit it for your review and opinion concerning this new RPC command before I work further on this topic. Do you think this new command makes sense? Please consider it a WIP/PoC as it does not have any error checking or tests.

Here is a sample return from the RPC command:

$ ./bitcoin-cli uptime  
{
  "uptime": 696
}

and the help function:

$bitcoin-cli help uptime  
uptime

Display the uptime of the Bitcoin server.

Best regards,
Ricardo Velhote

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 15, 2017

Member

After thinking a bit on how to perform this task, I concluded that the easiest way to do it is to check the modification time of the bitcoind.pid file.

There is no need to look at the timestamp of any files - the server could simply remember its own startup timestamp and use that as a reference. This would work just as well on windows.

Member

laanwj commented May 15, 2017

After thinking a bit on how to perform this task, I concluded that the easiest way to do it is to check the modification time of the bitcoind.pid file.

There is no need to look at the timestamp of any files - the server could simply remember its own startup timestamp and use that as a reference. This would work just as well on windows.

@rvelhote

This comment has been minimized.

Show comment
Hide comment
@rvelhote

rvelhote May 15, 2017

Contributor

Thank you for replying Wladimir. You are correct of course. Checking the PID file was the most simple and with the least changes to the codebase I could think of - while still being useful (I am not familiar with the Bitcoin core and have not worked with C++ since 2010).

So, if you think this command is useful for inclusion in the core, I could work on improving it and then squash the commits in the end.

Should this information be its own command or should it be included in another existing command - if so, which one?

Thank you,
Ricardo Velhote

Contributor

rvelhote commented May 15, 2017

Thank you for replying Wladimir. You are correct of course. Checking the PID file was the most simple and with the least changes to the codebase I could think of - while still being useful (I am not familiar with the Bitcoin core and have not worked with C++ since 2010).

So, if you think this command is useful for inclusion in the core, I could work on improving it and then squash the commits in the end.

Should this information be its own command or should it be included in another existing command - if so, which one?

Thank you,
Ricardo Velhote

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 15, 2017

Member

It seems to be useful.
Concept ACK.

Agree with @laanwj. Just set a variable during startup with GetTime() (now) and calculate the delta to "now()" when someone want's to know the current uptime.

Member

jonasschnelli commented May 15, 2017

It seems to be useful.
Concept ACK.

Agree with @laanwj. Just set a variable during startup with GetTime() (now) and calculate the delta to "now()" when someone want's to know the current uptime.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 15, 2017

Contributor

Concept ACK

The other way could be adding starttime to getinfo RPC. But I prefer your solution, separate simple approach, new RPC. Please also add a test case for this.

Contributor

paveljanik commented May 15, 2017

Concept ACK

The other way could be adding starttime to getinfo RPC. But I prefer your solution, separate simple approach, new RPC. Please also add a test case for this.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 15, 2017

Member

The other way could be adding starttime to getinfo RPC. But I prefer your solution, separate simple approach, new RPC. Please also add a test case for this.

A gentle reminder that getinfo should not be extended, see https://github.com/bitcoin/bitcoin/blob/master/src/rpc/misc.cpp#L34:

/**
 * @note Do not add or change anything in the information returned by this
 * method. `getinfo` exists for backwards-compatibility only. It combines
 * information from wildly different sources in the program, which is a mess,
 * and is thus planned to be deprecated eventually.
 *
 * Based on the source of the information, new information should be added to:
 * - `getblockchaininfo`,
 * - `getnetworkinfo` or
 * - `getwalletinfo`
 *
 * Or alternatively, create a specific query method for the information.
 **

One of the other get*info would be an option but I don't see where it'd clearly fit in. So agree a separate call is OK.

Member

laanwj commented May 15, 2017

The other way could be adding starttime to getinfo RPC. But I prefer your solution, separate simple approach, new RPC. Please also add a test case for this.

A gentle reminder that getinfo should not be extended, see https://github.com/bitcoin/bitcoin/blob/master/src/rpc/misc.cpp#L34:

/**
 * @note Do not add or change anything in the information returned by this
 * method. `getinfo` exists for backwards-compatibility only. It combines
 * information from wildly different sources in the program, which is a mess,
 * and is thus planned to be deprecated eventually.
 *
 * Based on the source of the information, new information should be added to:
 * - `getblockchaininfo`,
 * - `getnetworkinfo` or
 * - `getwalletinfo`
 *
 * Or alternatively, create a specific query method for the information.
 **

One of the other get*info would be an option but I don't see where it'd clearly fit in. So agree a separate call is OK.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 May 15, 2017

Member

There's already a variable in the GUI for getting the startup time:

static const int64_t nClientStartupTime = GetTime();

You could probably just move it so that it can be used for both the GUI and for this RPC call.

Member

achow101 commented May 15, 2017

There's already a variable in the GUI for getting the startup time:

static const int64_t nClientStartupTime = GetTime();

You could probably just move it so that it can be used for both the GUI and for this RPC call.

@rvelhote

This comment has been minimized.

Show comment
Hide comment
@rvelhote

rvelhote May 16, 2017

Contributor

Thank you for your feedback. I went ahead and made some modifications according to your suggestions.

I have pushed a new commit that removes nClientStartupTime from being specific to the QT interface and moved it to util.h where it can be used by both the QT interface and the RPC server.

I called it nStartupTime instead of nClientStartupTime because now it related to both the client and the daemon. I also decided on util.h instead of init.h. The reason is that the startup time is more of an utility/informational feature rather than a core initialization feature.

I am not sure of the naming conventions. Should I use g_nStartupTime to symbolize that it's a global variable or is that used for specific objects like g_connman?

I have build the QT client (QT5 in my case) and the daemon and from my testing every is working well. I have also started the QT client with the -server flag and both are returning the correct value. The QT client displays the startup date in an ISO format (Debug Window » Information) and the RPC server displays the amount of seconds that have passed since that startup date.

I will add a functional test to the RPC interface for this command. I'm not sure how the testing is done for the QT client.

Contributor

rvelhote commented May 16, 2017

Thank you for your feedback. I went ahead and made some modifications according to your suggestions.

I have pushed a new commit that removes nClientStartupTime from being specific to the QT interface and moved it to util.h where it can be used by both the QT interface and the RPC server.

I called it nStartupTime instead of nClientStartupTime because now it related to both the client and the daemon. I also decided on util.h instead of init.h. The reason is that the startup time is more of an utility/informational feature rather than a core initialization feature.

I am not sure of the naming conventions. Should I use g_nStartupTime to symbolize that it's a global variable or is that used for specific objects like g_connman?

I have build the QT client (QT5 in my case) and the daemon and from my testing every is working well. I have also started the QT client with the -server flag and both are returning the correct value. The QT client displays the startup date in an ISO format (Debug Window » Information) and the RPC server displays the amount of seconds that have passed since that startup date.

I will add a functional test to the RPC interface for this command. I'm not sure how the testing is done for the QT client.

Show outdated Hide outdated src/util.h Outdated
Show outdated Hide outdated src/rpc/server.cpp Outdated
Show outdated Hide outdated src/rpc/server.cpp Outdated
Show outdated Hide outdated src/rpc/server.cpp Outdated
@rvelhote

This comment has been minimized.

Show comment
Hide comment
@rvelhote

rvelhote May 16, 2017

Contributor

I have made a new commit with some changes suggested by Pavel.

While searching for examples of documentation I noticed getconnectioncount which only returns a single value (the node count). Previously the uptime command was returning an object with an uptime key so I followed the example of getconnectioncount and changed the return value to a single integer that represents the amount of seconds instead of an object.

Here is a sample of the documentation when running bitcoin-cli help uptime

$ bitcoin-cli help uptime
uptime

Returns the total uptime of the Bitcoin server.

Result:
ttt        (numeric) The number of seconds that the Bitcoin server has been running

Examples:
> bitcoin-cli uptime 
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "uptime", "params": [] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/

I used the ttt designation because I've seen it in other areas to indicate a time-based value (e.g. getpeerinfo).

Contributor

rvelhote commented May 16, 2017

I have made a new commit with some changes suggested by Pavel.

While searching for examples of documentation I noticed getconnectioncount which only returns a single value (the node count). Previously the uptime command was returning an object with an uptime key so I followed the example of getconnectioncount and changed the return value to a single integer that represents the amount of seconds instead of an object.

Here is a sample of the documentation when running bitcoin-cli help uptime

$ bitcoin-cli help uptime
uptime

Returns the total uptime of the Bitcoin server.

Result:
ttt        (numeric) The number of seconds that the Bitcoin server has been running

Examples:
> bitcoin-cli uptime 
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "uptime", "params": [] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/

I used the ttt designation because I've seen it in other areas to indicate a time-based value (e.g. getpeerinfo).

@rvelhote

This comment has been minimized.

Show comment
Hide comment
@rvelhote

rvelhote May 17, 2017

Contributor

I've added a functional test to check for the uptime value. In this test I've placed a time.sleep before calling the uptime command. The test then checks if the current uptime value reported by the RPC command is greater or equal than the value used in time.sleep.

As I've noted in the test comment:

Depending on how fast the setup executes, the RPC server starts and this test
executes the check for the uptime value might not deterministic. At the very
least we will check if the reported uptime is the amount of seconds we waited.

Contributor

rvelhote commented May 17, 2017

I've added a functional test to check for the uptime value. In this test I've placed a time.sleep before calling the uptime command. The test then checks if the current uptime value reported by the RPC command is greater or equal than the value used in time.sleep.

As I've noted in the test comment:

Depending on how fast the setup executes, the RPC server starts and this test
executes the check for the uptime value might not deterministic. At the very
least we will check if the reported uptime is the amount of seconds we waited.

Show outdated Hide outdated test/functional/server.py Outdated
@rvelhote

This comment has been minimized.

Show comment
Hide comment
@rvelhote

rvelhote May 18, 2017

Contributor

@paveljanik I made the change to setmocktime in the test and held off on the name change of the test file until I get your final feedback after my reply to your comment. renamed the testcase file for the uptime command.

Let me know when you think everything is in order and I will go ahead and squash the commits. Thank you for your help (and patience) in reviewing everything :)

Contributor

rvelhote commented May 18, 2017

@paveljanik I made the change to setmocktime in the test and held off on the name change of the test file until I get your final feedback after my reply to your comment. renamed the testcase file for the uptime command.

Let me know when you think everything is in order and I will go ahead and squash the commits. Thank you for your help (and patience) in reviewing everything :)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 26, 2017

Contributor

OK, let's squash please. The travis failure is unrelated (actually fixed in #10429).

Contributor

paveljanik commented May 26, 2017

OK, let's squash please. The travis failure is unrelated (actually fixed in #10429).

@jnewbery

A few nits. The only important one is nStartupTime shouldn't be defined as static in the util.h header file.

@@ -258,6 +258,22 @@ UniValue stop(const JSONRPCRequest& jsonRequest)
return "Bitcoin server stopping";
}
UniValue uptime(const JSONRPCRequest& jsonRequest)

This comment has been minimized.

@jnewbery

jnewbery Jun 8, 2017

Member

I think it makes more sense for this RPC to live in src/rpc/misc.cpp. I think server.cpp should be kept as lightweight and generic as possible.

@jnewbery

jnewbery Jun 8, 2017

Member

I think it makes more sense for this RPC to live in src/rpc/misc.cpp. I think server.cpp should be kept as lightweight and generic as possible.

This comment has been minimized.

@rvelhote

rvelhote Jun 8, 2017

Contributor

Thank you very much for your feedback John.

I placed it on src/rpc/server.php because the command was information related to the server rather than a utility command (although it can certainly be considered an utility command). That is my only argument for using server.cpp. Maybe let's wait for other people's opinion on this?

@rvelhote

rvelhote Jun 8, 2017

Contributor

Thank you very much for your feedback John.

I placed it on src/rpc/server.php because the command was information related to the server rather than a utility command (although it can certainly be considered an utility command). That is my only argument for using server.cpp. Maybe let's wait for other people's opinion on this?

This comment has been minimized.

@jnewbery

jnewbery Jun 8, 2017

Member

Maybe let's wait for other people's opinion on this?

certainly. Just my opinion that this should live in misc.cpp. I'm happy to defer to the consensus opinion.

@jnewbery

jnewbery Jun 8, 2017

Member

Maybe let's wait for other people's opinion on this?

certainly. Just my opinion that this should live in misc.cpp. I'm happy to defer to the consensus opinion.

This comment has been minimized.

@laanwj

laanwj Jun 19, 2017

Member

I understand both sides so don't have a strong opinion. We want to have multiple JSON-RPC endpoints at some point, so the class should be reusable without adding a lot of clutter, but it seems harmless if they all export uptime.

However: server.cpp should be independent of bitcoin, so if you plan on keeping it there, please remove the explicit reference to Bitcoin below in the documentation.

@laanwj

laanwj Jun 19, 2017

Member

I understand both sides so don't have a strong opinion. We want to have multiple JSON-RPC endpoints at some point, so the class should be reusable without adding a lot of clutter, but it seems harmless if they all export uptime.

However: server.cpp should be independent of bitcoin, so if you plan on keeping it there, please remove the explicit reference to Bitcoin below in the documentation.

@@ -0,0 +1,32 @@
#!/usr/bin/env python3

This comment has been minimized.

@jnewbery

jnewbery Jun 8, 2017

Member

I'd prefer not to add a whole extra test for just this one command. I think it'd be better to move the uptime rpc into misc.cpp and then combine signmessages.py, rpcnamedargs.py and this new test into a new test file names misc.py.

However, that tidying can definitely be done in a follow-up PR.

@jnewbery

jnewbery Jun 8, 2017

Member

I'd prefer not to add a whole extra test for just this one command. I think it'd be better to move the uptime rpc into misc.cpp and then combine signmessages.py, rpcnamedargs.py and this new test into a new test file names misc.py.

However, that tidying can definitely be done in a follow-up PR.

Show outdated Hide outdated src/rpc/server.cpp Outdated
Show outdated Hide outdated src/util.h Outdated
@rvelhote

This comment has been minimized.

Show comment
Hide comment
@rvelhote

rvelhote Jun 19, 2017

Contributor

@jnewbery @paveljanik When you have the opportunity, please let me know if you have any further feedback on this PR? The TravisCI build is failing but it's because of #10429 and I have not updated my branch since I started the PR. Thank you.

Contributor

rvelhote commented Jun 19, 2017

@jnewbery @paveljanik When you have the opportunity, please let me know if you have any further feedback on this PR? The TravisCI build is failing but it's because of #10429 and I have not updated my branch since I started the PR. Thank you.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 19, 2017

Contributor

Needs rebase

Contributor

paveljanik commented Jun 19, 2017

Needs rebase

Show outdated Hide outdated src/util.h Outdated

@laanwj laanwj self-assigned this Jun 26, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 27, 2017

Member

Tested ACK c074752

Member

laanwj commented Jun 27, 2017

Tested ACK c074752

@laanwj laanwj merged commit c074752 into bitcoin:master Jun 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 27, 2017

Merge #10400: [RPC] Add an uptime command that displays the amount of…
… time (in seconds) bitcoind has been running

c074752 [RPC] Add an uptime command that displays the amount of time that bitcoind has been running (Ricardo Velhote)

Tree-SHA512: 8f59d4205042885f23f5b87a0eae0f5d386e9c6134e5324598e7ee304728d4275f383cd154bf1fb25350f5a88cc0ed9f97edb099e9b50c4a0ba72d63ec5ca5b4

@laanwj laanwj referenced this pull request Jun 27, 2017

Closed

TODO for release notes 0.15.0 #9889

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