CLI revamp #386

jsumners · 2018-04-05T01:04:29Z

Here it is: the "revamp" of the CLI. Or rather, the complete removal of it. Notice that the target branch of this PR is the next-major branch not the master branch.

This PR:

Removes the CLI completely and reduces core to just a logger
Introduces the concept of Pino prettifier modules

Prettifier Modules

A prettifier module is one like pino-pretty. Such modules export a factory function that accepts a configuration object. That function returns a formatter function that can accept an ndJSON log line or a Pino log object. That function also includes an asMetaWrapper(destStream) method attached to it which returns a metadata API compatible stream object. This allows for users to do:

const log = require('pino')({
  prettyPrint: {
    someFooPrettifierOption: 'and-value'
  },
  prettifier: 'foo-prettifier'
})

This means people like @lrlna could update their awesome modules to be used directly in-process. 🎉

Semver Major 🚨🚨🚨 😱

I realize we would prefer not to issue a new major. But we have a few things queued up just waiting for us to be brave enough for a new major -- https://github.com/pinojs/pino/projects/1 . The majority of the recent issues deal with the prettifier and CLI. Those are not the problems Pino strives to solve. So by removing them from core and issuing a new major version we will be reducing Pino down to its purpose: generate logs. And I think the logger features are mature enough that v5.y.z will be with us for a long time: we just have to make sure the prettifier API is solid.

* fix issue #382 pretty print error with null stack * prints stack: "null" when error is a property * errors with a null stack, #382

mcollina · 2018-04-05T09:12:22Z

pino.js

