feat: improved metrics and added rate limit for the batch request feature in ws server (#2474) #2487

quiet-node · 2024-05-13T19:08:42Z

Description:

added shouldRateLimitOnMethod() function to ConnectionLimiter class which utilizes the shouldRateLimit() method from RateLimit class to apply rate limit logic on both batch requests and single requests in the WS server
added methodsCounter metric for batch_requests methods
added a list of acceptancetest for rateLimit in the WS server
modified acceptancetest:ratelimiter script to run tests with @web-socket-ratelimiter tag. This change will add the new acceptancetest to the ratelimiter CI workflow

Metrics result:
**Startup:

** After multiple batch requests calls:

Fixes #2474

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

github-actions · 2024-05-13T19:40:15Z

Acceptance Tests

  18 files 255 suites 30m 25s ⏱️
589 tests 581 ✔️ 3 💤 5 ❌
843 runs 830 ✔️ 7 💤 6 ❌

Results for commit 1713c0f.

♻️ This comment has been updated with latest results.

github-actions · 2024-05-13T22:58:14Z

Tests

    2 files 147 suites 13s ⏱️
819 tests 818 ✔️ 1 💤 0 ❌
831 runs 830 ✔️ 1 💤 0 ❌

Results for commit 1713c0f.

♻️ This comment has been updated with latest results.

Nana-EC

LG.
Missing acceptance tests

packages/ws-server/src/webSocketServer.ts

AlfredoG87

as @Nana-EC pointed is missing UT for rate limits.

also, atm we don't do automated tests for metrics but we usually add a screenshot of the /metrics endpoint where you can see the metrics added with some values on Notes for reviewer: section of the PR description.

quiet-node · 2024-05-14T19:39:27Z

as @Nana-EC pointed is missing UT for rate limits.

also, atm we don't do automated tests for metrics but we usually add a screenshot of the /metrics endpoint where you can see the metrics added with some values on Notes for reviewer: section of the PR description.

added prrof for metrics

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

sonarqubecloud · 2024-05-15T22:36:10Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

AlfredoG87

LGTM

Nana-EC · 2024-05-16T14:51:49Z

.github/workflows/acceptance.yml

@@ -47,6 +47,7 @@ jobs:
    uses: ./.github/workflows/acceptance-workflow.yml
    with:
      testfilter: ratelimiter
+      test_ws_server: true


Q: what's this config and why is it just for ws.
Shouldn't we test all the time or if there is a config shouldn't there be on for the HTTP and the WS server each?

Q: what's this config and why is it just for ws.

This config sets the process.env.TEST_WS_SERVER variable in the CI job. The test program uses this TEST_WS_SERVER environment variable to determine whether to run the WS server, as shown here.

Shouldn't we test all the time or if there is a config shouldn't there be on for the HTTP and the WS server each?
We didn't have tests for rate limiting on the WS server until this PR. Previously, we only had them for the HTTP server. That's why this workflow has been running exclusively against the HTTP server.

…ture in ws server (#2474) (#2487) * feat: added shouldRateLimitOnMethod() to connectionLimiter Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * feat: applied shouldRateLimitOnMethod() on batch requests Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * feat: applied shouldRateLimitOnMethod() on single requests Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * feat: added methodsCounter metric for batch_requests Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * fix: skipped rateLimit for eth_subscribe and eth_unsubscribe Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * test: added acceptancetest for rate limiter in the WS Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * test: enabled test_ws_server in ratelimiter CI Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

feat: improved metrics and added rate limit for the batch request feature in ws server (#2474) (#2487) * feat: added shouldRateLimitOnMethod() to connectionLimiter * feat: applied shouldRateLimitOnMethod() on batch requests * feat: applied shouldRateLimitOnMethod() on single requests * feat: added methodsCounter metric for batch_requests * fix: skipped rateLimit for eth_subscribe and eth_unsubscribe * test: added acceptancetest for rate limiter in the WS * test: enabled test_ws_server in ratelimiter CI --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

quiet-node added the enhancement New feature or request label May 13, 2024

quiet-node self-assigned this May 13, 2024

quiet-node requested review from AlfredoG87, ebadiere, georgi-l95, Ivo-Yankov and a team as code owners May 13, 2024 19:08

quiet-node linked an issue May 13, 2024 that may be closed by this pull request

Improve metrics and add rate limit for the batch request feature in WS server #2474

Closed

quiet-node force-pushed the 2474-improve-metrics-and-add-rate-limit-for-the-batch-request-feature-in-ws-server branch from 128d723 to bc20cf4 Compare May 13, 2024 22:53

Nana-EC requested changes May 13, 2024

View reviewed changes

packages/ws-server/src/webSocketServer.ts Show resolved Hide resolved

AlfredoG87 requested changes May 14, 2024

View reviewed changes

quiet-node force-pushed the 2474-improve-metrics-and-add-rate-limit-for-the-batch-request-feature-in-ws-server branch 3 times, most recently from 2bdf561 to 293328b Compare May 14, 2024 17:57

quiet-node marked this pull request as draft May 14, 2024 18:17

quiet-node force-pushed the 2474-improve-metrics-and-add-rate-limit-for-the-batch-request-feature-in-ws-server branch 3 times, most recently from 49d491e to 44ba45d Compare May 14, 2024 19:06

quiet-node marked this pull request as ready for review May 14, 2024 19:39

quiet-node requested review from Nana-EC and AlfredoG87 May 14, 2024 19:39

quiet-node added 6 commits May 15, 2024 18:34

feat: added shouldRateLimitOnMethod() to connectionLimiter

a1f79fc

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

feat: applied shouldRateLimitOnMethod() on batch requests

3fbabdd

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

feat: applied shouldRateLimitOnMethod() on single requests

f534244

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

feat: added methodsCounter metric for batch_requests

08cc182

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

fix: skipped rateLimit for eth_subscribe and eth_unsubscribe

79b678c

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

test: added acceptancetest for rate limiter in the WS

770cc35

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

test: enabled test_ws_server in ratelimiter CI

1713c0f

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

quiet-node force-pushed the 2474-improve-metrics-and-add-rate-limit-for-the-batch-request-feature-in-ws-server branch from 44ba45d to 1713c0f Compare May 15, 2024 22:35

AlfredoG87 approved these changes May 16, 2024

View reviewed changes

Nana-EC approved these changes May 16, 2024

View reviewed changes

quiet-node merged commit 94bf628 into main May 16, 2024
32 of 33 checks passed

quiet-node deleted the 2474-improve-metrics-and-add-rate-limit-for-the-batch-request-feature-in-ws-server branch May 16, 2024 15:45

quiet-node mentioned this pull request May 21, 2024

chore: cherry picked 2487 #2518

Merged

2 tasks

quiet-node added this to the 0.48.0 milestone May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improved metrics and added rate limit for the batch request feature in ws server (#2474) #2487

feat: improved metrics and added rate limit for the batch request feature in ws server (#2474) #2487

quiet-node commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading

Nana-EC left a comment

AlfredoG87 left a comment •

edited

Loading

quiet-node commented May 14, 2024

sonarqubecloud bot commented May 15, 2024

AlfredoG87 left a comment

Nana-EC May 16, 2024

quiet-node May 16, 2024

feat: improved metrics and added rate limit for the batch request feature in ws server (#2474) #2487

feat: improved metrics and added rate limit for the batch request feature in ws server (#2474) #2487

Conversation

quiet-node commented May 13, 2024 • edited Loading

github-actions bot commented May 13, 2024 • edited Loading

Acceptance Tests

github-actions bot commented May 13, 2024 • edited Loading

Tests

Nana-EC left a comment

Choose a reason for hiding this comment

AlfredoG87 left a comment • edited Loading

Choose a reason for hiding this comment

quiet-node commented May 14, 2024

sonarqubecloud bot commented May 15, 2024

Quality Gate passed

AlfredoG87 left a comment

Choose a reason for hiding this comment

Nana-EC May 16, 2024

Choose a reason for hiding this comment

quiet-node May 16, 2024

Choose a reason for hiding this comment

quiet-node commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading

AlfredoG87 left a comment •

edited

Loading