RPC: Improve help text and behavior of RPC-logging. by AkioNak · Pull Request #11191 · bitcoin/bitcoin

AkioNak · 2017-08-29T12:07:58Z

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.

maflcko · 2017-08-30T07:42:47Z

src/rpc/misc.cpp

        uint32_t flag = 0;
        std::string cat = cats[i].get_str();
-        if (!GetLogCategory(&flag, &cat)) {
+        if (!GetLogCategory(&flag, &cat) || (cat == "0" || cat == "1")) {


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

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

@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".

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.

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

maflcko · 2017-08-30T08:17:46Z

Concept ACK

AkioNak · 2017-09-06T07:47:40Z

@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.

kallewoof · 2017-11-15T04:00:06Z

src/rpc/misc.cpp

            throw JSONRPCError(RPC_INVALID_PARAMETER, "unknown logging category " + cat);
        }
+        if (flag == BCLog::NONE)
+            return 0;


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

if (..) { return 0; }

kallewoof · 2017-11-15T04:01:00Z

src/rpc/misc.cpp

    if (request.fHelp || request.params.size() > 2) {
        throw std::runtime_error(
-            "logging [include,...] <exclude>\n"
+            "logging (<include> <exclude>)\n"


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

kallewoof · 2017-11-15T04:01:48Z

src/rpc/misc.cpp

-            "When called without an argument, returns the list of categories that are currently being debug logged.\n"
-            "When called with arguments, adds or removes categories from debug logging.\n"
+            "When called without an argument, returns the list of categories with status that are currently being debug logged or not.\n"
+            "When called with arguments, adds or removes categories from debug logging and return the list above.\n"


and returns the list (s)

kallewoof · 2017-11-15T04:03:37Z

src/rpc/misc.cpp

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


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.

kallewoof · 2017-11-15T04:04:29Z

src/rpc/misc.cpp

-            "1. \"include\" (array of strings) add debug logging for these categories.\n"
-            "2. \"exclude\" (array of strings) remove debug logging for these categories.\n"
-            "\nResult: <categories>  (string): a list of the logging categories that are active.\n"
+            "In addition, the following are available as a category name with special meanings:\n"


as category names

AkioNak · 2017-11-15T07:40:56Z

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

kallewoof

utACK

fanquake · 2017-11-17T14:12:35Z

Please squash 1830d5c

promag

utACK.

promag · 2017-11-17T15:31:43Z

src/init.cpp

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

@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";})) {

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

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

Or simply:

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

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.

AkioNak · 2017-11-20T09:35:31Z

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

laanwj · 2017-11-30T09:12:12Z

Lightly tested ACK c60c49b

c60c49b Improve help text and behavior of RPC-logging (Akio Nakamura) Pull request description: 1. It is allowed `libevent` logging to be updated during runtime, but still described that restriction in the help text. So we delete these text. 2. Add a descrption about the evaluation order of `<include>` and `<exclude>` to clarify how debug loggig categories to be set. 3. Add a description about the available logging category `"all"` which is not explained. 4. Add `"optional"` to the help text of `<include>` and `<exclude>`. 5. Add missing new lines before `"Argument:"`. 6. `"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. 7. It always returns all logging categories with status. Fix the help text to match this behavior. Tree-SHA512: c2142da1a9bf714af8ebc38ac0d82394e2073fc0bd56f136372e3db7b2af3b6746f8d6b0241fe66c1698c208c124deb076be83f07dec0d0a180ad150593af415

Summary: c60c49b Improve help text and behavior of RPC-logging (Akio Nakamura) Pull request description: 1. It is allowed `libevent` logging to be updated during runtime, but still described that restriction in the help text. So we delete these text. 2. Add a descrption about the evaluation order of `<include>` and `<exclude>` to clarify how debug loggig categories to be set. 3. Add a description about the available logging category `"all"` which is not explained. 4. Add `"optional"` to the help text of `<include>` and `<exclude>`. 5. Add missing new lines before `"Argument:"`. 6. `"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. 7. It always returns all logging categories with status. Fix the help text to match this behavior. Tree-SHA512: c2142da1a9bf714af8ebc38ac0d82394e2073fc0bd56f136372e3db7b2af3b6746f8d6b0241fe66c1698c208c124deb076be83f07dec0d0a180ad150593af415 Backport Core PR11191 bitcoin/bitcoin#11191 Depends on D3684 Test Plan: make check ./bitcoind ./bitcoin-cli help logging Changes should be reflected in the help text Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3699

c60c49b Improve help text and behavior of RPC-logging (Akio Nakamura) Pull request description: 1. It is allowed `libevent` logging to be updated during runtime, but still described that restriction in the help text. So we delete these text. 2. Add a descrption about the evaluation order of `<include>` and `<exclude>` to clarify how debug loggig categories to be set. 3. Add a description about the available logging category `"all"` which is not explained. 4. Add `"optional"` to the help text of `<include>` and `<exclude>`. 5. Add missing new lines before `"Argument:"`. 6. `"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. 7. It always returns all logging categories with status. Fix the help text to match this behavior. Tree-SHA512: c2142da1a9bf714af8ebc38ac0d82394e2073fc0bd56f136372e3db7b2af3b6746f8d6b0241fe66c1698c208c124deb076be83f07dec0d0a180ad150593af415

Summary: c60c49b Improve help text and behavior of RPC-logging (Akio Nakamura) Pull request description: 1. It is allowed `libevent` logging to be updated during runtime, but still described that restriction in the help text. So we delete these text. 2. Add a descrption about the evaluation order of `<include>` and `<exclude>` to clarify how debug loggig categories to be set. 3. Add a description about the available logging category `"all"` which is not explained. 4. Add `"optional"` to the help text of `<include>` and `<exclude>`. 5. Add missing new lines before `"Argument:"`. 6. `"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. 7. It always returns all logging categories with status. Fix the help text to match this behavior. Tree-SHA512: c2142da1a9bf714af8ebc38ac0d82394e2073fc0bd56f136372e3db7b2af3b6746f8d6b0241fe66c1698c208c124deb076be83f07dec0d0a180ad150593af415 Backport Core PR11191 bitcoin/bitcoin#11191 Depends on D3684 Test Plan: make check ./bitcoind ./bitcoin-cli help logging Changes should be reflected in the help text Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3699

c60c49b Improve help text and behavior of RPC-logging (Akio Nakamura) Pull request description: 1. It is allowed `libevent` logging to be updated during runtime, but still described that restriction in the help text. So we delete these text. 2. Add a descrption about the evaluation order of `<include>` and `<exclude>` to clarify how debug loggig categories to be set. 3. Add a description about the available logging category `"all"` which is not explained. 4. Add `"optional"` to the help text of `<include>` and `<exclude>`. 5. Add missing new lines before `"Argument:"`. 6. `"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. 7. It always returns all logging categories with status. Fix the help text to match this behavior. Tree-SHA512: c2142da1a9bf714af8ebc38ac0d82394e2073fc0bd56f136372e3db7b2af3b6746f8d6b0241fe66c1698c208c124deb076be83f07dec0d0a180ad150593af415

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

AkioNak force-pushed the fix_rpc_logging branch from 73494d1 to 0a9c8c7 Compare August 30, 2017 02:48

maflcko reviewed Aug 30, 2017

View reviewed changes

maflcko added the Docs label Aug 30, 2017

AkioNak mentioned this pull request Nov 14, 2017

Cleanup logging RPC help text #11631

Closed

kallewoof reviewed Nov 15, 2017

View reviewed changes

kallewoof suggested changes Nov 15, 2017

View reviewed changes

AkioNak force-pushed the fix_rpc_logging branch from 18ceaaa to bff0625 Compare November 15, 2017 07:38

kallewoof approved these changes Nov 15, 2017

View reviewed changes

promag reviewed Nov 17, 2017

View reviewed changes

AkioNak force-pushed the fix_rpc_logging branch from 97a9531 to c60c49b Compare November 20, 2017 08:49

laanwj merged commit c60c49b into bitcoin:master Nov 30, 2017

AkioNak deleted the fix_rpc_logging branch December 11, 2017 06:52

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

AkioNak commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 30, 2017

Uh oh!

AkioNak commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkioNak commented Nov 15, 2017

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 17, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkioNak commented Nov 20, 2017

Uh oh!

laanwj commented Nov 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maflcko Aug 30, 2017 •

edited

Loading