Page MenuHomePhabricator

[rpc] Add logging RPC
Needs ReviewPublic

Authored by nakihito on Jun 28 2019, 18:47.

Details

Reviewers
deadalnix
Fabien
jasonbcox
markblundeberg
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

PR10150:
7fd50c3 allow libevent logging to be updated during runtime (John Newbery)
5255aca [rpc] Add logging RPC (John Newbery)
4d9950d Set BCLog::LIBEVENT correctly for old libevent versions. (John Newbery)

Tree-SHA512: d6788a7205372c0528da71eca052910dfb055f2940ca884f422ff3db66e23a2b49c6a15b8f27d5255554fe5c5a928f5dd903fdc63b0bd6c8fa7783e77bb30fe8

Backport of Core PR10150:
https://github.com/bitcoin/bitcoin/pull/10150

Also pulled the EnableOrDisableLogCategories() function and the updated libevent
filter code and Logger::GetCategoryMask() function from Core
PR12954
https://github.com/bitcoin/bitcoin/pull/12954

Test Plan
make check
./bitcoind

./bitcoin-cli help

logging rpc should not show up in the list of commands.

./bitcoin-cli help logging

This should display logging rpc help

./bitcoin-cli logging

This should display the list of logging options all set to false (default).

./bitcoin-cli logging "[\"all"\"]
./bitcoin-cli getbestblockhash
check debug.log

The first line should output the list of logging options all set to true if your libevent version is at or above v2.1.1. Otherwise, every option except libevent will be set to true.
The second line should cause the logger to write some extra debug information to the debug.log file.

./bitcoin-cli logging "[]" "[\"all\"]"
./bitcoin-cli getbestblockhash
check debug.log

The first line should output the list of logging options all set to false again.
The second line should cause the logger to revert back to its original logging behavior, outputting no extra logging information to debug.log.

./bitcoin-cli logging "[\"libevent"\"]

If your version of libevent is below v2.1.1, you should receive an error stating libevent logging cannot be updated when using libevent before v2.1.1. If your libevent is version is at or above 2.1.1, then it should proceed as normal. Tested by changing line 431 in httpserver.cpp from #if LIBEVENT_VERSION_NUMBER >= 0x02010100 to #if LIBEVENT_VERSION_NUMBER < 0x02010100.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR10150
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6599
Build 11245: Bitcoin ABC Teamcity Staging
Build 11244: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Owners added a reviewer: Restricted Owners Package.Jun 28 2019, 18:47
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 28 2019, 18:47
nakihito planned changes to this revision.Jun 28 2019, 18:47
nakihito requested review of this revision.Jun 28 2019, 19:42
Fabien added a comment.Jun 28 2019, 22:24

I understand that you need to diverge from core, but rather than adding the new functions enableCategoryMask and disableCategoryMask, you'd better port the core equivalent directly:

