Allow rpm test to run in non-tls mode by BenMatase · Pull Request #31 · cloudflare/networkquality-rs

BenMatase · 2025-09-30T13:44:38Z

Added a flag --no-tls to the mach rpm entrypoint and forced download/upload/latency requests to not go over TLS.

The reason for this change: We are seeing high cpu usage for mach's rpm test. We compared it to the ookla speedtest cli tool and mach was using significantly more cpu.

I instrumented mach and saw that a lot of the usage is in crypto operations. I also did some pcaps on the ookla tool and saw that they were doing bare HTTP traffic. This seems appropriate since the data that is being transferred isn't sensitive.

After the changes, you can see all of the crypto calls are now gone and we can see better results. The AIM reporting still will use HTTPS.

Results on a 2 core aws instance:

no tls: ~7Gbps (I saw one 10Gbps)
tls: ~5Gbps

Also cpu usage is better:

no tls: maxing out around 70%
tls: maxing out around 100%

The cpu is not as great as an improvement but when you consider that it is doing higher speeds (and is processing more packets), it is better than the numbers suggest. If the network was the bottleneck, I'd suspect the cpu usage would have a better improvement.

A couple of observations from making this change:

Just moving to http/port 80 was not enough. h2 was having issue with the http endpoint so had to force h1
h1 expects more headers in general and some other tweaks to work e2e with the default urls
the aim report seems to only be https which makes sense considering it is actually transporting interesting data that we would want to secure
uploads (POST) seemed to not like the really large uploads when doing just http. Had to limit the size of the uplaods to 16Mb. It seemed that the upload connection was being broken early in this case when too large. Probably endpoints didn't like someone uploading huge files over http.
The loaded latency measurements in http mode were having issues getting decent numbers. I believe it was because the connection was being overwhelmed and there is no multiplexing in http 1.1, so made a scheme where it would use finished connections

kornelski · 2025-09-30T21:40:31Z

Thanks for the PR.

I think having more control over the protocol is fine, but this change is not just removing TLS, but also downgrades H/2 to H/1. Could that affect measurements? Maybe it'd be better to make these separate options?

kornelski · 2025-09-30T21:43:41Z

cli/src/args/rpm.rs

@@ -86,6 +90,7 @@ impl Default for RpmArgs {
            interval_duration_ms: 1000, // 1s
            test_duration_ms: 12_000,   // 12s
            disable_aim_scores: false,


having both no- and disable- is inconsistent. no- is more typical for args, so maybe rename the other arg to no-aim-scores and add disable-aim-scores as an alias for backwards compatibility?

kornelski · 2025-09-30T21:46:11Z

cli/src/latency.rs

    let result = run_test(&LatencyConfig {
        url: url.parse()?,
        runs,
+        no_tls: false,


Please use tls: true.

--no-tls makes sense as an arg, but internally working with negated names is adding cognitive overhead.

kornelski · 2025-09-30T21:48:33Z

cli/src/report.rs

+            Some(v) => v,
+            None => {
+                tracing::warn!("no loaded latency measurements; defaulting to 0ms");
+                0.0


Reporting a wrong value is not desirable. Could you keep it as an error? Or make the field an Option?

kornelski · 2025-09-30T21:51:44Z

cli/src/rpm.rs

+        no_tls,
    ));

+    let conn_type = if no_tls { ConnectionType::H1 } else { ConnectionType::H2 };


unencrypted H/2 theoretically exists, so this downgrade isn't obvious. Maybe the no_tls option could have a different name? or tls and h2 have their own options?

I was having trouble with unencrypted http2 working with the cloudflare endopints. I'll try again, but @fisherdarling has agreed that might be the case

I absolutely expect that unencrypted H/2 will be difficult to use.

I'm mainly worried about H/1 vs H/2+TLS changing two variables at once, from UI perspective. In case somebody wanted to compare TLS overhead, it isn't apples to apples comparison.

So the toggle could be just clearer about what changes, maybe --plain-h1.

Or if the two could be toggled separately, H/1+TLS would be a possibile option.

kornelski · 2025-09-30T21:52:44Z

cli/src/rpm.rs

+    let conn_type = if no_tls { ConnectionType::H1 } else { ConnectionType::H2 };
    let response = Client::default()
-        .new_connection(ConnectionType::H2)
+        .plain_http_mode(no_tls)


plain_http_mode and no_tls mix terminology

kornelski · 2025-09-30T21:53:29Z

crates/nq-core/src/connection/http.rs

        ConnectionType::H1 => b"\x08http/1.1",
-        ConnectionType::H2 => b"\x02h2",
-        ConnectionType::H3 => b"\x02h3",
+        ConnectionType::H2 => b"\x02h2\x08http/1.1",


could this affect existing users not using --no-tls?

Yea, that is a good point. I added this when I was doing some testing. Let me see if I can remove this

kornelski · 2025-09-30T21:58:21Z

crates/nq-core/src/connection/http.rs

+        .ssl()
+        .selected_alpn_protocol()
+        .map(|p| String::from_utf8_lossy(p).to_string())
+        .unwrap_or_else(|| "<none>".to_string());


In this case it's doesn't make a noticeable difference, but just FYI you can avoid .to_string() in such cases if you first assign the Option<String> or Option<Cow> version to a variable, then use .as_deref(), and you'll be able to have &str fallback.

or you could use std::str::from_utf8(p).unwrap_or("<invalid>") and not allocate String at all.

kornelski · 2025-09-30T22:01:35Z

crates/nq-core/src/client.rs

+            let scheme = uri_scheme.as_deref();
+            let default_port = match scheme { Some("https") => 443, _ => 80 };
+            let need_port = uri_port.is_some() && uri_port.unwrap() != default_port;
+            let host_value = if need_port { format!("{}:{}", host_hdr, uri_port.unwrap()) } else { host_hdr


current versions of Rust automagically make this work: if need_port { &format!(…) } else { host_hdr }

kornelski · 2025-09-30T22:06:56Z

crates/nq-load-generator/src/lib.rs

+            .loads
+            .iter()
+            .filter(|l| l.finished_at.is_some())
+            .collect();


There's https://docs.rs/rand/latest/rand/seq/trait.IteratorRandom.html that doesn't allocate

kornelski · 2025-09-30T22:08:17Z

crates/nq-rpm/src/lib.rs

+                ThroughputClient::download().plain_http_mode(true)
+                    .with_connection(conn)
+                    .send(
+                        self.config.small_download_url.as_str().parse()?,


this is a lot of copy-paste. Could you factor out download() and send() out of the conditions?

Allow mach to run in non-tls mode

a3c7499

kornelski reviewed Sep 30, 2025

View reviewed changes

Conversation

BenMatase commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kornelski commented Sep 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BenMatase commented Sep 30, 2025 •

edited

Loading