+  var prettyModule = opts.prettifier || 'pino-pretty'
+  try {
+    prettyFactory = require(prettyModule)
+    return prettyFactory(opts).asMetaWrapper(process.stdout)


I think we might as well include asMetaWrapper in here instead. I do not see a reason why a prettifier might not want that.

I don't understand this statement. Can you reword to clarify?

mcollina · 2018-04-05T09:13:47Z

pino.js

+  var prettyFactory
+  var prettyModule = opts.prettifier || 'pino-pretty'
+  try {
+    prettyFactory = require(prettyModule)


I would not require a string, but rather support a function (the factory) or true. If true, we can then require.

That makes sense.

davidmarkclements · 2018-04-05T12:51:02Z

this is a great piece of work @jsumners

I think we want to be careful not to implicitly direct users to opt for in-process prettifying unless they have no option but to do so

I think we also need:

a minimal CLI tool under the pino command name that does very minimal prettifying
OR a gatekeeping/signposting CLI under the pino command name that lists available prettifiers and installs on demand
OR a mixture of both

mcollina · 2018-04-05T12:58:18Z

a minimal CLI tool under the pino command name that does very minimal prettifying

I'm -1 to that.

I think we can just ship a pino executable with pino-pretty

jsumners · 2018-04-05T13:09:28Z

@davidmarkclements I agree with @mcollina. The point here is that including the CLI in the core logger wasn't such a great plan.

The reason for maintaining the prettyPrint option is to reduce the breakage. I just opted to make it a bit more robust and turn it into a full fledged API for generic prettifiers. This is basically a "reporters" feature like with tap or https://github.com/nuxt/consola#reporters .

Also, users have to actually read the documentation to learn how to do this. And we know how well they do that 😆

davidmarkclements · 2018-04-05T13:10:55Z

we just got people used to the idea of piping to the pino command...

if we no longer bundle (and I'm not for bundling pino-pretty btw), then were setting up a learning flow where users will assume the right thing to do is use the programmatic API with pino-pretty or other prettifier.

mcollina · 2018-04-05T13:13:17Z

if we no longer bundle (and I'm not for bundling pino-pretty btw), then were setting up a learning flow where users will assume the right thing to do is use the programmatic API with pino-pretty or other prettifier.

I do not understand.

jsumners · 2018-04-05T13:18:23Z

@davidmarkclements do you think re-wording https://github.com/pinojs/pino/blob/d7d52a1ce23be1096be3268346bed81b5d9d1b51/docs/pretty.md#example would solve the issue? If so, what should it say?

davidmarkclements · 2018-04-05T13:46:35Z

I think we add a section right at the beginning, something to the effect of

We never want to prettify in production. The best way to prettify is to do so in a separate process and pipe the log output to that process. This precludes any need to have separate development/production logger configurations, and familiarizes us with the concept of always processing log output in a separate process.

The recommended way to prettify is to install a prettifier, and pipe output to it.
For instance, with pino-pretty we would achieve this like so:

npm i -g pino-pretty

node app.js | pino

Something to that effect, prioritizing command line based prettifying

davidmarkclements · 2018-04-05T13:47:03Z

and probably put that in the main readme as well, then for programatic usage link to the pretty doc (which also enforces command line piping)

davidmarkclements · 2018-04-05T13:47:31Z

@mcollina based on my follow up, does my prior comment make any more sense?

jsumners · 2018-04-05T13:49:37Z

@davidmarkclements I just pushed up a change that, without even having read your suggestion 🙂, includes a primary example of piping. Give it a review. Also, feel free to push up your ideas to this branch.

davidmarkclements · 2018-04-05T14:12:26Z

docs/pretty.md

+module like [`pino-pretty`][pp]:
+
+```sh
+$ cat app.log | pino-pretty


are we going with pino-pretty, or pino since it's default prettifer?

can we add a line to the effect of "For almost all situations, this is the recommended way to log. The programmatic API is mostly for integration purposes of other CLI based prettifiers"

At the moment, pino-pretty does not have a pino bin definition. I'm rather hesitant to add one as well. I don't want to set a precedent that prettifier modules can define the pino binary.

davidmarkclements · 2018-04-05T14:14:07Z

docs/pretty.md

+
+## Prettifier API
+
+Pino prettifier modules are extra modules that provide a CLI for parsing ndJSON


is this a consistent presentation? "ndJSON" – is that how we case it elsewhere?

I suppose nd should be capitalized. https://github.com/ndjson/ndjson-spec

davidmarkclements · 2018-04-05T14:15:01Z

docs/pretty.md

+
+## Example
+
+To use pretty printing in your project:


a caveat here would be good (e.g. BUT PIPE INSTEAD!)

davidmarkclements

awesome, can we stick something in the readme, a very quick example of prettifying (purely in the CLI)

mcollina · 2018-04-05T17:09:48Z

package.json

@@ -64,6 +61,7 @@
    "fresh-require": "^1.0.3",
    "log": "^1.4.0",
    "loglevel": "^1.6.1",
+    "pino-pretty": "^1.0.0",


I think you can remove chalk from dependencies.

Oops. Fixed.

Ditto for split2. But it still has to be a dev dependency, I think.

Thanks. It's moved.

mcollina

LGTM

mcollina

We should still provide a pino bin and exit the process with an error, saying to install "pino-pretty".

jsumners · 2018-04-05T17:34:37Z

pino bin added. Result:

% node example.js|pino
`pino` cli is deprecated. Use `pino-pretty` cli instead.

marcbachmann · 2018-04-06T18:13:46Z

bin.js

@@ -0,0 +1,3 @@
+#!/usr/bin/env node
+console.error('`pino` cli is deprecated. Use `pino-pretty` cli instead.')


Doesn't deprecated mean that something is still working but it's discouraged to use?
I'd change the message in here to 'pino' cli got removed. Use 'pino-pretty' cli instead.

A link to https://www.npmjs.com/package/pino-pretty could also be useful.

jsumners · 2018-04-06T21:21:31Z

@davidmarkclements are you satisfied with the state of this PR?

lrlna · 2018-04-10T08:22:17Z

oooh cool, i've had people asking for a programmatic interface on pino-colada, this will be useful! I am going to try this branch out with my module too ^__^

mcollina · 2018-04-10T10:30:19Z

pino.js

+  }
+  try {
+    var prettyFactory = require('pino-pretty')
+    return prettyFactory(opts).asMetaWrapper(process.stdout)


Can you move asMetaWrapper in this repository? I don't think it would be needed at all in pino-pretty itself.

I intend to when I get back to this later this week (maybe sometime today).

jsumners · 2018-04-11T12:44:53Z

The latest commit moves asMetaWrapper into Pino as an internal API and updates the doc/pretty.md accordingly. How does this look @mcollina and @davidmarkclements ?

mcollina

LGTM

jsumners · 2018-04-13T12:11:22Z

I think at this point @davidmarkclements issue has been addressed. If not, we can fix it before final v5.0.0 release. I'm merging this.

github-actions · 2022-02-07T00:04:15Z

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

grbr and others added 5 commits April 3, 2018 13:42

Issue #382, pretty print error with null stack (#383)

fd44c30

* fix issue #382 pretty print error with null stack * prints stack: "null" when error is a property * errors with a null stack, #382

Bumped v4.15.4.

fc4c83b

added lastTime meta parameter (#385)

62070d4

Bumped v4.16.0.

0703597

Remove CLI and add robust prettifier support

d7d52a1

mcollina reviewed Apr 5, 2018

View reviewed changes

Use required prettifier directly and tweak documentation

e99e957

davidmarkclements reviewed Apr 5, 2018

View reviewed changes

davidmarkclements requested changes Apr 5, 2018

View reviewed changes

Address documentation concerns

8fab9ec

mcollina reviewed Apr 5, 2018

View reviewed changes

Remove chalk

4395f5e

mcollina approved these changes Apr 5, 2018

View reviewed changes

mcollina reviewed Apr 5, 2018

View reviewed changes

Add bin with deprecation notice

6f1a5e5

Add URL to deprecation notice

f1ae8b6

jsumners mentioned this pull request Apr 6, 2018

using pino-colada at "pretty" in pino instance lrlna/pino-colada#25

Closed

This was referenced Apr 6, 2018

CLI documentation is a mess #378

Closed

More human-friendly pino formatting output #361

Closed

jsumners added this to the v5.0.0 milestone Apr 6, 2018

marcbachmann reviewed Apr 6, 2018

View reviewed changes

deprecated => removed

71e86d1

unjello mentioned this pull request Apr 6, 2018

Update for new revamped Pino API unjello/pino-pretty-min#1

Closed

4 tasks

unjello mentioned this pull request Apr 8, 2018

Metadata API documentation #388

Closed

Move split2 to dev dependencies

69c5443

mcollina reviewed Apr 10, 2018

View reviewed changes

Move asMetaWrapper into Pino and update pretty docs

bf8c33b

jsumners mentioned this pull request Apr 11, 2018

translateTime accept an optional format string pinojs/pino-pretty#8

Merged

3 tasks

mcollina approved these changes Apr 11, 2018

View reviewed changes

jsumners merged commit 145e724 into next-major Apr 13, 2018

jsumners deleted the cli-revamp branch April 13, 2018 12:11

jsumners mentioned this pull request Apr 13, 2018

new prettifier boilerplate #389

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2022


		## Prettifier API

		Pino prettifier modules are extra modules that provide a CLI for parsing ndJSON

		@@ -0,0 +1,3 @@
		#!/usr/bin/env node
		console.error('`pino` cli is deprecated. Use `pino-pretty` cli instead.')

CLI revamp #386

CLI revamp #386

Conversation

jsumners commented Apr 5, 2018 • edited Loading

Prettifier Modules

Semver Major 🚨🚨🚨 😱

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmarkclements commented Apr 5, 2018 • edited Loading

mcollina commented Apr 5, 2018

jsumners commented Apr 5, 2018

davidmarkclements commented Apr 5, 2018

mcollina commented Apr 5, 2018

jsumners commented Apr 5, 2018

davidmarkclements commented Apr 5, 2018

davidmarkclements commented Apr 5, 2018

davidmarkclements commented Apr 5, 2018

jsumners commented Apr 5, 2018

Choose a reason for hiding this comment

davidmarkclements Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmarkclements left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

jsumners commented Apr 5, 2018

Choose a reason for hiding this comment

jsumners commented Apr 6, 2018

lrlna commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners commented Apr 11, 2018

mcollina left a comment

Choose a reason for hiding this comment

jsumners commented Apr 13, 2018

github-actions bot commented Feb 7, 2022

jsumners commented Apr 5, 2018 •

edited

Loading

davidmarkclements commented Apr 5, 2018 •

edited

Loading

davidmarkclements Apr 5, 2018 •

edited

Loading