fix(autoware_traffic_light_map_based_detector): output from screen to both

[kminoda]

Description

Set output from screen to both to improve the debugging efficiency of Autoware in general.
With this change, the output from node will not only be displayed in screen but also to the log file.

See https://github.com/ros2/launch/blob/986ea832cec49c18bee4e29ee4df37fe3844fa90/launch/launch/logging/__init__.py#L397-L487

Related links

TIER IV INTERNAL THREAD

How was this PR tested?

Not tested

Notes for reviewers

None

Interface changes

None.

Effects on system behavior

Although there will be a slight increase in computational load due to the additional log, I would say that the risk is minimal since the node's output does not occupy terminal at this moment. But just in case, I would like to make this change for several nodes chosen from perception component which tends to have some issues pretty often.

[github-actions]

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

• You've checked our contribution guidelines.
• Your PR follows our pull request guidelines.
• All required CI checks pass before marking the PR ready for review.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.18%. Comparing base (103e086) to head (cd13087).
Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8411      +/-   ##
==========================================
+ Coverage   24.06%   29.18%   +5.12%     
==========================================
  Files        1384     1601     +217     
  Lines      102216   117338   +15122     
  Branches    38963    50569   +11606     
==========================================
+ Hits        24594    34250    +9656     
+ Misses      75105    73913    -1192     
- Partials     2517     9175    +6658     

Flag	Coverage Δ		*Carryforward flag
differential	0.00% <ø> (?)
total	29.24% <ø> (+5.18%)	⬆️	Carriedforward from 824b68c

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yukkysaito]

@mitsudome-r @xmfcx
Do you have any comments on this Pull Request? In the past, the .ros/log files in the system have grown significantly, which caused the disk space on the autonomous driving ECU to become constrained. I'm a bit concerned that if log files are output everywhere, it might lead to unintended system shutdowns.

@kminoda
Is this change only for this node? Do you plan to apply this to other nodes as well? If so, I think we should establish some rules.

[kminoda]

@yukkysaito Thank you for the comment. I am willing to get an feedback from the community 🙏

As a first step, I would like to merge this change for several nodes which tend to have a lot of issues. For these nodes, I think this change would make a lot of difference when debugging. By choosing several nodes, I think we can minimize the impact on the system performance as well.

[mitsudome-r]

Do you have any comments on this Pull Request? In the past, the .ros/log files in the system have grown significantly, which caused the disk space on the autonomous driving ECU to become constrained. I'm a bit concerned that if log files are output everywhere, it might lead to unintended system shutdowns.

I think we should consider giving a guidance about how users can setup log rotation if they wish to when we set output to "both" for all the nodes.
If we are just changing few nodes for the time being, I don't think it is going to be a critical issue.

[kminoda]

@mitsudome-r Thank you for the comment. As you've mentioned, I would like to make this type of change to a few nodes where the debugging efficiency is critical even in short term perspective.

@yukkysaito Would you confirm if it is OK to merge several PRs that I've created for now? I will wait for your comment before merging them.

[youtalk]

If you have another logger outside ROS system (e.g. syslog), I think the output="both" option performs duplicate logging.

[xmfcx]

@yukkysaito like @mitsudome-r suggested, creating a tutorial to set up https://linux.die.net/man/8/logrotate would be helpful.

As for this PR, we can merge as is.

My suggestion would be to set the log preferences of all nodes from a global variable.

So everything will be logged to "screen or log file or both" when you select it from the main launch file.

Maybe we could also have a simple python node package that monitors the log directory and keeps it at certain size too.

Now

• Merge the PR.

Short term

• Allow setting all node log method from a single global variable.

Long term

• Create a logrotate tutorial
• Or create a log rotation node package

[yukkysaito]

@yukkysaito like @mitsudome-r suggested, creating a tutorial to set up https://linux.die.net/man/8/logrotate would be helpful.
As for this PR, we can merge as is.
My suggestion would be to set the log preferences of all nodes from a global variable.
So everything will be logged to "screen or log file or both" when you select it from the main launch file.
Maybe we could also have a simple python node package that monitors the log directory and keeps it at certain size too.

@mitsudome-r @xmfcx Thanks for your comments. I agree with you.
It would be fine if it's just this PR, but there are many similar PRs coming in.

• fix(autoware_image_projection_based_fusion): output from screen to both #8410
• fix(autoware_compare_map_segmentation): output from screen to both #8409
• fix(autoware_map_based_prediction): output from screen to both #8408
• fix(autoware_multi_object_tracker): output from screen to both #8407

@kminoda If it's not urgent, I might want to consider adopting the proposed short term change (Allow setting all node log methods from a single global variable) before proceeding with the merge. What do you think?

[xmfcx]

Allow setting all node log methods from a single global variable

If that's the case, we should close all these PRs and go for this option.
And make the default "screen" to not fill up the disk space.

[yukkysaito]

@kminoda cc @xmfcx
Why was this pull request merged? I understood that it was supposed to be merged after completing this TODO.

If that's the case, we should close all these PRs and go for this option.
And make the default "screen" to not fill up the disk space.

[kminoda]

@yukkysaito Sorry I missed the latest comment 🙏

Do we have any timeline for incorporating the global variable? Although this PR is not critically important, it is kind of urgent to set screen=both for some of the important nodes to reduce the debugging cost. So I still would like to merge these PRs for limited packages (of course not for all the packages 👍 ) unless the global variable feature is incorporated within a short period of time.

[yukkysaito]

@kminoda
If it's not urgent, it's not preferable to change to screen=both for individual packages. If we need to merge such PRs in the future, could you please create a PR for the global variable feature?