nestjs-winston-logger icon indicating copy to clipboard operation
nestjs-winston-logger copied to clipboard

Added support for having different config across multiple loggers

Open rat-matheson opened this issue 3 years ago • 3 comments

I saw your comment about mongoose but I didn't quite understand what specifically you would like to see that is similar to mongoose. Please feel free to let me know on the code review and I'll make the appropriate changes.

As it stands, the method I originally posted in the comment seems to be working ok. I also did a couple other changes which perhaps I should not have included in this commit but just give me your feedback and I can change/revert.

Questionable changes

  • README.md - I added a comparison to nest-winston. Personally, I feel your library is better for my needs but for other people wondering the same, it would be good to specifically call out why they might choose to use a less popular library
  • package.json - I moved several dependencies to peerDependencies. I believe this is where they belong for a support package

Lastly, it might be worth considering making morgan and graphql optional dependencies. This is because some people may want to use the library without morgan or perhaps for rest just controllers. In those cases, we don't need the extra dependencies.

rat-matheson avatar Dec 20 '21 21:12 rat-matheson

Thanks for the review comments. All good points

rat-matheson avatar Dec 21 '21 14:12 rat-matheson

I found another issue as well. In addition to having different loggers, I want to have a request id across all logs. There is a great method to do this for a global log but if I want to do it for specific logs, my first approach is:

const requestLogger = new NestjsWinstonLoggerService({
    format:format.simple(),
    transports: [
      new transports.File({ filename: "request.log",
      format: format.printf(log => log.message.trim()) }),
    ],
  });

  let authLog = app.get(getLoggerToken(AUTH_LOG));

  app.use(appendIdToRequest);

  app.use(appendRequestIdToLogger(requestLogger));
  app.use(appendRequestIdToLogger(authLog));
  
  configMorgan.appendMorganToken("reqId", TOKEN_TYPE.Request, "reqId");
  app.use(morganRequestLogger(requestLogger));
  app.use(morganResponseLogger(requestLogger));

The issue here is that I'm now exposing some of the internals through getLoggerToken. I think there should be a better way to do this but I haven't thought much about it yet.

rat-matheson avatar Dec 21 '21 18:12 rat-matheson

I found another issue as well. In addition to having different loggers, I want to have a request id across all logs. There is a great method to do this for a global log but if I want to do it for specific logs, my first approach is:

const requestLogger = new NestjsWinstonLoggerService({
    format:format.simple(),
    transports: [
      new transports.File({ filename: "request.log",
      format: format.printf(log => log.message.trim()) }),
    ],
  });

  let authLog = app.get(getLoggerToken(AUTH_LOG));

  app.use(appendIdToRequest);

  app.use(appendRequestIdToLogger(requestLogger));
  app.use(appendRequestIdToLogger(authLog));
  
  configMorgan.appendMorganToken("reqId", TOKEN_TYPE.Request, "reqId");
  app.use(morganRequestLogger(requestLogger));
  app.use(morganResponseLogger(requestLogger));

The issue here is that I'm now exposing some of the internals through getLoggerToken. I think there should be a better way to do this but I haven't thought much about it yet.

This issue may adopt to use Nestjs middleware rather than global middleware. We can open another issue for further discussion. For you reference - https://docs.nestjs.com/middleware

Marcotsept avatar Dec 22 '21 02:12 Marcotsept