From 1bf2433670c4da0a35359f78b7cf1121de57ae52 Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Mon, 20 Jan 2025 14:44:24 +0530 Subject: [PATCH 01/11] fix(worker): remove redundant error logging in active jobs metric service --- .../src/app/workflow/services/active-jobs-metric.service.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/worker/src/app/workflow/services/active-jobs-metric.service.ts b/apps/worker/src/app/workflow/services/active-jobs-metric.service.ts index 05935f14e23..5178b4371b1 100644 --- a/apps/worker/src/app/workflow/services/active-jobs-metric.service.ts +++ b/apps/worker/src/app/workflow/services/active-jobs-metric.service.ts @@ -115,8 +115,6 @@ export class ActiveJobsMetricService { // eslint-disable-next-line no-promise-executor-return return resolve(); } catch (error) { - Logger.error({ error }, 'Error occurred while processing metrics', LOG_CONTEXT); - // eslint-disable-next-line no-promise-executor-return return reject(error); } From 7b2fd6a545db017c2e8a8f066cb1bb5b8290988c Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Mon, 20 Jan 2025 14:51:26 +0530 Subject: [PATCH 02/11] fix(worker): change error logging to info for missing digest jobs --- .../app/workflow/usecases/send-message/digest/digest.usecase.ts | 2 +- .../usecases/send-message/digest/get-digest-events.usecase.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/worker/src/app/workflow/usecases/send-message/digest/digest.usecase.ts b/apps/worker/src/app/workflow/usecases/send-message/digest/digest.usecase.ts index 44f3010dab2..71e632d3615 100644 --- a/apps/worker/src/app/workflow/usecases/send-message/digest/digest.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/send-message/digest/digest.usecase.ts @@ -122,7 +122,7 @@ export class Digest extends SendMessageType { if (!currentJob) { const message = `Digest job ${command.jobId} is not found`; - Logger.error(message, LOG_CONTEXT); + Logger.log(message, LOG_CONTEXT); throw new PlatformException(message); } diff --git a/apps/worker/src/app/workflow/usecases/send-message/digest/get-digest-events.usecase.ts b/apps/worker/src/app/workflow/usecases/send-message/digest/get-digest-events.usecase.ts index b876bfc738e..3fe07d600b2 100644 --- a/apps/worker/src/app/workflow/usecases/send-message/digest/get-digest-events.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/send-message/digest/get-digest-events.usecase.ts @@ -56,7 +56,7 @@ export abstract class GetDigestEvents { ); const message = `Trigger job for jobId ${currentJob._id} is not found`; - Logger.error(message, LOG_CONTEXT); + Logger.log(message, LOG_CONTEXT); throw new PlatformException(message); } From 5a2af724974cb26b09b9c4f2dd844eb04087c84b Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Mon, 20 Jan 2025 15:13:25 +0530 Subject: [PATCH 03/11] fix(worker): change error logging to standard log for job execution errors --- .../worker/src/app/workflow/usecases/run-job/run-job.usecase.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts b/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts index 6863acb511d..75cc7c4cb7d 100644 --- a/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts @@ -107,7 +107,7 @@ export class RunJob { await this.jobRepository.updateStatus(job._environmentId, job._id, JobStatusEnum.COMPLETED); } } catch (error: any) { - Logger.error({ error }, `Running job ${job._id} has thrown an error`, LOG_CONTEXT); + Logger.log({ error }, `Running job ${job._id} has thrown an error`, LOG_CONTEXT); if (job.step.shouldStopOnFail || this.shouldBackoff(error)) { shouldQueueNextJob = false; } From 3895402ee7bab549fe16b53fa658569808af166e Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Mon, 20 Jan 2025 15:13:29 +0530 Subject: [PATCH 04/11] fix(worker): remove redundant error logging in job execution --- apps/worker/src/app/workflow/services/standard.worker.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/apps/worker/src/app/workflow/services/standard.worker.ts b/apps/worker/src/app/workflow/services/standard.worker.ts index d2ed0dc5103..4d2e123b929 100644 --- a/apps/worker/src/app/workflow/services/standard.worker.ts +++ b/apps/worker/src/app/workflow/services/standard.worker.ts @@ -111,12 +111,6 @@ export class StandardWorker extends StandardWorkerService { .execute(RunJobCommand.create(minimalJobData)) .then(resolve) .catch((error) => { - Logger.error( - error, - `Failed to run the job ${minimalJobData.jobId} during worker processing`, - LOG_CONTEXT - ); - return reject(error); }) .finally(() => { From b04ec751b2c7a49244c7319f779549c0f2fb4621 Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Wed, 22 Jan 2025 13:24:19 +0530 Subject: [PATCH 05/11] fix(worker): change error logging to standard log for execution details --- .../workflow/usecases/send-message/send-message-push.usecase.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts index 05265551636..986084c2522 100644 --- a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts @@ -279,7 +279,7 @@ export class SendMessagePush extends SendMessageBase { }) ); } catch (error) { - Logger.error(error, 'Error creating execution details error'); + Logger.log(error, 'Error creating execution details error'); } } From 9293541666cdf05e1d9cd41d4b4eaef2b3c1caba Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Fri, 24 Jan 2025 12:26:10 +0530 Subject: [PATCH 06/11] fix(worker): standardize error logging and remove redundant logs in job execution --- .../app/workflow/services/standard.worker.ts | 6 ++++ .../usecases/run-job/run-job.usecase.ts | 1 - .../send-message/send-message-push.usecase.ts | 31 ++++++++----------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/apps/worker/src/app/workflow/services/standard.worker.ts b/apps/worker/src/app/workflow/services/standard.worker.ts index 4d2e123b929..d2ed0dc5103 100644 --- a/apps/worker/src/app/workflow/services/standard.worker.ts +++ b/apps/worker/src/app/workflow/services/standard.worker.ts @@ -111,6 +111,12 @@ export class StandardWorker extends StandardWorkerService { .execute(RunJobCommand.create(minimalJobData)) .then(resolve) .catch((error) => { + Logger.error( + error, + `Failed to run the job ${minimalJobData.jobId} during worker processing`, + LOG_CONTEXT + ); + return reject(error); }) .finally(() => { diff --git a/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts b/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts index 75cc7c4cb7d..d18aa7df27c 100644 --- a/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/run-job/run-job.usecase.ts @@ -107,7 +107,6 @@ export class RunJob { await this.jobRepository.updateStatus(job._environmentId, job._id, JobStatusEnum.COMPLETED); } } catch (error: any) { - Logger.log({ error }, `Running job ${job._id} has thrown an error`, LOG_CONTEXT); if (job.step.shouldStopOnFail || this.shouldBackoff(error)) { shouldQueueNextJob = false; } diff --git a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts index 986084c2522..b5e51dcc704 100644 --- a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts @@ -263,24 +263,19 @@ export class SendMessagePush extends SendMessageBase { raw?: string; } ): Promise { - // We avoid to throw the errors to be able to execute all actions in the loop - try { - await this.executionLogRoute.execute( - ExecutionLogRouteCommand.create({ - ...ExecutionLogRouteCommand.getDetailsFromJob(job), - detail, - source: ExecutionDetailsSourceEnum.INTERNAL, - status: ExecutionDetailsStatusEnum.FAILED, - isTest: false, - isRetry: false, - ...(contextData?.providerId && { providerId: contextData.providerId }), - ...(contextData?.messageId && { messageId: contextData.messageId }), - ...(contextData?.raw && { raw: contextData.raw }), - }) - ); - } catch (error) { - Logger.log(error, 'Error creating execution details error'); - } + await this.executionLogRoute.execute( + ExecutionLogRouteCommand.create({ + ...ExecutionLogRouteCommand.getDetailsFromJob(job), + detail, + source: ExecutionDetailsSourceEnum.INTERNAL, + status: ExecutionDetailsStatusEnum.FAILED, + isTest: false, + isRetry: false, + ...(contextData?.providerId && { providerId: contextData.providerId }), + ...(contextData?.messageId && { messageId: contextData.messageId }), + ...(contextData?.raw && { raw: contextData.raw }), + }) + ); } private async sendMessage( From f0f302ec39bc89e74d55aaa2457119c4f09cc248 Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Fri, 24 Jan 2025 17:38:57 +0530 Subject: [PATCH 07/11] fix(e2e): update expected execution details length in push notification tests --- apps/api/src/app/events/e2e/send-message-push.e2e.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/api/src/app/events/e2e/send-message-push.e2e.ts b/apps/api/src/app/events/e2e/send-message-push.e2e.ts index 83fcbafeda5..9f27473c144 100644 --- a/apps/api/src/app/events/e2e/send-message-push.e2e.ts +++ b/apps/api/src/app/events/e2e/send-message-push.e2e.ts @@ -76,7 +76,7 @@ describe('Trigger event - Send Push Notification - /v1/events/trigger (POST) #no _environmentId: session.environment._id, }); - expect(executionDetails.length).to.equal(8); + expect(executionDetails.length).to.equal(7); const noActiveChannel = executionDetails.find((ex) => ex.detail === DetailEnum.SUBSCRIBER_NO_ACTIVE_CHANNEL); expect(noActiveChannel).to.be.ok; expect(noActiveChannel?.providerId).to.equal('fcm'); @@ -104,7 +104,7 @@ describe('Trigger event - Send Push Notification - /v1/events/trigger (POST) #no _environmentId: session.environment._id, }); - expect(executionDetails.length).to.equal(9); + expect(executionDetails.length).to.equal(7); const fcm = executionDetails.find( (ex) => ex.detail === DetailEnum.PUSH_MISSING_DEVICE_TOKENS && ex.providerId === PushProviderIdEnum.FCM ); @@ -137,7 +137,7 @@ describe('Trigger event - Send Push Notification - /v1/events/trigger (POST) #no _environmentId: session.environment._id, }); - expect(executionDetails.length).to.equal(11); + expect(executionDetails.length).to.equal(9); const fcmMessageCreated = executionDetails.find( (ex) => ex.detail === `${DetailEnum.MESSAGE_CREATED}: ${PushProviderIdEnum.FCM}` && From 10cc67c0abfedc8a120bd50c8cb62bba794dffe8 Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Wed, 5 Feb 2025 16:21:49 +0530 Subject: [PATCH 08/11] test(e2e): update execution details assertions for push notification tests --- apps/api/src/app/events/e2e/send-message-push.e2e.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/apps/api/src/app/events/e2e/send-message-push.e2e.ts b/apps/api/src/app/events/e2e/send-message-push.e2e.ts index 85c315c7103..d5f2f91e06c 100644 --- a/apps/api/src/app/events/e2e/send-message-push.e2e.ts +++ b/apps/api/src/app/events/e2e/send-message-push.e2e.ts @@ -80,8 +80,6 @@ describe('Trigger event - Send Push Notification - /v1/events/trigger (POST) #no const noActiveChannel = executionDetails.find((ex) => ex.detail === DetailEnum.SUBSCRIBER_NO_ACTIVE_CHANNEL); expect(noActiveChannel).to.be.ok; expect(noActiveChannel?.providerId).to.equal('fcm'); - const genericError = executionDetails.find((ex) => ex.detail === DetailEnum.NOTIFICATION_ERROR); - expect(genericError).to.be.ok; }); it('should not create any message if subscriber has configured two providers without device tokens', async () => { @@ -104,7 +102,7 @@ describe('Trigger event - Send Push Notification - /v1/events/trigger (POST) #no _environmentId: session.environment._id, }); - expect(executionDetails.length).to.equal(7); + expect(executionDetails.length).to.equal(9); const fcm = executionDetails.find( (ex) => ex.detail === DetailEnum.PUSH_MISSING_DEVICE_TOKENS && ex.providerId === PushProviderIdEnum.FCM ); @@ -137,7 +135,7 @@ describe('Trigger event - Send Push Notification - /v1/events/trigger (POST) #no _environmentId: session.environment._id, }); - expect(executionDetails.length).to.equal(9); + expect(executionDetails.length).to.equal(11); const fcmMessageCreated = executionDetails.find( (ex) => ex.detail === `${DetailEnum.MESSAGE_CREATED}: ${PushProviderIdEnum.FCM}` && From 34c05e44fda7de66578e703b08ab03595442f03f Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Wed, 5 Feb 2025 16:24:21 +0530 Subject: [PATCH 09/11] fix(worker): handle errors in push message sending process --- .../send-message/send-message-push.usecase.ts | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts index c30e785e076..593902540f0 100644 --- a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts @@ -134,10 +134,22 @@ export class SendMessagePush extends SendMessageBase { for (const channel of pushChannels) { const { deviceTokens } = channel.credentials || {}; - const [isChannelMissingDeviceTokens, integration] = await Promise.all([ - this.isChannelMissingDeviceTokens(channel, command), - this.getSubscriberIntegration(channel, command), - ]); + let isChannelMissingDeviceTokens; + let integration; + try { + [isChannelMissingDeviceTokens, integration] = await Promise.all([ + this.isChannelMissingDeviceTokens(channel, command), + this.getSubscriberIntegration(channel, command), + ]); + } catch (error) { + integrationsWithErrors += 1; + Logger.error( + { jobId: command.jobId }, + [`Error processing channel for jobId ${command.jobId}`, error.message || error.toString()].join(' '), + LOG_CONTEXT + ); + continue; + } // We avoid to send a message if subscriber has not an integration or if the subscriber has no device tokens for said integration if (!deviceTokens || !integration || isChannelMissingDeviceTokens) { @@ -331,7 +343,11 @@ export class SendMessagePush extends SendMessageBase { const raw = JSON.stringify(e) !== JSON.stringify({}) ? JSON.stringify(e) : JSON.stringify(e.message); - await this.sendProviderError(command.job, message._id, raw); + try { + await this.sendProviderError(command.job, message._id, raw); + } catch (e) { + // Do nothing, as this throws error createExecutionDetailsError + } return { success: false, error: e }; } From 0b2b4451a3dfd9f5977f7030751e9982a1ddb8a8 Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Wed, 5 Feb 2025 17:39:51 +0530 Subject: [PATCH 10/11] fix(worker): log errors when sending provider error for jobId --- .../usecases/send-message/send-message-push.usecase.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts index 593902540f0..ec780803a9e 100644 --- a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts @@ -345,8 +345,12 @@ export class SendMessagePush extends SendMessageBase { try { await this.sendProviderError(command.job, message._id, raw); - } catch (e) { - // Do nothing, as this throws error createExecutionDetailsError + } catch (err) { + Logger.error( + { jobId: command.jobId }, + [`Error sending provider error for jobId ${command.jobId}`, err.message || err.toString()].join(' '), + LOG_CONTEXT + ); } return { success: false, error: e }; From e26d0c7df17a0267265fa7252b8d5b56c320716b Mon Sep 17 00:00:00 2001 From: Himanshu Garg Date: Thu, 6 Feb 2025 12:46:38 +0530 Subject: [PATCH 11/11] fix(worker): improve error logging format for push notification processing --- .../usecases/send-message/send-message-push.usecase.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts index ec780803a9e..9956608ed48 100644 --- a/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts +++ b/apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts @@ -145,7 +145,7 @@ export class SendMessagePush extends SendMessageBase { integrationsWithErrors += 1; Logger.error( { jobId: command.jobId }, - [`Error processing channel for jobId ${command.jobId}`, error.message || error.toString()].join(' '), + `Error processing channel for jobId ${command.jobId} ${error.message || error.toString()}`, LOG_CONTEXT ); continue; @@ -182,10 +182,7 @@ export class SendMessagePush extends SendMessageBase { Logger.error( { jobId: command.jobId }, - [ - `Error sending push notification for jobId ${command.jobId}`, - result.error.message || result.error.toString(), - ].join(' '), + `Error sending push notification for jobId ${command.jobId} ${result.error.message || result.error.toString()}`, LOG_CONTEXT ); } @@ -348,7 +345,7 @@ export class SendMessagePush extends SendMessageBase { } catch (err) { Logger.error( { jobId: command.jobId }, - [`Error sending provider error for jobId ${command.jobId}`, err.message || err.toString()].join(' '), + `Error sending provider error for jobId ${command.jobId} ${err.message || err.toString()}`, LOG_CONTEXT ); }