HTTP output plugin #2491

Dark0096 · 2017-03-04T09:10:29Z

HTTP output plugin

HTTP is one of the most commonly used protocols. So, I want to add http plugin to telegraf.
The features are as follows.

Feature

The metrics collected from telegraf are transmitted through the HTTP Post method.
Specify a code that can determine that the HTTP status code is not an error. (expected_status_codes)
Set the timeout. (dial_timeout, response_header_timeout)

Test Code

Wrote the test code using the httptest package.

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)
README.md updated (if adding a new plugin)

…Post method.

Dark0096 · 2017-03-12T09:04:59Z

@sparrc I modified the code to use []byte buffer and use newline.
I have also removed the unnecessary max_bulk_limit option.
Can you take a look at my code?
Thanks.

lin-credible · 2017-03-13T14:34:01Z

👍

Shugyousha · 2017-05-23T08:53:25Z

I think you are missing the change in plugins/outputs/all/all.go. You still have to import the plugin for it to be properly registered.

Shugyousha · 2017-05-24T13:50:07Z

plugins/outputs/http/http.go

+// Writes metrics over HTTP POST
+func (h *Http) Write(metrics []telegraf.Metric) error {
+	if err := validate(h); err != nil {
+		return err


'validate' is called on every write even though we don't expect the checked values to change once set (because they are read from the config on startup).

It may be a good idea to add a 'Validate() err' method to the output plugin interfaces (and others). That function could be called in https://github.com/influxdata/telegraf/blob/master/internal/config/config.go#L760 after unmarshaling the toml values. Any wrong configuration could be detected and acted upon there (fail). Then we could get rid of this call on every Write.

Obviously changing the interface would result in quite some code churn because the Validate method would have to be added to all (output) plugins. Adding a stub that just returns nil shouldn't be too much work though, if people don't want to implement proper configuration checking (yet).

Thoughts?

@Shugyousha Oh, It's interesting. You are right.
Every time the write function is called, the validation check is done continuously since the beginning.
If the output interface has Validate, check the required option in the individual Outputs in advance, and then it will be very good if the setting is wrong.
Until then, I will remove the corresponding check.
Thanks a lot. 👍

Please review again.
Thanks.

…tput interface later through the Validate() err function.

Dark0096

Every time the write function is called, the validation check is done continuously since the beginning.
If the output interface has Validate, check the required option in the individual Outputs in advance, and then it will be very good if the setting is wrong.
Until then, I will remove the corresponding check.
Thanks a lot. 👍

Dark0096 · 2017-05-27T10:38:07Z

plugins/outputs/http/http.go

+// Writes metrics over HTTP POST
+func (h *Http) Write(metrics []telegraf.Metric) error {
+	if err := validate(h); err != nil {
+		return err


Please review again.
Thanks.

Shugyousha · 2017-06-19T07:34:57Z

This looks good to me. Is there anything else to be changed before this can be merged?

If I can be reasonably confident that adding a Validate method to the output interface will be accepted, I can start working on it.

Since we are using this plugin in production already, for now I will have to fork and apply this PR until this has been merged upstream.

Dark0096 · 2017-06-19T13:00:08Z

I do not know how to add the Validate method to the output interface.
This is the part where each output implementation can be changed due to the code change of the output interface.
However, it is an honor to use this code for you.
Thanks. :)

P.S. Do you have any idea how to appeal this code to merge?

danielnelson · 2017-06-19T18:55:10Z

@Dark0096 I'll try to review soon

drunz · 2017-10-22T12:14:14Z

@danielnelson Any chance this could get a bump? Would be really appreciated.

danielnelson · 2017-10-23T21:11:57Z

plugins/outputs/http/README.md

+  ## The value is written as a delimiter(:).
+  ## Content-Type is required http header in http plugin.
+  ## so content-type of HTTP specification (plain/text, application/json, etc...) must be filled out.
+  http_headers = [ "Content-Type:application/json" ]


I think we should do this in the same format as the httpjson input:

[outputs.http] [outputs.http.headers] Content-Type = "application/json"

This can also be written as an inline table, which is similar to the array version:

[outputs.http] headers = {Content-Type = "application/json"}

I have changed to map[string]string.
awesome! :)

danielnelson · 2017-10-23T21:18:06Z

plugins/outputs/http/README.md

+  ## Configure response header timeout in seconds. Default : 3
+  response_header_timeout = 3
+  ## Configure dial timeout in seconds. Default : 3
+  dial_timeout = 3


Let's start with only a timeout that covers the entire request (http.Client.Timeout). I'm not sure we need timeouts that cover only parts of the request but we can add them in later if needed.

I got it. I remained only timeout configuration.

danielnelson · 2017-10-23T21:21:09Z

plugins/outputs/http/README.md

+[[outputs.http]]
+  ## It requires a url name.
+  ## Will be transmitted telegraf metrics to the HTTP Server using the below URL.
+  ## Note that not support the HTTPS.


I will add support for the regular HTTPS options once this is merged, but this code will support connecting to a HTTPS url as it is.

Do you want to do additional work on HTTPS after you first merge it? I have not worked on this part.

danielnelson · 2017-10-23T21:22:44Z

plugins/outputs/http/README.md

+  http_headers = [ "Content-Type:application/json" ]
+  ## With this HTTP status code, the http plugin checks that the HTTP request is completed normally.
+  ## As a result, any status code that is not a specified status code is considered to be an error condition and processed.
+  expected_status_codes = [ 200, 204 ]


Let's call this success_status_codes and include 201 in the default list.

I got it. I have to add the 201 status code and rename expected_status_codes => success_status_codes.

I have changed the code like below.
if resp.StatusCode < 200 || resp.StatusCode > 209

danielnelson · 2017-10-23T21:23:58Z

plugins/outputs/http/http.go

+// Connect to the Output
+func (h *Http) Connect() error {
+	h.client = http.Client{
+		Transport: &http.Transport{


Add Proxy: ProxyFromEnvironment to transport.

danielnelson · 2017-10-23T21:30:36Z

plugins/outputs/http/http.go

+		return err
+	}
+
+	res.Body.Close()


If the status code does not match the expected set the function returns but the body is not closed.

I have added below code
defer resp.Body.Close()

danielnelson · 2017-10-23T21:37:38Z

plugins/outputs/http/http.go

+		arrayJsonObj = append(arrayJsonObj, []byte("[")...)
+		arrayJsonObj = append(arrayJsonObj, reqBodyBuf...)
+		arrayJsonObj = append(arrayJsonObj, []byte("]")...)
+		return bytes.Replace(arrayJsonObj, []byte("\n"), []byte(","), mCount-1), nil


I understand why you did this since the Serializer interface does not provide a way to process a list of metrics, but I don't think it is robust enough. I will update the Serializer interface to take a []telegraf.Metrics and then we can do this in a better way.

It is a code that processes each individual metric using a serializer and creates a Body according to the Content-Type. This section has been modified.

You can see 92 line about serializer.

danielnelson · 2017-10-23T21:39:46Z

plugins/outputs/http/http.go

+	outputs.Add("http", func() telegraf.Output {
+		return &Http{
+			ResHeaderTimeout: DEFAULT_RESPONSE_HEADER_TIMEOUT,
+			DialTimeOut:      DEFAULT_DIAL_TIME_OUT,


Also set default status codes here.

The code has been modified to recognize that the 2xx status code is unconditionally normal.

Shugyousha · 2017-10-25T15:46:12Z

@danielnelson thanks for the review! Now it shouldn't take much longer until this PR can be merged, I hope.

Dark0096 · 2017-10-29T09:14:10Z

@danielnelson Thank you for taking a look at my code.
Please tell me if I can help you :)

Dark0096 · 2017-12-21T12:33:18Z

@danielnelson Open source review is first time to my life.
So, I am not understand about process.
I have a some question about your review.

Are you fixing my code? or Can I fix the code?

Thanks.

danielnelson · 2017-12-22T02:05:06Z

@Dark0096 can you do it for me?

JonoB · 2018-02-25T19:51:09Z

Anything we can do to help to get this plugin pushed though into core?

Dark0096 · 2018-02-28T01:05:01Z

@danielnelson Sure. It is an honour.

@JonoB
Tomorrow is a Korean holiday.
I have a lot of time, so let's work on the code to merge the code.

JonoB · 2018-02-28T09:33:37Z

@Dark0096 - sounds good! For reference, the datadog output module uses http (I think), so it may be a good source of reference for you.

https://github.com/influxdata/telegraf/tree/master/plugins/outputs/datadog

JonoB · 2018-03-01T09:26:24Z

plugins/outputs/http/http.go

+	if h.SuccessStatusCode == nil {
+		h.SuccessStatusCode = make(map[int]bool)
+
+		for _, expectedStatusCode := range h.SuccessStatusCodes {


I think that any 2xx status code should be considered accepted. https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#2xx_Success

This is also how the Datadog plugin works: https://github.com/influxdata/telegraf/blob/master/plugins/outputs/datadog/datadog.go#L123

If agreed, then this could be simplified as follows:

if resp.StatusCode < 200 || resp.StatusCode > 209 { return fmt.Errorf("received bad status code, %d\n", resp.StatusCode) }

It's a good idea.
I agree that.
I'll fix the code.

- Accept status code 2xx.

Dark0096 · 2018-03-04T17:13:39Z

@danielnelson Could you review the code?

Dark0096

I've finished fixing the code for the parts you commented on. Could you review the code?

Dark0096 · 2018-03-04T17:15:41Z

plugins/outputs/http/http.go

+func (h *Http) Connect() error {
+	h.client = http.Client{
+		Transport: &http.Transport{
+			Proxy: http.ProxyFromEnvironment,


I have added the Proxy: ProxyFromEnvironment to transport.

Dark0096 · 2018-03-04T17:17:52Z

plugins/outputs/http/README.md

+  ## Content-Type is required http header in http plugin.
+  ## so content-type of HTTP specification (plain/text, application/json, etc...) must be filled out.
+  [outputs.http.headers]
+    Content-Type = "plain/text"


Please check this configuration format.

Dark0096 · 2018-03-05T13:12:51Z

@JonoB Thanks a lot.
Your advice has been very helpful.

JonoB · 2018-03-05T13:34:18Z

@Dark0096 - you're welcome, but you're the one doing all the hard work !

JonoB · 2018-03-05T13:35:01Z

@danielnelson - please let us know if you have any further comments. Thanks!

danielnelson · 2018-03-05T18:55:37Z

I think we need to address the batch json metrics in a better way. The current interface only provides for serialization of a single metric, which works out for line based formats since we can just concat them all together. I would like to add a method for serializing a list of metrics along with an option to enable it for JSON for backwards compatibility. I wrote up some details in #3856 and I would like to get this implemented before this pull request, so that we can adjust how the JSON output is handled.

Dark0096 · 2018-03-16T04:33:58Z

Good idea. I strongly agree.

danielnelson · 2018-05-05T01:27:51Z

@Dark0096 I have added support for SerializeBatch in #4107, can you update this pull request to use this method for serialization?

Dark0096 · 2018-05-06T13:23:09Z

@danielnelson It is my honor.
I will tell you after the code work.

Dark0096

We modified the code for the http output plugin to use SerializeBatch. In addition, we did code-wide refactoring.

danielnelson · 2018-05-15T00:17:38Z

I'm going to make a quick follow up change to give Content-Type a default value of text/plain. This way you will only need to be required to set it for application/json. I'll add TLS support as well.

The metrics collected from telegraf are transmitted through the HTTP …

0658937

…Post method.

Dark0096 changed the title ~~The metrics collected from telegraf are transmitted through the HTTP …~~ HTTP output plugin Mar 4, 2017

Dark0096 mentioned this pull request Mar 4, 2017

HTTP output plugin #2303

Closed

3 tasks

Run go fmt and changed http.go

d79e011

Add http output plugin in all.go

f69caf9

Shugyousha reviewed May 24, 2017

View reviewed changes

dark added 2 commits May 27, 2017 19:14

Every time called the validate function. It is better to check the ou…

d9679f8

…tput interface later through the Validate() err function.

Merge branch 'master' into output_http

33954c3

Dark0096 commented May 27, 2017

View reviewed changes

danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017

danielnelson reviewed Oct 23, 2017

View reviewed changes

russorat added the new plugin label Jan 20, 2018

dark added 2 commits March 1, 2018 15:56

http output plugin refactoring

220c2e9

Merge branch 'master' into output_http

83782a2

JonoB approved these changes Mar 4, 2018

View reviewed changes

- makeReqBody is translated by content-type.

cc3673e

- Accept status code 2xx.

dark added 5 commits March 5, 2018 01:34

defer resp.Body.Close()

df4f0ac

Add Proxy: ProxyFromEnvironment to transport.

66df3e8

go fmt

1df63dc

h.client.Do error handling

bbb4062

default return nil

53a78d5

Dark0096 commented Mar 4, 2018

View reviewed changes

danielnelson added this to the 1.7.0 milestone Mar 5, 2018

Dark0096 closed this Mar 16, 2018

Dark0096 reopened this Mar 16, 2018

dark added 2 commits March 17, 2018 21:40

Connection reset by peer bug fetch

786c74d

fix fmtcheck

02157b1

dark added 2 commits May 14, 2018 00:55

Merge branch 'master' into output_http

b86a3c4

Support SerializeBatch for efficiently network usage in http output.

6af5344

Dark0096 commented May 14, 2018

View reviewed changes

danielnelson merged commit 190a412 into influxdata:master May 15, 2018

arkady-emelyanov pushed a commit to arkady-emelyanov/telegraf that referenced this pull request May 18, 2018

Add HTTP output plugin (influxdata#2491)

a32642c

leodido pushed a commit that referenced this pull request May 22, 2018

Add HTTP output plugin (#2491)

78cd090

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add HTTP output plugin (#2491)

ca11b63

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019

Add HTTP output plugin (influxdata#2491)

1d37f36

HTTP output plugin #2491

HTTP output plugin #2491

Conversation

Dark0096 commented Mar 4, 2017 • edited Loading