feat: report server-accepted upload bytes in results#121
feat: report server-accepted upload bytes in results#121devandrepascoa wants to merge 2 commits into
Conversation
Capture the cf-meta-upload-bytes response header on upload requests and surface it through the bandwidth measurement results. The final results payload now reports the server-accepted upload size per point instead of the client-requested size, falling back to the requested size when the header is absent. Downloads are unaffected.
There was a problem hiding this comment.
Summary
This PR captures the cf-meta-upload-bytes response header on upload requests and surfaces the server-accepted upload size through the measurement results.
Changes:
LoggingBandwidthEnginegains two exported helpers,parseUploadBytesHeader(validates and parses the header into a safe integer) andgetLoggedBytes(chooses accepted vs. requested bytes for logging). The response hook now records#uploadByteson every response, independent of per-measurement logging, and#applyUploadBytesattaches it to upload measurement results.BandwidthMeasurementTiming/BandwidthTiming/BandwidthPointgain an optionaluploadBytesfield, threaded throughgetBandwidthPoints.- The final AIM results payload (
bpsPointsParser) reports the server-accepted upload size per point, falling back to the requested size when the header is absent. Downloads are unaffected. - Adds unit tests for
parseUploadBytesHeader/getLoggedBytesand agetBandwidthPointstest.
Verification: pnpm test:unit (99 passed) and pnpm lint both pass locally on the PR head.
The implementation is clean and well-tested. The shared-mutable-state approach (#uploadBytes on the instance, read in a deferred setTimeout) is safe in practice here because bandwidth requests in this engine are strictly sequential and the setTimeout(0) result callback fires long before the next request's network response. I left a few inline notes — the main one worth discussing is the bytes/bps consistency in the final payload.
| const bpsPointsParser: ParserFn = pnts => | ||
| (pnts as BandwidthPoint[]).map(d => ({ | ||
| bytes: +d.bytes, | ||
| bytes: d.uploadBytes ?? +d.bytes, |
There was a problem hiding this comment.
Correctness / semantics: bytes and bps become internally inconsistent.
Here bytes is reported as the server-accepted size (uploadBytes), but bps (from d.bps) was computed by calcUploadSpeed in BandwidthEngine.ts using the client-requested numBytes, not the accepted size. When the server caps the upload (the whole motivation for cf-meta-upload-bytes), the emitted pair no longer reconciles: a consumer estimating bps ≈ bytes * 8 / duration would get a different number than the reported bps.
Worth confirming this is intended. If the goal is accurate accepted-size throughput, bps should probably be recomputed from uploadBytes; otherwise consider keeping bps derived from the same bytes value you report.
| #loggingResponseHook(r: ResponseHookPayload): void { | ||
| // Capture server-accepted upload bytes regardless of per-measurement | ||
| // logging, so the final results payload can report actual uploaded sizes. | ||
| this.#uploadBytes = parseUploadBytesHeader(r.headers); |
There was a problem hiding this comment.
Shared mutable state via #uploadBytes — fragile, please add a guarding comment or assertion.
#uploadBytes is set here synchronously in the response hook, but it is read later in #applyUploadBytes / #logMeasurement, which run inside the setTimeout(...)-deferred onMeasurementResult callback (see BandwidthEngine.#saveMeasurementResults). This is correct today only because bandwidth requests in this engine are strictly sequential and the setTimeout(0) result callback fires well before the next request's network response can overwrite #uploadBytes.
This is an implicit invariant. If request pipelining/concurrency is ever added to BandwidthEngine, the value captured here could be clobbered by a later request before the deferred callback reads it, silently attaching the wrong uploadBytes to a measurement. Consider attaching the parsed value to the per-request flow (e.g. carry it through the timing object) instead of an instance field, or at minimum document this ordering dependency.
| headers: Headers | ||
| ): number | undefined => { | ||
| const value = headers.get('cf-meta-upload-bytes'); | ||
| if (!value || !/^\d+$/.test(value)) return undefined; |
There was a problem hiding this comment.
Minor: parseUploadBytesHeader rejects 0 is fine, but note that Number.isSafeInteger already returns false for values exceeding 2^53-1, so the /^\d+$/ regex is the only guard against arbitrarily large strings — good. One edge case to confirm: a value of '0' parses to 0, which is falsy, so getLoggedBytes/#applyUploadBytes (uploadBytes !== undefined) would still treat an accepted size of 0 as valid and report bytes: 0. If the server can legitimately send 0, that's correct; if 0 should mean "no cap", this would mis-report. Worth a quick confirmation.
| serverTime, | ||
| transferSize | ||
| transferSize, | ||
| uploadBytes |
There was a problem hiding this comment.
Minor/style: uploadBytes is now spread onto every bandwidth point, including download points where it is always undefined. That's harmless (optional field), but it does add an explicit uploadBytes: undefined key to download point objects. If you want download points to stay shape-identical to before, you could conditionally include it. Non-blocking.
|
Review posted to PR #121. Here's a summary of my review. What the PR changesIt captures the
Issues flagged (inline)
No security issues found. The change is well-tested and otherwise clean. No commits pushed. |
Adds a browser e2e (real SpeedTest engine + local mock) proving the /__log and /__results payloads report cf-meta-upload-bytes, not the upload Content-Length. The mock forces the accepted size below the body size so the assertion can only pass if the client uses the header; a second case covers the fallback when the header is absent. Also disables the browser-compat lint rule for tests/, which run in Node and headless Chromium rather than the library's target browsers.
| ...CORS, | ||
| 'content-type': 'application/octet-stream' | ||
| }); | ||
| res.end(Buffer.alloc(size, 0x30)); |
Captures the
cf-meta-upload-bytesresponse header on upload requests and surfaces the server-accepted upload size through the measurement results.LoggingBandwidthEnginereadscf-meta-upload-byteson every response (independent of per-measurement logging) and attaches it to the upload measurement result.Results/getBandwidthPoints.Depends conceptually on the server returning
cf-meta-upload-bytes; works regardless of whether the client sendsbytes=on uploads (see #120).