Draft 09 conformance & 413 fix - #47
Open
HenriqueSFernandes wants to merge 7 commits into
Open
Conversation
Adds a process-wide TLS verification bypass (--insecure / nq_core::set_insecure_tls) for testing against servers with self-signed certificates (e.g. a local speed-test dev server behind a proxy). Also fixes HTTP/1.1 requests to use origin-form request targets with a Host header carrying the authority (incl. port); they were previously sent in absolute/proxy-form which origin servers reject.
There was a problem hiding this comment.
Pull request overview
Aligns the responsiveness (RPM) measurement implementation with draft-ietf-ippm-responsiveness-09 and fixes biased/incorrect results caused by cumulative foreign timing, missing TLS normalization, and treating HTTP 413 / connection failures as “successful” load.
Changes:
- Update RPM computation to draft-09 semantics (RPM-per-side then arithmetic mean), fix foreign probe phase accounting, normalize TLS handshake by TLS round-trips, and avoid “0 RPM” from wall-clock windowing.
- Make upload load generation robust to per-request body caps by reissuing bounded POSTs on the same connection; surface/handle failed load-generating connections via a configurable policy.
- Add CLI/config/reporting support for
--upload-max-request-bytes,--on-connection-error,--insecure, and expose degraded results viafailed_connections/ optional RPM.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/nq-rpm/src/lib.rs | Draft-09 RPM formula, foreign timing fixes, error-policy handling, and RPM reporting semantics (Option). |
| crates/nq-packetloss/src/lib.rs | Removes upload-size configuration tied to the old unbounded upload model. |
| crates/nq-load-generator/src/lib.rs | Implements bounded upload request reissue + failure detection/state tracking for load connections. |
| crates/nq-core/src/lib.rs | Re-exports new core types/helpers (InflightBody, insecure TLS toggles). |
| crates/nq-core/src/connection/mod.rs | Adds TLS round-trip tracking + TCP/TLS phase helpers on ConnectionTiming. |
| crates/nq-core/src/connection/http.rs | Adds global --insecure TLS verification bypass and HTTP/1.1 request normalization. |
| crates/nq-core/src/client.rs | Adds upload/download status checking, propagates failures via BodyEvent::Failed, and corrects timing start (post-DNS). |
| crates/nq-core/src/body/mod.rs | Expands InflightBody fields and semantics for event channel closure. |
| crates/nq-core/src/body/counting_body.rs | Adds BodyEvent::Failed + sender handle for out-of-band failure reporting with tests. |
| cli/src/up_down.rs | Wires --insecure into up/down commands. |
| cli/src/rpm.rs | Wires new RPM CLI knobs, adds degraded-result warnings, and passes new config fields through. |
| cli/src/report.rs | Treats absent RPM as an error (not zero) and serializes failed connection count. |
| cli/src/args/up_down.rs | Adds --insecure flag to upload/download args. |
| cli/src/args/rpm.rs | Adds --on-connection-error and --upload-max-request-bytes parsing/validation plus defaults. |
| cli/src/aim_report.rs | Maintains telemetry compatibility by flattening missing RPM to 0.0 with explicit rationale. |
Suppressed comments (1)
crates/nq-load-generator/src/lib.rs:312
- This error message also reports the wrong request number (it uses the count of requests already started, but the failure occurred while starting the next request).
let _ = events_tx.send(BodyEvent::Failed {
at: self.time.now(),
reason: format!("upload request {requests} failed to start: {error:#}"),
});
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+297
to
+300
| let _ = events_tx.send(BodyEvent::Failed { | ||
| at: self.time.now(), | ||
| reason: format!("could not start upload request {requests}: {error:#}"), | ||
| }); |
Comment on lines
+552
to
+555
| anyhow::bail!( | ||
| "aborting test: {failed} load-generating connection(s) terminated with an \ | ||
| error (most recent: {reason})" | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was done:
tcp_f/tls_f/http_fwere measured cumulatively from before DNS instead of as three independent round-trips, roughly doubling the foreign RTT and halving the RPM.413counted as a successful transfer: against a local 10 MiB body cap the upload leg reported 952 RPM against an honest ~335. Added--on-connection-error {retire,abort}.413once it passes their body cap, killing the load mid-run and leaving the score artificially high. Now an open-ended sequence of bounded POSTs re-issued on the same connection, via--upload-max-request-bytes(default 100 MB).0 RPMon healthy runs: the reported value came from a wall-clock window that the newest sample sat exactly on the edge of; 1 run in 14 matched nothing and reported0. Reporting is now clock-free.--insecure: for testing against a local proxy with a self-signed certificate.