search

ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
215 stars 194 forks source link

Update for supporting multiple logging level settings #290

Closed iuhilnehc-ynos closed 4 years ago

iuhilnehc-ynos commented 4 years ago

Signed-off-by: Chen Lihui Lihui.Chen@sony.com

iuhilnehc-ynos commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

iuhilnehc-ynos commented 4 years ago

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

the last one wins

gbiggs commented 4 years ago

Then the design document must say that.

iuhilnehc-ynos commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

fujitatomoya commented 4 years ago

I am really good to go with either one.

fujitatomoya commented 4 years ago

@ivanpauno @hidmic @wjwwood

friendly ping.

ivanpauno commented 4 years ago

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 commented 4 years ago 
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 commented 4 years ago

@clalancette FYI

clalancette commented 4 years ago

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 commented 4 years ago

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

clalancette commented 4 years ago

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.

gbiggs commented 4 years ago

LGTM

clalancette commented 4 years ago

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