pinojs / pino

🌲 super fast, all natural json logger
http://getpino.io
MIT License
14.3k stars 878 forks source link

Add CLI formatter function to pino module #184

Closed jamesplease closed 7 years ago

jamesplease commented 7 years ago

Related:

https://github.com/trentm/node-bunyan/pull/102 https://github.com/trentm/node-bunyan/issues/84

This could be pretty handy in different situations. If there's any interest, I could try my hand at whipping up a PR. What do y'all think?

jsumners commented 7 years ago

As in the formatter option on .pretty()? https://github.com/pinojs/pino/blob/master/docs/API.md#pretty

mcollina commented 7 years ago

Yes, I think we should add the possibility to pass that as an argument.

jsumners commented 7 years ago

@mcollina I'm actually unsure what the request is asking for. I think it's asking for a way to replace the default output of line delimited JSON with something else. In that case, we already support it via .pretty().

I have also found the boilerplate of setting up pretty output to be cumbersome, but have been undecided if the following would be worth adding:

var pino = require('pino')
var log = pino({pretty: true, extreme: false})

With the backing code being:

if (pretty) {
  this.stream = pretty()
  this.stream.pipe(process.stdout)
}
mcollina commented 7 years ago

@jmeas I understood we wanted an equivalent of bunyan -c 'this.foo == "bar", so that you could filter the log lines. Is this correct?

@jsumners I'm 👎 on { pretty: true } option, as we are wasting a lot of CPU time in parsing with that enabled.

jsumners commented 7 years ago

@mcollina right. But I find myself adding stuff like the following to every project:

var appConfig = (function readTheConfig() {}())
var pino = require('pino')
var log
if (appConfig.log.pretty) {
  var pretty = pino.pretty()
  pretty.pipe(process.stdout)
  log = pino(appConfig.log, pretty)
} else {
  log = pino(appConfig.log)
}

As I said, I've been torn about submitting a PR that would reduce that to var log = pino(appConfig.log). But if you're against it, then I'll just keep doing what I'm doing.

But the way I read this issue, that's pretty much what @jmeas is wanting. He just wants to also supply the custom formatter function. Maybe I'm way off base.

mcollina commented 7 years ago

@jsumners go ahead and send a PR then, that code is ugly, also https://github.com/pinojs/hapi-pino/blob/master/index.js#L28-L32 :D.

jsumners commented 7 years ago

@mcollina okay, I'll get it in soon-ish.

jamesplease commented 7 years ago

Hey there – sorry for the delayed response. I feel silly, because .pretty() is all that I was looking for. I skimmed the README, and didn't realize that the links in the table of contents were a mix of things on the page as well as not on the page, so I didn't even click into API.md file. My mistake.

Anyway, I was intending to write code just like what @jsumners wrote above. The option will definitely make it a lot cleaner.

Thanks for putting that together so quickly, @jsumners :v:

jsumners commented 7 years ago

If you can think of a better way to highlight the sectioning of the documentation, a PR would be very welcome.

jamesplease commented 7 years ago

If you can think of a better way to highlight the sectioning of the documentation, a PR would be very welcome.

The ToC could be sectioned into two groups (say, "README Table of Contents" and "Other Guides"), and that may help. What do you think?

It may just be me, but in general if there's a long README, and a list of links, then I assume that the list of links is just for navigation within the README. If nobody else makes this assumption then it may not be worth changing.

Happy to make a PR tho' if you think that separation is a good idea.

jsumners commented 7 years ago

We had a massive README until #153. The trouble is that, currently, our README serves as the primary introduction to Pino (we haven't solved https://github.com/pinojs/pinojs/issues/1). So it needs to present well on npmjs.com. Keeping that in mind, I'm happy to see any PR that addresses the issue.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.