Update for supporting multiple logging level settings

[iuhilnehc-ynos]

Signed-off-by: Chen Lihui [email protected]

[iuhilnehc-ynos]

To support multiple logging level settings, there might exist some other ways which could be convenient for users, such as

    a. use multiple items "--log-level", such as  
        
        --log-level debug --log-level talker1=error --log-level talker2=warn

    b. use one item, such as
        
        --log-level debug,talker1=error,talker2=warn

But to keep consistent with other arguments that using multiple items and use := as a separator, such as

--ros-args --param key1:=value1 --param key2:=value2

I wonder if users would like to use the multiple logger level setting as follows,

--log-level debug,talker1:=error,talker2:=warn

or just use

--log-level debug --log-level talker1:=error --log-level talker2:=warn

I'd like to hear your opinions.

[fujitatomoya]

But to keep consistent with other arguments that using multiple items and use := as a separator, such as

for user experience, i'd like to go with := separator to keep the consistency.

[iuhilnehc-ynos]

But to keep consistent with other arguments that using multiple items and use := as a separator, such as

for user experience, i'd like to go with := separator to keep the consistency.

@fujitatomoya
Thanks. I agree with you.

There are some ways to use :=,

A. --log-level debug,talker1:=error,talker2:=warn
B. --log-level debug --log-level talker1:=error --log-level talker2:=warn
C. both A and B

I think I need to hear others' suggestions.

[gbiggs]

Option A makes sense if you consider it to be the combination of all specifications given in option B. I think that option B is clearer and easier to parse, though.

[gbiggs]

What is the order of precedence if someone gives two different logging levels for a single node?

[iuhilnehc-ynos]

What is the order of precedence if someone gives two different logging levels for a single node?

the last one wins

[gbiggs]

Then the design document must say that.

[iuhilnehc-ynos]

Then the design document must say that.

Thanks. But I think it's the policy of all arguments, not just for --log-level. So maybe it should not be recorded here.

[gbiggs]

If you choose option A then it matters because the argument parser will only see a single argument, and whatever parses that string will need to know what to do.

[iuhilnehc-ynos]

If you choose option A then it matters because the argument parser will only see a single argument, and whatever parses that string will need to know what to do.

After reconsideration, I think I will use option B. (Actually I use option B in the design file).
No need to break the existing rule ( --param doesn't support --param key1:=value1,key2:=value2 )

[iuhilnehc-ynos]

@gbiggs
Anyway, thanks for your suggestion. (option B is an easy way)

[fujitatomoya]

I am really good to go with either one.

[fujitatomoya]

@ivanpauno @hidmic @wjwwood

friendly ping.

[ivanpauno]

What is the order of precedence if someone gives two different logging levels for a single node?

We currently have a lot of inconsistencies on how argument precedence works, I recently opened a PR about that.

[ivanpauno]

As @gbiggs suggested, I think it's good to clarify precedence order.
Using a "last specified argument prevails" rule sounds like a good idea.

This proposal is pretty consistent with how we parse other kinds of arguments, 👍 to that!

[ivanpauno]

A. --log-level debug,talker1:=error,talker2:=warn
B. --log-level debug --log-level talker1:=error --log-level talker2:=warn
C. both A and B

I would also like to mention that option C sounds reasonable to me.
But in that case, we should support similar syntax for arguments and parameters:

ros2 run package_name exec_name --ros-args -p param1:=value1,param2:=value2

That would be a much bigger effort that only implementing the new log level arguments.
I would try to keep the proposal as minimal as possible.

[ivanpauno]

@clalancette FYI

[clalancette]

A. --log-level debug,talker1:=error,talker2:=warn
B. --log-level debug --log-level talker1:=error --log-level talker2:=warn
C. both A and B

While I have my own personal preferences, I think consistency is more important here. With that in mind, I'd definitely stick with the := as the separator, as that is consistent with parameters.

I would also stick with option B here, as it more closely conforms to what parameters are doing. If we want to expand in the future to support syntax A, we would do it across all of the parameters we support.

[wjwwood]

I agree with B, until we want to support something like A everywhere, aka C.

[fujitatomoya]

LGTM

[ivanpauno]

I think it's worth clarifying the precedence in the design article.

[clalancette]

Both Ivan and I are happy with this. I'll let this sit another day before merging to see if there are any additional comments on this proposal, then I'll merge. Pinging @wjwwood @gbiggs for any other thoughts.

[wjwwood]

lgtm

I only had two nitpick comments that could be addressed or ignored.

[wjwwood]

So does Some here mean there are loggers you cannot set this way? If not, I'd drop it. If so, then I think we need some clarification on that.

[wjwwood]

It might be nice to have an example that's not just a node, like maybe rclcpp or talker1.rclcpp or something more interesting like that.

[gbiggs]

LGTM

[clalancette]

I'm going to go ahead and merge this with the latest fixes now. Thanks everyone for reviewing this and moving it forward.