RPC: Improve help text and behavior of RPC-logging.

AkioNak commented at 12:07 pm on August 29, 2017: contributor

It is allowed libevent logging to be updated during runtime, but still described that restriction in the help text. So we delete these text.
Add a descrption about the evaluation order of <include> and <exclude> to clarify how debug loggig categories to be set.
Add a description about the available logging category "all" which is not explained.
Add "optional" to the help text of <include> and <exclude>.
Add missing new lines before "Argument:".
"0","1" are allowed in both array of <include> and <exclude>. "0" is ignored and "1" is treated same as "all". It is confusing, so forbid them.
It always returns all logging categories with status. Fix the help text to match this behavior.

fanquake added the label RPC/REST/ZMQ on Aug 29, 2017

AkioNak force-pushed on Aug 30, 2017

in src/rpc/misc.cpp:577 in 0a9c8c7ce0 outdated

573@@ -574,7 +574,7 @@ uint32_t getCategoryMask(UniValue cats) {
574     for (unsigned int i = 0; i < cats.size(); ++i) {
575         uint32_t flag = 0;
576         std::string cat = cats[i].get_str();
577-        if (!GetLogCategory(&flag, &cat)) {
578+        if (!GetLogCategory(&flag, &cat) || (cat == "0" || cat == "1")) {

MarcoFalke commented at 7:42 am on August 30, 2017:

According to the help text, this is a valid “category”:

0  -debug=<category>
1       Output debugging information (default: 0, ...

AkioNak commented at 9:21 am on August 30, 2017:

@MarcoFalke Thank you for your review. Oh, indeed. I think -debug=1(or0) is very clear for initial parameter, but I’m afraid that it is a little bit hard to understand that "0" is ignored in this RPC command. So, I will add {BCLog::NONE, "none"} to LogCategories[] and add to the help text that "0" and "1" are the shorthand of "none" and "all".

MarcoFalke commented at 9:30 am on August 30, 2017:

I wasn’t even aware of this rpc. You might want to wait for feedback by someone who used this. Though, I think your suggestion makes sense.

AkioNak commented at 10:06 am on August 30, 2017:

Then, I will wait for feedback for a while, thanks.

MarcoFalke commented at 8:17 am on August 30, 2017: member

Concept ACK

MarcoFalke added the label Docs and Output on Aug 30, 2017

AkioNak commented at 7:47 am on September 6, 2017: contributor

@MarcoFalke I added the followings:

So, I will add {BCLog::NONE, "none"} to LogCategories[] and add to the help text that "0" and "1" are the shorthand of "none" and "all".
made "0" and "none" to be treated as same in -debug forbitcoind.
make "0" and "none" means ignore other specified logging category of "include" or "exclude" for this RPC like when they are given to -debug.

in src/rpc/misc.cpp:581 in 3a9ae20e63 outdated

576@@ -577,6 +577,8 @@ uint32_t getCategoryMask(UniValue cats) {
577         if (!GetLogCategory(&flag, &cat)) {
578             throw JSONRPCError(RPC_INVALID_PARAMETER, "unknown logging category " + cat);
579         }
580+        if (flag == BCLog::NONE)
581+            return 0;

kallewoof commented at 4:00 am on November 15, 2017:

Style: Either use same line (if (..) return 0;) or use braces (

0if (..) {
1    return 0;
2}

in src/rpc/misc.cpp:591 in 3a9ae20e63 outdated

587@@ -586,16 +588,31 @@ UniValue logging(const JSONRPCRequest& request)
588 {
589     if (request.fHelp || request.params.size() > 2) {
590         throw std::runtime_error(
591-            "logging [include,...] <exclude>\n"
592+            "logging (<include> <exclude>)\n"

kallewoof commented at 4:01 am on November 15, 2017:

The standard is to have put space after ( and before ) for optional args, i.e. ( <include> <exclude> )

in src/rpc/misc.cpp:594 in 3a9ae20e63 outdated

592+            "logging (<include> <exclude>)\n"
593             "Gets and sets the logging configuration.\n"
594-            "When called without an argument, returns the list of categories that are currently being debug logged.\n"
595-            "When called with arguments, adds or removes categories from debug logging.\n"
596+            "When called without an argument, returns the list of categories with status that are currently being debug logged or not.\n"
597+            "When called with arguments, adds or removes categories from debug logging and return the list above.\n"

kallewoof commented at 4:01 am on November 15, 2017:

and returns the list (s)

in src/rpc/misc.cpp:595 in 3a9ae20e63 outdated

593             "Gets and sets the logging configuration.\n"
594-            "When called without an argument, returns the list of categories that are currently being debug logged.\n"
595-            "When called with arguments, adds or removes categories from debug logging.\n"
596+            "When called without an argument, returns the list of categories with status that are currently being debug logged or not.\n"
597+            "When called with arguments, adds or removes categories from debug logging and return the list above.\n"
598+			"The argument are evaluated for \"include\" first. That is, \"exclude\" takes precedence.\n"

kallewoof commented at 4:03 am on November 15, 2017:

A bit hard to understand. Maybe:

The arguments are evaluated in order \"include\", \"exclude\". If an item is both included and excluded, it will thus end up being excluded.

in src/rpc/misc.cpp:597 in 3a9ae20e63 outdated

600-            "libevent logging is configured on startup and cannot be modified by this RPC during runtime."
601-            "Arguments:\n"
602-            "1. \"include\" (array of strings) add debug logging for these categories.\n"
603-            "2. \"exclude\" (array of strings) remove debug logging for these categories.\n"
604-            "\nResult: <categories>  (string): a list of the logging categories that are active.\n"
605+            "In addition, the following are available as a category name with special meanings:\n"

kallewoof commented at 4:04 am on November 15, 2017:

as category names

kallewoof changes_requested

AkioNak force-pushed on Nov 15, 2017

AkioNak commented at 7:40 am on November 15, 2017: contributor

@kallewoof Thank you for your review. I fixed that which you pointed out.

kallewoof approved

kallewoof commented at 7:56 am on November 15, 2017: member

utACK

fanquake commented at 2:12 pm on November 17, 2017: member

Please squash 1830d5c

in src/init.cpp:929 in 1830d5c60a outdated

924@@ -925,7 +925,8 @@ bool AppInitParameterInteraction()
925         // Special-case: if -debug=0/-nodebug is set, turn off debugging messages
926         const std::vector<std::string> categories = gArgs.GetArgs("-debug");
927 
928-        if (find(categories.begin(), categories.end(), std::string("0")) == categories.end()) {
929+        if (std::none_of(categories.begin(), categories.end(),
930+            [](std::string cat){return cat == std::string("0") || cat == std::string("none");})) {

promag commented at 3:31 pm on November 17, 2017:

0[](const auto& cat){return cat == "0" || cat == "none";}

AkioNak commented at 1:51 pm on November 19, 2017:

@promag thank you for your review.

auto specifier can not use in lambda epression when -std=c++11.
std::string has == operater with c-string.

so I will change to following: [](std::string cat){return cat == "0" || cat == "none";})) {

promag commented at 10:31 pm on November 19, 2017:

BTW, this is not the 1st time I would prefer to read:

0std::set<std::string> categories = gArgs.GetUniqueArgs("-debug");
1
2if (!categories.count("0") && !categories.count("none")) {

Or simply:

0if (!gArgs.IsArgSet("-debug", "0") && !gArgs.IsArgSet("-debug", "none")) {

promag commented at 3:32 pm on November 17, 2017: member

utACK.

Improve help text and behavior of RPC-logging

A) The changes in behavior are as follows:
1. Introduce logging category "none" as alias of "0" for
   both RPC-logging and bitcoind "-debug" parameter.
2. Same as "0" is given to argument of "-debug",
   if "none" or "0" is given to <include>, all other given logging
   categories are ignored. The same is true for <exclude>.
   (Before this PR, "0" was accepted but just be ignored itself.)

B) The changes in the help text are as follows:
1. Add a descrption about the evaluation order of <include> and
   <exclude> to clarify how debug loggig categories to be set.
2. Delete text that describe restriction about libevent because
   it's already allowed libevent logging to be updated during runtime.
3. Add a description for category "all", "1", "none" and "0".
4. Add "optional" to the help text of <include> and <exclude>.
5. Add missing new lines before "Argument:".
6. This RPC always returns all logging categories with status.
   Fix the help text to match this behavior.

c60c49b679

AkioNak force-pushed on Nov 20, 2017

AkioNak commented at 9:35 am on November 20, 2017: contributor

@promag fixed simplify the right side of the equations. @fanquake squashed.

laanwj commented at 9:12 am on November 30, 2017: member

Lightly tested ACK c60c49b6794279325725a5c3c1a8d3dc6764b966

laanwj merged this on Nov 30, 2017

laanwj closed this on Nov 30, 2017

laanwj referenced this in commit ef14f2e3ff on Nov 30, 2017

AkioNak deleted the branch on Dec 11, 2017

jasonbcox referenced this in commit be32cf1c6c on Oct 25, 2019

PastaPastaPasta referenced this in commit f6ba93e2cb on Feb 27, 2020

PastaPastaPasta referenced this in commit f0500962b0 on Feb 27, 2020

PastaPastaPasta referenced this in commit 734cbe29d5 on Feb 27, 2020

PastaPastaPasta referenced this in commit 6557f54064 on Mar 14, 2020

PastaPastaPasta referenced this in commit d472588002 on Mar 14, 2020

PastaPastaPasta referenced this in commit 62987bee8c on Mar 14, 2020

jonspock referenced this in commit b8f1acbdd2 on Oct 1, 2020

jonspock referenced this in commit bad12a89c0 on Oct 5, 2020

jonspock referenced this in commit ff7f95f940 on Oct 10, 2020

ckti referenced this in commit 4450d49c00 on Mar 28, 2021

gades referenced this in commit bee498b241 on Jun 30, 2021

DrahtBot locked this on Sep 8, 2021

RPC: Improve help text and behavior of RPC-logging. #11191