Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(application-generic): potential get preferences performance improvement #7467

Merged
merged 2 commits into from
Jan 9, 2025
Merged

fix(application-generic): potential get preferences performance improvement #7467

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3192a48
fix(application-generic): potential get preferences performance impro…
LetItRock Jan 8, 2025
a438294
chore(application-generic): suggestions from the pr
LetItRock Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 96 additions & 55 deletions ...ation-generic/src/usecases/get-subscriber-preference/get-subscriber-preference.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ import {
SubscriberRepository,
PreferencesRepository,
NotificationTemplateEntity,
PreferencesEntity,
} from '@novu/dal';
import {
ChannelTypeEnum,
IPreferenceChannels,
ISubscriberPreferenceResponse,
PreferencesTypeEnum,
StepTypeEnum,
Expand All @@ -15,7 +17,11 @@ import {
import { GetSubscriberPreferenceCommand } from './get-subscriber-preference.command';
import { Instrument, InstrumentUsecase } from '../../instrumentation';
import { MergePreferences } from '../merge-preferences/merge-preferences.usecase';
import { GetPreferences, PreferenceSet } from '../get-preferences';
import {
GetPreferences,
GetPreferencesResponseDto,
PreferenceSet,
} from '../get-preferences';
import {
filteredPreference,
overridePreferences,
Expand Down Expand Up @@ -121,68 +127,103 @@ export class GetSubscriberPreference {
return acc;
}, {});

const mergedPreferences: ISubscriberPreferenceResponse[] = workflowList.map(
(workflow) => {
const preferences = workflowPreferenceSets[workflow._id];
const mergeCommand = MergePreferencesCommand.create({
workflowResourcePreference: preferences.workflowResourcePreference,
workflowUserPreference: preferences.workflowUserPreference,
subscriberWorkflowPreference:
preferences.subscriberWorkflowPreference,
...(subscriberGlobalPreference ? { subscriberGlobalPreference } : {}),
});
const merged = MergePreferences.execute(mergeCommand);
const workflowPreferences: ISubscriberPreferenceResponse[] =
this.calculateWorkflowPreferences(
workflowList,
workflowPreferenceSets,
subscriberGlobalPreference,
command.includeInactiveChannels,
);

const nonCriticalWorkflowPreferences = workflowPreferences.filter(
(preference) => !preference.template.critical,
);

return nonCriticalWorkflowPreferences;
}
Comment on lines +130 to +143
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all I did in this file is moved the code parts to the class functions to be able to instrument them and see the perf bottleneck


const includedChannels = this.getChannels(
workflow,
command.includeInactiveChannels,
);
@Instrument()
private calculateWorkflowPreferences(
workflowList: NotificationTemplateEntity[],
workflowPreferenceSets: Record<string, PreferenceSet>,
subscriberGlobalPreference: PreferencesEntity,
includeInactiveChannels: boolean,
): ISubscriberPreferenceResponse[] {
return workflowList.map((workflow) => {
const preferences = workflowPreferenceSets[workflow._id];
const merged = this.mergePreferences(
preferences,
subscriberGlobalPreference,
);

const initialChannels = filteredPreference(
{
email: true,
sms: true,
in_app: true,
chat: true,
push: true,
},
includedChannels,
);
const includedChannels = this.getChannels(
workflow,
includeInactiveChannels,
);

const { channels, overrides } = overridePreferences(
{
template: GetPreferences.mapWorkflowPreferencesToChannelPreferences(
merged.source.WORKFLOW_RESOURCE,
),
subscriber:
GetPreferences.mapWorkflowPreferencesToChannelPreferences(
merged.preferences,
),
workflowOverride: {},
},
initialChannels,
);
const initialChannels = filteredPreference(
{
email: true,
sms: true,
in_app: true,
chat: true,
push: true,
},
includedChannels,
);

return {
preference: {
channels,
enabled: true,
overrides,
},
template: mapTemplateConfiguration({
...workflow,
critical: merged.preferences.all.readOnly,
}),
type: PreferencesTypeEnum.SUBSCRIBER_WORKFLOW,
};
const { channels, overrides } = this.calculateChannelsAndOverrides(
merged,
initialChannels,
);

return {
preference: {
channels,
enabled: true,
overrides,
},
template: mapTemplateConfiguration({
...workflow,
critical: merged.preferences.all.readOnly,
}),
type: PreferencesTypeEnum.SUBSCRIBER_WORKFLOW,
};
});
}

@Instrument()
private calculateChannelsAndOverrides(
merged: GetPreferencesResponseDto,
initialChannels: IPreferenceChannels,
) {
return overridePreferences(
{
template: GetPreferences.mapWorkflowPreferencesToChannelPreferences(
merged.source.WORKFLOW_RESOURCE,
),
subscriber: GetPreferences.mapWorkflowPreferencesToChannelPreferences(
merged.preferences,
),
workflowOverride: {},
},
initialChannels,
);
}

const nonCriticalWorkflowPreferences = mergedPreferences.filter(
(preference) => !preference.template.critical,
);
@Instrument()
private mergePreferences(
preferences: PreferenceSet,
subscriberGlobalPreference: PreferencesEntity,
) {
const mergeCommand = MergePreferencesCommand.create({
workflowResourcePreference: preferences.workflowResourcePreference,
workflowUserPreference: preferences.workflowUserPreference,
subscriberWorkflowPreference: preferences.subscriberWorkflowPreference,
...(subscriberGlobalPreference ? { subscriberGlobalPreference } : {}),
});

return nonCriticalWorkflowPreferences;
return MergePreferences.execute(mergeCommand);
}

private getChannels(
Expand Down
9 changes: 3 additions & 6 deletions libs/application-generic/src/usecases/merge-preferences/merge-preferences.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { merge } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 100% manual tree shaking

Suggested change
import { merge } from 'lodash';
import merge from 'lodash/merge';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh yeah missed that

import { PreferencesTypeEnum } from '@novu/shared';
import { PreferencesEntity } from '@novu/dal';
import { deepMerge } from '../../utils';

import { GetPreferencesResponseDto } from '../get-preferences';
import { MergePreferencesCommand } from './merge-preferences.command';
import { DeepRequired } from '../../http/utils.types';

/**
* Merge preferences for a subscriber.
Expand Down Expand Up @@ -44,9 +43,7 @@ export class MergePreferences {
...(isWorkflowPreferenceReadonly ? [] : subscriberPreferences),
];

const mergedPreferences = deepMerge(
preferencesList as DeepRequired<PreferencesEntity>[],
);
const mergedPreferences = merge({}, ...preferencesList);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We assume that the potential issue of slow response times could be that we used the custom deepMerge code that was copied from the lib and had not been maintained for the last 2 years. We want to verify that assumption.


// Build the source object
const source = {
Expand Down
Loading