feat: egress tracker middleware #120

fforbeck · 2024-10-28T17:10:57Z

Egress Tracker Middleware

Summary:
This PR introduces an egress tracking middleware for the Freeway project, enabling accurate measurement and recording of egress data for served content. The middleware tracks the bytes sent in each response body and logs them with the accounting service tied to each content ID (CID).

Key Changes:

Egress Middleware (withEgressHandler):
- Wraps response handlers to track and count bytes sent to the client.
- Controlled by the FF_EGRESS_TRACKER_ENABLED feature flag, enabling or disabling egress tracking as needed. It is disabled by default.
Accounting Service Integration:
- Logs egress data with the accounting service, using either an ACCOUNTING_SERVICE from the context or a new instance based on the ACCOUNTING_SERVICE_URL environment variable.
- Egress data is linked to the CID of the served content, ensuring precise tracking. (The actual accounting service implementation, integrating w3up-client for the new usage/record capability, will follow in a separate PR.)
Efficient Byte Counting via TransformStream:
- Utilizes a TransformStream (createEgressPassThroughStream) to passively count bytes in the response body without altering content.
- On stream completion, the flush method records total egress to the accounting service using ctx.waitUntil() for non-blocking calls.

Error Handling:

Logs errors encountered during data streaming and halts byte counting without interrupting the original response chain. This ensures resilience even in cases of partial or interrupted streams.

Testing:

Added thorough tests to validate egress recording across scenarios, including complete responses, interrupted streams, and error cases.

Next Steps:

Integration tests for verifying egress tracking accuracy and accounting service interactions in various streaming conditions (planned for a future PR).
w3up-client integration to execute the new usage/record capability in subsequent development.

src/services/accounting.js

fforbeck · 2024-10-29T15:23:17Z

test/unit/middlewares/egress-tracker.spec.js

+          write: (block) => { blocks.push(block) }
+        }))
+
+      // expect(blocks[0].bytes).to.deep.equal(carBytes) - FIXME (fforbeck): how to get the correct byte count?


Still need to figure out how to check the byte count of the returned CAR file.

@Peeja / @hannahhoward, any advice on comparing the total bytes of the returned CAR file with expectedTotalBytes? I suspect I'm not converting the file correctly. Any tips would be helpful!

Peeja

Looking good! Some suggestions:

test/fixtures/worker-fixture.js

src/handlers/egress-tracker.js

src/bindings.d.ts

src/handlers/rate-limiter.js

test/unit/middlewares/egress-tracker.spec.js

hannahhoward

LGTM but agree with @Peeja 's comments

src/handlers/egress-tracker.js

waiting for another review.

Peeja

Woohoo! Looking even better!

I dug into the tests a bit more. My main concern is that they're a bit hard to follow when you don't have all the context loaded in your brain. Hopefully these suggestions improve that?

src/middleware/withEgressTracker.js

test/unit/middleware/withEgressTracker.spec.js

src/middleware/withEgressTracker.js

test/unit/middleware/withEgressTracker.spec.js

Peeja · 2024-10-30T14:48:46Z

test/unit/middleware/withEgressTracker.spec.js

+      expect(accountingRecordMethodStub.args[0][1], 'second argument should be the total bytes').to.equal(totalBytes)
+    })
+
+    it('should record egress bytes for a CAR file request', async () => {


question: Is this testing anything in particular about withEgressTracker? It seems to me like it's just the same as any wrapped response.

suggestion: If I'm not missing something here, I'd remove this test and the setup complexity that comes with it.

test/unit/middleware/withEgressTracker.spec.js

fforbeck · 2024-10-30T15:30:41Z

@Peeja - thanks for the review! Since these suggestions are test-related and don’t seem to be blockers, would you be open to merging this now and handling the remaining refactor in PR #122? (which will affect the egress handler and accounting service anyway).

Peeja

Approving so test changes can happen in #123

🤖 I have created a release *beep* *boop* --- ## [2.21.0](v2.20.2...v2.21.0) (2024-11-06) ### Features * **blob-fetcher:** use updated blob fetcher ([#124](#124)) ([90bb605](90bb605)) * egress tracker middleware ([#120](#120)) ([847829b](847829b)) * rate limiter + unit tests + readme ([#115](#115)) ([7bc4c6d](7bc4c6d)) ### Bug Fixes * **test:** enable nodejs compat for miniflare ([#127](#127)) ([0165521](0165521)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

fforbeck force-pushed the feat/egress-tracking branch 3 times, most recently from f21680e to b99bf75 Compare October 29, 2024 14:43

fforbeck commented Oct 29, 2024

View reviewed changes

src/services/accounting.js Show resolved Hide resolved

fforbeck force-pushed the feat/egress-tracking branch from b99bf75 to 232c5fc Compare October 29, 2024 15:22

fforbeck commented Oct 29, 2024

View reviewed changes

fforbeck marked this pull request as ready for review October 29, 2024 15:42

fforbeck requested review from Peeja, alanshaw and travis October 29, 2024 15:42

fforbeck changed the title ~~feat: egress tracking~~ feat: egress tracker middleware Oct 29, 2024

Peeja previously requested changes Oct 29, 2024

View reviewed changes

hannahhoward approved these changes Oct 29, 2024

View reviewed changes

src/handlers/egress-tracker.js Outdated Show resolved Hide resolved

src/handlers/egress-tracker.js Outdated Show resolved Hide resolved

fforbeck force-pushed the feat/egress-tracking branch 2 times, most recently from 6bad2ac to 67cb972 Compare October 29, 2024 19:45

feat: count egress bytes

b6fffc0

fforbeck force-pushed the feat/egress-tracking branch from 67cb972 to f0fca41 Compare October 30, 2024 12:25

fforbeck requested a review from Peeja October 30, 2024 12:26

fforbeck force-pushed the feat/egress-tracking branch from f0fca41 to 50383a6 Compare October 30, 2024 12:50

reviewer suggestions implemented

25982f4

fforbeck force-pushed the feat/egress-tracking branch from 50383a6 to 25982f4 Compare October 30, 2024 12:56

Peeja requested changes Oct 30, 2024

View reviewed changes

Peeja approved these changes Oct 30, 2024

View reviewed changes

fforbeck merged commit 847829b into main Oct 30, 2024

fforbeck deleted the feat/egress-tracking branch October 30, 2024 15:39

github-actions bot mentioned this pull request Oct 30, 2024

chore(main): release 2.21.0 #117

Merged

fforbeck mentioned this pull request Nov 6, 2024

Update the Gateway to track egress [Accounting Service] storacha/project-tracking#154

Closed

15 tasks

feat: egress tracker middleware #120

feat: egress tracker middleware #120

Uh oh!

Conversation

fforbeck commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Egress Tracker Middleware

Uh oh!

Uh oh!

fforbeck Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

fforbeck Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

Peeja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hannahhoward left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Peeja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Peeja Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fforbeck commented Oct 30, 2024

Uh oh!

Peeja left a comment • edited by fforbeck Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fforbeck commented Oct 28, 2024 •

edited

Loading

Peeja left a comment •

edited by fforbeck

Loading