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

Add support for compression in http api #3752

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

yfouquet
Copy link
Contributor

The need has been spotted in issue #3687.
Using "NYTimes/gziphandler", the HTTP api responses can now be compressed if required.

The Go API requires compressed response if possible and handle the compressed response (natively).
We here only change the HTTP "v1" endpoints (not "ui" or "debug" for instance).

Note that, in our context, the compression has a strong impact on the UI latency which takes more than 10 seconds to load without compression because of "/v1/internal/ui/nodes" response size (> 40MB).

@yfouquet yfouquet force-pushed the issue_3687 branch 2 times, most recently from 258342a to 6205918 Compare December 15, 2017 15:45
@yfouquet
Copy link
Contributor Author

yfouquet commented Dec 15, 2017

Hi @slackpad,
What do you think about using thirdparty NYTimes/gziphandler for HTTP response compression?

@yfouquet
Copy link
Contributor Author

Hi @kyhavlov,
Could you please give me a feedback on this PR?

@yfouquet
Copy link
Contributor Author

yfouquet commented Feb 5, 2018

Hi @slackpad,
Could you please have a look at this PR and give me feedbacks?
If you think that it may be good to not compress HTTP responses, we may consider adding an option in the consul agent (with disable as default). What do you think?

@yfouquet yfouquet force-pushed the issue_3687 branch 2 times, most recently from 3a031bf to 6a40e50 Compare February 15, 2018 08:35
@mkeeler
Copy link
Member

mkeeler commented Mar 29, 2018

Thanks for the PR @yfouquet!

As to your original question using the NYTimes/gziphandler library is fine and this would be a good addition to Consul. However in order to merge the PR there are 4 items that need addressing:

  1. This branch should be rebased onto the current master branch to get rid of conflicts.
  2. Update the gziphandler library to its 1.0.1 tag
  3. Unconditionally use the gzip handler. Including for the monitor endpoint. The 1.0.1 version of the lib should support the streaming gzip cases needed for that endpoint.
  4. Update the gziphandler creation to set the minSize before flushing to 0. I think that can be done like this:
		gzipWrapper, _ := gziphandler.GzipHandlerWithOpts(gziphandler.MinSize(0))
		gzipHandler := gzipWrapper(http.HandlerFunc(wrapper))

@yfouquet
Copy link
Contributor Author

yfouquet commented Apr 3, 2018

Hi @mkeeler,

Thanks for your response.
All is done, and works fine (even for /agent/monitor endpoint).
I will have a look at the failing tests this evening.

@mkeeler
Copy link
Member

mkeeler commented Apr 3, 2018

@yfouquet Thanks for the fixes. The code looks good to me and I figured out why the test failed. Even with the gziphandler setup to use MinSize(0) without any writes to the stream it wouldn't go ahead and send the HTTP response headers. If you add the following into agent/agent_endpoint.go at line 703 (after the WriteHeader call and before the Flush call) then it should work properly.

	// 0 byte write is needed before the Flush call so that if we are using 
	// a gzip stream it will go ahead and write out the HTTP response header
	resp.Write([]byte(""))

Its a bit of a hack to get around the issue with the gziphandler library but seems to work well enough.

The need has been spotted in issue hashicorp#3687.
Using "NYTimes/gziphandler", the http api responses can now be compressed if required.
The Go API requires compressed response if possible and handle the compressed response.
We here change only the http api (not the UI for instance).
@yfouquet
Copy link
Contributor Author

yfouquet commented Apr 3, 2018

Great @mkeeler,
Everything is now fine.

@mkeeler mkeeler merged commit 0d1d03c into hashicorp:master Apr 4, 2018
@yfouquet yfouquet deleted the issue_3687 branch April 4, 2018 17:02
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.

2 participants