Making logging optional

[doctorrokter]

• get rid of ConsoleLogger inheritance:
  • use SentryService implements LoggerService instead of SentryService extends ConsoleLogger;
• add an ability to provide custom logger;
• add an ability to disable logging entirely;
• default logger is ConsoleLogger for backward compatibility;

[holm]

This looks good to me. I added a few comments just based on my perspective.

[holm]

I'm a little confused on the injection of the options. It both seems to be injected here via the provider config, as well as in the constructor of the class via the inject-decorator. I would think just the inject-decorator would be infer, and this can be changed to just use useClass instead of useFactory and remove the inject from here.

[holm]

I would move this to the constructor of SentryService, so the defaulting is done there. That makes it easier in general to use this service I think, without having to rely on this factory.

[doctorrokter]

Actually, the users of that lib should not have an access to that low level implementation. All what other devs have to do - use @InjectSentry() decorator in their projects without caring about what happenes under the hood.

But yeah, maybe for internal usage it makes sense to re-think the instantiation of SentryService.

[doctorrokter]

Oh, I see, you wanted to move default logger instantiation to SentryService's constructor! Yep, makes sense!

[doctorrokter]

Fixed!

[holm]

Is there any change verbose is not defined on the logger? Isn't that part of the interface?

(same for debug above)

[doctorrokter]

As you can see the NestJS's LoggerService interface declares debug and verbose methods as optional. ConsoleLogger implements all methods, but some custom loggers may omit these optional methods. This is just to be safe. Also, TS compiler suggested to check the availability of these methods.

export interface LoggerService {
    /**
     * Write a 'log' level log.
     */
    log(message: any, ...optionalParams: any[]): any;
    /**
     * Write an 'error' level log.
     */
    error(message: any, ...optionalParams: any[]): any;
    /**
     * Write a 'warn' level log.
     */
    warn(message: any, ...optionalParams: any[]): any;
    /**
     * Write a 'debug' level log.
     */
    debug?(message: any, ...optionalParams: any[]): any;
    /**
     * Write a 'verbose' level log.
     */
    verbose?(message: any, ...optionalParams: any[]): any;
    /**
     * Set log levels.
     * @param levels log levels
     */
    setLogLevels?(levels: LogLevel[]): any;
}```

[holm]

Ah, I missed that, makes sense!

[holm]

Nice. This looks good from my perspective.

Thanks again for doing it.

[ntegral]

This currently has conflicts with the master branch...

[doctorrokter]

@ntegral merged!

[nkovacic]

@holm @doctorrokter any chance of merging this? Would enable custom logger usage in combination with nestjs-sentry.

[ntegral]

Please update the changes to resolve conflicts. Thanks

[CalMlynarczyk]

@doctorrokter I wanted to ping this one more time to see if it's feasible to get the merge conflicts resolved and merge this PR.

I understand the OP was pretty responsive to feedback and yet the PR still didn't get merged, which can be frustrating. I'm hoping the repo maintainers can jump on this if the merge conflicts get resolved again. If the merge conflicts don't get addressed soon, I might re-fork these changes and resubmit a matching PR, because this change would really help my team deal with messy logs.

[rubiin]

LGTM