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

rvelhote · 2017-05-14T18:44:46Z

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 · 2017-05-15T05:03:36Z

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 · 2017-05-15T07:48:56Z

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 · 2017-05-15T07:50:44Z

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 · 2017-05-15T09:00:10Z

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 · 2017-05-15T10:40:12Z

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 · 2017-05-15T15:24:18Z

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

bitcoin/src/qt/clientmodel.cpp

Line 29 in 86ea3c2

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 · 2017-05-16T07:55:49Z

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.

paveljanik · 2017-05-16T10:25:33Z

src/util.h

// Application startup time (used for uptime calculation)

paveljanik · 2017-05-16T10:30:03Z

src/rpc/server.cpp

Do we need this at all?

paveljanik · 2017-05-16T10:35:11Z

src/rpc/server.cpp

Started in the future!? ;-)

I imagined that for some weird reason the clock would change to before the start and the user would get a negative value. Maybe I'm over-thinking this? ;)

Yup, I do not have a problem with this. Just return negative value and do not crash.

paveljanik · 2017-05-16T10:36:25Z

src/rpc/server.cpp

Let's document output as Result: as other RPCs. Add unit specification (seconds).

rvelhote · 2017-05-16T23:24:13Z

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 · 2017-05-17T21:58:12Z

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.

paveljanik · 2017-05-18T06:23:29Z

test/functional/server.py

You can use setmocktime to not really sleep/slow down tests for 6 seconds.

Arghh I didn't know about this setmocktime. Didn't think of searching either. Will get to it.

paveljanik · 2017-05-18T06:24:14Z

test/functional/test_runner.py

Please use the name uptime.py or so, not generic server.py.

I took as an example the net.py test file that referred to tests in rpc/net.cpp. Since the uptime command is in the server.cpp - although there are no tests for this file - I chose to create a server.py test file. Do you still think it's best to use uptime.py?

@rvelhote Yup, OK.

rvelhote · 2017-05-18T19:33:54Z

@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 · 2017-05-26T09:29:29Z

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.

jnewbery · 2017-06-08T18:21:27Z

src/rpc/server.cpp

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.

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?

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.

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.

jnewbery · 2017-06-08T18:25:32Z

test/functional/uptime.py

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 · 2017-06-08T18:26:38Z

src/rpc/server.cpp

nit: I don't really like ttt here. I think it's fine just to have:

"\"time\" (numeric) The number of seconds that the Bitcoin server has been running\n"

I used ttt because I saw it referenced in other commands for usage with time related values. For example in getpeerinfo you have conntime for a timestamp and also timeoffset for a time-based value in seconds. I think ttt will keep it consistent with other commands.

sure. Again, just my personal preference. ttt is fine.

jnewbery · 2017-06-08T19:06:35Z

src/util.h

This shouldn't be static (a static variable in a header file means that any .cpp file that includes the header will have its own local variable called nStartupTime).

Instead, I think you should define this variable in util.cpp and then have an extern declaration here. See BITCOIN_CONF_FILENAME below for example.

I don't know if you just copied the DEFAULT_LOG... constants below, but I believe those should also be defined in util.cpp.

The DEFAULT_LOG constants are already in master. Might be something to improve then.

Also, I don't know if you noticed, this variable was moved from src/qt/clientmodel.cpp to a more global area so that it can be reused by both the server and the QT client.

I will make this change as you suggested and force-push the commit.

Yes, sorry I wasn't clear - I didn't expect you to change the DEFAULT_LOG constants. I do however think you need to move the nStartupTime definition to avoid the following build warnings:

In file included from addrman.h:14:0, from addrdb.cpp:8: util.h:33:22: warning: ‘nStartupTime’ defined but not used [-Wunused-variable] static const int64_t nStartupTime = GetTime(); ^ CXX libbitcoin_server_a-bloom.o In file included from addrman.h:14:0, from addrman.cpp:6: util.h:33:22: warning: ‘nStartupTime’ defined but not used [-Wunused-variable] static const int64_t nStartupTime = GetTime(); ^ CXX libbitcoin_server_a-blockencodings.o /bin/bash ./config.status In file included from bitcoind.cpp:18:0: util.h:33:22: warning: ‘nStartupTime’ defined but not used [-Wunused-variable] static const int64_t nStartupTime = GetTime(); ^ ...(repeated many times)

No problem John, I understood what you meant :) When I mentioned I would make the change I was referring to moving nStartupTime to util.cpp. I pushed a new commit with this change.

rvelhote · 2017-06-19T08:47:56Z

@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 · 2017-06-19T11:12:35Z

Needs rebase

laanwj · 2017-06-19T14:04:09Z

src/util.h

Please add a GetStartupTime function instead of exporting this variable.

…coind has been running

laanwj · 2017-06-27T09:34:13Z

Tested ACK c074752

… 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

…ount 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

fanquake added the RPC/REST/ZMQ label May 15, 2017

paveljanik reviewed May 16, 2017

View reviewed changes

paveljanik reviewed May 18, 2017

View reviewed changes

rvelhote force-pushed the rpc-uptime branch from c5eb545 to 5ca8f57 Compare June 5, 2017 01:06

jnewbery reviewed Jun 8, 2017

View reviewed changes

rvelhote force-pushed the rpc-uptime branch from 5ca8f57 to 4e51e6d Compare June 8, 2017 22:05

laanwj reviewed Jun 19, 2017

View reviewed changes

src/util.h Outdated

Copy link

Member

laanwj Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a GetStartupTime function instead of exporting this variable.

rvelhote force-pushed the rpc-uptime branch from 4e51e6d to e0c650c Compare June 25, 2017 17:24

[RPC] Add an uptime command that displays the amount of time that bit…

c074752

…coind has been running

rvelhote force-pushed the rpc-uptime branch from 46915f9 to c074752 Compare June 25, 2017 19:26

laanwj self-assigned this Jun 26, 2017

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

laanwj mentioned this pull request Jun 27, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

TheBlueMatt mentioned this pull request Oct 16, 2017

estimatefee is returning way-off value #11502

Closed

sickpig mentioned this pull request Jan 16, 2020

[port][rpc] Add getchaintxstats and uptime RPC calls BitcoinUnlimited/BitcoinUnlimited#2070

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

rvelhote commented May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented May 15, 2017

Uh oh!

rvelhote commented May 15, 2017

Uh oh!

jonasschnelli commented May 15, 2017

Uh oh!

paveljanik commented May 15, 2017

Uh oh!

laanwj commented May 15, 2017

Uh oh!

achow101 commented May 15, 2017

Uh oh!

rvelhote commented May 16, 2017

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!

rvelhote commented May 16, 2017

Uh oh!

rvelhote commented May 17, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

rvelhote commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery 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!

laanwj Jun 19, 2017 • 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!

jnewbery Jun 8, 2017 • 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!

jnewbery Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

rvelhote commented May 14, 2017 •

edited

Loading

rvelhote commented May 17, 2017 •

edited

Loading

rvelhote commented May 18, 2017 •

edited

Loading

paveljanik commented May 26, 2017 •

edited

Loading

laanwj Jun 19, 2017 •

edited

Loading

jnewbery Jun 8, 2017 •

edited

Loading

jnewbery Jun 8, 2017 •

edited

Loading