Refactor the broadcast method to utilize the new queue #4526

djabarovgeorge · 2023-10-15T09:37:04Z

What change does this PR introduce?

Refactor the new broadcast method to utilize the new trigger-handler and process-subscriber queues, instead of doing that at the moment of the trigger.

Why was this change needed?

Other information (Screenshots)

linear · 2023-10-15T09:37:07Z

NV-3018 Refactor the `/broadcast` method, to utilize the new queue

What?

Refactor the new broadcast method to utilize the new trigger-handler and process-subscriber queues, instead of doing that at the moment of the trigger.

Why? (Context)

Definition of Done

djabarovgeorge · 2023-10-15T09:42:02Z

packages/application-generic/src/modules/queues.module.ts

+const APP_PROVIDERS: Provider[] = [
+  InboundParseQueue,
+  InboundParseWorker,
+  InboundParseQueueServiceHealthIndicator,
+  WebSocketsQueueService,
+  WebSocketsQueueServiceHealthIndicator,
+  WorkflowQueueService,
+  WorkflowQueueServiceHealthIndicator,
+];
+@Module({
+  providers: [...APP_PROVIDERS],
+  exports: [...APP_PROVIDERS],
+})
+export class BaseAppQueuesModule {}


This is not the final solution but a temporary one, in this part, I extracted the relevant queues, so we wont initialize not relevant queues in the API APP.
Will make a discovery in order to check if we could take advantage of nest dynamic modules in order to make it more convenient.
https://docs.nestjs.com/fundamentals/dynamic-modules

djabarovgeorge · 2023-10-15T09:47:36Z

apps/api/src/app/widgets/widgets.module.ts

