-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(common): Support JSON format in ConsoleLogger #13561
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 579eb5c0-6c1b-4f78-90af-24910b29a78eDetails
💛 - Coveralls |
) { | ||
const output = this.stringifyMessage(message, logLevel); | ||
|
||
if (this.options?.asJSON) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this condition located below the 256 line (stringifyMessage call) if you don't need the output
variable
) { | ||
const output = this.stringifyMessage(message, logLevel); | ||
|
||
if (this.options?.asJSON) { | ||
return `${JSON.stringify({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this check (if condition) and JSON stringification logic outside this function (check in the printMessages method and call the corresponding function)
@@ -267,6 +295,10 @@ export class ConsoleLogger implements LoggerService { | |||
} | |||
|
|||
protected colorize(message: string, logLevel: LogLevel) { | |||
if (this.options?.asJSON) { | |||
return message; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once you apply the change i described here https://github.com/nestjs/nest/pull/13561/files#r1713357714, this if should no longer be necessary
@@ -227,6 +237,10 @@ export class ConsoleLogger implements LoggerService { | |||
} | |||
|
|||
protected formatContext(context: string): string { | |||
if (this.options?.asJSON) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once you apply the change i described here https://github.com/nestjs/nest/pull/13561/files#r1713357714, this if should no longer be necessary
/** | ||
* If enabled, logs will be in form of JSON strings. | ||
*/ | ||
asJSON?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asJSON?: boolean; | |
json?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly to log levels, we should be able to specify (globally) that the logger should use json
format for all logs by default
PR Checklist
Please check if your PR fulfills the following requirements:
ConsoleLoggerOptions
interface has the new optionasJSON
with a JSDoc commentPR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, there is no way to make
ConsoleLogger
log JSON stringsIssue Number: N/A
What is the new behavior?
Now there is a option to log as JSON string
Does this PR introduce a breaking change?
Other information
There is this issue #10342 that is closed, but still there is no support for JSON string format.
We are in a need of JSON string logs - it's easier to parse in Google Cloud and there are less problems with newlines.
Some projects that handle this issue exists:
but they doesn't seems to be updated. And I think a basic form of this functionality should be provided without external library.