Skip to content
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

fix enable http/2 by default as intended by flags #2908

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

wolfeidau
Copy link
Contributor

@wolfeidau wolfeidau commented Jul 30, 2024

Description

At some point the agent HTTP client configuration was updated however this resulted in it falling back to HTTP/1.1. Ideally we would take advantage of HTTP/2 to improve overall performance, especially for agents who are connecting from locations outside the US.

This will also lead to less TLS handshakes, and connections overall, which in turn should reduce the likelyhood of stalls uploading artifacts.

Context

The defaulting to HTTP 1.1 was discovered while debugging higher than expected latency while uploading artifacts.

Changes

Use the default transport to ensure that the ForceAttemptHTTP2 is set and we use HTTP/2.0. Cloning the default transport is the simplest approach to ensure you get the most up to date configuration.

See golang/go#26013 for some background on the Clone method in the default transport.

From https://github.com/golang/go/blob/master/src/net/http/transport.go#L50.

	// ForceAttemptHTTP2 controls whether HTTP/2 is enabled when a non-zero
	// Dial, DialTLS, or DialContext func or TLSClientConfig is provided.
	// By default, use of any those fields conservatively disables HTTP/2.
	// To use a custom dialer or TLS config and still attempt HTTP/2
	// upgrades, set this to true.
	ForceAttemptHTTP2 bool

To disable HTTP/2.0 we need to update a few things, these are documented in this change as well.

Testing

Before.

2024-07-30 13:25:03 DEBUG  POST https://agent.buildkite.com/v3/connect command=start agent_version=3.75.1+x..dirty
2024-07-30 13:25:04 DEBUG  ↳ POST https://agent.buildkite.com/v3/connect command=start proto=HTTP/1.1 status=200 Δ=743.567041ms agent_version=3.75.1+x..dirty

After.

2024-07-30 13:24:44 DEBUG  POST https://agent.buildkite.com/v3/connect command=start agent_version=3.75.1+x..dirty
2024-07-30 13:24:44 DEBUG  ↳ POST https://agent.buildkite.com/v3/connect command=start proto=HTTP/2.0 status=200 Δ=733.139667ms agent_version=3.75.1+x..dirty

At some point the agent HTTP client configuration was updated however this resulted in it falling back to HTTP/1.1. Ideally we would take advantage of HTTP/2 to improve overall performance, especially for agents who are connecting from locations outside the US.

This will also lead to less TLS handshakes, and connections overall, which in turn should reduce the likehood of stalls uploading artifacts.
Copy link
Contributor

@SorchaAbel SorchaAbel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one!!

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

@yob
Copy link
Contributor

yob commented Jul 30, 2024

Fascinating! Do we know what version we regressed back to HTTP 1.1 by default? Might be a good detail to flag with our support friends so they can keep an eye out for opportunities to suggest upgrading.

discovered while debugging higher than expected latency while uploading artifacts

Do you suspect this is a cause of slow artifact uploads? Or is this a fix the you noticed while debugging, but slow artifact uploads still possible?

@DrJosh9000
Copy link
Contributor

DrJosh9000 commented Jul 30, 2024

Appears to have been this way since #1020

...which used Go 1.12, and ForceAttemptHTTP2 was added in Go 1.13

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌴😎🍹

api/client.go Outdated Show resolved Hide resolved

// use the default transport as it is optimized and configured for http2
// and will avoid accidents in the future
tr := http.DefaultTransport.(*http.Transport).Clone()
Copy link
Contributor

@DrJosh9000 DrJosh9000 Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a note in case this is important later on - no changes needed.

The diff from the old transport to the default is:

- 	TLSHandshakeTimeout:   30 * time.Second,
+ 	ForceAttemptHTTP2:     true,
+	TLSHandshakeTimeout:   10 * time.Second,
+	ExpectContinueTimeout: 1 * time.Second,

and the use of defaultTransportDialContext wrapping net.Dialer, but that is equivalent to .DialContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout, thanks!

@wolfeidau wolfeidau merged commit e139c86 into main Jul 31, 2024
1 check passed
@wolfeidau wolfeidau deleted the fix_enable_http2_by_default branch July 31, 2024 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants