Conversation
🦋 Changeset detectedLatest commit: eafcf3d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
zbrydon
changed the title
| maxObjectDepth, | ||
| omitHeaderNames, | ||
| redactText, |
There was a problem hiding this comment.
Do we intend to ship this as a breaking change for consumers that currently specify these as top-level opts, or will we have backward compatibility and a softer @deprecated notice?
There was a problem hiding this comment.
No strong opinions but I feel like a breaking interface change for the consumers using these formatting options isn't too aggressive 🤷
src/index.ts
Outdated
| type LogFormattingOptions = FormatterOptions & | ||
| SerializerOptions & { | ||
| redact?: pino.LoggerOptions['redact']; | ||
| formatters?: pino.LoggerOptions['formatters']; | ||
| serializers?: pino.LoggerOptions['serializers']; | ||
| }; |
There was a problem hiding this comment.
Thoughts on defining a new type here that only includes the extra options that we're bolting on top? We can then leave the built-in Pino options alone where they currently are.
src/index.ts
Outdated
| omitFunctions: DEFAULT_OMIT_FUNCTIONS, | ||
| maxObjectDepth: DEFAULT_MAX_OBJECT_DEPTH, | ||
| omitHeaderNames: DEFAULT_OMIT_HEADER_NAMES, | ||
| stringLength: DEFAULT_STRING_LENGTH, |
There was a problem hiding this comment.
Then maybe we can define these all here / in a central place rather than them being scattered across different formatter / redact / serializer files.
src/index.ts
Outdated
| Eeeoh.Options<CustomLevels> & | ||
| FormatterOptions & | ||
| SerializerOptions; | ||
| Eeeoh.Options<CustomLevels> & { logFormattingOptions?: LogFormattingOptions }; |
There was a problem hiding this comment.
I don't love logFormattingOptions given there's already https://getpino.io/#/docs/api?id=formatters-object but not confident on an alternative at the moment 😓.
STELA suggested
processingsanitizationsanitizetransform
| */ | ||
| const defaultLogFormattingOptions: LogFormattingOptions = { | ||
| omitFunctions: DEFAULT_OMIT_FUNCTIONS, | ||
| maxObjectDepth: DEFAULT_MAX_OBJECT_DEPTH, |
There was a problem hiding this comment.
Any idea if https://getpino.io/#/docs/api?id=depthlimit-number is better than dtrim?
There was a problem hiding this comment.
pino uses https://github.com/BridgeAR/safe-stable-stringify, it seems to be proud of its performance benchmarks but dtrim doesn't publish any so 🤷
I'd been keen to swap dtrim for built in pino functionality if it's a complete replacement but dtrim provides a few more options eg omitting functions which we're making use of that pino doesn't have AFAICT.
dtrim is also seems quite simple compared to safe-stable-stringify (at a glance) and Dmitry has been receptive and responsive to our feature contributions so I think that's worth keeping in mind 🤔
src/index.ts
Outdated
| omitFunctions: DEFAULT_OMIT_FUNCTIONS, | ||
| maxObjectDepth: DEFAULT_MAX_OBJECT_DEPTH, | ||
| omitHeaderNames: DEFAULT_OMIT_HEADER_NAMES, | ||
| stringLength: DEFAULT_STRING_LENGTH, |
There was a problem hiding this comment.
Thoughts on naming here:
- Rename to
maxStringLengthto align withmaxObjectDepth - Keep as
stringLength(and renamemaxObjectDepthtoobjectDepth?) - Something like
stringLimitto align with https://getpino.io/#/docs/api?id=depthlimit-number?
3 participants
Having a go at consolidating the log formatting options available, which will hopefully be better documented in the process 🤷
Thoughts??