fix: Context is lost when logging errors (fixes #473)

[abrindam]

See discussion in #473.

The downside of this PR is that it will break anyone who was previously using WinstonLogger.error(message, trace) because it will now be interpreted as WinstonLogger.error(message, stack).

But it will fix the more common use case of Logger.error(message) not losing context.

[gremo]

Hi, still interested in this PR? If yes, I've added an upgrade section in README. Feel free to add a note for the new version 2.x, and document the "problem" related to using Logger.error (context lost) and why we are changing the way of how it works (following the Nest "behaviour").

Thanks

[sorgloomer]

Hi, what are the next steps regarding this issue? This is a fix that I am looking forward to.

[gremo]

@sorgloomer I asked @abrindam to add a note in the README to upgrade to version 2.x (because this change is a BC break) and to "document" the issue using error function, that is why the context is lost.

[chihabeeddine]

Hello @gremo, I Found this explanation when trying to understand the issue https://github.com/nestjs/nest/issues/10069#issue-1328595256 if this is what is missing im willing to help update the readme and resolve this issue.

// ...
  private logger: Logger = new Logger('topContext');
  private loggerWithoutTopContext: Logger = new Logger();
  private consoleLogger = new ConsoleLogger('topContext');
  private consoleLoggerWithoutTopContext = new ConsoleLogger();
// ...
    this.logger.log('message'); // works as intended
    this.logger.log('message', 'context'); // context doesn't override logger context but is printed as an additional message instead
    this.logger.error('message', 'stack'); // works as intended
    this.logger.error('message', 'stack', 'context'); // context doesn't override logger context and stack is not printed as a stack trace but rather as an additional message

    this.loggerWithoutTopContext.log('message'); // works as intended
    this.loggerWithoutTopContext.log('message', 'context'); // works as intended
    this.loggerWithoutTopContext.error('message', 'stack'); // stack is misinterpreted as context
    this.loggerWithoutTopContext.error('message', 'stack', 'context'); // works as intended

    this.consoleLogger.log('message'); // works as intended
    this.consoleLogger.log('message', 'context'); // works as intended
    this.consoleLogger.error('message', 'stack'); // stack is misinterpreted as context
    this.consoleLogger.error('message', 'stack', 'context'); // works as intended

    this.consoleLoggerWithoutTopContext.log('message'); // works as intended
    this.consoleLoggerWithoutTopContext.log('message', 'context'); // works as intended
    this.consoleLoggerWithoutTopContext.error('message', 'stack'); // stack is misinterpreted as context
    this.consoleLoggerWithoutTopContext.error('message', 'stack', 'context'); // works as intended
    ```

[stephenkelzer]

any word on when this can be merged...? I'm currently getting burned by this :(

[invakid404]

I am currently running into this as well and it's quite annoying, would definitely appreciate if this gets merged!

[github-actions[bot]]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions[bot]]

This PR was closed because it has been stalled for 10 days with no activity.