Page MenuHomePhabricator

[rpc] Add logging RPC
ClosedPublic

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
Commits
rABCa1f767f4b1e9: [rpc] Add logging RPC
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

Our logging code was refactored some time ago in a way that looks a lot like Core PR12954. Functionally, however, we are still behind in comparison to Core when their refactor occurred. That has made this backport very complicated. I have made comments on each change that directly link to the PR it was backported from to assist in review. As mentioned in the summary, some changes had to be modified to bridge the gap between the state of Core's logging code before and after the refactor in PR12954.

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

As is, Core's logging code as seen in PR10150 is incompatible with our own logging code. I Also pulled the EnableOrDisableLogCategories() function and the updated libevent filter code and Logger::GetCategoryMask() function from Core PR12954 which is a refactor of Core's logging code that makes it more closely mirror our own.
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 6873
Build 11793: Bitcoin ABC Buildbot (legacy)
Build 11792: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nakihito added a comment.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.Sep 12 2019, 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

This does the exact reverse of the original code.

src/logging.h
12

This is completely unnecessary.

73

Where does this change come from?

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

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

nakihito planned changes to this revision.Sep 17 2019, 00:40
nakihito requested review of this revision.Sep 17 2019, 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

nakihito edited the summary of this revision. (Show Details)Oct 1 2019, 20:08
nakihito added inline comments.Oct 1 2019, 20:39
src/httpserver.cpp
386 ↗(On Diff #11357)
389 ↗(On Diff #11357)
451 ↗(On Diff #11357)
src/httpserver.h
48 ↗(On Diff #11357)
src/logging.cpp
142 ↗(On Diff #11357)
src/logging.h
30 ↗(On Diff #11357)
72 ↗(On Diff #11357)
97 ↗(On Diff #11357)
119 ↗(On Diff #11357)
124 ↗(On Diff #11357)
src/rpc/client.cpp
130 ↗(On Diff #11357)
src/rpc/misc.cpp
418 ↗(On Diff #11357)
449 ↗(On Diff #11357)
452 ↗(On Diff #11357)

This and the above if differ from PR12954's code here, but this is fixed in D3997.

453 ↗(On Diff #11357)
476 ↗(On Diff #11357)
486 ↗(On Diff #11357)
530 ↗(On Diff #11357)
nakihito added a comment.Oct 1 2019, 20:51

Our logging code was refactored some time ago in a way that looks a lot like Core PR12954. Functionally, however, we are still behind in comparison to Core when their refactor occurred. That has made this backport very complicated. I have made comments on each change that directly link to the PR it was backported from to assist in review. As mentioned in the summary, some changes had to be modified to bridge the gap between the state of Core's logging code before and after the refactor in PR12954.

nakihito edited the summary of this revision. (Show Details)Oct 1 2019, 20:51
nakihito updated this revision to Diff 13331.Oct 2 2019, 23:38

Fixed comment formatting.

jasonbcox accepted this revision.Oct 2 2019, 23:49
deadalnix accepted this revision.Oct 17 2019, 01:25
This revision is now accepted and ready to land.Oct 17 2019, 01:25
This revision was automatically updated to reflect the committed changes.