@@ -8,7 +10,7 @@ import { SubscribersModule } from '../subscribers/subscribers.module';
 import { IntegrationModule } from '../integrations/integrations.module';

 @Module({
-  imports: [SharedModule, SubscribersModule, AuthModule, IntegrationModule],
+  imports: [SharedModule, SubscribersModule, AuthModule, IntegrationModule, BaseAppQueuesModule],


Was needed because we removed QueueModule from the shared module.
Here we need BaseAppQueuesModule because we use WebSocketsQueueService.

djabarovgeorge · 2023-10-15T09:50:28Z

apps/api/src/app/shared/shared.module.ts

-    QueuesModule,
  ],
  providers: [...PROVIDERS],
-  exports: [...PROVIDERS, LoggerModule, QueuesModule],


Not sure why we need this one here therefore I removed it.
If someone knows the reason please let me know.

It was added to provide the QueuesModule as provider in any module that is being dependant of SharedModule so in case of need QueuesModule would be available without having to inject it in said module.

djabarovgeorge · 2023-10-15T09:50:41Z

apps/api/src/app/inbound-parse/inbound-parse.module.ts

  controllers: [InboundParseController],
  providers: [...PROVIDERS, ...USE_CASES],
-  exports: [...USE_CASES, QueuesModule],


Do we need to export this one here?

It probably not anymore. I think it can be safe to be removed.

djabarovgeorge · 2023-10-15T09:54:02Z

apps/api/src/app/health/health.controller.ts

@@ -26,7 +25,6 @@ export class HealthController {
  healthCheck(): Promise<HealthCheckResult> {
    const checks: HealthIndicatorFunction[] = [
      async () => this.dalHealthIndicator.isHealthy(),
-      async () => this.standardQueueHealthIndicator.isHealthy(),


We no longer need it because we will use only the workflow queue in the API.

djabarovgeorge · 2023-10-15T09:54:31Z

apps/api/src/app/events/usecases/process-bulk-trigger/process-bulk-trigger.usecase.ts

@@ -10,7 +9,7 @@ import { ParseEventRequest } from '../parse-event-request/parse-event-request.us

 @Injectable()
 export class ProcessBulkTrigger {
-  constructor(private parseEventRequest: ParseEventRequest, private mapTriggerRecipients: MapTriggerRecipients) {}


redundant 🧹

djabarovgeorge · 2023-10-15T09:54:45Z

apps/api/src/app/events/events.module.ts

-  AddJob,
-  AddDelayJob,
-  MergeOrCreateDigest,
  CreateExecutionDetails,
-  CreateNotificationJobs,
-  DigestFilterSteps,
-  DigestFilterStepsBackoff,
-  DigestFilterStepsRegular,
-  DigestFilterStepsTimed,
  GetNovuProviderCredentials,
  StorageHelperService,
  EventsDistributedLockService,
-  ProcessSubscriber,
-  ProcessTenant,
  SendTestEmail,
-  StoreSubscriberJobs,
-  TriggerEvent,
-  MapTriggerRecipients,


i suspect we have more redundant providers in the DI.

apps/api/src/app/events/events.controller.ts

apps/api/src/app/events/usecases/trigger-event-to-all/trigger-event-to-all.usecase.ts

…lize-the-new-queue

djabarovgeorge · 2023-10-23T11:20:17Z

packages/application-generic/src/usecases/trigger-event/trigger-event.usecase.ts

+        recipients: command.to,
+        transactionId: command.transactionId,
+        userId: command.userId,
+        actor: actorProcessed,


Noticed that in the previous state is was using mappedActor Does anyone knows if there were particular reason? if there is i will refactor in order to pass mappedActor

I personally have no idea.
@ainouzgali tagging you because it is my memory about the implementation of mapped actors was made by you, but I might be wrong, so apologies in advance.

I believe only the subscriberId is needed, so thats why only the mapping was sent. But the command MapTriggerRecipientsCommand is expecting ISubscriberDefine, not SubscriberEntity.

@djabarovgeorge , a few days ago my PR for adding actor was merged, let me know if you need help resolving conflicts or reintegrating it. As it seems we touched a lot of the same logic

Thanks for bringing it up, i removed all of the actor/tenant i saw before the trigger event use-case (there was one redundant due to duplication in the flow), so now the trigger event use-case is responsible for the mapping.

djabarovgeorge · 2023-10-23T11:24:16Z

apps/api/src/app/events/usecases/parse-event-request/parse-event-request.command.ts

Was debating with myself if to update the to type, to contain AddressingTypeEnum as well or not.
In the end decided to add additional addressingType param.
Will love to hear if anyone has any ideas regarding the new Command interface.

small note, if addressingType === AddressingTypeEnum.BROADCAST the to is redundant because we will trigger to all of the subscribers.

p-fernandez · 2023-10-23T13:10:24Z

apps/api/src/app/inbound-parse/inbound-parse.module.ts

  controllers: [InboundParseController],
  providers: [...PROVIDERS, ...USE_CASES],
-  exports: [...USE_CASES, QueuesModule],


It probably not anymore. I think it can be safe to be removed.

p-fernandez · 2023-10-23T13:11:27Z

apps/api/src/app/shared/shared.module.ts

-    QueuesModule,
  ],
  providers: [...PROVIDERS],
-  exports: [...PROVIDERS, LoggerModule, QueuesModule],


It was added to provide the QueuesModule as provider in any module that is being dependant of SharedModule so in case of need QueuesModule would be available without having to inject it in said module.

p-fernandez · 2023-10-23T13:13:19Z

packages/application-generic/src/modules/queues.module.ts

+const APP_PROVIDERS: Provider[] = [
+  InboundParseQueue,
+  InboundParseWorker,
+  InboundParseQueueServiceHealthIndicator,
+  WebSocketsQueueService,
+  WebSocketsQueueServiceHealthIndicator,
+  WorkflowQueueService,
+  WorkflowQueueServiceHealthIndicator,
+];
+
+@Module({
+  providers: [...APP_PROVIDERS],
+  exports: [...APP_PROVIDERS],
+})
+export class BaseApiQueuesModule {}


Are you trying to gather all the QueuesModule dependencies used only in API app?

Yes exactly, for two reasons

that way we won't initialize redundant clients in API.

in the near future (i hope) i want to try creating dynamic health validation, meaning if we inject a new HealthIndicator in the DI it will be validated out of the box, for example without adding it to the healthCheck in the api/src/app/health/health.controller.ts.

Ok, now explained it is a great idea and didn't think about it (even though the clients should be only initialised in the custom providers if needed) and the services only when injected, if I am not wrong.
But the separation of the different services based on the App usage I like it. I would just suggest to create all them and name them as:
ApiQueuesModule
WorkerQueuesModule
WebSocketQueuesModule
etc.

I am happy to take that responsibility in a following task to this one so you don't have to take that work as we need this to move forward.

From the small test I made locally before making the changes I saw that they do initialize even if not injected, i guess because they are singletons maybe?

Regarding the separation i totally agree, make it only for the API in order to keep the scope as small as possible for this PR as it is an extra for it. We definitely can update the other ones as well.

Once again I agree we can merge this one for now, the additional improvements can wait for now. I made this one as a preparation for the future.

p-fernandez · 2023-10-23T13:13:34Z

packages/application-generic/src/modules/index.ts

@@ -1 +1,2 @@
 export { QueuesModule } from './queues.module';
+export { BaseApiQueuesModule } from './queues.module';


You can reuse the same export line.

p-fernandez · 2023-10-23T13:14:02Z

packages/application-generic/src/usecases/trigger-event/trigger-event.command.ts

-  @IsDefined()
-  to: ISubscribersDefine[];
+  @IsOptional()
+  to?: ISubscribersDefine[];


What's the reason behind making this optional?

Good question, it is related to this thought i had
#4526 (comment)
I was not sure how to provide extra data to parse-event and trigger-event usecases that we need to execute broadcast trigger.

Oh, wasn't sure what that meant.
I think we might need to explore to create specific command to each case sharing a base command.

p-fernandez · 2023-10-23T13:15:46Z

packages/application-generic/src/usecases/trigger-event/trigger-event.usecase.ts

+        recipients: command.to,
+        transactionId: command.transactionId,
+        userId: command.userId,
+        actor: actorProcessed,


I personally have no idea.
@ainouzgali tagging you because it is my memory about the implementation of mapped actors was made by you, but I might be wrong, so apologies in advance.

…lize-the-new-queue

p-fernandez

🌟

packages/application-generic/src/usecases/trigger-event/trigger-event.usecase.ts

…lize-the-new-queue

…provider

…ethod-to-utilize-the-new-queue' into NV-3018-refactor-the-broadcast-method-to-utilize-the-new-queue # Conflicts: # pnpm-lock.yaml

…lize-the-new-queue

…ethod-to-utilize-the-new-queue' into NV-3018-refactor-the-broadcast-method-to-utilize-the-new-queue # Conflicts: # pnpm-lock.yaml

…lize-the-new-queue

feat: refactor the broadcast method to utilize the new queue

c599eee

github-actions bot added the @novu/api label Oct 15, 2023

djabarovgeorge commented Oct 15, 2023

View reviewed changes

apps/api/src/app/events/events.controller.ts Outdated Show resolved Hide resolved

djabarovgeorge changed the title ~~feat: refactor the broadcast method to utilize the new queue~~ Refactor the broadcast method to utilize the new queue Oct 15, 2023

scopsy reviewed Oct 17, 2023

View reviewed changes

apps/api/src/app/events/events.controller.ts Outdated Show resolved Hide resolved

apps/api/src/app/events/usecases/trigger-event-to-all/trigger-event-to-all.usecase.ts Outdated Show resolved Hide resolved

fix: missing actor mapping

41dc6bd

Base automatically changed from refactor-queue-health-indicator to next October 17, 2023 14:03

djabarovgeorge and others added 2 commits October 23, 2023 09:47

Merge branch 'next' into NV-3018-refactor-the-broadcast-method-to-uti…

092193e

…lize-the-new-queue

feat: handle broadcast in trigger event usecase

908f7dc

github-actions bot added the @novu/shared label Oct 23, 2023

djabarovgeorge commented Oct 23, 2023

View reviewed changes

feat: make to optional, because it could be broadcast

92bb3ae

djabarovgeorge commented Oct 23, 2023

View reviewed changes

p-fernandez reviewed Oct 23, 2023

View reviewed changes

Merge branch 'next' into NV-3018-refactor-the-broadcast-method-to-uti…

d1204c0

…lize-the-new-queue

p-fernandez approved these changes Oct 24, 2023

View reviewed changes

ainouzgali reviewed Oct 25, 2023

View reviewed changes

packages/application-generic/src/usecases/trigger-event/trigger-event.usecase.ts Outdated Show resolved Hide resolved

djabarovgeorge and others added 5 commits October 25, 2023 16:29

feat: remove redundant and before time mapping

3931058

Merge branch 'next' into NV-3018-refactor-the-broadcast-method-to-uti…

8a80504

…lize-the-new-queue

fix: remove standard queue service and add map trigger recipients as …

010ff5e

…provider

Merge remote-tracking branch 'origin/NV-3018-refactor-the-broadcast-m…

b0d91a1

…ethod-to-utilize-the-new-queue' into NV-3018-refactor-the-broadcast-method-to-utilize-the-new-queue # Conflicts: # pnpm-lock.yaml

fix: update pnpm lock

601b977

djabarovgeorge and others added 10 commits October 26, 2023 17:39

fix: command validation & tenant mapping

61449fd

feat: split trigger commands by type

e44c267

fix: command type

0079dc1

Merge branch 'next' into NV-3018-refactor-the-broadcast-method-to-uti…

775a87b

…lize-the-new-queue

fix: tenant fetch from string

bc81155

fix: pnpm lock

3bd3a71

fix: pnpm lock

81f5f0d

Merge remote-tracking branch 'origin/NV-3018-refactor-the-broadcast-m…

02614f8

…ethod-to-utilize-the-new-queue' into NV-3018-refactor-the-broadcast-method-to-utilize-the-new-queue # Conflicts: # pnpm-lock.yaml

fix: pnpm lock

d0d22e8

Merge branch 'next' into NV-3018-refactor-the-broadcast-method-to-uti…

1de3c4e

…lize-the-new-queue

djabarovgeorge merged commit 7722653 into next Nov 3, 2023
25 of 27 checks passed

djabarovgeorge deleted the NV-3018-refactor-the-broadcast-method-to-utilize-the-new-queue branch November 3, 2023 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the broadcast method to utilize the new queue #4526

Refactor the broadcast method to utilize the new queue #4526

djabarovgeorge commented Oct 15, 2023

linear bot commented Oct 15, 2023

What?

Why? (Context)

Definition of Done

djabarovgeorge Oct 15, 2023

djabarovgeorge Oct 15, 2023

djabarovgeorge Oct 15, 2023

p-fernandez Oct 23, 2023

djabarovgeorge Oct 15, 2023

p-fernandez Oct 23, 2023

djabarovgeorge Oct 15, 2023

djabarovgeorge Oct 15, 2023

djabarovgeorge Oct 15, 2023

djabarovgeorge Oct 15, 2023

djabarovgeorge Oct 23, 2023

p-fernandez Oct 23, 2023

ainouzgali Oct 25, 2023

ainouzgali Oct 25, 2023

djabarovgeorge Oct 26, 2023

djabarovgeorge Oct 23, 2023 •

edited

Loading

p-fernandez Oct 23, 2023

p-fernandez Oct 23, 2023

p-fernandez Oct 23, 2023

djabarovgeorge Oct 23, 2023

p-fernandez Oct 23, 2023

djabarovgeorge Oct 24, 2023

p-fernandez Oct 23, 2023

p-fernandez Oct 23, 2023

djabarovgeorge Oct 23, 2023

p-fernandez Oct 23, 2023

p-fernandez Oct 23, 2023

p-fernandez left a comment

		@@ -1 +1,2 @@
		export { QueuesModule } from './queues.module';
		export { BaseApiQueuesModule } from './queues.module';

Refactor the broadcast method to utilize the new queue #4526

Refactor the broadcast method to utilize the new queue #4526

Conversation

djabarovgeorge commented Oct 15, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Oct 15, 2023

What?

Why? (Context)

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djabarovgeorge Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-fernandez left a comment

Choose a reason for hiding this comment

djabarovgeorge Oct 23, 2023 •

edited

Loading