Implement support for json-only logs #193
Conversation
lundibundi
force-pushed
the
impl-json-logging
branch
from
3659dce
to
f7cc600
Compare
|
It's not a |
| class Console { | ||
| constructor(write) { | ||
| this._write = write; | ||
| this._groupIndent = ''; | ||
| this._groupIndent = 0; |
There was a problem hiding this comment.
_groupIndent is precalculated string, if you need a number we may add _groupIndentCount
There was a problem hiding this comment.
I considerend this to be a very "narrow" use-case to optimize for. I'd expect for it to be used very infrequently and in examples mostly.
IMO I'm not sure if we need to support this at all for 'logger'.
metalog.js
Outdated
| @@ -89,7 +92,7 @@ class Console { | |||
| try { | |||
| console.assert(assertion, ...args); | |||
| } catch (err) { | |||
| this._write('error', `${this._groupIndent}${err.stack}`); | |||
| this._write('error', this._groupIndent, err.stack); | |||
There was a problem hiding this comment.
Can't catch an idea why do you pass this._groupIndent as an argument to _write
There was a problem hiding this comment.
Because the formatter has to decide what to do with this indent. (i.e. json - ignore, pretty - indent everything).
This also allows to properly indent multiline messages if we need it later.
There was a problem hiding this comment.
Dropping indentation will be ok, but as a separate PR 😄
There was a problem hiding this comment.
Do we need to pass it as a parameter? Maybe we can read it from property?
There was a problem hiding this comment.
Actually yes, we can add it in this._write.
There was a problem hiding this comment.
Though we didn't have "indent" for count and table methods and this change will add them there.
| trace(...args) { | ||
| const msg = util.format(...args); | ||
| const err = new Error(msg); | ||
| this._write('debug', `${this._groupIndent}Trace${err.stack}`); | ||
| } | ||
|
|
||
| warn(...args) { |
There was a problem hiding this comment.
Why do you drop trace and warn methods?
There was a problem hiding this comment.
They are just moved to have them near other log (info, debug, error etc) methods
| return `${dateTime} [${type}] ${msg}`; | ||
| } | ||
|
|
||
| formatJson(type, indent, ...args) { |
There was a problem hiding this comment.
Maybe it's better to have a collection of formatters not multiple methods in Logger class
There was a problem hiding this comment.
Yes, but it is hard to "connect" that to current stdout-vs-file separation.
tshemsedinov
force-pushed
the
impl-json-logging
branch
from
177d1b2
to
d0b710a
Compare
|
@lundibundi I am going to land this |
|
Landed in 33695c4 |
|
@lundibundi Can you please remove timestamp printed at the beginning of each line as a separate PR because we have a json field so it duplicates? |
npm t)npm run fmt)Sample output
