fix(api): reduce activity stats queries and improve performance #3029

p-fernandez · 2023-03-15T22:57:25Z

What change does this PR introduce?

Reduce the number of queries against the DB for the Activity Stats endpoint (it calculates 3 numbers, yearly total notifications, monthly and weekly).
Also introduces an index for Notification schema for the _organizationId and a test.

Why was this change needed?

We were being notified by performance degradation. Slow responses in the Activity feed. Thanks to one of our users we could spot one of the slow endpoints was /notifications/stats. Investigating the code we can find that 3 count queries were done. It is acknowledged that MongoDB count queries for large collections can be quite slow. This tries to mitigate that for large collections reducing the queries and calculating the numbers in an alternative way.

Other information (Screenshots)

We are calling this endpoint every time we load the Activity Feed page. We should think if it is useful for the users to have the yearly stat getting calculated every time. At least for large volume users.

p-fernandez · 2023-03-15T22:59:06Z

apps/api/src/app/notifications/dtos/activities-response.dto.ts

@@ -173,7 +173,7 @@ export class ActivityNotificationResponseDto {
  transactionId: string;

  @ApiPropertyOptional()
-  createdAt?: Date;
+  createdAt?: string;


I think this doesn't break anything as I think we were truly returning strings (this DTO is used in API responses and I couldn't find any transformation to Date in the code). Worth review this in the Web app. Running in my local couldn't find any problem with this change though. 😕

Yep, it's indeed an ISO string, so it's more accurate like this.

p-fernandez · 2023-03-15T22:59:19Z

apps/api/src/app/notifications/e2e/get-activity-stats.e2e.ts

