Added support for having different config across multiple loggers

[rat-matheson]

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]

These didn't seem useable. I think I could also remove "start:prod" and maybe even the "test" and jest config as well since the package doesn't seem to be using any of that

[rat-matheson]

These aren't referenced anywhere in the code. I'm guessing they were included as they are peer dependencies of @nesths/graphql. However, it is not necessary to include them here. The main app package.json will include all the peer dependencies

[owen1002]

I would prefer updating the dependencies in another pull request.

[rat-matheson]

You are right. I'll submit a second one after this one is complete.

[rat-matheson]

Maybe I should have moved these to devDependencies? Built fine without them

[Marcotsept]

static forRoot(options: LoggerOptions, overrides: ContextOverrides = {} ): DynamicModule {
How do you feel about the second parameter?

[owen1002]

I was wondering if it would be better to do it like the mongoose appraoch.

Reference: https://github.com/nestjs/mongoose/blob/74f7d48fa1ad632270e66de5f4198619b784498a/lib/mongoose-core.module.ts#L33

Ideas:
User will be providing name and specific config to construct a logger, the name will help to identify the responsibility of the logger(e.g. auth_logger, req_logger) and the config is the LoggerOptions of winston.

These loggers module will be imported into user's module and logger service will be injected into user's provider/service like:

@Injectable()
export class DemoService {
  constructor(@InjectLogger("auth_logger") private authLogger: NestjsWinstonLoggerService, @InjectLogger("req_logger") private authLogger: NestjsWinstonLoggerService) {}
}

Any thought or concerns about this approach?

[Marcotsept]

I'm no comment on this approach, it is good at single responsibility of the logger.

If we adopt this approach, we need to revise nestjs-winston-logger.decorator.ts which distinguishes by context name in the current version.

Also, we can allow user optionally pass some metadata into decorator such as context name
@InjectLogger("auth_logger", { // something } ) private authLogger: NestjsWinstonLoggerService
something like that, what do you think?

[rat-matheson]

@Marcotsept - Re: forRoot(options: LoggerOptions, overrides: ContextOverrides = {} )
I like your approach much better than mine. Easier to use.

@owen1002 - Re: Mongoose approach
I'm afraid I don't understand what you are getting at. The injection between the approach I submitted and mongoose seems the same, so I think you are getting at something about how we set up the config in the forRoot function?

@Marcotsept - Re: ```@InjectLogger("auth_logger", { // something })
I like it. But am having an issue. Suppose we want to keep @InjectLogger(MyService.name) as an option. It writes the metadata {..., "service":"MyService", ...} to the log. If we do @InjectLogger("auth_logger", { service:MyService.name }) then we can do something similar by just making sure the second parameter is present so that we know the first is not the service name. But then if I want to do just @InjectLogger("auth_logger"), I'm unable to distinguish that it is not a class name and will end up writing {..., "service":"auth_logger", ...}. Any suggestions on this?

Sorry, can distinguish using the same method I'm already using by just checking the overrides. Will add this feature in the next review

[rat-matheson]

One other question. I don't think "overrides" is the right term for the way I have it implemented. It is a bit unclear as to whether or not it just overrides common settings from the defaultOptions or completely replaces the defaultOptions. Any suggestions on a good alternative or is it best to handle with just better docs.

[rat-matheson]

Thanks for the review comments. All good points

[Marcotsept]

I would like to suggest you write a shorter form.

const loggerOptions = overrides[context] || defaultOptions;

[Marcotsept]

There has a problem when importing the same AUTH_LOG of the logger inside the same module. Maybe the second will override the first according to the import sequence.

// Demo1Service.ts
export class Demo1Service {
  constructor(
    @InjectLogger(AUTH_LOG, { service: Demo1Service.name}) private authLogger: NestjsWinstonLoggerService) { }
}

// Demo2Service.ts
export class Demo2Service {
  constructor(
    @InjectLogger(AUTH_LOG, { service: Demo2Service.name}) private authLogger: NestjsWinstonLoggerService) { }
}

[owen1002]

Yes, since we are getting the logger through the token.

The real situation is quite different from what I thought at my first impression (implement something like nest/mongoose). The schema remains unchanged in mongoose, while the logger may need different config in different provider...

[owen1002]

When I was thinking about this issue, A question popped into my head: Is there any use case that we really need to have two loggers in the same nestjs module?

E.g. The original issue, auth and request - shouldn't we use different module (or possibly another microservice?) to handle the auth process?
@Marcotsept @rat-matheson

[rat-matheson]

I believe there are cases where you need multiple loggers in the same nestjs module. I personally don't want everything to just fall into my application log. I have different needs from different logs in terms of metrics, log processing, and just debugging my app in general. I also want a separate request log in addition to the auth log. With respect to just the specific auth case, it is probably not safe to assume that users will have their auth in a separate module (although ideally, that would be the case).

With regards to importing the same AUTH_LOG, is your concern that logging a message in Demo1Service will produce {... "service": "Demo2Service" ...} in the case where Demo2Service gets injected after Demo1Service? Eg, the metadata would be replaced? I thought the transient scope would handle this, but if not, we can just drop setting the metadata from the @InjectLogger decorator. One way to test it though could be to inject Demo1Service into Demo2Service, and log at various points during the process. I'll give it a try and let you know what I find.

[rat-matheson]

@Marcotsept - you were right in your prediction. The metadata got overwritten in the second injection. I suggest we remove that option from @InjectLogger so that it only has a single parameter again. Just taking a look a nest-winston, they allow the addition of metadata when actually logging rather than supplying it ahead of time (see winston.classes.ts, the commit). A different approach that might be worth considering.

[Marcotsept]

Hi @rat-matheson , @owen1002 - Owen mentions that if the user or you wanna multiple loggers, they can import multiple modules which is our dynamic module but the current version doesn't support multiple modules because the token does not identify which logger of which module.

That means single responsibility of a single logger, it doesn't combine multiple configs.

import { Module } from "@nestjs/common";
import { NestjsWinstonLoggerModule } from "nestjs-winston-logger";
import { format, transports } from "winston";
import { DemoService } from "./demo.service";
import { AUTH_LOG } from "./demo.constants"

const defaultConfig = {
    format: format.combine(
      format.timestamp({ format: "isoDateTime" }),
      format.json(),
      format.colorize({ all: true }),
    ),
    transports: [
      new transports.File({ filename: "error.log", level: "error" }),
      new transports.File({ filename: "combined.log" }),
      new transports.Console(),
    ],
  };

const authConfig = {
    ...defaultConfig,
    transports: [
      new transports.File({ filename: "authentication.log" })
    ],
  };

@Module({
  imports: [
    NestjsWinstonLoggerModule.forRoot('Log', defaultConfig),
    NestjsWinstonLoggerModule.forRoot('Auth', authConfig),
  ],
  providers: [DemoService],
})
export class DemoModule {}

something like that.

[rat-matheson]

Oh, I understand. For some reason I mistakenly was assuming Owen was referring to having a log declared in my authentication module, and not importing separate logger modules. I should be able to try that approach. It does seem more appropriate

[rat-matheson]

@Marcotsept @owen1002
With regards to having separate instances of the NestjsWinstonLoggerModule, I'm wondering if it is possible to do without breaking changes. The issue I'm having is within the forRoot, I need to be able to distinguish between a token and a context. Previously, I was able to do this by looking at the keys in overrides as shown below.

 const loggerOptions = overrides[context] || defaultOptions;

return {
  provide: getLoggerToken(context),
  useFactory: () => {
    const logger = new NestjsWinstonLoggerService(loggerOptions);
    if (!overrides[context]) {
      logger.setContext(metadata);
    }    
    return logger;
}

But now I'm not able to do that because there is no master list of overrides. So when generating a token for @InjectLogger(DemoClass.name), where it doesn't match any of the logging module instances logToken, there is no way to determine which module actually logs it. I considered some kind of option forRoot setting that would require a context to token match for some module instances, however, the instances that don't have the setting would then just log everything.

Some options:

1. Break Inject Logger
   @InjectLogger(logToken:string, context?:string) - in this case, logToken is always required which makes it easy to create a token that incorporates it. It breaks the case @InjectLogger(SomeClass.name)

2. Have some construct that makes it possible to determine if the first parameter is a token or context
   @InjectLogger(logTokenOrContext:string, context?:string) - in this case, it is assumed the first param is context unless there are 2 params
   @InjectLogger(logTokenOrContext:string | { token:string } )

3. Create a new injector @InjectLogToken(token:string)

I have other ideas but they keep getting worse so I'll stop here.

[rat-matheson]

@Marcotsept @owen1002 - do you guys have any ideas with regards to the breaking change issues I encountered?

[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.

[Marcotsept]

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