pinojs / pino

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

Custom interpolation? #1376

Open JaneJeon opened 2 years ago

JaneJeon commented 2 years ago

Hi, searched the docs and the issues page for formatters and interpolation, but I don't see this anywhere.

Is there a way I can specify a custom interpolator (e.g. %t) akin to this so that log.$method('%t', objToBeInterpolated) can be interpolated in that custom format?

mcollina commented 2 years ago

I don't think this is supported but a PR to quick-format-unescaped could add this capability.

JaneJeon commented 2 years ago

Okay, so full disclosure, I wanted the custom interpolator - %t - specifically to format milliseconds into a readable "datetime string"; however, it most certainly will require configuration (after all, there are as many ways to display time as there are milliseconds), and the current structure of quick-format-unescaped seems unfit to do something like this.

In particular, the fact that you're supposed to use the exported format function directly, and not have a generator that returns the format function that will eventually be used, means that there is no space for configuration, which means you can't have custom interpolators in pinojs.

And if one were to change quick-format-unescaped to export a generator rather than the format function directly, it would most certainly be a breaking change/major semver, and I'm just not comfortable with breaking someone else's library, especially when it is being used by a large project like pino.

mcollina commented 2 years ago

In particular, the fact that you're supposed to use the exported format function directly, and not have a generator that returns the format function that will eventually be used, means that there is no space for configuration, which means you can't have custom interpolators in pinojs.

And if one were to change quick-format-unescaped to export a generator rather than the format function directly, it would most certainly be a breaking change/major semver, and I'm just not comfortable with breaking someone else's library, especially when it is being used by a large project like pino.

I'm pretty sure @davidmarkclements don't mind. I prefer that configuration pattern anyway, you can make it semver minor with the following trick:

function build (...opts) {
  // do stuff
}
module.exports = build() // default options
module.exports.build = build
JaneJeon commented 2 years ago

I suppose so. But pino is using it, so it will have to adapt to this usage as well (after all, pino should expose this option in order for the quick-format changes to be useful, because it currently isn’t exposing any configuration options for quick-format). Are you okay with that? If so, I’ll make the PR

mcollina commented 2 years ago

Yeah, no prob.

JaneJeon commented 2 years ago

Well, shit. I'm not able to make "the trick" work (the test complains that $import.build() is not a function, not to mention I'm preeeetty sure this trick doesn't work with ESM, too).

Would it be okay to just export build directly?

Also what kind of machine did you use to benchmark this, because I'm getting nowhere near the same baseline numbers (ie. before I even made any code changes) on my M1 Pro

util*100000: 1.230s
quick*100000: 1.208s
utilWithTailObj*100000: 1.351s
quickWithTailObj*100000: 1.253s
util*100000: 1.289s
quick*100000: 1.202s
utilWithTailObj*100000: 1.303s
quickWithTailObj*100000: 1.209s

M1 Pro MBPr, 32GB RAM, node LTS

With the custom formatter included:

util*100000: 1.228s
quick*100000: 1.233s
utilWithTailObj*100000: 1.296s
quickWithTailObj*100000: 1.210s
util*100000: 1.235s
quick*100000: 1.199s
utilWithTailObj*100000: 1.296s
quickWithTailObj*100000: 1.207s

Doesn't seem like there's a regression, but it's so hard to tell because the baseline is so big in the first place

JaneJeon commented 2 years ago

Welp, published it anyway.