Add Logging to Api, Mail, Webhook, and Websocket #2755
Conversation
# Conflicts: # apps/api/package.json # apps/inbound-mail/package.json # pnpm-lock.yaml
apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # apps/api/src/app/integrations/integrations.controller.ts
…nto winston-logger-implementation
| @@ -88,7 +83,6 @@ export function createNestLoggingModuleOptions(settings: ILoggerSettings) { | |||
| return { | |||
| pinoHttp: { | |||
There was a problem hiding this comment.
@Cliftonz probably lost in my previous comments, but why are we implementing masking from scratch and not using the built in redact for pino? https://github.com/pinojs/pino/blob/master/docs/redaction.md
There was a problem hiding this comment.
I attempted to implement this but the wildcard pathing was not working. @scopsy If you can take a look at this I think that would be best.
In this pr, I have kept my original implementation and made sure that the request logging processing is only done if log level is set to debug and uses promises to ensure some level of multi-processing.
# Conflicts: # apps/api/src/app/events/services/workflow-queue/workflow.queue.service.ts # apps/api/src/app/events/usecases/run-job/run-job.usecase.ts # package.json # pnpm-lock.yaml
.idea/runConfigurations/API_E2E.xml
Outdated
There was a problem hiding this comment.
Was this removed by mistake? @Cliftonz
| await this.workflowQueueService.addToQueue(job._id, job, delay); | ||
|
|
||
| if (delay) { | ||
| this.createExecutionDetails.execute( | ||
| Logger.verbose('Delay is active, Creating execution details'); | ||
| await this.createExecutionDetails.execute( |
There was a problem hiding this comment.
@Cliftonz those are not awaited by design here to avoid blocking the response to the user
|
|
||
| @Controller({ | ||
| path: 'events', | ||
| scope: Scope.REQUEST, | ||
| }) | ||
| @ApiTags('Events') | ||
| @LogDecorator() |
There was a problem hiding this comment.
@Cliftonz not sure if you have answered this previously but couldn't find it, if nest-pino already have an auto logger for HTTP requests, why do we need the LogDecorator?
| }) | ||
| ); | ||
| return await new Promise(async (resolve, reject) => { | ||
| storage.run(new Store(PinoLogger.root), () => { |
There was a problem hiding this comment.
@Cliftonz this what was missing and caused the unit tests to fail ^
| createNestLoggingModuleOptions({ | ||
| serviceName: 'novu/api', | ||
| version: '0.12.0', |
There was a problem hiding this comment.
@Cliftonz why are we initializing it here twice? Once in the app module and the shared module? Should be only once, and if it's here let's make sure not to hard code those values
There was a problem hiding this comment.
Seems like the log decorator is breaking a lot of existing modules, as it converts all non promise methods to be promise methods. For example the toArray method provided by in widgets.controller.ts file.
Mine suggestion is, Don't do a global decorator to decorate ALL methods in a class, but have a more targeted decorator that will decorate individual methods to unwanted side effects.
What change does this PR introduce?
This PR introduces standardized logging for the application. The creation of the loggers can be found in packages -> application-generic where a logger can be instantiated with either createLogger for non-nest apps and create NestLogger for nest based apps.
Why was this change needed?
This change will allow us to start creating quality logs for better remediation of issues on premise and in novu cloud.
Notes
This PR also introduces the env variable LOGGING_LEVEL to all backends and we will need to ensure that new relic licenses, app name, and logging level to all deployments.
This deployment is also blocked by the eu pr as it contains updates to the workflow.
We will also want to look at being able to dynamically update the logging level while the system is running so we can collect debug or verbose logs while the system is running without redeploying it.