-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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: allow custom log msg formatters #9316
feat: allow custom log msg formatters #9316
Conversation
Pull Request Test Coverage Report for Build 29fdf5da-5e63-4e46-a11a-de81f6c77791
💛 - Coveralls |
6e4a266
to
89d9765
Compare
}); | ||
} | ||
|
||
protected formatMessage( | ||
pidMessage: string, |
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 think having this interface wouldn't be that easy to write the output that that Issue need because pidMessage
is too tied with the default formatting and uses that color
function
Can you try to rewrite this formatMessage
like this:
protected formatMessage(
pid: number,
logLevel: string,
context: string,
timestampDiff: number,
output: string,
): string {
return `` ...
}
but then we'll need to change the updateAndGetTimestampDiff
method to extract the coloring stuff from it and make it return a number instead of string.
And then make formatMessage
return an string with the color applied instead of applying it on printMessages
. But yeah, that could be a bit harsh
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.
Yeah I think this may be better, I will refactor it with your suggestions.
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.
So with the latest changes we are more or less like the first version.
I am not 100% sure if the format version should just place the arguments or also be able to generate them.
The minimal implementation is here: stanimirovv@89d9765
The "accept less arguments and do more" implementation is here (also the current one):
https://github.com/stanimirovv/nest/blob/feat/custom-logger-formatters/packages/common/services/console-logger.service.ts#L192
Personally I lean towards the second version because you have more control - over colors for example.
What is the intent - to allow some control or full control ?
If some control - go with stanimirovv@89d9765
If full go with - https://github.com/stanimirovv/nest/blob/feat/custom-logger-formatters/packages/common/services/console-logger.service.ts#L192
does that make sense ?
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.
This approach stanimirovv@89d9765 better fits our needs but we should also allow opt-in coloring the messages. In this case, we should probably declare another dedicated method (let's say colorize
) that takes the same set of arguments as formatMessages
and adds colors. This method should be executed from within the formatMessages
and have protected
modifier to make it feasible to call it (if needed) from within the custom logger implementation.
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.
One final question -
Wouldn't it be a simpler interface if colorize
takes a single argument and returns it colorised ?
function colorize(str: string): string
It will be called for the 2 parts of the message colored currently.
That way the user may add custom colorized elements to the log message.
But, again, depends on the intent.
The alternative is to have the following signature:
function colorize(
pidMessage: string,
formattedLogLevel: string,
contextMessage: string,
output: string,
timestampDiff: string,):
[
pidMessage: string,
formattedLogLevel: string,
contextMessage: string,
output: string,
timestampDiff: string,
]
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.
Yeah, I think as long as we make getColorByLogLevel
protected as well (to make it accessible) it could take two arguments (one being a message to format and the second - optional - color to be used that defaults to this.getColorByLogLever()
)
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 think we should be very close:
- I extracted the colorize method and moved all logic having to do with colors into the formatMessage function as discussed
- I took the liberty to extract one more protected message - stringifyMessage I can change it to private, but it felt like something that should also optionally be configurable -> the way the objects are serialized.
ad01d5b
to
674a09c
Compare
LGTM |
PR Checklist
Implements option to have custom log formatters as discussed here:
#9281
Please check if your PR fulfills the following requirements:
PR Type
What is the current behavior?
Custom log message formaters aren't supported.
Issue Number: #9281
What is the new behavior?
Custom log message formaters are supported
Does this PR introduce a breaking change?
Other information