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.

Sign up for GitHub

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

Jump to bottom

Add influxdb gzip support #2978

Merged
merged 4 commits into from
Aug 14, 2017
Merged

Add influxdb gzip support #2978

merged 4 commits into from
Aug 14, 2017

Conversation

bobmshannon
Copy link
Contributor

@bobmshannon bobmshannon commented Jul 3, 2017 •
edited
Loading

An approach for fixing #2802. Adds an optional parameter gzip (default: false), which if enabled, will compress the payload of each HTTP request sent to InfluxDB using GZIP.

While testing I was able to get on average 67% space savings when comparing the content length of the compressed vs. uncompressed payloads using the default sample telegraf.conf with no input filters.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@bobmshannon bobmshannon force-pushed the enhancement/influxdb_gzip_support branch from aa4cc13 to 905b4fa Compare July 3, 2017 03:42
@bobmshannon bobmshannon force-pushed the enhancement/influxdb_gzip_support branch 2 times, most recently from 7309ac5 to 599aa5d Compare July 3, 2017 19:18
@bobmshannon bobmshannon force-pushed the enhancement/influxdb_gzip_support branch from 669ad0b to 8abde2a Compare July 3, 2017 19:48
@danielnelson danielnelson added this to the 1.4.0 milestone Jul 5, 2017
@danielnelson
Copy link
Contributor

I think it should be possible to do this without buffering the entire request body again, similar to this gist https://gist.github.com/schmichael/7379338#file-gistfile1-go-L41. Is this something you are interested in looking into?

@bobmshannon
Copy link
Contributor Author

@danielnelson Yes, definitely. Thanks for the example. Let me see what I can do to make this more efficient.

@bobmshannon bobmshannon force-pushed the enhancement/influxdb_gzip_support branch 2 times, most recently from 576d96e to 9e62fc3 Compare July 15, 2017 21:27
danielnelson
danielnelson reviewed Jul 17, 2017
View reviewed changes
plugins/outputs/influxdb/README.md Outdated
@@ -43,6 +43,9 @@ This plugin writes to [InfluxDB](https://www.influxdb.com) via HTTP or UDP.

## HTTP Proxy Config
# http_proxy = "http://corporate.proxy:3128"

## Compress each HTTP request payload using GZIP, defaults to false.
# gzip = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the option to be content_encoding = "gzip" supporting only gzip, this will allow us to potentially add more encodings later and IMO reads better.

plugins/outputs/influxdb/client/http.go Outdated
// }
if c.config.Gzip {
req.Header.Set("Content-Encoding", "gzip")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move all the Header code into makeRequest, I'm not sure why they should be separate.

plugins/outputs/influxdb/client/http.go Outdated
@@ -233,15 +233,24 @@ func (c *httpClient) makeWriteRequest(
return nil, err
}
req.Header.Set("Content-Length", fmt.Sprint(contentLength))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to complicate things... if we are using gzip we need to report the length of the body here after compression, but since we are now streaming the data it is trickier. I think we need to use Transfer-Encoding: chunked and this header is unset for gzip.

I haven't ever done this with go, maybe we can use https://golang.org/pkg/net/http/httputil/#NewChunkedWriter? Or maybe it will be automatically handled if we don't set this header?

@danielnelson danielnelson mentioned this pull request Aug 4, 2017
Closed
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 12, 2017
@bobmshannon bobmshannon force-pushed the enhancement/influxdb_gzip_support branch 4 times, most recently from fae10de to 37a9468 Compare August 13, 2017 03:02
@bobmshannon
Copy link
Contributor Author

bobmshannon commented Aug 13, 2017 •
edited
Loading

@danielnelson I ran through some testing and I think that the HTTP client handles streaming automatically if the content length header is unset.

Basic smoke test

A sample telegraf.conf was generated as per the docs (./telegraf config > telegraf.conf) and telegraf was started. content_encoding was set to gzip. The successful ingestion of metrics was manually confirmed by querying InfluxDB directly.

content_encoding = "gzip" (gzip compression enabled)

screenshot 2017-08-12 22 57 13

screenshot 2017-08-12 22 57 32

@bobmshannon bobmshannon force-pushed the enhancement/influxdb_gzip_support branch from 37a9468 to c8e0b4e Compare August 13, 2017 03:20
@danielnelson danielnelson merged commit 5fbdd09 into influxdata:master Aug 14, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
@bobmshannon @danielnelson
Add gzip content-encoding support to influxdb output (#2978)
67fe167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielnelson danielnelson danielnelson left review comments

Assignees
No one assigned
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bobmshannon @danielnelson