feat(ai): Set a rate limit for vector store rebuild #9404

miya · 2024-11-14T04:21:50Z

Task

#156796 [RAG] rebuild 時 file をアップロードする際に rate limit を考慮した実装にできる
- #157022 POST /_api/v3/openai/rebuild-vector-store に対して rate limit を設定できる

changeset-bot · 2024-11-14T04:21:53Z

⚠️ No Changeset found

Latest commit: 0914f31

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

…ld-api' into feat/157512-set-a-rate-limit-for-vector-store-rebuild

miya · 2024-11-17T07:15:35Z

apps/app/src/features/rate-limiter/middleware/factory.ts

@@ -19,7 +19,7 @@ const logger = loggerFactory('growi:middleware:api-rate-limit');
 // API_RATE_LIMIT_010_FOO_METHODS=GET,POST
 // API_RATE_LIMIT_010_FOO_MAX_REQUESTS=10

-const POINTS_THRESHOLD = 100;
+export const POINTS_THRESHOLD = 100;


test で利用するために export

このままだと結局 index.tx によって外部に export されてしまうので、factory.ts から外に出してください

修正しました

miya · 2024-11-17T07:15:40Z

apps/app/src/features/rate-limiter/middleware/factory.ts

@@ -37,9 +37,9 @@ const keysWithRegExp = Object.keys(configWithRegExp).map(key => new RegExp(`^${k
 const valuesWithRegExp = Object.values(configWithRegExp);


-const _consumePoints = async(
+export const _consumePoints = async(


test で利用するために export

こちらも同様、このままだと結局 index.tx によって外部に export されてしまうので、factory.ts から外に出してください

修正しました

miya · 2024-11-17T07:18:47Z

apps/app/src/features/rate-limiter/middleware/factory.ts

-  const consumePoints = (POINTS_THRESHOLD + 0.0001) / maxRequests;
-  await rateLimiter.consume(key, consumePoints);
+  const consumePoints = POINTS_THRESHOLD / maxRequests;
+  const rateLimiterRes = await rateLimiter.consume(key, consumePoints);


maxRequest より 1 回少ない request 数で rate limit になってしまっていたので修正

yuki-takei · 2024-11-18T08:42:55Z

apps/app/src/features/rate-limiter/middleware/factory.ts

@@ -19,7 +19,7 @@ const logger = loggerFactory('growi:middleware:api-rate-limit');
 // API_RATE_LIMIT_010_FOO_METHODS=GET,POST
 // API_RATE_LIMIT_010_FOO_MAX_REQUESTS=10

-const POINTS_THRESHOLD = 100;
+export const POINTS_THRESHOLD = 100;


このままだと結局 index.tx によって外部に export されてしまうので、factory.ts から外に出してください

yuki-takei · 2024-11-18T08:43:04Z

apps/app/src/features/rate-limiter/middleware/factory.ts

@@ -37,9 +37,9 @@ const keysWithRegExp = Object.keys(configWithRegExp).map(key => new RegExp(`^${k
 const valuesWithRegExp = Object.values(configWithRegExp);


-const _consumePoints = async(
+export const _consumePoints = async(


こちらも同様、このままだと結局 index.tx によって外部に export されてしまうので、factory.ts から外に出してください

yuki-takei · 2024-11-18T08:54:30Z

apps/app/src/features/rate-limiter/middleware/factory.spec.ts

意味のあるテストにしないといけない

unit test からテスト用 MongoDB と連携させた rate-limiter-flexible をテストする integration test へ

境界値テストにしないといけない

意味がある境界 = 500, 501 の間 (maxRequests=500 の時)

@faker-js/faker も使う

意味がある境界 = n, n+1 の間 (maxRequests=n の時)

修正しました

miya · 2024-11-21T10:19:54Z

apps/app/src/features/rate-limiter/middleware/consume-points.ts

+  rateLimiter.points = maxRequests;
+  const rateLimiterRes = await rateLimiter.consume(key, 1);
+  return rateLimiterRes;
+};


features dir 外から import できないように外部モジュール化した

miya · 2024-11-21T10:37:30Z

apps/app/src/features/rate-limiter/middleware/consume-points.ts

+  }
+
+  rateLimiter.points = maxRequests;
+  const rateLimiterRes = await rateLimiter.consume(key, 1);


機能改修はここのみ

Before

// because the maximum request is reduced by 1 if it is divisible by // https://github.com/weseek/growi/pull/6225 const consumePoints = (POINTS_THRESHOLD + 0.0001) / maxRequests; await rateLimiter.consume(key, consumePoints);

maxRequest: 1 の場合 2 回目の request で rate limit error べきだが、 (100 + 0.0001) / 1 = 100.0001 になるので1回目の request で rate limit error になる。

Before (この PR の当初の改修)

const consumePoints = POINTS_THRESHOLD / maxRequests; const rateLimiterRes = await rateLimiter.consume(key, consumePoints);

maxRequest: 500 の場合 100 / 500 = 0.2 になる。0.2 * 500 で 100 (閾値) に達するので良いと思っていた。しかし 1回の request で consumedPoints (消費ポイント) が 0.2 として保存される。2回目の request で consumedPoints が 0.4 として保存される。3回目の request で consumedPoints が 0.6000000000000001 として保存されれ、期待する通りに動かなかったためこれも却下になった。

参考

https://qiita.com/kihara-takahiro/items/4518902225304f4e15b6#%E5%B0%8F%E6%95%B0%E7%82%B9%E3%81%AE%E5%8F%96%E3%82%8A%E6%89%B1%E3%81%84%E6%B3%A8%E6%84%8F

After (最終的に)

rateLimiter.points = maxRequests; const rateLimiterRes = await rateLimiter.consume(key, 1);

RateLimiterMongo 初期化時には point の閾値を設定せず、request される度に更新するように変更した。point 消費は 1 (整数) で固定するように変更した。

miya · 2024-11-21T11:53:26Z

apps/app/src/features/rate-limiter/middleware/consume-points.integ.ts

+const testRateLimitErrorWhenExceedingMaxRequests = async(method: string, key: string, maxRequests: number): Promise<void> => {
+  // dynamic import is used because rateLimiterMongo needs to be initialized after connecting to DB
+  // Issue: https://github.com/animir/node-rate-limiter-flexible/issues/216
+  const { rateLimiter } = await import('./rate-limiter-mongo-client');


DB との connection が確立する前に RateLimiterMongo が初期化されると issue 記載のエラーになるため、RateLimiterMongo を初期化するモジュールを dynamic import している (issue の解法とは異なる)。

Issue comment

animir/node-rate-limiter-flexible#216 (comment)

my solution to this problem is to simply wait for the database to connect before creating a RateLimitMongo instance

miya · 2024-11-21T11:54:11Z

apps/app/src/features/rate-limiter/middleware/consume-points.integ.ts

+      const res = await consumePoints(rateLimiter, method, key, { method, maxRequests });
+      if (count === maxRequests) {
+        // Expect consumedPoints to be equal to maxRequest when maxRequest is reached
+        expect(res?.consumedPoints).toBe(maxRequests);


maxRequest 回目で consumedPoints (消費ポイント) が maxRequest と等しくなることを期待

miya · 2024-11-21T11:54:24Z

apps/app/src/features/rate-limiter/middleware/consume-points.integ.ts

+        // Expect consumedPoints to be equal to maxRequest when maxRequest is reached
+        expect(res?.consumedPoints).toBe(maxRequests);
+        // Expect remainingPoints to be 0 when maxRequest is reached
+        expect(res?.remainingPoints).toBe(0);


maxRequest 回目で remainingPoints (残ポイント) が 0 になることを期待

miya · 2024-11-21T11:54:40Z

apps/app/src/features/rate-limiter/middleware/consume-points.integ.ts

+        expect(res?.remainingPoints).toBe(0);
+      }
+      if (count > maxRequests) {
+        throw new Error('Exception occurred');


maxRequest を超過しても rate limit error にならなかった場合 error を throw する

miya · 2024-11-21T11:54:47Z

apps/app/src/features/rate-limiter/middleware/consume-points.integ.ts

+  }
+  catch (err) {
+    // Expect not to exceed maxRequest
+    expect(err.message).not.toBe('Exception occurred');


maxRequest 超過時に rate limit error が throw されていることを期待

miya · 2024-11-21T11:55:00Z

apps/app/src/features/rate-limiter/middleware/consume-points.integ.ts

+    // Expect not to exceed maxRequest
+    expect(err.message).not.toBe('Exception occurred');
+    // Expect rate limit error at maxRequest + 1
+    expect(count).toBe(maxRequests + 1);


(maxRequest + 1) 回目の時点で rate limit error になっていることを期待

yuki-takei · 2024-11-22T12:03:43Z

apps/app/src/features/rate-limiter/middleware/factory.ts

factory で RateLimiterMongo をインスタンス化することで 1 instance per endpoint になる設計だったのだが、この PR では rate-limiter-mongo-client.ts を用意していることにより singleton になっている。
テストでは問題ないのかもしれないが、実環境では多くのリクエストが同時にやってくる中、points を都度上書きするような実装で問題はでないのかという心配がある。

factory で RateLimiterMongo をインスタンス化することで 1 instance per endpoint になる設計だったのだが

1 instance per endpoint (key) になるような実装に修正しました

実環境では多くのリクエストが同時にやってくる中、points を都度上書きするような実装で問題はでないのかという心配がある。

確かに調べてみるとこの実装には race condition というリスクがあるみたいでした。

miya · 2024-11-24T11:17:03Z

apps/app/src/features/rate-limiter/middleware/rate-limiter-factory.ts

+    this.rateLimiters.set(key, rateLimiter);
+
+    return rateLimiter;
+  }


key ごとに独立したインスタンスを作成できるようにした

Add config

0b480db

miya requested a review from yuki-takei November 14, 2024 04:21

miya self-assigned this Nov 14, 2024

github-actions bot added the type/feature label Nov 14, 2024

miya added 5 commits November 16, 2024 09:01

Add test

3874e1f

Return the result of rateLimiter.consume (for debug)

beb8397

bugFIx

9a57cd5

Add endpoint

eed5bc8

Merge branch 'feat/157512-add-rate-limiting-to-the-vector-store-rebui…

baf1cf5

…ld-api' into feat/157512-set-a-rate-limit-for-vector-store-rebuild

miya commented Nov 17, 2024

View reviewed changes

miya changed the title ~~feat: Set a rate limit for vector store rebuild~~ feat(ai): Set a rate limit for vector store rebuild Nov 17, 2024

yuki-takei requested changes Nov 18, 2024

View reviewed changes

miya added 10 commits November 18, 2024 09:48

Change to integration testing

4612eeb

test

4b62d1b

Cleanup

5a98dc9

cleanup test

6178450

cleanup test

f8e2506

Modified to set a threshold for each request

bca807e

fix test

b6c8644

fix test

bfd378c

Separate rate-limiter-flexible client and wrapper in separate files

1d570c1

fix comment

f8d97f2

miya commented Nov 21, 2024

View reviewed changes

miya added 2 commits November 21, 2024 10:42

fix comment

3aa1ef9

middleware/factory.integ.ts -> middleware/consume-points.integ.ts

4115f6f

miya requested a review from yuki-takei November 21, 2024 11:51

fix commnent

1e94b7d

miya commented Nov 21, 2024

View reviewed changes

clean code

c7dada2

yuki-takei requested changes Nov 22, 2024

View reviewed changes

miya added 3 commits November 24, 2024 11:02

1 endpoint per 1 instance

5a64143

fix test

891506f

Refactoring of RateLimiterFactory

4a28490

miya commented Nov 24, 2024

View reviewed changes

miya requested a review from yuki-takei November 24, 2024 11:40

miya added 2 commits November 25, 2024 02:01

1 instance per key

a91a192

fix config

0914f31

yuki-takei approved these changes Nov 25, 2024

View reviewed changes

mergify bot added a commit that referenced this pull request Nov 25, 2024

Merge of #9404

1e7757d

mergify bot mentioned this pull request Nov 25, 2024

merge queue: embarking master (0c32e88) and #9404 together #9437

Closed

15 tasks

mergify bot merged commit a120c07 into master Nov 25, 2024
22 checks passed

mergify bot deleted the feat/157512-set-a-rate-limit-for-vector-store-rebuild branch November 25, 2024 09:27

github-actions bot mentioned this pull request Nov 25, 2024

Release v7.1.3 #9423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ai): Set a rate limit for vector store rebuild #9404

feat(ai): Set a rate limit for vector store rebuild #9404

miya commented Nov 14, 2024 •

edited

Loading

changeset-bot bot commented Nov 14, 2024 •

edited

Loading

miya Nov 17, 2024

yuki-takei Nov 18, 2024

miya Nov 21, 2024

miya Nov 17, 2024

yuki-takei Nov 18, 2024

miya Nov 21, 2024

miya Nov 17, 2024

yuki-takei Nov 18, 2024

yuki-takei Nov 18, 2024

yuki-takei Nov 18, 2024 •

edited

Loading

miya Nov 21, 2024

miya Nov 21, 2024

miya Nov 21, 2024 •

edited

Loading

miya Nov 21, 2024 •

edited

Loading

miya Nov 21, 2024 •

edited

Loading

miya Nov 21, 2024 •

edited

Loading

miya Nov 21, 2024

miya Nov 21, 2024

miya Nov 21, 2024

yuki-takei Nov 22, 2024

miya Nov 24, 2024 •

edited

Loading

miya Nov 24, 2024 •

edited

Loading

feat(ai): Set a rate limit for vector store rebuild #9404

feat(ai): Set a rate limit for vector store rebuild #9404

Conversation

miya commented Nov 14, 2024 • edited Loading

Task

changeset-bot bot commented Nov 14, 2024 • edited Loading

⚠️ No Changeset found

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuki-takei Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miya Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Before

Before (この PR の当初の改修)

参考

After (最終的に)

miya Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Issue comment

miya Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

miya Nov 21, 2024 • edited Loading

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

miya Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

miya Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

miya commented Nov 14, 2024 •

edited

Loading

changeset-bot bot commented Nov 14, 2024 •

edited

Loading

yuki-takei Nov 18, 2024 •

edited

Loading

miya Nov 21, 2024 •

edited

Loading

miya Nov 21, 2024 •

edited

Loading

miya Nov 21, 2024 •

edited

Loading

miya Nov 21, 2024 •

edited

Loading

miya Nov 24, 2024 •

edited

Loading

miya Nov 24, 2024 •

edited

Loading