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.

Sign up for GitHub

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

Jump to bottom

This is the pull request for the patch of issue #548 and the already existing PR #418 #551

Closed
wants to merge 2 commits into from
Closed

This is the pull request for the patch of issue #548 and the already existing PR #418 #551

wants to merge 2 commits into from

Conversation

adrienbarral
Copy link

I propose to patch roslaunch to redirect stdout and stderr of the node subprocess in a python.logging.logger object that will be created with a RotatingFile handler. Parameters of this RotatingFile handlers are "maximum log file size" and "maximum log file count". These paramters will be two new attributes of the XML tag "node".

As suggested by dirk-thomas, I did a "cherry pick" of the feature introduced by PR #418 . With the use of logging.logger this was quite fastforward...

As I said in the commit comments I added some unit test, and I did some tests by hand because I don't find an "easy" way to build automatic unit test for file Rotation etc ...

I am open to any comments ...

aba added 2 commits January 16, 2015 11:37
Issue ros#548
2bf0f86 
Here is a patch that allow to keep control on the maximum size of the logfiles.
Two new attributes have been added to the "node" element of roslaunch XML files :
* max_logfile_size : a positive or null integer. This is the max size in byte that the logfile of a node will count.
* logfile_count : a positive or null integer. This is how much logfile will be rotated. To understand how this work, read the RotatingFileHandler of logging.handlers python module.

Unit Tests :
* I did some change in test-node-valid.xml, to ensure that adding the two new attributes don't break anything.
* I added two test XML files which are tested in a new unit test test_xmlloader.py.

That seem quite complexe to automate the testing of the feature I introduce in this patch (launching a node, and test that a rotating log file is created). So I only test it "by hand" analyzing a log folder when launching some big projects I do in my company, and some trivial launch files...
As suggested by dirk-thomas, here, I did a kine of "Cherry Pick" of t…
61f6fd4 
…he PR ros#418 submitted by eggerdo.

Now, if "output" attribute is "both", a file will be created, and output will be displayed on screen. Created file can be a Rotating file thanks to the previous commit.
@ros-pull-request-builder
Copy link
Member

Can one of the admins verify this patch?

@wjwwood
Copy link
Member

wjwwood commented Jan 16, 2015

Patches issue #548 and incorporates #418.

(hash tags don't auto link when in the subject line)

@IanTheEngineer
Copy link
Contributor

Wow, this PR seems to address many of the issues I've encountered with roslaunch logging. Is there any reason it wasn't fully incorporated into ros_comm (other than the merge conflicts)?

@IanTheEngineer IanTheEngineer mentioned this pull request Jul 6, 2016
Merged
@IanTheEngineer
Copy link
Contributor

Out of curiosity, has this not been merged due to failing tests? The tests themselves aren't entirely clear on what is wrong. The benefits of this PR are tangible - stdout and stderr in launch node logfiles, auto rotating of logs that get too big, etc. - so I'd prefer not to see this PR continue to languish.

@dirk-thomas
Copy link
Member

I can't comment on why it hasn't been reviewed earlier but I see the following issues with the current patch:

  • It adds a required argument to the LocalProcess constructor which is not feasible. It should instead add an optional keyword argument to the end of the argument list in order to not break compatibility.
  • Since the destination (stdout and stderr) is now always split (even when all the output should just go to the screen) I would expect the output order to get messed up (as commented on previous PRs: roslaunch option to log to both: screen and logfile #413 (comment)). I think it is reasonable to expect that with any change to the logging it shouldn't regress in cases which are currently supported.
  • Last time it was able to run the tests it failed several tests.
  • It has merge conflicts. When being updated it should be made against kinetic-devel since that is the current release.

@dirk-thomas dirk-thomas mentioned this pull request Aug 8, 2016
Closed
@harakas
Copy link

harakas commented Sep 17, 2016

Just a random comment: please add support for asynchronous logging into a file. As in prevent the node from blocking when file io is slow.

@asmodehn asmodehn mentioned this pull request Apr 28, 2017
Closed
@mohiz
Copy link

mohiz commented Jul 25, 2017

Does a plan exist for merging this PR in the near future? Log rotation is an important feature in a production level programs and I am wonder why this feature does not exist in ros log already. I am looking for a clean and standard way to use log rotation in my programs.

@dirk-thomas
Copy link
Member

Please see the one year old comment what needs to happen to get the patch merged. The PR will also need to retarget the latest ROS distro (currently lunar-devel).

@jacobperron jacobperron mentioned this pull request Sep 24, 2018
Closed
@jacknlliu
Copy link

@dirk-thomas any update?

@dirk-thomas
Copy link
Member

Closing due to long time of inactivity and targeting an EOL branch. Please consider opening a new PR targeting the current default branch if you are still interested in getting this patch merged.

@dirk-thomas dirk-thomas closed this Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
requires-changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@adrienbarral @ros-pull-request-builder @wjwwood @IanTheEngineer @dirk-thomas @harakas @mohiz @jacknlliu