skuhl / RobotRemote

0 stars 0 forks source link

Use Some Logging Framework. #47

Closed BinaryFissionGames closed 6 years ago

BinaryFissionGames commented 6 years ago

Right now, logging is done through console.log. It would be cool if we could have some framework that could automatically timestamp and tell what file/line log statements are coming from.

Some things I'm looking into:

BinaryFissionGames commented 6 years ago

For the record, I like log4js-node the best (mainly because of the built in express support), and I dislike npmlog the most (no timestamps, too basic). Winston seems like it could work well too though.

GraysonHoward commented 6 years ago

I feel like Winston is kinda complicated log4js-node seems a little more straight forward.

BinaryFissionGames commented 6 years ago

Agree, we'll use log4js-node. Essentially, we should replace all console.log statements with log4js stuff.

GraysonHoward commented 6 years ago

Okay then I'll work on this because it seems like something I can figure out and is Medium Priority. You expressed interest above in all our logs going to one file if using log4js is this still the case?

BinaryFissionGames commented 6 years ago

Yeah, if we can get it all in one file (or 2, one for errors, another for info), that would be nice.

GraysonHoward commented 6 years ago

Okay my question right now is how do I tell where a file is currently printing? I know that some of the messages were going to standard out like in single_machine_setup.js I'm just not sure if there are others going to standard out.

BinaryFissionGames commented 6 years ago

So, the run.js script redirects stdout and stderr for each of these processes. All console.log/.error statements print to stdout/stderr.

If you mean going to the console, none of the processes currently do.

So, if you're looking for the files that are currently set up, just take a look at run.js

GraysonHoward commented 6 years ago

Before I go too much further can you look at what I did in database_setup.js and see if that's roughly what we want?

BinaryFissionGames commented 6 years ago

Looks good to me.

GraysonHoward commented 6 years ago

Should be changed over all that really needs to be done is if you want any other info logged

BinaryFissionGames commented 6 years ago

Re-opening this 'cuz I got some new ideas on stuff we could add to the logging stuffs.

  1. Automatically print file names and line numbers, if possible. (Looking into this right now, I think this is possible, just might take a little tweaking)
  2. If everything is local, we ought to append everything to the same file, right? Maybe we should look into using the multiprocess loggers?
  3. I've decided that I like everything in one file (err and info) better; That's not a big deal, the way you have it set up makes that change easy, and we could easily switch back if we need to.
BinaryFissionGames commented 6 years ago

So, I added some automatic line number & file name logging. It's pretty naive, so I'm going to tweak it here in a sec.

I also changed where log4js loggers are in the index.js file. They are now local to the server class. This also created a problem where i needed to bind() a lot of functions, because that's how you make 'this' in those functions refer to the instance of the class.

Still need to tweak the other 2 servers to integrate these changes.

BinaryFissionGames commented 6 years ago

Alright, I've tweaked the other two files. It was actually more difficult than I expected. This is because the multiprocess appender doesn't have a layout you can apply, so the layout is always applied on the master side (this means that line numbers and files are from the master side, which means it just thinks that events.js is where all messages originate).

So, I ended up creating a new appender that just applies the given layout, then passes it onto the next appender as though it were just the base message. So, it the message goes through that appender, then through the multiprocess appender, which then logs it (using the messagePassThrough layout, so only the sent message has the layout applied).

It works OK, only thing we might want to look into is differentiating different server instances.