Skip to content

Conversation

@chris-smith
Copy link
Collaborator

Pull request for issue #20. This is built on #19. I apparently also based it off of changes to add persistent service clients, which I thought was already in upstream but I guess I forgot to put up...

  • Pulls in bunyan for logging backend.
  • Exposes various log methods through rosnodejs.log.
  • Publish logs to /rosout
  • Logging tests

let service = rosNode.advertiseService('/set_bool', SetBool,
(req, resp) => {
console.log('Handling request! ' + JSON.stringify(req));
rosnodejs.log.info('Handling request! ' + JSON.stringify(req));
Copy link
Member

Choose a reason for hiding this comment

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

In general, I am a fan of verbosity. And I do like this fact that it is super obvious the kind of logger you are using here. But I am wondering how easy would it be for a user to alias rosnodejs.log.info as something like ros.info? Not saying that we need to do it for them though, just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could provide that - rospy provides all the methods with a log prefix to them (logerr, loginfo, logwarn, etc). This method lets you pull the logger out yourself and never refer to rosnodejs again though. So you could do

let log = rosnodejs.log;
log.info('A Message!');

You can also generate your owned named logger as a child of the root logger if you want to (and then it would be available through /get_loggers and /set_logger_level)

let customLogger = rosnodejs.log.getLogger('MyLogger');
customLogger.info('My other message');

You're less likely to get throttle or once clashes too if you create your own loggers as they each keep their own sets.
The log property from rosnodejs is actually the LoggingManager though. It exposes the logging methods of its root logger for ease of use. So all the methods available through rosnodejs.log are not necessarily available through loggers that you ask it to create.

@IanTheEngineer
Copy link
Member

Thank you for adding logging! Looks really good to me

@chris-smith
Copy link
Collaborator Author

chris-smith commented Jun 19, 2016

I'll squash this again before this gets merged - had a few fixes to make after pulling in upstream though and figured I'd make it clearer - should just be fd193d2.

@IanTheEngineer IanTheEngineer merged commit 184c867 into RethinkRobotics-opensource:kinetic-devel Jun 28, 2016
@chris-smith chris-smith deleted the updateLogging branch November 29, 2016 16:02
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.

2 participants