-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
feat(api): Add automatic quota throttling #5485
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for novu-design ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -71,7 +71,7 @@ export const SettingsNavMenu: React.FC = () => { | |||
testId="side-nav-settings-branding-link" | |||
></NavMenuLinkButton> | |||
<NavMenuLinkButton | |||
label="Billing plans" | |||
label="Billing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a regression during Information Architecture feature development.
@@ -47,7 +48,7 @@ export class ResourceThrottlerInterceptor implements NestInterceptor { | |||
organizationId, | |||
environmentId: 'system', | |||
userId: 'system', | |||
key: FeatureFlagsKeysEnum.IS_EVENT_RESOURCE_LIMITING_ENABLED, | |||
key: FeatureFlagsKeysEnum.IS_EVENT_QUOTA_LIMITING_ENABLED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming to align with intended purpose, which is to limit resource quotas.
hash.update(str); | ||
|
||
return hash.digest('hex'); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separating into a new file to keep independent from the key builder use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! , i wanted to separate this one and another 'createHmac' but was not able to do it because we had changes in multiple PRs.
IMO this one is a generic function and can live outside of the cache
scope, wdyt?
buildEvaluateApiRateLimitKey, | ||
buildServiceConfigApiRateLimitMaximumKey, | ||
buildVariablesKey, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of a code smell to update here, so we instead export each of the required builders in-line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file only has internal API changes, no change to external exports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file only has internal API changes, no changes to external exports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has comments added to provide a description of when to modify and use each base key builder. There are changes to the exports of these builders which are in-turn, consumed by the entities
and queries
.
@@ -87,15 +241,26 @@ export enum IdentifierPrefixEnum { | |||
GROUPED_BLUEPRINT = 'g_b', | |||
API_RATE_LIMIT_CATEGORY = 'a_r_l_c', | |||
SERVICE_CONFIG = 's_c', | |||
RESOURCE_TYPE = 'r_t', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New prefix to handle storage of quota usage for each resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactoring all the identifiers out into a separate file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a couple of comments for your review.
all of them as thought
s
hash.update(str); | ||
|
||
return hash.digest('hex'); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! , i wanted to separate this one and another 'createHmac' but was not able to do it because we had changes in multiple PRs.
IMO this one is a generic function and can live outside of the cache
scope, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
* Wraps the entire prefix string with curly braces. This has the effect of ensuring | ||
* that the entire prefix string is treated as a single key part by Redis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this one!
*************************************** | ||
* BASE KEY BUILDERS | ||
*************************************** | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could create a separate file for every section that has such a title, wdyt?
* fix(web): Rename Billing plans to Billing * refactor(shared): Tidy up builders and add comments * chore(shared): Add feature flags for quota limiting * test(shared): Update key builder test * fix(api): Use new ff key * chore(shared): Remove uncessary out-of-line exports * chore(app-gen): Fix typo * refactor(app-gen): Separate file for key builder identifiers * chore: Update per PR comments * fix: App gen imports * fix: testing controller import * fix: imports * fix: naming * chore: Upgrade to Redlock with typescript * chore: Remove unused decorator * feat: Add locking to cached entity interceptor * test: fix create-usage-records test * test: fix customer subscription updated test * test(api): add evaluate-event-resource-limit tests * chore: remove redundant resource guard * feat(api): Add safe EE import for quota throttler * chore: Add nestjs imports for billing * test(api): Add ci ee flag to api ee tests * test(api): Use enums for test * test(api): Add quota throttler guard test * chore: update submodule * revert(api): Accidental package script change * chore: Update submodule * chore: submodule * chore: submodule update * chore: update submodule * test(web): Update cypress tests * chore: update submodule * fix(billing): Add mongoose dependency * test(web): Update billing widget assertion * fix(app-gen): Separate method resolution and cache set * chore: update submodule * fix(app-gen): unlock only on error
What changed? Why was the change needed?
Screenshots
Passing key builder Tests
Expand for optional sections
Related enterprise PR
https://github.com/novuhq/packages-enterprise/pull/133
Special notes for your reviewer