Adds logger on cloud code request object

[flovilmart]

@blacha that's what you wanted I guess.

[facebook-github-bot]

@flovilmart updated the pull request.

[codecov-io]

Current coverage is 92.91%

Merging #1550 into master will not affect coverage as of c8a0e31

@@            master   #1550   diff @@
======================================
  Files           87      87       
  Stmts         5494    5515    +21
  Branches      1021    1025     +4
  Methods          0       0       
======================================
+ Hit           5105    5124    +19
  Partial          9       9       
- Missed         380     382     +2

Review entire Coverage Diff as of c8a0e31

Powered by Codecov. Updated on successful CI builds.

[drew-gross]

This seems redundant with logger. Why have both?

[flovilmart]

to have a quick access to it, but I can remove.

[facebook-github-bot]

@flovilmart updated the pull request.

[drew-gross]

Cool. LGTM, but lets get some opinions from @gfosco and @nlutsenko since this is a new public API.

[nlutsenko]

Lgtm. Logger as a property on a request sounds good to me. I wonder if a global would be something safer to maintain, but I guess it doesn't necessarily matter much.

[flovilmart]

@nlutsenko I prefer the scope based logger so we can later maybe add an app id to log prefixes

[drew-gross]

It could be app scoped. That would be in between request scoped and global scoped. Too bad cloud code is already global scoped :(

Or with request scoped we could add the function/trigger name. That seems like a quite useful feature actually.

[blacha]

I have never used winston before so forgive my ignorance. Is there no way to do child loggers in winston?

If you used something like bunyan inside the handleCloudFunction you could do something like

{ 
    ...
    log: currentLogger.child({ 
        function: req.params.functionName,
        user: req.auth && req.auth.user && req.auth.user.id
   })

Then every call inside of the cloud function would log out those parameters as well

req.log.info('message')
// {fuction: 'foo', user: 'bar', level: 'info', msg: 'message'}

more info : https://github.com/trentm/node-bunyan#logchild

[flovilmart]

the logger is already a child logger, that will log specifically in a cloud-code file. Like discussed above, we could add more default parameters like function name, trigger type etc... you can also do it like in the tests.

[flovilmart]

@drew-gross it's request scoped and generated for each request life-cycle, we can add more parameters like functionName, trigger etc.. to it if we want

[drew-gross]

@blacha note that we are trying to avoid being tied to winston, we have LoggerAdapter so you can provide your own logger (for e.g. logging to some 3rd party service)

[flovilmart]

@drew-gross on that note, it never worked that well as at many places, the logger adapter is not available, that's why I added the ./logger. which is more 'static' but we could interface easily from there for writing logs. the loggerAdapter is more for reading.

[blacha]

@flovilmart It isn't really a "child" logger in the bunyan sense, you have actually created a entire new logger pointing at a different file.

How does the DailyRotateFile handle having 10, 100, 1000? instances pointing at the same file?

The bunyan child logger only add parameters to log messages, and uses the parent's logging streams as the transports.

[flovilmart]

I believe you're going off topic here. There is only one file log per server, it's up to you to aggregate with your own means, with log entries or another strategy. I don't believe we'll implement a logging aggregator as parse dashboard don't support multiple instances either.

[blacha]

@flovilmart you have made 2 DailyRotateFile onto each of parse-server.info and parse-server.err by using generateTransports() inside of addGroup

I don't think its off topic, as there is already discussion about adding more information into a per request basis, cloud function name, request username. If that is going to be handled by some sort of addGroup function then in theory you could get 10, 100, 1000 DailyRotateFile loggers made.

[flovilmart]

I'm closing that PR and let you implement it then