feature/Force HTTP1 functionality

[sjordhani22]

Greetings,

We as a team of college students at WPI have been recently using K6 for some of our load-testing needs. In K6’s current implementation, it is not possible to force requests to be sent over the HTTP/1 protocol. We realized this is something that we would like to be able to do with K6 and while doing some research on the current issues, it seems this is something that others have requested as well (#936). This PR attempts to address this issue by allowing users to specify a “--force-http1” flag on the command line, setting “ForceHTTP1” to true inside the options object in a script, or setting it as an environment variable. If enabled, this flag will send all HTTP requests using HTTP/1.1 - independent of the HTTP version supported by the endpoint. While a global option like this is not ideal, the code does allow people to select which protocol to use, is not very intrusive and can be easily removed in the future, and allows users to run their tests with HTTP/1.1 until the new HTTP implementation is complete. We are aware that you all want to see a new HTTP implementation, however, we believe that this change is small, straightforward, solves a major use case, and will allow for users to get tests in HTTP/1 running today.

Additionally, we don’t mind updating the documentation to introduce this new feature, we would just need to know where to modify the documentation.

Co-authored-by: Ashwin Pai [email protected]
Co-authored-by: Saniya Syeda [email protected]

[CLAassistant]

All committers have signed the CLA.

[codecov-commenter]

Codecov Report

Merging #2222 (0d8c647) into master (42f2e60) will increase coverage by 0.18%.
The diff coverage is 71.42%.

❗ Current head 0d8c647 differs from pull request most recent head 7b04260. Consider uploading reports for the commit 7b04260 to get more accurate results

@@            Coverage Diff             @@
##           master    #2222      +/-   ##
==========================================
+ Coverage   72.71%   72.90%   +0.18%     
==========================================
  Files         184      184              
  Lines       14571    14743     +172     
==========================================
+ Hits        10596    10748     +152     
- Misses       3333     3350      +17     
- Partials      642      645       +3     

Flag	Coverage Δ
ubuntu	72.85% <71.42%> (+0.21%)	⬆️
windows	72.63% <71.42%> (+0.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/cloud.go	4.68% <0.00%> (-0.03%)	⬇️
cmd/login_cloud.go	0.00% <0.00%> (ø)
cmd/login_influxdb.go	0.00% <0.00%> (ø)
cmd/ui.go	20.61% <0.00%> (ø)
js/compiler/compiler.go	56.75% <ø> (+1.75%)	⬆️
lib/execution_segment.go	92.73% <ø> (ø)
lib/executor/ramping_vus.go	94.66% <ø> (ø)
lib/metrics/metrics.go	100.00% <ø> (ø)
ui/form_fields.go	45.71% <0.00%> (ø)
output/influxdb/output.go	76.52% <53.12%> (-5.66%)	⬇️
... and 44 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8164ee...7b04260. Read the comment docs.

[na--]

Thanks for making this! Unfortunately, while the PR looks good technically, we can't merge it as it is... 😞

While a global option like this is not ideal, the code does allow people to select which protocol to use, is not very intrusive and can be easily removed in the future, and allows users to run their tests with HTTP/1.1 until the new HTTP implementation is complete.

That's the crux of the problem - once we add such a global new option, however small, it can almost never be removed afterwards. Removing it will be a major breaking change, so we want to avoid adding a new option that is known from the start will be temporary... We are slowly chipping away on the issues around a better HTTP API, but we're not quite three yet 😞

As an alternative to your proposal, I can suggest bringing back support for GODEBUG (see bottom of this section). As I mentioned in #936 (comment), because of the way we've implemented it with golang.org/x/net/http2, k6 doesn't currently respect GODEBUG=http2client=0. Since that doesn't involve any long-term compatibility guarantees, we would probably be OK with a pull requests that adds support for it.

[sjordhani22]

Thank you so much for getting back to us, @na-- ! We were wondering if we can get a little bit more clarity on:

"...k6 doesn't currently respect GODEBUG=http2client=0. Since that doesn't involve any long-term compatibility guarantees, we would probably be OK with a pull requests that adds support for it."

If we remove the global option, and instead check for whether the environment variable is set here, would this be something you would consider merging?

Some pseudocode showing how we are imagining it:

// psuedocode

disable_h1 := os.getEnv("GODEBUG")
if disable_h1 == "http2client=0" 
   // use h1
else
   /// configure_transport
end

[na--]

Yeah, I think that would be ok to merge as a temporary workaround, until we have a proper fix for this. It's far from ideal (e.g. I don't like the fact that we'll depend on the os package and global state in that part of the code), but it should be better than adding another explicit global k6 option 🤷‍♂️

Also, keep in mind that GODEBUG can have other things in it as well, so it's probably better to parse it in a separate function, with a comment that it's temporary until #936 is resolved.

[sjordhani22]

Sorry for the late response @na-- ! We made the changes you had requested, please let us know if this looks good or if there is anything else we must change. Thanks!

[codebien]

Hi @sjordhani22,
thanks for your contribution!

Added some small things in the comments, also can you add a test, please?

[sjordhani22]

Thanks for the feedback, @codebien! I wrote two test cases that set the GODEBUG variable, initiate a request, and then check to see that the intended protocol was used. In the tests I send requests to https://k6.io/, please let me know if this is alright. I also made the changes you requested.

[sjordhani22]

Greetings!

I was taking a look at the lint errors that came back from the review and I made changes to fix those. One of the issues however, mentioned that the new tests that were written were not run in parallel. I had originally tried running the tests in parallel but it seems like when I did that, the force h1 feature was not working and the tests would fail. I think it has to do with the setting of the environment variable in the test. Any ideas on this? Thanks!

[codebien]

Hi @sjordhani22,
sorry for the late response, I think it's fine to set a nolint line for excluding the parallel check for this specific test. Generally, you would resolve a problem like this by having the env var injected but we decided to not have a new option so, for now, I think this is the best we can do.

Apparently, the two tests are very similar in terms of code, I think we could improve them by creating a table test and injecting the params required instead to have one dedicated for each case. This should have the benefit of having only one //nolint directive.

[sjordhani22]

Hello @codebien,

I took care of those changes that you asked for. Please let me know if this looks good to merge.

Thanks!

[codebien]

Hey @sjordhani22,
some small improvements, I think we are pretty close. Thanks for your effort. 🙏

[sjordhani22]

@codebien, thanks for the feedback! Made those changes as well.

Thanks!

[codebien]

LGTM 👏

Let's wait for a 2nd review by @na--, @oleiade I remember you were working on js/runner tests maybe you want to review this PR before we get it merged.

[oleiade]

LGTM 👍🏻

A couple nitpicks that you can safely ignore if you prefer. Great work! 🙏🏻

[sjordhani22]

Made those additional changes requested. Not sure why GitHub UI says I "dismissed stale reviews" after a push.

Thanks!

[na--]

Thanks, LGTM now. The stale review dismissal is because we've configured the GitHub repo so that every commit you make in the PR automatically dismisses the old PR reviews. This way we have to review the latest code before it's merged in master.

I see that one of the commit authors, @ssyeda422, hasn't signed the CLA: #2222 (comment)

That also needs to happen, or the their commit needs to be rewritten and the PR squashed, before we can merge the PR

[ssyeda422]

Hi, sorry about that! Totally forgot to resign. I did that just now, let me know if it’s all set.

Thank you!

[marcelmfa]

How use this feature in CLI? I haven't found in docs and checking that (https://k6.io/docs/using-k6/protocols/), it says that's automatic, but I'd like to force h1, just like said in the description of this feature.

[mstoykov]

Hi @marcelmfa I have opened grafana/k6-docs#664 to document this, but you need to set GODEBUG env variable to http2client=0 so GODEBUG=http2client=0 k6 run script.js for LInux/MacOs for windows it's different depending on what you use so see this blog for example.