Add option to enable multuple H/3 connections

[ManickaP]

dotnet/runtime#101531 follow up

cc @CarnaViire

[CarnaViire]

I wonder if we maybe should keep the default as false for the time being.
Otherwise if we merge it like that, all the CI tests will start using that by default, but I think it should be a clear parameter as with H/2 (h2 vs h2multiconn)
It's not a blocker, just a way not to mix up measurements.

Or we can change both crank config yml and ci test config yml right away to include the parameter

[ManickaP]

???
Do you want me to change something here or not?

[CarnaViire]

(1)
"Keep the default as false" -- Minimal change

- ... new Option<bool>(..., () => true, ...);
+ ... new Option<bool>(..., () => false, ...);

-OR-

(2)
"Change crank config yml" -- Modify scenarios/httpclient.benchmarks.yml to include new parameter as a variable (with default value = false), e.g.:

Benchmarks/scenarios/httpclient.benchmarks.yml

Line 27 in 9c454b7

http20EnableMultipleConnections: true

and pass it to client, e.g.: --http20EnableMultipleConnections {{http20EnableMultipleConnections}}

Benchmarks/scenarios/httpclient.benchmarks.yml

Line 171 in 9c454b7

arguments: '--address {{host}} --port {{serverPort}} --useHttps {{useHttps}} --path /{{scenario}} --scenario {{scenario}} --httpVersion {{httpVersion}} --numberOfHttpClients {{numberOfHttpClients}} --concurrencyPerHttpClient {{concurrencyPerHttpClient}} --http11MaxConnectionsPerServer {{http11MaxConnectionsPerServer}} --http20EnableMultipleConnections {{http20EnableMultipleConnections}} --useWinHttpHandler {{useWinHttpHandler}} --useHttpMessageInvoker {{useHttpMessageInvoker}} --collectRequestTimings {{collectRequestTimings}} --contentSize {{requestContentSize}} --contentWriteSize {{requestContentWriteSize}} --contentFlushAfterWrite {{requestContentFlushAfterWrite}} --contentUnknownLength {{requestContentUnknownLength}} {{headersDictionary[requestHeaders]}} --generatedStaticHeadersCount {{generatedStaticRequestHeadersCount}} --generatedDynamicHeadersCount {{generatedDynamicRequestHeadersCount}} --useDefaultRequestHeaders {{useDefaultRequestHeaders}} --warmup {{warmup}} --duration {{duration}}'

-OR-

(3)
"Change both crank config yml and ci test config yml" -- (2) AND modify build/httpclient-scenarios.yml to include new parameter with both true and false to test matrix, e.g.:

Benchmarks/build/httpclient-scenarios.yml

Lines 31 to 34 in 9c454b7

	- displayName: "HTTP/2.0"
	arguments: --variable httpVersion=2.0 --variable useHttps=true --variable http20EnableMultipleConnections=false --property httpversion=h2
	- displayName: "HTTP/2.0 (mult conn)"
	arguments: --variable httpVersion=2.0 --variable useHttps=true --variable http20EnableMultipleConnections=true --property httpversion=h2multconn

and as httpVersion is already passed to the scenario (we're not adding a new dimension), it should then appear automatically in "pretty graphs" as soon as the new value will appear in the result table.

---

Do you want me to change something here or not?

It is hard for me to understand how urgently you'd like to merge the changes (I guess not really, given the PR's in draft), and how much effort are you willing to put into it.

My preferred option is (3) but it can be done as a follow-up, as long as the default is false.

But I am open to the discussion about the default as well.

[ManickaP]

It is hard for me to understand how urgently you'd like to merge the changes

Not urgent at all, it just wasn't obvious to me from the original comment whether you expect any changes to this PR or not.

[ManickaP]

I attempted to do (3). I searched for all http20EnableMultipleConnections occurrences and added h3 equivalents.

But I am open to the discussion about the default as well.

I copy pasted H/2 settings everywhere. If that's not what you wanted, we can chat, but I think it should follow the same approach as H/2.

[CarnaViire]

Suggested change

	arguments: --variable httpVersion=3.0 --variable useHttps=true --variable http30EnableMultipleConnections=true --property httpversion=h3mulconn
	arguments: --variable httpVersion=3.0 --variable useHttps=true --variable http30EnableMultipleConnections=true --property httpversion=h3multconn

[CarnaViire]

Changes LGTM -- have you tried to use the updated config YML to verify it works?

You can do it this way:

1. in <local-path-to-repo>/scenarios/httpclient.benchmarks.yml, change the pointers to the sources for the client job (jobs.httpClient.source.repository and jobs.httpClient.source.branchOrCommit) to point at your branch:

      repository: https://github.com/ManickaP/aspnet-benchmarks.git
      branchOrCommit: h3-mul-conns

2. use this local config file (instead of the remote one) in crank, and verify that both true and false values for the new parameter are passed and used correctly, e.g.:

-crank --config https://raw.githubusercontent.com/..../httpclient.benchmarks.yml --scenario ....
+crank --config <local-path-to-repo>/scenarios/httpclient.benchmarks.yml         --scenario .... --variable http30EnableMultipleConnections=false

HOW-TO: verify parameter is passed correctly

1. go to the job metadata on the agent (e.g. http://asp-citrine-lin:5001/jobs/100) and check the command line used to start the app, e.g.:

{
"driverVersion": ....,
....
"arguments": "--address .... --http30EnableMultipleConnections false ....",
....
}

-AND/OR-

2. go to the job logs on the agent (e.g. http://asp-citrine-lin:5001/jobs/100/output) and check the options printed, e.g.

[10:10:50.344] HttpClient benchmark
[10:10:50.353] Options: Address=....; .... Http30EnableMultipleConnections=False; ....
....

[ManickaP]

Resurrecting this PR, I rebased this on main and tested it:

• false: http://asp-perf-win:5001/jobs/7/output
• true: http://asp-perf-win:5001/jobs/6/output

So unless you have any more comments, can we get this in @CarnaViire ?

[CarnaViire]

LGTM, thanks!