Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to set logger severity level using a config file #347

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from

Conversation

JafarAbdi
Copy link

Add ability to specify the logger severity using a config file similar to ROS1's ROSCONSOLE_CONFIG_FILE env-variable, this can be done by export RCUTILS_LOGGING_CONFIG_FILE=path_to_config_file

This's a demo:

logging-2021-09-23_15.52.29.mp4

src/logging.c Outdated Show resolved Hide resolved
src/logging.c Outdated Show resolved Hide resolved
src/logging.c Show resolved Hide resolved
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some questions and comments,

  • environment variable prevails logging level arguments? I am not sure which one prevails to the other.
  • it should be able to set default level as well in the configuration file?
  • probably it would be nice to follow up tutorials how to configure logging level via RCUTILS_LOGGING_CONFIG_FILE?

// call to this function due to RCUTILS_LOGGING_AUTOINIT Check for the
// Check for the environment variable for selecting logging level
const char * logging_config_filename;
ret_str = rcutils_get_env("RCUTILS_LOGGING_CONFIG_FILE", &logging_config_filename);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about define for RCUTILS_LOGGING_CONFIG_FILE, so that we can refer to it.

src/logging.c Outdated
Comment on lines 311 to 312
char logger_name[50];
char severity[10]; // fatal error debug info warn case insensitive
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about allocate memory dynamically? I think that how it is done when parsing the arguments logging level.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I did some googling, looks like having scanf allocate memory is a posix extension which mean that it may not work on windows (I'm not sure)

Copy link

@destogl destogl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be very, very useful!

@aprotyas
Copy link
Member

aprotyas commented Sep 27, 2021

* environment variable prevails logging level arguments? I am not sure which one prevails to the other.

@fujitatomoya is there such a facility to set logging levels through environment variables? I don't think so, in which case this question would be moot for now. (correct me if I'm wrong here please)

@fujitatomoya
Copy link
Collaborator

is there such a facility to set logging levels through environment variables?

No, i do not think so. not via environmental variable, but we can change it with ros arguments. so i just thought that it should be discussed as design before implementation?

Signed-off-by: JafarAbdi <[email protected]>
@JafarAbdi
Copy link
Author

environment variable prevails logging level arguments?

No, logging level arguments prevail the environment variable ones, so if we have logger1 set to debug in the RCUTILS_LOGGING_CONFIG_FILE and we set it to error in the argument (ros2 run pkg node --ros-args --log-level logger1:=error) it will be set to error

it should be able to set default level as well in the configuration file?

Done in the latest commit

probably it would be nice to follow up tutorials how to configure logging level via RCUTILS_LOGGING_CONFIG_FILE?

I'm working on it now

@JafarAbdi
Copy link
Author

I'll add tests for this PR later this week

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also update the API documentation, describing the new environment variable and the expected file format.

@JafarAbdi
Copy link
Author

Please also update the API documentation, describing the new environment variable and the expected file format.

You mean this one, right .?

@jacobperron
Copy link
Member

@JafarAbdi I mean the docblock for the function being modified:

https://github.com/ros2/rcutils/blob/master/include/rcutils/logging.h#L49-L107

@clalancette clalancette self-assigned this Nov 4, 2021
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants