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.

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

Update RedisCache and rate limit related libraries #1290

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

qtomlinson
Copy link
Collaborator

@qtomlinson qtomlinson commented Feb 27, 2025

This PR addresses the upgrade of our Redis and rate-limiting libraries, which had breaking changes. The following are the specific modifications and improvements made:

  1. Refactor Rate Limiting Logic to ratelimit.js

    • Moved the rate-limiting logic to a dedicated ratelimit.js module for better encapsulation and to facilitate unit testing.
    • Added adaptation of breaking changes introduced by the packages, e.g. the behavior when max === 0.
    • Ensure that the health check is not rate-limited.
  2. Reduced the number of Redis clients from three to one

    • The redis client is shared among the API rate limit, batch API rate limit, and Redis cache services.
  3. Standardization of Rate Limiting Headers

    • Rate-limiting related headers now use standardHeaders instead of legacy headers. This is a breaking change that may affect clients using the old rate-limit headers.
  4. Addition of Unit and Integration Tests

    • unit and integration tests have been added to ensure the correct functioning of the redis cache and rate-limiting behaviour.

@qtomlinson qtomlinson force-pushed the qt/update_ratelimit branch from 2b9af0b to e3d72a2 Compare March 6, 2025 00:53
@qtomlinson qtomlinson force-pushed the qt/update_ratelimit branch 6 times, most recently from 10144c4 to 1ffe770 Compare March 6, 2025 23:53
The application test starts a timer on the curation queue. This timer is not cleaned up, which causes intermittent failures in the "Curation queue processing" unit tests.

Disable the unit test.
@qtomlinson qtomlinson force-pushed the qt/update_ratelimit branch from 1ffe770 to d0160f4 Compare March 7, 2025 01:55
@qtomlinson qtomlinson force-pushed the qt/update_ratelimit branch from f7150d5 to 99a9aa4 Compare March 10, 2025 17:35
@qtomlinson qtomlinson marked this pull request as ready for review March 10, 2025 17:49
bin/www Outdated
const cachingService = config.caching.service()
await Promise.all([cachingService.initialize()]).catch(error => {
logger.error('Error connecting to cachingService', error)
process.exit(1)
Copy link
Collaborator Author

@qtomlinson qtomlinson Mar 10, 2025

Choose a reason for hiding this comment

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

Fail fast here when redis fails to connect during startup. In the event of this happens, we have the option to update configuration to use in-memory caching for deployment as a fallback. Another option is to increase the connection timeout. Currently, default value is used here, same as the original code. Any thoughts?

Choose a reason for hiding this comment

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

Problem is that while you wait for connection, your service is not exposing any endpoint. How do you deal with the cloud probes ?

test/app.js Outdated
})

it.skip('should initialize', done => {
const app = Application(config, {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test starts several timers, one of which operates on the curation queue. The timers are not properly cleaned up and interfere with the curation unit tests. Disabled the test for now.

Choose a reason for hiding this comment

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

can't you mock the timers ?

@qtomlinson qtomlinson requested a review from mpcen March 10, 2025 18:16
bin/www Outdated
const cachingService = config.caching.service()
await Promise.all([cachingService.initialize()]).catch(error => {
logger.error('Error connecting to cachingService', error)
process.exit(1)

Choose a reason for hiding this comment

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

Problem is that while you wait for connection, your service is not exposing any endpoint. How do you deal with the cloud probes ?

test/app.js Outdated
})

it.skip('should initialize', done => {
const app = Application(config, {

Choose a reason for hiding this comment

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

can't you mock the timers ?

Comment on lines 351 to 353
debug: () => {},
error: () => {},
info: () => {}

Choose a reason for hiding this comment

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

Since it is static, why not setting those line 348 ?

@qtomlinson qtomlinson marked this pull request as draft March 13, 2025 16:34
@qtomlinson qtomlinson marked this pull request as ready for review March 14, 2025 01:41
@ArnaudBuchholz
Copy link

@qtomlinson I am not familiar enough with the project to approve all the changes but we had a great discussion on the app startup.

… middleware

Restored the integration test code for app.  The test itself is still disabled because createApp starts recursive timers, which interfere with other unit tests.
@qtomlinson
Copy link
Collaborator Author

qtomlinson commented Mar 14, 2025

@ArnaudBuchholz Thanks so much for your feedback! I really enjoyed our discussion.

@qtomlinson qtomlinson force-pushed the qt/update_ratelimit branch from fc59b7f to 357f8fa Compare March 14, 2025 19:43
@qtomlinson qtomlinson force-pushed the qt/update_ratelimit branch from 357f8fa to f101ef3 Compare March 14, 2025 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants