Enabled Logger based on Severity

[CursedRock17]

Originally added in pull request #411 there is the following TODO:

TODO(dhood): determine this automatically from RCLCPP_LOG_MIN_SEVERITY

referencing the enabled of normal loggers. This pull request is meant to solve that TODO, or at least get a grasp on what determining loggers based on severity should look like. This could be useful for giving the user more control over how loggers turn to dummies based on the extent of failure.

[Barry-Xu-2018]

typo
RCLCPP_LOGG_MIN_SEVERITY_NONE -> RCLCPP_LOG_MIN_SEVERITY_NONE

RCLCPP_LOGG_MIN_SEVERITY_NONE means compile out all macros.
Is it correct to use this macro?

Maybe

#if RCLCPP_LOG_MIN_SEVERITY != RCLCPP_LOG_MIN_SEVERITY_NONE
#define RCLCPP_LOGGING_ENABLED 1
#endif

[CursedRock17]

That sounds more correct, I'll change that up soon.

[CursedRock17]

Should we set LOGGING_ENABLED to 1 based on any other LOG_MIN_SEVERITY?

[CursedRock17]

After the rebuilt CI it seems to have no problems

[fujitatomoya]

@CursedRock17 @Barry-Xu-2018

So the user is expected to specify RCLCPP_LOG_MIN_SEVERITY, and that configure the logging macros for severity during build time. (basically this is for more performance.)

i do not think this does not work anymore as expected (7 years ago, i wasn't here...) with current implementation of Logger class?

for example, if user application has something like this?

  // pseudo code
  rclcpp::Logger logger = rclcpp::get_logger("user_logger");
  logger.set_level(rclcpp::Logger::Level::Debug);  // generate RCLError exception if RCLCPP_LOGGING_ENABLED is 0
  RCLCPP_DEBUG(logger, "message %s", "debug");
  rclcpp::Logger::Level level = logger.get_effective_level();  // generate RCLInvalidArgument exception if RCLCPP_LOGGING_ENABLED is 0

originally, i believe that this TODO is meant to achieve the null operation for logging macros and Logger methods.
i think those methods need to be updated according to do the null operation without generating the exceptions if RCLCPP_LOGGING_ENABLED is 0?

[CursedRock17]

I can't seem to follow your train of thought all the way here, I would assume that we would want to throw an exception if RCLCPP_LOGGING_ENABLED is 0 as the user would be trying to reverse something they wanted at compile time. Take set_level, it will take the Level that we give it all the way through rcutils. We just have to ensure the user wants one setting (RCLCPP_LOG_MIN_SEVERITY), that isn't NONE before it gets passed down the stack.

Apart from where we should check for RCLCPP_LOGGING_ENABLED, what null operator are you talking about? Do you mean entirely removing the Logger, before getting to these functions in the first place?

[fujitatomoya]

@CursedRock17 sorry for the confusion, let me try to rephrase it.

for user experience, with #2384 (comment) sample code.

• RCLCPP_LOGGING_ENABLED=1 no problem, that works with configured RCLCPP_LOG_MIN_SEVERITY.
• RCLCPP_LOGGING_ENABLED=0, all the sudden, user code will generate the exception here and there. i think this is not expected behavior, user expects all the null operation for the logger for the performance w/o these exceptions.

i may be missing something, but it looks like originally this TODO was trying to address something like this?

[CursedRock17]

Oh, you're saying we don't want the Logger to be available at all, that way we won't have to worry about any incorrect exceptions. The only thing was throwing me off was the message in the TODO

determine this automatically from RCLCPP_LOG_MIN_SEVERITY

As I assumed that needed to be handled at compile time.

So, should we scrap what's already done in the PR, or should we just extend to these certain methods?

[fujitatomoya]

Oh, you're saying we don't want the Logger to be available at all, that way we won't have to worry about any incorrect exceptions.

yeah, that is what i thought of this TODO.

or should we just extend to these certain methods?

i think adding null operation for set_level and get_effective_level would be okay. lets keep this open to get more feedback?