[rpc] Add logging rpc #10150

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)

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.

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.

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.

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.

sounds sensible. Done

Yup, thanks, much better

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)

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.

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.

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.

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