Skip to content
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 logging rpc #10150

Merged
merged 3 commits into from Apr 12, 2017

Conversation

@jnewbery
Copy link
Member

commented Apr 4, 2017

Adds an RPC to get/set active logging categories.

First commit allows all categories except libevent to be reconfigured during runtime.
Second commit modifies InitHTTPServer() to allow leveldb logging to be reconfigured during runtime.

@jnewbery jnewbery changed the title [rpc Add logging rpc [rpc] Add logging rpc Apr 4, 2017
@jnewbery jnewbery force-pushed the jnewbery:logging_rpc branch Apr 4, 2017
src/httpserver.cpp Outdated
} else {
event_enable_debug_logging(EVENT_DBG_NONE);
}
event_enable_debug_logging(EVENT_DBG_ALL);

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 5, 2017

Member

Don't do this! This existed for a reason. What you did will reduce libevent performance extremely by always enabling their internal debugging. Same is probably true for leveldb. I'd prefer just not to allow to switch those at runtime.
(unless it can be done in a thread-safe way later on, but I don't think so)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 5, 2017

Author Member

Are you sure? I've tested this and logging is only enabled when libevent is built in debug mode:

https://github.com/libevent/libevent/blob/2e52bace9f9998826bd3819af328efc8d18decf9/log-internal.h#L46

and

https://github.com/libevent/libevent/blob/2e52bace9f9998826bd3819af328efc8d18decf9/log-internal.h#L86

I would have thought that if we're running libevent in debug, then performance is already hosed.

If I'm wrong, I left this as a separate commit so we can leave it out if needed. The first commit forbids trying to change libevent logging during runtime and stands on its own.

leveldb: #9999 always hands a logging class to leveldb at startup, even if logging is disabled. I don't know if that affects performance. This PR doesn't change that behaviour.

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 6, 2017

Member

Yes, it is possible to compile libevent wholesale without debug messages. However, most distributions enable them.
Even if it is compiled with debug messages, normally they just sink at the libevent side before even being formatted: https://github.com/libevent/libevent/blob/master/log-internal.h#L80
Remember that libevent debugging is extremely noisy. It always is - the mask is currently unused - there are no specific flags.
There will be significant overhead to dropping the messages at our side, after formatting. That's worth it for the questionable advantage of being able to enable it during runtime.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 6, 2017

Author Member

Turns out libevent logging can be changed at runtime: https://github.com/libevent/libevent/blob/2e52bace9f9998826bd3819af328efc8d18decf9/whatsnew-2.1.txt#L269

I've pushed a new commit that leaves InitHTTPServer() unchanged and uses event_enable_debug_logging() to update logging.

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

Concept ACK ofcourse

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

Oh. That's great.
Concept ACK. Will test soon.

@jnewbery jnewbery force-pushed the jnewbery:logging_rpc branch Apr 6, 2017
@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

Pushed a new commit that I hope addresses @laanwj's point here: #10150 (comment)

src/rpc/misc.cpp Outdated
);
}

if (request.params.size() > 0 && request.params[0].isArray()) {

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 6, 2017

Member

It looks like this code is almost duplicated with the exclude case (except the |/& and true/false). Might make sense to roll this into a function that converts a list of flag strings into a bitmask and apply that wholesale.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 7, 2017

Author Member

sounds sensible. Done

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 10, 2017

Member

Yup, thanks, much better

@jnewbery jnewbery force-pushed the jnewbery:logging_rpc branch 3 times, most recently Apr 7, 2017
src/rpc/misc.cpp Outdated
for (const auto& logCatActive : vLogCatActive) {
result.pushKV(logCatActive.category, logCatActive.active);
}
result.pushKV("WARNING", "libevent logging cannot be updated when using libevent before v2.1.1.");

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 10, 2017

Member

If you add this warning I'd suggest it be context-sensitive:

  • libevent logging is being updated
  • the libevent version is older than 2.1.1

Though you could also make it an error in that case! (e.g. make UpdateHTTPServerLogging return false and then raise a JSON exception here)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 10, 2017

Author Member

I initially tried doing something like that, but it's a bit tricky:

  • we only want to raise an error if only libevent logging is changed (ie if the user wants to include or exclude "all", then we shouldn't throw an error simply because "all" includes libevent).
  • I've already applied the include/excude bitmasks by this point, and we need the before/after state to know whether to throw.

I'll see if I can come up with something sensible.

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 10, 2017

Member

I'm perfectly fine with removing this warning from this pull and doing that later. I just don't think it should be added unconditionally.

I've already applied the include/excude bitmasks by this point, and we need the before/after state to know whether to throw.

Sure. if((before ^ after) & BCLog::LIBEVENT) would work :-) Though it's probably best to do the check inside UpdateHTTPServerLogging because there you have the previous state, the desired new state, and the libevent version information available.

we only want to raise an error if only libevent logging is changed

That's a good point, and indeed speaks for making it a warning instead.

@jnewbery jnewbery force-pushed the jnewbery:logging_rpc branch 2 times, most recently Apr 10, 2017
@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2017

Pushed an updated version that hopefully addresses @laanwj's comments. Changes:

  • UpdateHTTPServerLogging() is now called by InitHTTPServer(). This contains the #if LIBEVENT_VERSION_NUMBER >= 0x02010100 preprocessor logic to one place.
  • UpdateHTTPServerLogging() now removes the BCLog::LIBEVENT bit flag if the libevent version does not support debug logging. That means that a call to the logging RPC will return the correct value for libevent (0 if libevent logging is not supported).
  • the logging RPC will throw an error if:
    • the libevent version does not support debug logging; and
    • the user tries to update only the BCLog::LIBEVENT flag
src/httpserver.cpp Outdated
return true;
#else
// Can't update libevent logging if version < 02010100
logCategories &= ~BCLog::LIBEVENT;

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 10, 2017

Member

Looks good to me. One nit: I don't think this global flag should be changed here as a side-effect. Maybe move the "disable the flag in logCategories if this function returns false" logic outside the function.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 10, 2017

Author Member

ok, changed so that the caller updates the logCategories bitfield (happens in two places - InitHTTPServer() and the logging() rpc).

@jnewbery jnewbery force-pushed the jnewbery:logging_rpc branch to 7fd50c3 Apr 10, 2017
@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

Tested ACK 7fd50c3

@laanwj laanwj merged commit 7fd50c3 into bitcoin:master Apr 12, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Apr 12, 2017
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
@jnewbery jnewbery deleted the jnewbery:logging_rpc branch Apr 12, 2017
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2019
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
Signed-off-by: Pasta <pasta@dashboost.org>
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 11, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 15, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019
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
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 20, 2019
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
@codablock codablock referenced this pull request Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.