Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 14, 2022
Merged

feat: allow custom log msg formatters #9316

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89d9765
feat: allow custom log formatters
Mar 10, 2022
674a09c
refactor: extract colorize
Mar 11, 2022
50bb472
feat: extract protected method for pid
Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 41 additions & 14 deletions packages/common/services/console-logger.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,27 +183,54 @@ export class ConsoleLogger implements LoggerService {
logLevel: LogLevel = 'log',
writeStreamType?: 'stdout' | 'stderr',
) {
const color = this.getColorByLogLevel(logLevel);
messages.forEach(message => {
const output = isPlainObject(message)
? `${color('Object:')}\n${JSON.stringify(
message,
(key, value) =>
typeof value === 'bigint' ? value.toString() : value,
2,
)}\n`
: color(message as string);

const pidMessage = color(`[Nest] ${process.pid} - `);
const pidMessage = `[Nest] ${process.pid} - `;
stanimirovv marked this conversation as resolved.
Show resolved Hide resolved
const contextMessage = context ? yellow(`[${context}] `) : '';
const timestampDiff = this.updateAndGetTimestampDiff();
const formattedLogLevel = color(logLevel.toUpperCase().padStart(7, ' '));
const computedMessage = `${pidMessage}${this.getTimestamp()} ${formattedLogLevel} ${contextMessage}${output}${timestampDiff}\n`;
const formattedLogLevel = logLevel.toUpperCase().padStart(7, ' ');
const formatedMessage = this.formatMessage(
logLevel,
message,
pidMessage,
formattedLogLevel,
contextMessage,
timestampDiff,
);

process[writeStreamType ?? 'stdout'].write(computedMessage);
process[writeStreamType ?? 'stdout'].write(formatedMessage);
});
}

protected formatMessage(
logLevel: LogLevel,
message: unknown,
pidMessage: string,
Copy link
Member

@micalevisk micalevisk Mar 10, 2022 •
edited
Loading

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

Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@micalevisk @kamilmysliwiec

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 ?

Copy link
Member

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.

Copy link
Author

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,
    ]

Copy link
Member

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())

Copy link
Author

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:

  1. I extracted the colorize method and moved all logic having to do with colors into the formatMessage function as discussed
  2. 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.

formattedLogLevel: string,
contextMessage: string,
timestampDiff: string,
) {
const output = this.stringifyMessage(message, logLevel);
pidMessage = this.colorize(pidMessage, logLevel);
formattedLogLevel = this.colorize(formattedLogLevel, logLevel);
return `${pidMessage}${this.getTimestamp()} ${formattedLogLevel} ${contextMessage}${output}${timestampDiff}\n`;
stanimirovv marked this conversation as resolved.
Show resolved Hide resolved
}

protected stringifyMessage(message: unknown, logLevel: LogLevel) {
return isPlainObject(message)
? `${this.colorize('Object:', logLevel)}\n${JSON.stringify(
message,
(key, value) =>
typeof value === 'bigint' ? value.toString() : value,
2,
)}\n`
: this.colorize(message as string, logLevel);
}

protected colorize(message: string, logLevel: LogLevel) {
const color = this.getColorByLogLevel(logLevel);
return color(message);
}

protected printStackTrace(stack: string) {
if (!stack) {
return;
Expand Down
59 changes: 58 additions & 1 deletion packages/common/test/services/logger.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { expect } from 'chai';
import 'reflect-metadata';
import * as sinon from 'sinon';
import { ConsoleLogger, Logger, LoggerService } from '../../services';
import { ConsoleLogger, Logger, LoggerService, LogLevel } from '../../services';

describe('Logger', () => {
describe('[static methods]', () => {
Expand Down Expand Up @@ -520,4 +520,61 @@ describe('Logger', () => {
});
});
});
describe('ConsoleLogger', () => {
let processStdoutWriteSpy: sinon.SinonSpy;

beforeEach(() => {
processStdoutWriteSpy = sinon.spy(process.stdout, 'write');
});
afterEach(() => {
processStdoutWriteSpy.restore();
});

it('should support custom formatter', () => {
class CustomConsoleLogger extends ConsoleLogger {
protected formatMessage(
logLevel: LogLevel,
message: unknown,
pidMessage: string,
formattedLogLevel: string,
contextMessage: string,
timestampDiff: string,
) {
return `Prefix: ${message}`;
}
}

const consoleLogger = new CustomConsoleLogger();
consoleLogger.debug('test');

expect(processStdoutWriteSpy.firstCall.firstArg).to.equal(`Prefix: test`);
});

it('should support custom formatter and colorizer', () => {
class CustomConsoleLogger extends ConsoleLogger {
protected formatMessage(
logLevel: LogLevel,
message: unknown,
pidMessage: string,
formattedLogLevel: string,
contextMessage: string,
timestampDiff: string,
) {
const strMessage = this.stringifyMessage(message, logLevel);
return `Prefix: ${strMessage}`;
}

protected colorize(message: string, logLevel: LogLevel): string {
return `~~~${message}~~~`;
}
}

const consoleLogger = new CustomConsoleLogger();
consoleLogger.debug('test');

expect(processStdoutWriteSpy.firstCall.firstArg).to.equal(
`Prefix: ~~~test~~~`,
);
});
});
});