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

feat: use keep alive connections #166

Merged
merged 3 commits into from
Sep 11, 2023
Merged

feat: use keep alive connections #166

merged 3 commits into from
Sep 11, 2023

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Sep 8, 2023

🧿 Current issues / What's wrong ?

There's a lot of recurring connections to the same services, at a high rate.

  • Sentry via SentryTunnel
  • Snapshot-hub via the votes CSV generation

Opening a new connection on each request is costly, and create overhead, particularly on frequently requested url

💊 Fixes / Solution

We should reuse the connection when possible, by using a keep alive connection.

Fix #167
Fix #168
Fix SNAPSHOT-SIDEKICK-2
Fix SNAPSHOT-SIDEKICK-E

🚧 Changes

  • Use node-fetch instead of cross-fetch (don't need browser support)
  • Add a wrapper function to node-fetch, injecting keep-alive connection by using a custom agent.
  • Use that custom fetch function whenever possible

🛠️ Tests

  • Run yarn test:integration
  • Not sure if HTTP interceptor can detect the keep-alive header correctly, but you can also try intercepting a request, such as curl -X POST localhost:3005/api/votes/0xa34107e34b4dc4ff4cd16b77d66e62a51f4d35457a4f4b1f68ab8ac821f58561

@wa0x6e wa0x6e requested review from Todmy and ChaituVR September 8, 2023 16:40
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Patch coverage: 72.00% and project coverage change: +0.39% 🎉

Comparison is base (417f011) 55.41% compared to head (17902d4) 55.80%.

❗ Current head 17902d4 differs from pull request most recent head 6727a5b. Consider uploading reports for the commit 6727a5b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   55.41%   55.80%   +0.39%     
==========================================
  Files          19       19              
  Lines        1570     1584      +14     
  Branches      111      114       +3     
==========================================
+ Hits          870      884      +14     
  Misses        691      691              
  Partials        9        9              
Files Changed Coverage Δ
src/lib/metrics/digitalOcean.ts 0.00% <0.00%> (ø)
src/sentryTunnel.ts 0.00% <0.00%> (ø)
src/helpers/snapshot.ts 91.71% <100.00%> (ø)
src/helpers/utils.ts 57.62% <100.00%> (+13.18%) ⬆️
src/lib/nftClaimer/utils.ts 64.05% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Todmy Todmy left a comment

Choose a reason for hiding this comment

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

utACK

@wa0x6e wa0x6e merged commit 696f511 into main Sep 11, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants