First integration with rate-limiter service: log warning only #932
Conversation
|
This pull request has been linked to and will mark 1 task as "Done" when merged:
|
|
Link T-12166 |
src/controllers/v1.controller.ts
Outdated
| if (!gigastakeOptions.gigastaked) { | ||
| const { shouldLimit, numRateLimitedApps } = await shouldRateLimit(application.id, this.cache) | ||
| if (numRateLimitedApps === 0) { | ||
| logger.log('warn', 'Rate-limited applications list is empty; rate-limiting disabled') |
There was a problem hiding this comment.
this warn log would appear on each relay in case the list is empty, which can occur large log ingestion charges from our log management system and is something that, from a coding side, we already know if we left the list empty. I recommend to perform the check of existing apps in rate limit in applications.ts and throw the warning there so is made only once
There was a problem hiding this comment.
Thank you for the review. Fixed.
src/controllers/v1.controller.ts
Outdated
| @@ -428,6 +448,20 @@ export class V1Controller { | |||
| const applicationID = application.id | |||
| const applicationPublicKey = application.gatewayAAT.applicationPublicKey | |||
|
|
|||
| const { shouldLimit, numRateLimitedApps } = await shouldRateLimit(applicationID, this.cache) | |||
| if (numRateLimitedApps === 0) { | |||
| logger.log('warn', 'Rate-limited applications list is empty; rate-limiting disabled') | |||
There was a problem hiding this comment.
same comment as above
There was a problem hiding this comment.
Thank you for the review. Fixed.
| @@ -37,6 +37,7 @@ export class EnvironmentObserver implements LifeCycleObserver { | |||
| 'INFLUX_ORG', | |||
| 'ARCHIVAL_CHAINS', | |||
| 'BLOCKED_ADDRESSES_URL', | |||
| 'RATE_LIMITER_URL', | |||
There was a problem hiding this comment.
is this truly required? should rate limiter be on by default or should it be a toggle?
There was a problem hiding this comment.
This allows:
- Configuration of new endpoint for
rate-limiterif necessary - Toggling
rate-limiteron/off if any potentially urgent issues arise.
There was a problem hiding this comment.
Not providing the env there will cause an error on the api (as envs declared there must be provided) so I don't see how can it be toggle from there. There's another array on that same file of optional envs, you could add it to that array and check for that in applications.ts as is the file where services are boostraped and check on api boot
There was a problem hiding this comment.
Thank you for the review. Fixed.
src/utils/cache.ts
Outdated
| } as AxiosRequestConfig | ||
|
|
||
| const { data } = await axios(axiosConfig) | ||
| const { ApplicationIDs: rateLimitedAppsList } = data |
There was a problem hiding this comment.
| const { ApplicationIDs: rateLimitedAppsList } = data | |
| const { applicationIDs: rateLimitedAppsList } = data |
There was a problem hiding this comment.
This has to stay upper-case to match the output of rate-limiter(otherwise we will miss the output)
| return obj | ||
| } | ||
|
|
||
| const url = process.env.RATE_LIMITER_URL ?? '' |
There was a problem hiding this comment.
the portal-api usually reads the url as a global variable in top of the file or is passed as a function, I personally prefer the later as is more clear where the value is coming from but I recommend one of the two for consistency
There was a problem hiding this comment.
Thank you for the review. Fixed.
Signed-off-by: Arash Deshmeh <[email protected]>
adshmh
force-pushed
the
rate-limiter-integration-in-log-only-mode
branch
from
0efd069
to
3781bf4
Compare
|
Lgtm |
Signed-off-by: Arash Deshmeh <[email protected]>
|
Close T-12166 |
Signed-off-by: Arash Deshmeh [email protected]