static void EnableOrDisableLogCategories(UniValue cats, bool enable) {
    cats = cats.get_array();
    for (unsigned int i = 0; i < cats.size(); ++i) {
        std::string cat = cats[i].get_str();

        bool success;
        if (enable) {
            success = LogInstance().EnableCategory(cat);
        } else {
            success = LogInstance().DisableCategory(cat);
        }

        if (!success) {
            throw JSONRPCError(RPC_INVALID_PARAMETER, "unknown logging category " + cat);
        }
    }
}
src/logging.cpp
134 ↗(On Diff #9787)

Use a range base loop, see ListLogCategories() for an example.

src/rpc/misc.cpp
443 ↗(On Diff #9787)

static

445 ↗(On Diff #9787)

size_t. Sadly, Univalue does not support range based loops (yet).

456 ↗(On Diff #9787)

static

Fabien requested changes to this revision.Jun 28 2019, 22:24
This revision now requires changes to proceed.Jun 28 2019, 22:24
Fabien added a comment.Jun 28 2019, 22:26

Also I don't see the filter on libevent, even if there is a comment remaining about it. Is there a reason why it is not required for us ?

nakihito planned changes to this revision.Jun 28 2019, 23:39
In D3464#81957, @Fabien wrote:

Also I don't see the filter on libevent, even if there is a comment remaining about it. Is there a reason why it is not required for us ?

I think I just forgot a squash since I had originally planned to do each commit individually. Will add this in the next revision. I did notice I missed the Makefile.test.include changes because it was moved and rearranged in D987.

In D3464#81950, @Fabien wrote:

I understand that you need to diverge from core, but rather than adding the new functions enableCategoryMask and disableCategoryMask, you'd better port the core equivalent directly:

static void EnableOrDisableLogCategories(UniValue cats, bool enable) {
    cats = cats.get_array();
    for (unsigned int i = 0; i < cats.size(); ++i) {
        std::string cat = cats[i].get_str();
        bool success;
        if (enable) {
            success = LogInstance().EnableCategory(cat);
        } else {
            success = LogInstance().DisableCategory(cat);
        }
        if (!success) {
            throw JSONRPCError(RPC_INVALID_PARAMETER, "unknown logging category " + cat);
        }
    }
}

This is not present in the PR. After some digging, I found the PR that added it here: https://github.com/bitcoin/bitcoin/pull/12954
Would you prefer it if I combine this PR and the relevant part of PR12954 into one patch? I was unaware of this change until now, so I'm not committed to my solution of two separate functions.

Fabien added a comment.Jun 29 2019, 07:07

I think I just forgot a squash since I had originally planned to do each commit individually. Will add this in the next revision. I did notice I missed the Makefile.test.include changes because it was moved and rearranged in D987.

OK I thought it was left apart on purpose. Yes, you don't need the Makefile part.

This is not present in the PR. After some digging, I found the PR that added it here: https://github.com/bitcoin/bitcoin/pull/12954
Would you prefer it if I combine this PR and the relevant part of PR12954 into one patch? I was unaware of this change until now, so I'm not committed to my solution of two separate functions.

It's obviously not part of the PR, but neither are your enable/disable functions.
No need to squash X backports here, just copy this single function to match core's master. It achieves the same goal and will prevent conflicts with later backports.
We are already out of order for the logger code, I'll rather avoid creating more intermediate state.

nakihito updated this revision to Diff 9869.Jul 1 2019, 18:28

Removed enableCategoryMask and disableCategoryMask functions and replaced them with EnableOrDisableLogCategories from Core PR12954. Also squashed libevent commit.

jasonbcox requested changes to this revision.Jul 1 2019, 21:22

Please update the summary with all of the changes that were pulled into this diff. It's hard to know what's included without digging through the comments.

This revision now requires changes to proceed.Jul 1 2019, 21:22
nakihito requested review of this revision.Jul 1 2019, 21:47
nakihito edited the summary of this revision. (Show Details)
Fabien requested changes to this revision.Jul 2 2019, 06:32
Fabien added inline comments.
src/httpserver.h
44

Nit: layout

/**
 * Lorem ipsum dolor sit amet ...
 * [...]
 */
src/rpc/misc.cpp
495

The libevent filter is still missing.

It is required as it adds 2 features:

  • Throw an error if the libevent flag is to be set/unset alone with a libevent version < 2.1.1
  • If multiple flags are changed, discard the libevent one if version < 2.1.1

You also want to test these 2 features (you can mock the version to test it).

This revision now requires changes to proceed.Jul 2 2019, 06:32
nakihito updated this revision to Diff 9969.Jul 3 2019, 18:24

Added libevent filter, but had to pull the updated code from PR12954 along with some other changes to make everything fit together.

nakihito planned changes to this revision.Jul 3 2019, 18:24
nakihito edited the summary of this revision. (Show Details)Jul 3 2019, 18:36
nakihito edited the test plan for this revision. (Show Details)Jul 3 2019, 18:56
nakihito updated this revision to Diff 10016.Jul 5 2019, 17:31
nakihito edited the summary of this revision. (Show Details)

Fixed comment.

nakihito edited the test plan for this revision. (Show Details)Jul 5 2019, 17:36
Fabien accepted this revision.Jul 5 2019, 20:50
deadalnix requested changes to this revision.Jul 6 2019, 14:36
deadalnix added inline comments.
src/rpc/misc.cpp
462 ↗(On Diff #10016)

Presumably, this should be static?

This revision now requires changes to proceed.Jul 6 2019, 14:36
nakihito updated this revision to Diff 10098.Jul 8 2019, 17:01

Changed logging() to static.

nakihito updated this revision to Diff 10298.Jul 15 2019, 18:18

Added PR11626 to make the logging rpc public.

nakihito edited the summary of this revision. (Show Details)Jul 15 2019, 18:19
nakihito edited the test plan for this revision. (Show Details)
nakihito planned changes to this revision.Jul 16 2019, 22:18
nakihito updated this revision to Diff 10329.Jul 16 2019, 22:34

Removed PR11626. It will be made into its own diff.

nakihito edited the summary of this revision. (Show Details)Jul 16 2019, 22:35
nakihito edited the test plan for this revision. (Show Details)
jasonbcox accepted this revision.Jul 16 2019, 23:14
deadalnix requested changes to this revision.Thu, Sep 12, 08:51

This is just too big and unfortunately, too littered with errors to be trusted. Reviewing this kind of patches impose a very high cognitive load on the reviewer. This will need to improve or the review cycle is going to be very slow.

src/httpserver.cpp
381 ↗(On Diff #10329)

This does the exact reverse of the original code.

src/logging.h
12 ↗(On Diff #10329)

This is completely unnecessary.

73 ↗(On Diff #10329)

Where does this change come from?

This revision now requires changes to proceed.Thu, Sep 12, 08:51
nakihito updated this revision to Diff 11357.Tue, Sep 17, 00:40

Fix comment, removed unnecessary #include, and fixed libevent filter.

nakihito planned changes to this revision.Tue, Sep 17, 00:40
nakihito requested review of this revision.Tue, Sep 17, 18:45
nakihito added inline comments.
src/httpserver.cpp
389 ↗(On Diff #11357)

From PR12954

src/logging.h
97 ↗(On Diff #11357)

From PR12954.

src/rpc/misc.cpp
418 ↗(On Diff #11357)

From PR12954