-
Notifications
You must be signed in to change notification settings - Fork 78
feat: report server-accepted upload bytes in results #121
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,25 @@ import type { | |
| } from './BandwidthEngine'; | ||
| import type { ParallelLatencyOptions } from './ParallelLatency'; | ||
|
|
||
| export const parseUploadBytesHeader = ( | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: |
||
|
|
||
| const bytes = Number(value); | ||
| return Number.isSafeInteger(bytes) ? bytes : undefined; | ||
| }; | ||
|
|
||
| /** Upload logs use server-accepted bytes when the server reports a cap. */ | ||
| export const getLoggedBytes = ( | ||
| measData: Pick<BandwidthTimingResult, 'type' | 'bytes'>, | ||
| uploadBytes: number | undefined | ||
| ): number => | ||
| measData.type === 'up' && uploadBytes !== undefined | ||
| ? uploadBytes | ||
| : measData.bytes; | ||
|
|
||
| export interface LoggingBandwidthEngineOptions extends ParallelLatencyOptions { | ||
| measurementId?: string; | ||
| logApiUrl?: string; | ||
|
|
@@ -38,8 +57,10 @@ class LoggingBandwidthEngine extends BandwidthEngine { | |
| super.qsParams = logApiUrl ? { measId: this.#measurementId! } : {}; | ||
| super.responseHook = (r: ResponseHookPayload) => | ||
| this.#loggingResponseHook(r); | ||
| super.onMeasurementResult = (meas: BandwidthTimingResult) => | ||
| super.onMeasurementResult = (meas: BandwidthTimingResult) => { | ||
| this.#applyUploadBytes(meas); | ||
| this.#logMeasurement(meas); | ||
| }; | ||
| } | ||
|
|
||
| // Overridden attributes | ||
|
|
@@ -66,6 +87,7 @@ class LoggingBandwidthEngine extends BandwidthEngine { | |
| meas: BandwidthTimingResult, | ||
| ...restArgs: [BandwidthEngineResults] | ||
| ) => { | ||
| this.#applyUploadBytes(meas); | ||
| onMeasurementResult(meas, ...restArgs); | ||
| this.#logMeasurement(meas); | ||
| }; | ||
|
|
@@ -75,11 +97,27 @@ class LoggingBandwidthEngine extends BandwidthEngine { | |
| #measurementId: string | undefined; | ||
| #token: string | null | undefined; | ||
| #requestTime: number | null | undefined; | ||
| #uploadBytes: number | undefined; | ||
| #logApiUrl: string | undefined; | ||
| #sessionId: string | undefined; | ||
|
|
||
| // Internal methods | ||
|
|
||
| /** | ||
| * Records server-accepted upload bytes on the measurement result so the | ||
| * final results payload can report the actual uploaded size (uploads only). | ||
| */ | ||
| #applyUploadBytes(measData: BandwidthTimingResult): void { | ||
| if (measData.type === 'up' && this.#uploadBytes !== undefined) { | ||
| measData.uploadBytes = this.#uploadBytes; | ||
| } | ||
| } | ||
|
|
||
| #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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shared mutable state via
This is an implicit invariant. If request pipelining/concurrency is ever added to |
||
|
|
||
| if (!this.#logApiUrl) return; | ||
|
|
||
| // get request time | ||
|
|
@@ -94,7 +132,7 @@ class LoggingBandwidthEngine extends BandwidthEngine { | |
|
|
||
| const logData = { | ||
| type: measData.type, | ||
| bytes: measData.bytes, | ||
| bytes: getLoggedBytes(measData, this.#uploadBytes), | ||
| ping: Math.round(measData.ping), // round to ms | ||
| ttfb: Math.round(measData.ttfb), // round to ms | ||
| payloadDownloadTime: Math.round(measData.payloadDownloadTime), | ||
|
|
@@ -109,6 +147,7 @@ class LoggingBandwidthEngine extends BandwidthEngine { | |
|
|
||
| this.#token = null; | ||
| this.#requestTime = null; | ||
| this.#uploadBytes = undefined; | ||
|
|
||
| fetch(this.#logApiUrl, { | ||
| method: 'POST', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,10 +50,14 @@ const round = ( | |
| const latencyPointsParser: ParserFn = durations => | ||
| (durations as number[]).map(d => round(d, 2)); | ||
|
|
||
| /** Extracts bytes and rounded bps from each bandwidth data point. */ | ||
| /** | ||
| * Extracts bytes and rounded bps from each bandwidth data point. For uploads, | ||
| * `bytes` is the server-accepted size (`cf-meta-upload-bytes`) when reported, | ||
| * falling back to the requested size; downloads always use the requested size. | ||
| */ | ||
| const bpsPointsParser: ParserFn = pnts => | ||
| (pnts as BandwidthPoint[]).map(d => ({ | ||
| bytes: +d.bytes, | ||
| bytes: d.uploadBytes ?? +d.bytes, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correctness / semantics: Here Worth confirming this is intended. If the goal is accurate accepted-size throughput, |
||
| bps: round(d.bps) | ||
| })); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import { | ||
| getLoggedBytes, | ||
| parseUploadBytesHeader | ||
| } from '../../../src/engines/BandwidthEngine/LoggingBandwidthEngine.ts'; | ||
|
|
||
| describe('LoggingBandwidthEngine', () => { | ||
| describe('parseUploadBytesHeader', () => { | ||
| it('reads a valid cf-meta-upload-bytes header', () => { | ||
| const headers = new Headers({ 'cf-meta-upload-bytes': '4000000000' }); | ||
|
|
||
| expect(parseUploadBytesHeader(headers)).toBe(4000000000); | ||
| }); | ||
|
|
||
| it('ignores missing or invalid cf-meta-upload-bytes headers', () => { | ||
| expect(parseUploadBytesHeader(new Headers())).toBeUndefined(); | ||
| expect( | ||
| parseUploadBytesHeader(new Headers({ 'cf-meta-upload-bytes': '1e3' })) | ||
| ).toBeUndefined(); | ||
| expect( | ||
| parseUploadBytesHeader(new Headers({ 'cf-meta-upload-bytes': '-1' })) | ||
| ).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getLoggedBytes', () => { | ||
| it('uses server-reported upload bytes for upload measurements', () => { | ||
| expect( | ||
| getLoggedBytes({ type: 'up', bytes: 5000000000 }, 4000000000) | ||
| ).toBe(4000000000); | ||
| }); | ||
|
|
||
| it('falls back to measured bytes when the header is absent', () => { | ||
| expect(getLoggedBytes({ type: 'up', bytes: 5000000000 }, undefined)).toBe( | ||
| 5000000000 | ||
| ); | ||
| }); | ||
|
|
||
| it('does not override download measurements', () => { | ||
| expect( | ||
| getLoggedBytes({ type: 'down', bytes: 5000000000 }, 4000000000) | ||
| ).toBe(5000000000); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor/style:
uploadBytesis now spread onto every bandwidth point, including download points where it is alwaysundefined. That's harmless (optional field), but it does add an explicituploadBytes: undefinedkey to download point objects. If you want download points to stay shape-identical to before, you could conditionally include it. Non-blocking.