@@ -43,7 +53,7 @@ describe('Get activity stats - /notifications/stats (GET)', async () => {
      {
        _environmentId: session.environment._id,
      },
-      null,
+      undefined,


TS warning.

p-fernandez · 2023-03-15T22:59:54Z

apps/api/src/app/notifications/e2e/get-activity-stats.e2e.ts

+     * We generate notifications avoiding clashes of leap years and different month lengths
+     * so this test can be executed any time with same results
+     * Create 7 notifications during the week


I just wanted to avoid dates edge cases based on the execution time.

p-fernandez · 2023-03-15T23:01:35Z

apps/api/src/app/notifications/usecases/get-activity-stats/get-activity-stats.usecase.ts

-      _organizationId: command.organizationId,
-      createdAt: {
-        $gte: subMonths(new Date(), 1),
+    const yearlyNotifications = await this.notificationRepository.find(


We retrieve all the notifications in a year and then loop them to calculate the monthly and the weekly rather than hitting the DB again. Also to help for large collections we just retrieve 2 fields: _id and createdAt. We could even just retrieve createdAt.
All this replaces the 3 DB calls.

Unfortunately this approach is not very good :( Imagine an organization sending 10M events per month, you will return 120M record from the database to calculate this. Let alone the traffic bandwidth for this, the server will have to load it all in memory.

My suggestion would be to follow your approach but perhaps using a monthly aggregation pipeline and than from the aggregation results do a similar thing you've done.

For example here is a query that does it all in mongo:

db.getCollection('notifications').aggregate([ // match events sent this week, this month, and this year { $match: { createdAt: { $gte: // Date of last year } } }, // group events by week, month, and year { $group: { _id: { week: { $week: "$createdAt" }, month: { $month: "$createdAt" }, year: { $year: "$createdAt" } }, count: { $sum: 1 } } }])

But to be honest, I'm not sure how it will be more performant than the count counterparts.

Another thing we can do here, is to also change the read preference here to query the replica and not the main db to reduce the overhead on it. wdyt?

@scopsy this was meant more like a short term fix. I do think we need to review the UI and what's the benefit of showing one year long usage all the time.
But you are right for a long term your proposal should perform better. Let me try to experiment a bit with it in MongoDB.

p-fernandez · 2023-03-15T23:02:38Z

libs/dal/src/repositories/base-repository.ts

+  async insertMany(
+    data: FilterQuery<T_DBModel> & T_Enforcement[]
+  ): Promise<{ acknowledged: boolean; insertedCount: number; insertedIds: Types.ObjectId[] }> {
+    const result = await this.MongooseModel.insertMany(data, { ordered: false });
+
+    const insertedIds = result.map((inserted) => inserted._id);
+
+    return {
+      acknowledged: true,
+      insertedCount: result.length,
+      insertedIds,
+    };
+  }


Introduced to help with the testing.

p-fernandez · 2023-03-15T23:04:10Z

libs/dal/src/repositories/notification/notification.schema.ts

@@ -19,6 +19,7 @@ const notificationSchema = new Schema<NotificationDBModel>(
    _organizationId: {
      type: Schema.Types.ObjectId,
      ref: 'Organization',
+      index: true,


The query for the stats is done with _environmentId and _organizationId. I would expect this field not being indexed, to slow down the query. Another option would be to just query based on the indexed field _environmentId as it should be enough.

I have introduced the compound index of:

{ _environmentId: 1, createdAt: -1}

yesterday, because of the cardinality of the environment field it will be better the the organizationId, and mongoDB is smart enough to use the new compound index for the new query even if it contains the organizationId.

One thing we can actually do is to remove the organizationId from the query.

@scopsy The compound index was a great move. 👍🏻
I think we should also introduce the index for organization for future multitenancy purposes. We might forget.

scopsy

Added a few comments regarding the selected query, I'm afraid it might introduce more performance issues once implemented :(

scopsy · 2023-03-16T07:37:19Z

apps/api/src/app/notifications/dtos/activities-response.dto.ts

@@ -173,7 +173,7 @@ export class ActivityNotificationResponseDto {
  transactionId: string;

  @ApiPropertyOptional()
-  createdAt?: Date;
+  createdAt?: string;


Yep, it's indeed an ISO string, so it's more accurate like this.

scopsy · 2023-03-16T08:07:44Z

apps/api/src/app/notifications/usecases/get-activity-stats/get-activity-stats.usecase.ts

-      _organizationId: command.organizationId,
-      createdAt: {
-        $gte: subMonths(new Date(), 1),
+    const yearlyNotifications = await this.notificationRepository.find(


Unfortunately this approach is not very good :( Imagine an organization sending 10M events per month, you will return 120M record from the database to calculate this. Let alone the traffic bandwidth for this, the server will have to load it all in memory.

My suggestion would be to follow your approach but perhaps using a monthly aggregation pipeline and than from the aggregation results do a similar thing you've done.

For example here is a query that does it all in mongo:

db.getCollection('notifications').aggregate([ // match events sent this week, this month, and this year { $match: { createdAt: { $gte: // Date of last year } } }, // group events by week, month, and year { $group: { _id: { week: { $week: "$createdAt" }, month: { $month: "$createdAt" }, year: { $year: "$createdAt" } }, count: { $sum: 1 } } }])

But to be honest, I'm not sure how it will be more performant than the count counterparts.

Another thing we can do here, is to also change the read preference here to query the replica and not the main db to reduce the overhead on it. wdyt?

scopsy · 2023-03-16T08:09:53Z

libs/dal/src/repositories/notification/notification.schema.ts

@@ -19,6 +19,7 @@ const notificationSchema = new Schema<NotificationDBModel>(
    _organizationId: {
      type: Schema.Types.ObjectId,
      ref: 'Organization',
+      index: true,


I have introduced the compound index of:

{ _environmentId: 1, createdAt: -1}

yesterday, because of the cardinality of the environment field it will be better the the organizationId, and mongoDB is smart enough to use the new compound index for the new query even if it contains the organizationId.

One thing we can actually do is to remove the organizationId from the query.

scopsy

Looking good!

p-fernandez requested review from davidsoderberg, LetItRock, ainouzgali, scopsy, BiswaViraj, djabarovgeorge and Cliftonz March 15, 2023 22:57

p-fernandez self-assigned this Mar 15, 2023

github-actions bot added @novu/api @novu/dal labels Mar 15, 2023

p-fernandez commented Mar 15, 2023

View reviewed changes

scopsy reviewed Mar 16, 2023

View reviewed changes

fix(api): reduce activity stats queries and improve performance

019245a

p-fernandez force-pushed the fix-activity-stats-queries branch from 06c0293 to f8d2881 Compare March 16, 2023 18:19

fix(api): pr suggestions

594a351

p-fernandez force-pushed the fix-activity-stats-queries branch from f8d2881 to 594a351 Compare March 16, 2023 18:30

p-fernandez requested a review from scopsy March 16, 2023 18:32

scopsy approved these changes Mar 16, 2023

View reviewed changes

p-fernandez added this pull request to the merge queue Mar 17, 2023

Merged via the queue into next with commit 6854964 Mar 17, 2023

p-fernandez deleted the fix-activity-stats-queries branch March 17, 2023 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): reduce activity stats queries and improve performance #3029

fix(api): reduce activity stats queries and improve performance #3029

p-fernandez commented Mar 15, 2023 •

edited

p-fernandez Mar 15, 2023

scopsy Mar 16, 2023

p-fernandez Mar 15, 2023

p-fernandez Mar 15, 2023

p-fernandez Mar 15, 2023 •

edited

scopsy Mar 16, 2023

p-fernandez Mar 16, 2023

p-fernandez Mar 15, 2023

p-fernandez Mar 15, 2023

scopsy Mar 16, 2023

p-fernandez Mar 16, 2023

scopsy left a comment

scopsy Mar 16, 2023

scopsy Mar 16, 2023

scopsy Mar 16, 2023

scopsy left a comment

fix(api): reduce activity stats queries and improve performance #3029

fix(api): reduce activity stats queries and improve performance #3029

Conversation

p-fernandez commented Mar 15, 2023 • edited

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

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 Mar 15, 2023 • edited

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

scopsy left a 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

scopsy left a comment

Choose a reason for hiding this comment

p-fernandez commented Mar 15, 2023 •

edited

p-fernandez Mar 15, 2023 •

edited